Skip to content

Estimate initial bbox from url points, disable zoom animation#171

Merged
easbar merged 3 commits intomasterfrom
estimate_bbox_from_url
Jan 19, 2022
Merged

Estimate initial bbox from url points, disable zoom animation#171
easbar merged 3 commits intomasterfrom
estimate_bbox_from_url

Conversation

@easbar
Copy link
Copy Markdown
Member

@easbar easbar commented Dec 8, 2021

Following the discussion in #168 here I disabled the bbox update after the /info request and disabled the zoom animation. For URLs with points I estimate the bounding box of the resulting /route request to prevent loading the world view just before we zoom to the route. This should improve the map/page loading performance because less tiles need to be loaded (#132).

Comment thread src/stores/ViewportStore.ts Outdated
Comment on lines +41 to +44
if (initialBBox) {
const updatedState = calculateLatLngFromBbox(this.state, initialBBox, isSmallScreenQuery())
super.setState(updatedState)
}
Copy link
Copy Markdown
Member Author

@easbar easbar Dec 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Janekdererste I wasn't sure how else I shoud do this. I had to set Store#setState public for this. The problem is that getInitialState is called in the super-constructor and subclasses like ViewportStore have no chance to make use of the constructor parameters before. Here I'd like to pass the initialBBox via the constructor and actually make use of it to set the initial state.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the only way to alter a store's state should be through actions, I guess you need an action for this. Like UpdateBboxAction.

Initialize all the stores in index.ts then parse the bbox and issue an UpdateBboxAction to the Dispatcher.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I did this in my last commit.

Comment on lines +49 to +50
width: window.screen.width,
height: window.screen.height,
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the window.screen.height/width here is useful to get a rough estimation of the bounding box size, but maybe it could be more precise. The windows size is not equal to the map size and I think this is the reason why we still see a small flickering of the map: First the map is zoomed such that the markers are right at the map borders and then zoomed out a bit more. @Janekdererste do you have an idea how we could improve this?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a reasonable heuristic, since the map is pretty much the same size as the window. This gets called before the first render pass and before any css is applied though. Therefore, it is not really possible to guess the map size correctly

Copy link
Copy Markdown
Collaborator

@Janekdererste Janekdererste left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would think that some of this will become obsolete once the viewport information is stored in the map again. I'd assume that this is the case once you merge your PR with the switch to openlayers.

Comment thread src/stores/Store.ts Outdated
abstract reduce(state: TState, action: Action): TState

private setState(value: TState) {
public setState(value: TState) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want to have this public.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok yes it's no longer needed since I use an action to set the initial state now.

Comment thread src/stores/ViewportStore.ts Outdated
Comment on lines +41 to +44
if (initialBBox) {
const updatedState = calculateLatLngFromBbox(this.state, initialBBox, isSmallScreenQuery())
super.setState(updatedState)
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the only way to alter a store's state should be through actions, I guess you need an action for this. Like UpdateBboxAction.

Initialize all the stores in index.ts then parse the bbox and issue an UpdateBboxAction to the Dispatcher.

Comment thread src/index.tsx Outdated
return url.searchParams.has('key') ? url.searchParams.get('key') : config.keys.graphhopper
}

function parseURLBBox() : Bbox | null {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ultimately this could be in the NavBar component as well. Once you update the viewport state by dispatching an action, you could put this logic there.

Comment on lines +49 to +50
width: window.screen.width,
height: window.screen.height,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a reasonable heuristic, since the map is pretty much the same size as the window. This gets called before the first render pass and before any css is applied though. Therefore, it is not really possible to guess the map size correctly

@easbar easbar merged commit 5c72483 into master Jan 19, 2022
@easbar easbar deleted the estimate_bbox_from_url branch January 19, 2022 09:14
ZeroGxMax pushed a commit to minhhpkp/graphhopper-maps that referenced this pull request Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants