Improve handling of bbox state #77

easbar · 2021-05-19T20:09:52Z

Handling the bbox state (or more generally the view port state) is a bit tricky, because we partly control it from our application (using /info, selectedPath.bbox etc.) and partly it is maintained by direct user interactions with the map (zooming, scrolling etc.).

In some cases we just want to set the bounds of the map (fitBounds), for example when we want to zoom to a calculated route. This means we want to control the viewport ourselves. In other cases we want to keep the current viewport, for example when we change the map layer or clear a query input box. This means we want to neglect the bbox state of our application and the internal map state should take precedence.

Here I refactored some of the bbox state handling and fixed #72. ~~Unfortunately with my current solution the map no longer zooms to the calculated route when the page is loaded initially. See more comments inline.~~ (<- this is fixed now)

Fixes: #72

easbar · 2021-05-19T20:12:24Z

src/App.tsx

+    const [bbox, setBbox] = useState<Bbox>([-180, -90, 180, 90])
+    useEffect(() => {
+        // make sure the path bbox takes precedence over the info bbox
+        if (!route.selectedPath.bbox)
+            setBbox(info.bbox)
+    }, [info])
+    useEffect(() => {
+        if (route.selectedPath.bbox)
+            setBbox(route.selectedPath.bbox)
+    }, [route])


Using these two effects seems better than the previous code, because now we only update the bbox when the route or info changed, whereas previously we always had to determine some bbox even when we did not want to change anything. This way we also no longer need the shouldUseInfoBbox flag.

Yes, this is much better. Should we maybe combine those into a single effect though? Since you are using route in both effects maybe switch to something like

useEffect(() => setBbox(route.selectedPath.bbox ? route.selectedPath.bbox : info.bbox), [route, info])

I guess this is only a matter of taste though

Ah yes this looks short and clean. But are you sure it does the same? Considering there were quite a few edge-cases (for which we have no tests currently :() and I was sure this version works and we might change this again in #87 I think I would rather leave it as is for this PR. Not the best reasons, I know...

easbar · 2021-05-19T20:16:55Z

src/map/Map.tsx

+        if (prevViewPort)
+            mapWrapper.setViewPort(prevViewPort)


This code is what was missing for #72. When we re-create the map instance we have to restore the previous viewport. However, just doing this is not enough, because without the changes in App.tsx there would be another call to fitBounds that moves the map to the info or route bbox again.

easbar · 2021-05-19T20:24:13Z

src/map/Map.tsx

-    useEffect(() => map?.fitBounds(bbox), [bbox, map])
+    useEffect(() => map?.fitBounds(bbox), [bbox])


This seems to be the most tricky part to me. Keeping the map dependency means that after a style change the call to setMap will trigger fitBounds again. If we calculate a route, scroll somewhere else and then change the style this means the map will center at the calculated route again.

On the other hand if we do not keep the map dependency map will be undefined here when the page loads initially and the call to fitBounds won't happen. Not sure how to fix this tbh.

Update: With my last commit it is 'working' now: 2fadb17, but honestly this seems way too complicated. I now use useRef to keep track of the latest bbox value so it is available by the time onMapReady is called and we can call fitBounds(prevBbox.current). Is there maybe a better way to construct the map instance before we receive the bbox etc.?

Update2: I think I found a better solution here: 933e958, 3ba7c79 and now I think the PR is ready to merge.

easbar · 2021-05-20T05:41:18Z

Btw not sure if we already need this, but the Mapbox documentation also describes how to keep the lat/lng/zoom state of the map in sync with the application state: https://docs.mapbox.com/help/tutorials/use-mapbox-gl-js-with-react/#store-the-new-coordinates

The code is here: https://github.com/mapbox/mapbox-react-examples/tree/master/basic

karussell · 2021-05-20T09:07:43Z

Thanks for investigating this, this works for me. I think this also supersedes #71?

And is it easy to reduce the world bbox to avoid seeing some continents twice?

Also this is related to #53.

easbar · 2021-05-20T09:26:54Z

I think this also supersedes #71?

Yes, I think so.

And is it easy to reduce the world bbox to avoid seeing some continents twice?

I think we can do this in Mapbox.ts:

 this.map = new Map({
            // ...
            maxBounds: [[-180, -90], [180, 90]],
            renderWorldCopies: false
        })

See also here:
https://docs.mapbox.com/mapbox-gl-js/example/restrict-bounds/
https://docs.mapbox.com/mapbox-gl-js/example/render-world-copies/

Also this is related to #53.

Not sure. This PR is mostly about how the bbox is applied to the map, but #53 seems also about determining the right bounding box for a route (use appropriate padding, do not zoom in too much etc.)

easbar · 2021-05-20T09:28:20Z

I just disabled the world copies here: b29801b, but feel free to adjust

easbar · 2021-05-20T09:56:56Z

I briefly tried avoiding the initial jump when points are specified (also not fixed in #71), but let's do this in a separate PR. I opened #82 for this.

easbar · 2021-05-20T10:17:24Z

I also wonder what it would take to setup tests for things like "map does not go back to info box when we clear query point box after route was calculated" etc.

# Conflicts: # src/App.tsx # src/map/Map.tsx # src/map/Mapbox.ts

# Conflicts: # src/App.tsx # src/api/Api.ts # src/stores/QueryStore.ts

Janekdererste

This looks good overall.

Janekdererste · 2021-06-03T08:10:47Z

src/App.tsx

+    const [bbox, setBbox] = useState<Bbox>([-180, -90, 180, 90])
+    useEffect(() => {
+        // make sure the path bbox takes precedence over the info bbox
+        if (!route.selectedPath.bbox)
+            setBbox(info.bbox)
+    }, [info])
+    useEffect(() => {
+        if (route.selectedPath.bbox)
+            setBbox(route.selectedPath.bbox)
+    }, [route])


Yes, this is much better. Should we maybe combine those into a single effect though? Since you are using route in both effects maybe switch to something like

useEffect(() => setBbox(route.selectedPath.bbox ? route.selectedPath.bbox : info.bbox), [route, info])

I guess this is only a matter of taste though

Janekdererste · 2021-06-03T08:12:48Z

src/map/layer-group-solid.svg

Was this change on purpose?

Ah no sorry, this was just auto-formatting, I reverted this change.

Janekdererste · 2021-06-03T08:22:48Z

src/map/Map.tsx

@@ -19,22 +19,42 @@ type MapProps = {
 export default function({ selectedPath, paths, queryPoints, bbox, mapStyle }: MapProps) {
    const mapContainerRef: React.RefObject<HTMLDivElement> = useRef(null)
    const [map, setMap] = useState<Mapbox | null>(null)
+    const prevViewPort = useRef<ViewPort | null>(null)


I wonder if the functionality to remeber the viewport when the map instance is replaced can be achieved with a functional state update https://reactjs.org/docs/hooks-reference.html#functional-updates

Here we would get the previous map instance as parameter to useState for the map something like this maybe?

const mapWrapper = new Mapbox( mapContainerRef.current!, mapStyle, () => { setMap(prevWrapper => { mapWrapper.setViewPort(prevWrapper.getViewPort()) return mapWrapper }) Dispatcher.dispatch(new MapIsLoaded()) } )

On the other hand this might be to late and cause fickering of the map again, because the viewport would be set after the map is loaded.

Ah, the code example is for line 35, but it also concerns the useRef

Ok interesting. I followed the pattern to use the previous state here: https://reactjs.org/docs/hooks-faq.html#how-to-get-the-previous-props-or-state. If you don't mind also here I would leave as is for now and either change this in a follow-up commit/PR or maybe we do not need to replace the map instance anyway using the react wrapper #87.

Sorry, for the late answer. I totally agree especially, since we want to replace the map layer anyway.

# Conflicts: # src/App.tsx

easbar added 2 commits May 19, 2021 21:59

Use react state management for bbox

b471108

Preserve view port when switching style

9287fe2

easbar commented May 19, 2021

View reviewed changes

easbar added 5 commits May 19, 2021 23:03

Fix bbox on initial page load

2fadb17

Add comment

e387ffe

Add another comment...

f683542

Better solution

933e958

Fix

3ba7c79

easbar added this to the 0.3 milestone May 20, 2021

This was referenced May 20, 2021

Do not show flicker/jump when map loads with route coordinates #82

Closed

avoid initial fitBounds #71

Closed

Merge branch 'master' into bbox_state

8178245

easbar added 4 commits May 20, 2021 15:28

Merge branch 'master' into bbox_state

c23d103

# Conflicts: # src/App.tsx # src/map/Map.tsx # src/map/Mapbox.ts

Prettier all source files

57a2061

Merge branch 'master' into bbox_state

22da295

# Conflicts: # src/App.tsx # src/api/Api.ts # src/stores/QueryStore.ts

minor

545acea

easbar mentioned this pull request May 25, 2021

Mapbox/React wrapper #87

Merged

6 tasks

Janekdererste approved these changes Jun 3, 2021

View reviewed changes

easbar added 5 commits June 3, 2021 10:55

Revert svg formatting

3900b5b

Merge branch 'master' into bbox_state

1d2ff34

Add comments

3303169

Update formatting

050aa2d

Merge branch 'master' into bbox_state

a9b4950

# Conflicts: # src/App.tsx

easbar merged commit 856b767 into master Jun 7, 2021

easbar deleted the bbox_state branch June 7, 2021 08:30

easbar mentioned this pull request Jun 9, 2021

Problems with map style selection on firefox mobile #80

Closed

ZeroGxMax pushed a commit to minhhpkp/graphhopper-maps that referenced this pull request Nov 12, 2024

Improve handling of bbox state (graphhopper#77)

608f724

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve handling of bbox state #77

Improve handling of bbox state #77

easbar commented May 19, 2021 •

edited

Loading

easbar May 19, 2021 •

edited

Loading

Janekdererste Jun 3, 2021

easbar Jun 3, 2021

easbar May 19, 2021 •

edited

Loading

easbar May 19, 2021

easbar May 19, 2021 •

edited

Loading

easbar May 20, 2021 •

edited

Loading

easbar commented May 20, 2021 •

edited

Loading

karussell commented May 20, 2021 •

edited

Loading

easbar commented May 20, 2021

easbar commented May 20, 2021

easbar commented May 20, 2021

easbar commented May 20, 2021

Janekdererste left a comment

Janekdererste Jun 3, 2021

Janekdererste Jun 3, 2021

easbar Jun 3, 2021

Janekdererste Jun 3, 2021

Janekdererste Jun 3, 2021

easbar Jun 3, 2021

Janekdererste Jun 7, 2021

		useEffect(() => map?.fitBounds(bbox), [bbox, map])
		useEffect(() => map?.fitBounds(bbox), [bbox])

Improve handling of bbox state #77

Improve handling of bbox state #77

Conversation

easbar commented May 19, 2021 • edited Loading

easbar May 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easbar May 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easbar May 19, 2021 • edited Loading

Choose a reason for hiding this comment

easbar May 20, 2021 • edited Loading

Choose a reason for hiding this comment

easbar commented May 20, 2021 • edited Loading

karussell commented May 20, 2021 • edited Loading

easbar commented May 20, 2021

easbar commented May 20, 2021

easbar commented May 20, 2021

easbar commented May 20, 2021

Janekdererste left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easbar commented May 19, 2021 •

edited

Loading

easbar May 19, 2021 •

edited

Loading

easbar May 19, 2021 •

edited

Loading

easbar May 19, 2021 •

edited

Loading

easbar May 20, 2021 •

edited

Loading

easbar commented May 20, 2021 •

edited

Loading

karussell commented May 20, 2021 •

edited

Loading