Repository navigation
[QMS-1280] Show surfaces and way types of tracks from OpenStreetMap data - #1282
Conversation
|
Huh, this is a huge one. As I am currently traveling this will take quite some time to review.Please be patient. |
|
Rebased on current dev. |
|
@switch87: I compiled this PR on Windows 11 without any trouble using the existing build environment. No additional step for use of Qt Concurrent required. I tested it with a track in Luxembourg. Way type and surface type are shown as expected. Nice new feature proposal! |
|
@wthaem Thanks for building and testing it on Windows, good to know Qt Concurrent needs no extra step there. |
|
I had some time to test, too. Nice feature! From the first smoke test: Surface needs it's own tab in the track details dialog and probably it should be done in a scroll view. Reason: For small screens the splitter with the tab widget get to large in height. |
|
@kiozen Thanks for testing it and for the feedback! Good point about small screens: I'll move the surface view into its own tab in the track details, inside a scroll area. |
|
Done in 468b5cd: the surface view now has its own "Surface" tab next to Info, inside a scroll area, so the Info tab is back to its old height. I haven't touched the .ts files; the new tab title is a plain string in the .ui. |
|
There are some compile issues now 😉 |
|
Sorry about that, I pushed the tab change without building it. Fixed in 564c7e5: uic creates the widget inside a scroll area without a parent, so |
Good question. Somehow establishing tests was never really done in this project. And I fear they get outdated over time as they will be ignored when updating the code. My current best practice is to create test on the fly by AI when tinkering on a complex topic. And remove them. So unless you say the test are really important for future changes I would tend to remove them.
Keep it if you keep the test.
I think Qt Concurrent is fine.
The established concept for those paths is a list of paths. The idea is to have data on multiple disks. e.g. some everyday usage data on your internal disk and huge datasets on an external disk. This is the same for Routino, maps and DEM.
Yes that's fine. If that is really an issue later it can be changed.
Yes, that's fine
Yes, that's fine
No that's fine. If we are done with the review squash it again. Or amend the commit. |
|
Some remarks on the UI:
|
|
These are the results of an AI review: I did not verify them. So take them with quite a grain of salt. |
|
Thanks for passing the review on, even unverified it was worth it: most points were real. I checked each one against the code:
Built (Debug, |
|
Your other UI remarks:
Built (Debug, |
|
Here another review round (AI generated): Likely bugs
Performance Breaks CLAUDE.md rules And we need better icons 😄 I attached a few suggestions. But if you have better ones in mind go ahead. surface_icons.zip README_ICON.md tells you how to add icons. |
|
Somehow my last comment is not visible on the web UI:
|
|
Thanks for the second round, and for the icons! I went through each point against the code:
Icons: I took two of yours. I also ran clang-format over Built (Debug, |
|
Ok, we are getting close. Another batch: Let's see if these are still relevant. |
|
Thanks, all ten were still relevant. Pushed as separate commits:
Debug build with |
|
And more 😄 Looks like the agents now got pretty deep into the rabbit's den. @JFritzle can you check the "Packaging and platform" section? |
|
About the unit tests: I'd like to keep them in this PR. In more than ten years as a developer, especially on projects where several people work together, I've seen that projects with tests run into far fewer problems with existing code later on. So I don't remove tests myself. Once it's merged, what you do with them is of course your call. |
|
Thanks, this round went deep indeed 😄 I checked every point against the code, the unchecked leads too. Each fix has a test where it can be tested outside the GUI. Functional bugs
Performance
Bad PBF input
Cleanup
Parser and database (leads)
Analysis and threads (leads)
GUI (leads)
Packaging and platform (@JFritzle, I'll leave the Windows/macOS decisions to you)
Tests: new tests for the leg changes, the name of a picked extract, the canceled job, Built (Debug, |
|
Now it's getting to the bottom: I have to check with a Garmin later. I don't really think surface is needed for tracks on devices. The usual process is to copy them from the device into a project before you do anything. Not sure if you want to address 3 as it's not part of this PR. It's just there because I compare against your dev branch which is not 100% up-to-date. |
|
Thanks, I went through all nine against the code. Checked
From your unchecked list
Tests: new or extended tests for the road types (unit test plus the reference table), the node pass in sorted, required-sorted and unsorted files (block count included), the optional header features, and the translated errors (a test translator that marks every text, including the reason inside a "Broken PBF file" message). Each one fails without its fix. Line coverage of the surface core ( Built (Debug, |
|
As expected this works perfectly with tracks on a Garmin, too. @wthaem I assume as you already tested the patch that SQLite on Windows has no problems with the R*tree. Right? @switch87 I think we are done with the review. Cleanup the commit history and what ever else you want to clean up and I can merge it. |
|
@kiozen: Rtree is used to show POIs of versions 2 and 4 and most likely with the surface commit I used for a first test. I didn't see any issues arising from Rtree handling using these features. |
CSurface maps a way's highway, surface, tracktype and footway tags to a surface class (paved, semi-paved, unpaved, unknown), a detailed surface and a way type, filling missing tags with the usual assumption for the highway type. CSurfaceDb reads a surface database (SQLite, R*Tree over chunks of up to 16 way segments) and finds the way at a position as the majority of the ways sampled every ~10 m in its ~20 m cell, with the nearest way within a given distance as fallback. CSurfaceDbSet uses several databases together, switched on and off by their full path. CSurfaceDbBuilder writes a surface database from an .osm.pbf extract, the same file Routino databases are built from. It reads the file twice (highways, then the locations of their nodes) with CPbfReader, a small reader for zlib compressed PBF blocks, dense nodes and ways, so no new library is needed. Belgium (700 MB) takes ~40 s and ~300 MB of memory. qms_surfacedb builds a database from the command line, with a man page. Whenever a track's points change, CSurfaceTrk samples it every 20 m in a background job (CSurfaceAnalyzer), looks up the way at each sample in the active surface databases (CSurfaceDbManager) and sums the lengths per surface class, way type and detailed surface. A single sample that differs from two equal neighbours is taken for a crossing and smoothed. The result is derived data like slope and is not saved; an unchanged track reuses it at once, and a database change reanalyzes only the tracks its area covers. The Surface tab of the track details, placed after Activity and remembered by its name so that a new tab never shifts it, shows two stacked bars with a legend (surface classes and way types, length and share) and a collapsible list of the detailed surfaces, or how to build a database when none covers the track. The track point tooltip names surface and way type, and the tab says so when the database covering the track is switched off or a device track was never analyzed. The map draws each leg in the colors of its samples, so it shows what the Surface tab counts; a stretch without a classification keeps the track's ordinary color. Tool -> Create Surface Database opens a tab to build databases in a worker thread (inputs locked while building, cancel is no error) and to choose the active ones; the Routino router panel links to it. "Surface*" and "Way Type*" become color sources of a track. They are categories, so they are drawn with a fixed color per category instead of the gradient, and the legends on the map and in the Style tab list the categories.
|
@kiozen Done. The branch is now one commit on top of current The tree is unchanged — The commit message was brought up to date with the design the review changed: the surfaces live in their own Surface tab after Activity (not below the comment in the Info tab) and are remembered by their name, a database change reanalyzes only the tracks its area covers, the map draws each leg in the colors of its samples, Nothing else turned up to clean: no debug output in the diff, and the only new |
|
Thx! |
What is the linked issue for this pull request:
QMS-#1280
What you have done:
Surfaces and way types for every track, from a per-region surface database built from an OSM extract (
.osm.pbf, the same file Routino databases are built from).CSurfaceDbBuilderreads the extract with a small own PBF reader (CPbfReader: zlib blocks, dense nodes, ways — no new library) and writes an SQLite file with an R*Tree over chunks of up to 16 way segments; tags are stored unchanged, classification happens when reading. Belgium: 205 MB, ~40 s, ~300 MB memory. GUI: Tool → Create Surface Database (worker thread, progress, cancel, choose active databases; also linked from the Routino panel). CLI:qms_surfacedb.CSurface): highway/surface/tracktype/footway → paved / semi-paved / unpaved / unknown, a detailed surface and a way type; missing tags get the usual assumption for the highway type.CSurfaceDb,CSurfaceDbSet): the way at a position is the majority of the ways passing its ~20 m cell, nearest way within 25 m as fallback.CSurfaceAnalyzer,CSurfaceTrk,CSurfaceDbManager): sample every 20 m in a background job (Qt Concurrent, own pool, own DB connection per job, stale jobs cancelled), smooth single-sample crossings, sum km per class, way type and detailed surface. Derived data like slope, not saved.BUILD_TESTING(off by default) with four QtTest tests run by ctest (classification table, PBF reader/builder/lookup on a tiny extract, analyzer, and a real 28 km hike that is skipped without external data).New build requirement: Qt6::Concurrent (qtbase); Qt6::Test only with
BUILD_TESTING=ON. SQLite needs the R*Tree module — present in the system SQLite on Linux; not tested with the SQLite Qt bundles on Windows/macOS.Tested on Linux (Gentoo, Qt 6.11.2, GCC 15): builds, all 4 tests pass, clang-format, clang-tidy and clazy (level 0/1) clean on the new code. The same feature runs as a patch on 1.21.1 in daily use. Not tested on Windows or macOS. No
.tsfiles touched; a Dutch translation (~70 strings) is ready if wanted.Questions where I'd like your preference:
BUILD_TESTINGoption with QtTest welcome, or should the tests go?test/data/osmsurface.py, GPL-3.0-or-later) as source of the reference tables — keep it or only the generated tables?QThread?Surface/next to the first Routino path, else the user data folder — fine, or a setting of its own?CSurface(data colours like the activity colours) — OK with rule 1 of README_CODING?qms_surfacedbnext to theqmt_*tools, built underBUILD_QMAPSHACK— OK?qint8fields per track point inCTrackData::trkpt_t— acceptable?Steps to perform a simple smoke test:
-DBUILD_TESTING=ON, build, runctest.Does the code comply to the coding rules and naming conventions Coding Guideline:
Is every user facing string in a tr() macro?
Did you add the ticket number and title into the changelog? Keep the numeric order in each release block.