Use OpenLayers instead of Mapbox #176

easbar · 2022-01-18T19:09:30Z

This is a follow-up of #159, this time trying to actually do this migration rather than exploring its implications.

Here is a small walk-through:

package.json/dependencies: I removed the react-map-gl dependency (the react/mapbox wrapper including mapbox) obviously and added OpenLayers 6 (ol) instead. I also added ol-mapbox-style to show the MapTiler tiles, even though we might remove this entirely since we rather focus on raster tiles for now. I also had to add the geojson dependency directly, because we were using it here and there and we got it from Mapbox so far.
tsconfig.json: I just modified this to make the app run. Possibly we can revert parts of this especially if we remove ol-mapbox-style.
Now to the more interesting changes: How do we create and show the map? First thing we need is the OL Map object. I'm not entirely sure where we should create it but right now I simply create it in map.ts and there is also a reference to it so we can import it from everywhere. The next thing we need to do is attach the map to the DOM. For this I added MapComponent.tsx which uses useRef to create a container div for the map. It simply receives the map object as a prop.
The next thing we need to do is add some layers to the map. Let's start with the background layer. If we just used the same layer forever we could just add it to the map initially and would be done with it. But since we can change the layer via the layers option menu we need code that first removes the existing layer and then adds the new layer. This should run whenever the styleOption changes, however we don't want to run this code for every render call of our App component. Therefore I use useEffect and put this useEffect into another function called useBackgroundLayer (a custom hook: https://reactjs.org/docs/hooks-custom.html). I added the other layers (like query points and paths) the same way. One advantage here is that we can add layers without passing all the props we need for the different layers into our previous MapComponent. useBackgroundLayer, just like MapComponent, simply receives the map (and the styleOption) as a function parameter in App.tsx. Note that if we did not use useEffect our 'remove-old-and-add-new-layer'-code would run for every render call.
We also need to adjust the map view for certain actions like RouteRequestSuccess or SetSelectedPath etc. Therefore I use an ActionReceiver that is registered at our Dispatcher. It is called MapActionReceiver and created in index.ts similar to our stores. It also gets a reference to the map objects so it can adjust the map view for whatever Action we want.
This is basically it, most of the remaining code is simply about adding the other layers. I'll add a todo list and a few more comments inline.

….json

* the types are included in the distribution since 6.6.0 (openlayers/openlayers@9e88134)

* not sure about tsconfig changes, took this from here: openlayers/ol-mapbox-style#269 (comment)

# Conflicts: # src/App.tsx # src/map/Map.tsx # src/map/Popup.tsx

src/NavBar.ts

easbar · 2022-01-18T19:13:11Z

src/api/graphhopper.d.ts

@@ -54,6 +54,7 @@ export interface RoutingProfile {
 }

 export interface Path extends BasePath {
+    // todo: do we really need the geojson dependency? it was included in mapbox but its not included in OL


I don't see such type definition in the ol package. They have the geojson stuff as actual classes, that's not compatible with this I think. So I would think yes.

Ok I was more thinking about not using it at all. But let's just keep it for now.

easbar · 2022-01-18T19:14:12Z

src/layers/ContextMenu.module.css

+/* todo: this is just copy-pasta from https://openlayers.org/en/latest/examples/popup.html, I have no idea what all this means,
+   and the popup looks ugly still, at the very minimum the background color needs to be set though
+   the popup design needs an overhaul anyway... https://github.com/graphhopper/graphhopper-maps/issues/136
+   */
+.popup {


Compared to the react-map-gl wrapper OpenLayers does not provide a popup component. Therefore I used a simple div and copy-pasted some css from the OpenLayers examples for the context menu and path details popup. I'm pretty sure this can be improved...

I don't have a better template at hand at the moment. Since this is such a big PR already, I would suggest to go with this version.

easbar · 2022-01-18T19:14:36Z

src/layers/PathDetailPopup.module.css

+/* todo: this is just copy-pasta from https://openlayers.org/en/latest/examples/popup.html, I have no idea what all this means,
+   and the popup looks ugly still, at the very minimum the background color needs to be set though
+   the popup design needs an overhaul anyway... https://github.com/graphhopper/graphhopper-maps/issues/136
+   */


Same as for the context menu

I would also just use it as is and make an issue for styling this. This popup is indeed a little too big I think. It doesn't break anything though

Ok. And we have this issue already: #136

easbar · 2022-01-18T19:15:43Z

src/layers/PathDetailPopup.tsx

+        // todo: use createMapMarker from heightgraph?
+        // {createMapMarker(point.elevation, point.description)}


This todo is just copied from master

easbar · 2022-01-18T19:17:36Z

src/map/Map.module.css

+.mapContainer {
+    height: 100%;
+    width: 100%;
+}


Not sure, but I needed this to make the map visible

I think this is fine. It alternatively, in App.module.css the .map class could have display: grid, which should stretch the map container by default.

Yes, you're right this also works. I changed it.

Hm, this works in Firefox, Chrome and also when using the mobile view in dev tools. But for the aws deployment the map stayed invisible on mobile for @karussell and me (even though it worked for desktop). So I reverted it to my initial version.

Hm, this is odd.

Yes, but I tried it twice and we both saw the same results. Feel free to try it again, but otherwise I'll leave as it is this for now.

easbar · 2022-01-18T19:18:10Z

src/stores/MapOptionsStore.ts

@@ -96,6 +98,7 @@ const tfCycle: RasterStyle = {
    attribution:
        osmAttribution +
        ', <a href="https://www.thunderforest.com/maps/opencyclemap/" target="_blank">Thunderforest Cycle</a>',
+    tilePixelRatio: 2,


This way the raster tiles aren't blurry like they were for Mapbox

Isn't the avoidance of fractional numbers for zoom that avoids the blurry look? (constrainResolution: true)
See your comment openlayers/openlayers#12963 (comment)

And tilePixelRatio (additionally) improves blurry for high dpi devices? (but for mapbox we had these "2x" in the URL)

(then also the question would be: could we set it to 1 for low dpi devices somehow?)

Isn't the avoidance of fractional numbers for zoom that avoids the blurry look? (constrainResolution: true)
See your comment openlayers/openlayers#12963 (comment)

Yes, avoiding fractional zoom levels is part of it. But tilePixelRatio is still needed I think. As far as I know we need to tell OpenLayers also about the resolution of our tiles. From the documentation: https://openlayers.org/en/latest/apidoc/module-ol_source_XYZ-XYZ.html

The pixel ratio used by the tile service. For example, if the tile service advertizes 256px by 256px tiles but actually sends 512px by 512px images (for retina/hidpi devices) then tilePixelRatio should be set to 2.

So we need to set this number depending on the tile service.

but for mapbox we had these "2x" in the URL

Yes, for some tile providers adding 2x changes the resolution of the tiles they send, but either way we need to tell OpenLayers the resolution of the tiles we feed to it?

(then also the question would be: could we set it to 1 for low dpi devices somehow?)

Do you see blurry tiles on some device, or what do you want to improve here?

Also see this and the following comments: #159 (comment)

easbar · 2022-01-18T19:18:37Z

src/stores/MapOptionsStore.ts

-    // The original client has these but those options yield cors errors with mapbox yields a cors error
-    // lyrk,
-    // wanderreitkarte,
-    // This works but is extremely slow with mapbox
+    lyrk,
+    wanderreitkarte,
+    // This one is extremely slow with mapbox and openlayers?!


lyrk and wanderreitkarte work with OpenLayers while they did not for Mapbox. sorbian is still very slow

easbar · 2022-01-18T19:18:51Z

src/stores/MapOptionsStore.ts

@@ -208,7 +213,7 @@ export default class MapOptionsStore extends Store<MapOptionsStoreState> {
                `Could not find tile layer specified in config: '${config.defaultTiles}', using default instead`
            )
        return {
-            selectedStyle: selectedStyle ? selectedStyle : mapTiler,
+            selectedStyle: selectedStyle ? selectedStyle : osmOrg,


We probably want to change this to Omniscale

And shall we even keep map tiler?

Let's use Omniscale as default (but keep map tiler)

But as the maptiler layers and mapilion are so slow we should probably mark them as experimental (e.g. gray out the label a bit?)

Ok I made Omniscale the default now. If the two vector layers are so slow that they should not be used why not remove them altogether? Or is there any case where they would be useful?

For testing and improving I would keep them. We could then ask / contract e.g. OL maintainers to improve this somehow :)

We can keep them, but maybe we should remove them from the menu? We could still use them by setting the url parameter... They are too slow to use, aren't they? So why should users be confronted with this if it is just for internal testing? If we keep them in the menu we need to find a way to tell users that this is for testing of vector tiles only. Just graying them out doesn't really accomplish this? Or maybe we change the menu entries to MapTiler (slow) or something?

easbar · 2022-01-18T19:19:19Z

tsconfig.json

@@ -1,5 +1,6 @@
 {
    "compilerOptions": {
+        "allowJs": true,


If this important we can try to get rid of this again but I needed it to make the code compile

Interesting. Which parts would not compile?

Hm, I just tried again and it seems to work also without this now :)

src/map/map.ts

easbar · 2022-01-18T19:21:21Z

src/stores/MapActionReceiver.ts

+    }
+
+    receive(action: Action) {
+        // todo: port old ViewportStore.test.ts or otherwise test this


I'll take a look at this, but leaving this as a reminder here for now.

# Conflicts: # src/actions/Actions.ts # src/stores/ViewportStore.ts

Janekdererste

This looks very good overall. The layers part being inside custom hooks turned out pretty I think. It is quite easy to get an idea what's going on.

I noticed, that the context menu sometimes doesn't open, after I have set some points via the context menu. It might be related to the change between wide and narrow layouts. Also, the context menu doesn't close anymore at all, except one selects something from it. I think this is a separate issue, which we have somewhere already thoug.

Janekdererste · 2022-01-19T09:38:14Z

src/map/Map.module.css

+.mapContainer {
+    height: 100%;
+    width: 100%;
+}


I think this is fine. It alternatively, in App.module.css the .map class could have display: grid, which should stretch the map container by default.

Janekdererste · 2022-01-19T09:39:34Z

src/map/MapComponent.tsx

+
+/** A small react component that simply attaches our map instance to a div to show the map **/
+export default function ({ map }: MapComponentProps) {
+    const mapElement = useRef<any>()


This could be useRef<HtmlDivElement | null>(null). I think this is how it is suggested by the react documentation.

Janekdererste · 2022-01-19T09:47:19Z

src/api/graphhopper.d.ts

@@ -54,6 +54,7 @@ export interface RoutingProfile {
 }

 export interface Path extends BasePath {
+    // todo: do we really need the geojson dependency? it was included in mapbox but its not included in OL


I don't see such type definition in the ol package. They have the geojson stuff as actual classes, that's not compatible with this I think. So I would think yes.

Janekdererste · 2022-01-19T09:53:00Z

src/layers/ContextMenu.module.css

+/* todo: this is just copy-pasta from https://openlayers.org/en/latest/examples/popup.html, I have no idea what all this means,
+   and the popup looks ugly still, at the very minimum the background color needs to be set though
+   the popup design needs an overhaul anyway... https://github.com/graphhopper/graphhopper-maps/issues/136
+   */
+.popup {


I don't have a better template at hand at the moment. Since this is such a big PR already, I would suggest to go with this version.

Janekdererste · 2022-01-19T09:53:58Z

src/layers/ContextMenu.tsx

+    const [menuCoordinate, setMenuCoordinate] = useState<Coordinate | null>(null)
+    const [overlay, setOverlay] = useState<Overlay | undefined>()
+
+    const container = useRef()


This could be typed. useRef<HTMLDivElement | null

Janekdererste · 2022-01-19T09:59:32Z

src/layers/UsePathDetailsLayer.tsx

+
+export default function usePathDetailsLayer(map: Map, pathDetails: PathDetailsStoreState) {
+    useEffect(() => {
+        removePathSegmentsLayer(map)


the custom hook is called usePathDetailsLayer, the sub methods are named <something>PathSegementsLayer. I think the naming should be the same, since it concerned about the same thing

Hm the layer is related to the path details diagram so usePathDetailsLayer kind of make sense. But what it actually does is just highlight segments of the path that are above/below a certain elevation. I don't know.. I'm not sure if renaming is an improvement here. I'll add a comment that says what this layer does for now.

Janekdererste · 2022-01-19T11:02:48Z

src/layers/createMarkerSVG.ts

+// todo: this is mostly duplicated from `Marker.tsx`. To use `Marker.tsx` we would probably need to add ol.Overlays, i.e.
+//       create a div for each marker and insert the svg from `Marker.tsx`.
+export function createSvg({ color, number, size = 0 }: MarkerProps) {
+    return `<svg aria-hidden="true" focusable="false" role="img" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 384 512" width="${
+        // todo: we do not use width in Marker.tsx, but without this the markers are not shown in Firefox :( (but they are shown in Chrome...)


It would be nice to have this mounted as a Marker.tsx component. This would indeed mean that we would have a wrapper div for each point in a route. One would then create a new rendering context for each point. Maybe this would be too complicated.

The way this works now, is that ol renders the svg on the canvas as an image directly. I'd assume that this is the most efficient way to do it.

Leave it like this, fix the translation and the number?

src/map/map.ts

Janekdererste · 2022-01-19T11:05:10Z

src/stores/MapActionReceiver.ts

@@ -0,0 +1,56 @@
+import { Action, ActionReceiver } from '@/stores/Dispatcher'


This looks quite nice so far. I think this is a good direction. (Meaning the entire module)

Janekdererste · 2022-01-19T11:06:19Z

tsconfig.json

@@ -1,5 +1,6 @@
 {
    "compilerOptions": {
+        "allowJs": true,


Interesting. Which parts would not compile?

easbar · 2022-01-19T12:47:23Z

This looks very good overall. The layers part being inside custom hooks turned out pretty I think. It is quite easy to get an idea what's going on.

Ok cool, thanks for your review.

I noticed, that the context menu sometimes doesn't open, after I have set some points via the context menu. It might be related to the change between wide and narrow layouts.

Hm, can you reproduce this consistently somehow?

Also, the context menu doesn't close anymore at all, except one selects something from it. I think this is a separate issue, which we have somewhere already thoug.

Yes, I think it master it does not close when we click (see #135). But it does close when we pan the map. In this branch it only closes when we select an action. Let's fix both in #135?

easbar · 2022-01-19T12:55:18Z

Yes, I think it master it does not close when we click (see #135). But it does close when we pan the map. In this branch it only closes when we select an action. Let's fix both in #135?

I just tried this briefly and it is easy to close the menu on click. Just add

        map.on('click', () => {
            overlay?.setPosition(undefined)
            setMenuCoordinate(null)
        })

in ContextMenu.tsx. The tricky thing is to make this not interfere with the long press handler, make it work on mobile etc, so let's do this later in #135. I'm also wondering: Should we close it when we pan the map or not?

karussell · 2022-01-20T07:55:53Z

Thanks a lot for this @easbar, great work!

Everything works and due to the raster tile speed and precision the project feels even more polished :)

I played a bit around and tried to find the longest shortest path ;)

The marker offset is not yet perfect. E.g. have a look about the start of this route which seems to be in the sea instead of the land: http://localhost:3000/?point=-34.356099%2C18.497489&point=66.638129%2C179.986909&profile=car&layer=OpenStreetMap (Interestingly the marker in current master is also wrong but in a different way)

Then there seems to be some bbox issue (but very likely this is related to #53) when the route cannot be found:
http://localhost:3000/?point=66.641111%2C-179.99621&point=66.64092%2C179.99635&profile=car&layer=OpenStreetMap

easbar · 2022-01-20T08:26:18Z

Thanks for testing!

The marker offset is not yet perfect.

Yes, see also this discussion: #176 (comment)

Then there seems to be some bbox issue

Yes, this is strange. I did not figure out what causes this yet. First we estimate the bbox from the url points (as in #171) and then we adjust it to the actual route once it is calculated. But if no route could be calculated the bbox should just remain the same. But for some reason this does not work... It seems like the map is trying to fetch tiles for a very large zoom level (like 27) so nothing is shown, but I don't understand where this large zoom level is coming from. The estimation of the bbox from the url points is not working yet. Maybe because the map is not ready at the time we set the bounds. So if the route is calculated correctly we get the bbox of the route and if the route fails we end up in this strange state where the camera position is broken and the zoom level is way too high (like 27).

easbar · 2022-01-20T11:02:07Z

Then there seems to be some bbox issue

I fixed this here: dd0ade8. Now we see the map also for urls for which no route can be found. The problem was that at the time we set the initial bounding box the map's screen size was still undefined as it hadn't been rendered at that point yet. Therefore we need to estimate the map size as well.

…vert css change" This reverts commit e41435c.

…how map on mobile, try to revert css change"" This reverts commit d401067.

easbar · 2022-01-26T08:26:51Z

I'll add separate issues for some of the things we discussed here.

Co-authored-by: Janekdererste <janek.laudan@mailbox.org>

easbar and others added 30 commits November 10, 2021 13:41

Replace awesome-typescript-loader with ts-loader, update package-lock…

f3d4246

….json

Upgrade to npm 8.1.0, lockfileVersion 2

c33d240

remove mapbox, add ol

233faaa

hello ol

83e637d

disable test for now

02e1e1e

Remove @types/ol

18f51f9

* the types are included in the distribution since 6.6.0 (openlayers/openlayers@9e88134)

code formatting

8f26073

Enable other raster layers

5c6bf18

Enable commented out raster layers too (they work with OL)

a5aa582

Include vector tiles using ol-mapbox-style

2dc764d

* not sure about tsconfig changes, took this from here: openlayers/ol-mapbox-style#269 (comment)

Properly remove existing layers

64e119f

trigger build

be71e3e

Move MapStore and BackgroundLayer out of Map component

f24b454

Show paths layer and dummy markers

686b2a3

Handle view change actions

520fc7e

remove controls

bb2f14a

Remove MapLayer, add some comments

03e6186

Update route when dragging markers

7ccf309

code formatting

9529a3d

Allow click selection of alternative paths

f75c4d9

Add context menu

a270a38

disable user selection on ios

24c4d68

Make context menu work on ios using long touch handler

cb3e74e

code formatting

4e1b5e1

Add map attributions

ddc0764

Add function names

738db85

Update comments

bf63e92

Merge branch 'master' into fix_package_lock

44aba35

Merge branch 'fix_package_lock' into ol_exp

ff9ccb1

# Conflicts: # src/App.tsx # src/map/Map.tsx # src/map/Popup.tsx

Merge branch 'master' into fix_package_lock

09e426f

easbar added 5 commits January 18, 2022 17:05

estimate bbox from query points

fa2af4d

Use custom hooks for layers

c2ba47c

Make map just an action receiver instead of a store

2e4dddc

Separate map creation from action receiving code

8c75821

minor

3e0cb14

easbar commented Jan 18, 2022

View reviewed changes

src/map/map.ts Show resolved Hide resolved

easbar commented Jan 18, 2022

View reviewed changes

easbar mentioned this pull request Jan 18, 2022

Migrate from Mapbox to OpenLayers #159

Closed

11 tasks

easbar added this to the 0.4 milestone Jan 18, 2022

Merge branch 'master' into ol_exp

ff745d0

# Conflicts: # src/actions/Actions.ts # src/stores/ViewportStore.ts

Janekdererste reviewed Jan 19, 2022

View reviewed changes

easbar added 2 commits January 19, 2022 13:32

Remove allowJs, use typed useRef etc.

adb8315

Use display grid for App module instead of 100% height/width for map

d9213ec

Use Omniscale tiles by default

74ab4a7

Fix map size estimation for initial bbox choice

dd0ade8

easbar added 3 commits January 20, 2022 12:50

aws deployment does not show map on mobile, try to revert css change

e41435c

Really? Revert "aws deployment does not show map on mobile, try to re…

d401067

…vert css change" This reverts commit e41435c.

go back to start... Revert "Really? Revert "aws deployment does not s…

3976084

…how map on mobile, try to revert css change"" This reverts commit d401067.

easbar merged commit 92f7883 into master Jan 26, 2022

easbar deleted the ol_exp branch January 26, 2022 08:26

This was referenced Jan 26, 2022

Indicate that vector tiles are slow #178

Closed

re-use Marker.tsx #179

Open

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

Use OpenLayers instead of Mapbox (graphhopper#176)

e2e856f

Co-authored-by: Janekdererste <janek.laudan@mailbox.org>

		// todo: use createMapMarker from heightgraph?
		// {createMapMarker(point.elevation, point.description)}

		@@ -0,0 +1,56 @@
		import { Action, ActionReceiver } from '@/stores/Dispatcher'

Use OpenLayers instead of Mapbox #176

Use OpenLayers instead of Mapbox #176

Conversation

easbar commented Jan 18, 2022

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

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

karussell Jan 20, 2022 • edited Loading

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

karussell Jan 20, 2022 • edited Loading

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

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

Choose a reason for hiding this comment

Janekdererste Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easbar commented Jan 19, 2022

easbar commented Jan 19, 2022

karussell commented Jan 20, 2022 • edited Loading

easbar commented Jan 20, 2022 • edited Loading

easbar commented Jan 20, 2022

easbar commented Jan 26, 2022

karussell Jan 20, 2022 •

edited

Loading

karussell Jan 20, 2022 •

edited

Loading

Janekdererste Jan 19, 2022 •

edited

Loading

karussell commented Jan 20, 2022 •

edited

Loading

easbar commented Jan 20, 2022 •

edited

Loading