Closed rinigus closed 6 years ago
otsaloma
commented
7 years ago Now that I'm finished with the server (and made it before Sailfish X release!), I'll look at the voice PR. So, that's at the front of the queue. If you think it's better to base on that branch, it's probably fine.
Regarding changes, yes, I expect there is a lot needed. The UI has been written for the QtLocation API, and yes, a lot of weird hacks and workarounds have been needed.
rinigus
commented
7 years ago The work on Mapbox GL implementation is going on at https://github.com/rinigus/poor-maps/tree/gl . Its progressing nicely, but its not functional at the moment. I have replaced the map widget and I am going through the menu from the top.
Main changes were:
Root.qml is a Page now and there is no need to track rotation of the screen. Map responds to rotation perfectly as it is.
Position marker is just an additional source/layer that is updated as it changes and drawn with either non-moving or moving icon. Icon changes in accordance with the direction, as before. On click, autocenter is flipped.
POI markers are also extra source/layer; clickable to expose POI balloons.
Dummy page is not used anymore.
While breaking things and trying to streamline them, I am in trouble with the page stack manipulation. As far as I can understand, you used Root as an item that you could just flip on and off via visibility. I do wonder whether we can get similar design using regular page stacking? Or is the main reason for going through
dummy -> menu 1 -> menu 2 root lives in parallel
since it was best to design UI using such approach?
otsaloma
commented
7 years ago The comments should explain it.
Map is outside the page stack, so that we can retain the page stack and return the last viewed page as-is, e.g. so that you can browse through search results one by one (the last page in dummy + menu + search + results). If you put the Map in the page stack, then that won't work, because to return to the first page you destroy the rest, or at least I didn't find a way to save and recreate the page stack, I had to avoid destroying it all together.
Root is a consequence of the above. Pages have automatic orientation handling, because the map is outside the page stack, I needed implement rotation handling myself and that's what Root and the "revolver" item below it do.
Dummy is just an animation thing, it's a trick to get the page switch animation while the Map is not actually in the page stack. I suspect if the main menu just appears without animation, it might not be clear that you need to swipe right to get back to the map.
This is a bit hackish, but has worked fine in the context we've had before. If you have a better solution, go ahead. As far as I understand, you simply can't put the Map in the page stack, that won't work. You can get rid of Root, if you have another way to handle orientation changes. You can probably go ahead and get rid of the Dummy, the animation is something we can worry about later.
otsaloma
commented
7 years ago And, about the big picture. The changes seem to be very comprehensive and quality will be experimental, I can't release that as a version of Poor Maps and I'd like the new GL app to be parallel-installable. So, I'm thinking we'd
Does this sound OK? This would also mean that you could just do a rough PR, enough to get the map component in order and basic stuff working, the rest could be polished working straight in the master of the fork (and I expect I could do at least the majority of that polish).
rinigus
commented
7 years ago @otsaloma, thank you very much for taking time and explaining it on the top of the comments in the code! I did cut too far with the craft-cutting-mode, sorry for that. I will recover the code dealing with the page stack and decouple page-induced rotation from the rotation in the map induced by gps/navigation directions.
As for big picture: yes, this sounds excellent! I am not sure that we have to rename too many files, but we'll see better when we get there. When I get over the hiccup with the page stack/rotation, I expect to be able to port all the functionality over.
There will be polishing needed, for sure, since we cannot fill DummyPage nor Cover from tiles lying around anymore, for example. We would have to discuss how to make new basemaps configuration files as well (we should be even able to mix raster [but a bit blurry] and vector maps in the new code).
I'll keep an eye on the voice PR work and then I can push PR as soon as you are ready. I assume that I'll get it into such "rough PR" stage soon :)
rinigus
commented
7 years ago Revolver and pageStack manipulation restored and I am back on track - all is rotating as it should. Thank you for the help!
rinigus
commented
7 years ago I think the first stage is done. The comparison between voice and gl branches is available at https://github.com/rinigus/poor-maps/compare/voice...rinigus:gl . I would probably have to wait for PR until the voice is done, since otherwise you'll get the both changesets combined into one with the GL PR.
What works:
What's missing:
Cover page doesn't show map (no tiles to show). Its probably a matter for discussion on what should be shown and whether its OK to miss the map there.
Dummy page is blank. So, trouble in animation
Cache is in /tmp right now - would have to move the database file into .cache probably
Basemap selection is lacking. That's probably the most important one. In the new widget, we just need to give either URL that has the style or style in JSON format. The styles should be easily able to cover vector and raster maps (raster a bit blurry), so we could design some universal approach. I would like to keep the current flexibility and have the basemaps configurable via settings. Its just now we need to read in the basemap settings and forward them via some call to the map. It could be already done via method in BasemapPage, something like map.setBaseMap(stylename) with the style JSON loaded by a Python call, for example.
I did try to load cartago maps into the widget, but couldn't get it to work in few minutes I tried. I can get the style from URL with the API key, but not further. I would have to read on it on how to specify API key (seems like just replacing MAPBOX key was insufficient).
Two illustrative screenshots are attached. Styling I better leave to you. On the screenshots notice that we can have multiple lines representing the road (case and line); route is under the labels; POIs could have names next to them that will appear/disappear on the basis of the busyness of the map.
otsaloma
commented
7 years ago Thanks, looks good. I'll get to the voice PR latest this weekend.
In that diff, there's a lot of stuff in qml/MapMouseArea.qml -- is that temporary, doesn't it belong in your MapboxMap implementation? Also qml/PositionMarker.qml looks a bit verbose.
I did try to load cartago maps into the widget, but couldn't get it to work in few minutes I tried. I can get the style from URL with the API key, but not further. I would have to read on it on how to specify API key (seems like just replacing MAPBOX key was insufficient).
The current setup is such that all calls require the API key: JSON tiles, font glyphs, icon sprites, etc. -- see the source of cartago.io/maps/vector/ for how it's done with transformRequest with Mapbox GL JS. Maybe there's something similar in Mapbox GL native? (Also, my API-key setup is very ad-hoc, if it's too silly, I can change it.)
rinigus
commented
7 years ago Re qml/MapMouseArea.qml: I guess we'll move it to the plugin, that's right. I haven't thought about it before.
Re qml/PositionMarker.qml: Indeed, its more chatty than before. Its induced by the requirement of adding the position marker as a layer and the source of the map. I also added commented out code that just draws the position as circles to illustrate that we can get rid of one of the icons if we wish.
Re transformRequest: there seem to be mechanism for it in GL Native version, but its not exposed yet to Qt bindings. I'll see if I can make PR for exposing it for us as well.
rinigus
commented
7 years ago As a continuation regarding transformRequest: I filed an issue at Mapbox GL Native. It seems that its not too hard to fix, just one additional setting has to be exposed which has been done for other platforms. As soon as the fix is done, I was thinking to add a separate property in QML bindings, such as urlAppend, where we could specify a string that's appended to all URLs (?apikey=XXX, for example).
Meanwhile, I'll look into mouse interaction and maybe writing up documentation.
rinigus
commented
7 years ago Re transformRequest: I made a patch and submitted it upstream. All worked very nicely with Cartago when accessing from C++. Later tonight will write extension for QML module that would allow us to get Cartago (and similar) maps for Poor Maps. So, don't get surprised by few requests to vector maps :)
rinigus
commented
6 years ago I have added now support for different basemaps. There are two types - mapbox (according to the mapbox style spec, any supported type) and slippy. Basemaps are, in essence, passed as parameters to Mapbox QML item which handles download and rendering. Raster slippy maps are configured as before in Poor Maps and, when set as a basemap, are wrapped into styleJson for Mapbox GL handling.
It all works quite decently with the main shortcoming that raster tiles look best at pixelRatio = 1. That, although, reduces the size of the drawn objects (position marker, route while navigating). Not sure how much we should put effort into raster basemaps though. I have few ideas on how to fix it, will probably try a bit later.
Next, I moved mouse event handling into Mapbox GL QML plugin, as you suggested.
I will probably tinker with the few things, some in the plugin (flicking gesture), some in Poor Maps (cache settings - size and path).
I think already now we are in the good functional stage of GL version. I wonder whether I should start removing some clearly non-needed code, such as basemap download handling? I can make the cleanup gradual, so we could easily revert the commit if needed.
otsaloma
commented
6 years ago Not sure how much we should put effort into raster basemaps though. I have few ideas on how to fix it, will probably try a bit later.
I'm not even sure I want to include raster maps at all, they are perhaps an unnecessary complication both in terms of UI and code. So, I wouldn't suggest spending much time on it, unless of course that's part of MapboxMap and other clients might need it.
I wonder whether I should start removing some clearly non-needed code, such as basemap download handling?
If it looks clear to you, go ahead, but no problem if you don't, I'm such familiar with the Python code that it wouldn't be a bother for me to do it either.
rinigus
commented
6 years ago Re raster: so far, adding raster tiles involves about 10 lines of code + 10 lines of JSON - rather trivial. I'll keep the code for now, but we can remove it later as well. There is no raster-specific code in MapboxMap - Mapbox GL takes care of it all for us. But since we focus on vector maps - we don't have to fix annoyances either.
I will remove the most of raster sources then - will keep one of OSM Scout Server styles until it gets vector maps support. I hope to get OSM Scout Server supporting vector maps in the near future, just want to finish few things with rendering part.
rinigus
commented
6 years ago I think I have GL branch in the state where I wanted to get to. Over the weekend I added proper attribution display, cleaned up a bit away the code dealing with the tile downloads and cache, added cache handling on MapboxMap side and used it in Poor Maps.
The main shortcomings are DummyPage animation and Cover. They both don't show any map and I am not sure how to handle it. But I guess we can look into it when you will be ready for it.
I'll work on offline support for vector maps in OSM Scout Server unless some bugs / feature requests will come flying in.
rinigus
commented
6 years ago Hi! I am finishing the work on the first release of OSM Scout Server with Mapbox GL offline tiles and would like to release it with a client that would be able to work with it. There is still some work to do on the server, maybe few days, maybe a week. However, when done, would you mind if I will release an unofficial Poor Maps based on Mapbox GL widget?
I will rename its configuration storage settings to avoid disturbing the official version and state that this is not a permanent fork, just preview release. I am using it myself every time I need it, so there has been some testing already. As mentioned above, there are things that are not done (cover page and transition), but the main window is functional and working.
With the renaming configuration settings - have you thought about new name for GL version?
otsaloma
commented
6 years ago Sure, go ahead. I'm thinking "Whogo Maps" for the new GL version, but I think it'd better if you used neither the current nor the future name, because I might use the breaking opportunity to slightly change config file formats if there's some choices that I regret. My suggestion would be for you to go with "Poor Maps GL".
rinigus
commented
6 years ago Thank you! I'll use Poor Maps GL then. This will give sufficient room for changes in future :)
ksiazkowicz
commented
6 years ago "Rich Maps", please ._.
rinigus
commented
6 years ago Good evening! I have prepared a branch that can be merged with the current master: https://github.com/rinigus/poor-maps/tree/gl_master . Almost the same version was released as Poor Maps GL, but based on the older version of voice PR. This branch is against the latest version of voice branch that got merged by you into the master. I had a chance to test it today and all seemed to work as expected.
Since the voice PR was squashed into one commit, I had to merge all commits into one to make it possible to merge with the current Poor Maps master. If you wish to see the changes as they were done one-by-one, they are available at https://github.com/rinigus/poor-maps/commits/gl
I have not included the commit that changed the name from Poor Maps to Poor Maps GL, but its available https://github.com/rinigus/poor-maps/commit/3dddcfc9382168e4b679b50af9695d7fe2a1cffa
So, I am basically ready to submit as soon as you are ready.
otsaloma
commented
6 years ago Since the voice PR was squashed into one commit, I had to merge all commits into one to make it possible to merge with the current Poor Maps master.
Sorry if that caused trouble. I thought about whether to squash or not for a while, but ended up with the usual squashing.
So, I am basically ready to submit as soon as you are ready.
I still have the couple small fixes and a release to do before I fork a new project, so it'll probably be closer to new year.
rinigus
commented
6 years ago No problem - learned few tricks with git. I will focus then on map import, styles, and updating databases for offline maps.
otsaloma
commented
6 years ago Forked: https://github.com/otsaloma/whogo-maps
Please file a PR there.
I have started to fiddle with the code with the look on replacing Map with MapboxMap QML type. As far as I could see, there is a significant amount of code that is made around limitations of used Map component.
At present, I am planning to start replacing the components and propagate the functionality of MapboxMap throughout the code. I would like to use voice branch as a base with the hope that it would make the life easier in future, even if there are significant changes expected in that branch.
For me, its the best to start changing and tag the problems that I encounter. Already now I have seen lots of magic in poor-maps.qml with the respect of page pushing and pop'ing. But if I don't understand what's going on, I'll report it here and would keep changes minimal. Right now, I am not going to touch the page movement code.
In addition, by doing such code reading and changing the main map widget in Poor Maps, I can further develop QML API as well. For example, I am planning to add a call that would allow us to find zoom and the center of the map that ensures that all requested points are visible. Such call exists in QMapboxGL, but I haven't exposed it yet. So, when we see some issues with the QML type, maybe its better to fix it in the plugin code when possible.