-
Notifications
You must be signed in to change notification settings - Fork 114
feature: use maplibre-gl instead of leaflet #258
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Preview is ready! 🚀 You can view it here: https://valhalla-app-tests.gis-ops.com/258 |
I think this is the first time I saw OSM vector tiles and I must say, I do like the raster ones better, both in style as in zoom-dependent features. One has to be awfully closely zoomed in to see pedestrian paths e.g. in parks and the map style is fairly bad IMO. This is not a review of the work being done, either way we should switch to vector tiles and I guess most of that could be tweaked by us if we wanted to, or we can eventually include more base map/style options like Stadia's styles. |
Why did they decide to make actual forest a light green?! I'll give it the benefit of the early-stage doubt I guess |
@nilsnolde I believe when you get raster ones, you get the maps already styled for you. But when you get vector tiles, you can create your own configuration with style configuration. I mean if you download the style.json file from my commit and try to open it https://maputnik.github.io editor, you will see that everything is customizable. You can see some styled maps from https://maps.black, for example I like openstreetmap-protomaps/protomaps/seafoam |
Yep that's one of the beauties of vector tiles. However, those style jsons are data dependent, and osm just released their own data. I doubt there's many styles available at this point, but maybe if we dig a little. We can certainly make small tweaks to existing ones, but creating one from scratch is a big-ish exercise. @ianthetechie you're a vector tiles guru:) did you hear anything about pretty and reasonable OSM vector tile style.json? |
Ooh, good question! IIRC the tiles that the OSMF is deploying use the Shortbread schema. There are a few styles here that I think look better: https://shortbread-tiles.org/styles/. The only challenge with some of these might be (I haven't inspected them deeply) lack of details like pedestrian paths, crossings, gates, etc. that can be useful for routing. The classic OSM carto style may be ugly and Leaflet may be funky, but they are probably the most detailed view, and there are cases where that's nice. I'd love to see a style switcher; MapLibre can display raster maps just as well too. And there's a React wrapper too IIRC. |
Thanks @ianthetechie :) yeah my impression was as well that it requires wayyyy too high zoom levels to see ped paths. +1 for base map or even style selection at runtime! |
style selection at runtime is definitely easy to do -of course we should find style options for it- but i am confused how to continue with maplibre as current state. there is no change with the plan, right? |
Nice, apart from the default OSM style I think this looks really promising! Some things I noticed:
But I think overall it's great, rendering is super smooth, and the drawing with terra draw seems very ergonomic (I was a bit surprised that you're still in edit mode after finishing a polygon, but I think this is the better user experience; it's common to draw more than one! |
1476ba1
to
e57d97a
Compare
@nilsnolde @chrstnbwnkl can you test again? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @mustaphaturhan , looks & feels good IMO! the only 2 minor things I observed:
- when selecting a polygon, we should probably mute the normal left-click behavior if possible; currently the normal pop-up overlaps the polygon one selects
- when one polygon out of multiple is selected and the "delete polygon" button is clicked, it should probably only delete the selected one instead of all polygons; or alternatively (and feasible) have it similar as currently: activate the "delete" button and every polygon one clicks on will be removed
it's not the most used feature I'd say, so never mind the last point if it's not fairly trivial code changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I just saw that the polygons are not yet taken into account when requesting. the request should contain an array exclude_polygons
with the coords from the drawn ones.
@nilsnolde are we looking at the same thing? 🤔 because in the latest deploy, I am sending |
95e76db
to
4d79bac
Compare
Closes #245
Closes #158
There are a lot of AI changes in it. I mıght decline the PR completely and try again. Just wanted to test how it looks in production.
Some missing points:
or alternatively (and feasible) have it similar as currently: activate the "delete" button and every polygon one clicks on will be removedunfortunately, alternative method is not feasible for terradraw library.