feat(gps): add track map, lap detection, and map tiles - #83
Conversation
|
@irudoy This is incredibly cool and such a great idea. Give me a day or so to review this and run my tests but I am excited to merge this in! |
There was a problem hiding this comment.
Pull request overview
Adds a GPS-focused right-side data panel with track visualization, lap detection, color mapping, and optional cached map tiles.
Changes:
- Adds GPS detection, track sanitization, lap analysis, and interactive map rendering.
- Introduces reusable data-panel widgets and persisted map settings.
- Extends MLG parsing, fixtures, diagnostics, localization, and release tooling.
Reviewed changes
Copilot reviewed 40 out of 45 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
tests/parsers/speeduino_tests.rs |
Tests GPS-enabled MLG parsing. |
tests/parsers/romraider_tests.rs |
Strengthens unit extraction assertions. |
src/ui/widgets/track_map.rs |
Implements the Track Map widget. |
src/ui/widgets/mod.rs |
Defines the widget registry and contract. |
src/ui/mod.rs |
Exports data-panel modules. |
src/ui/data_panel.rs |
Implements the right-side widget host. |
src/ui/chart.rs |
Restricts chart scrolling to its layout area. |
src/tiles.rs |
Adds tile providers, workers, and caching. |
src/state.rs |
Adds map, panel, cache, and tab state. |
src/settings.rs |
Persists tile and widget preferences. |
src/parsers/speeduino.rs |
Supports hexadecimal MLG bitfield IDs. |
src/lib.rs |
Exports new GPS-related modules. |
src/laps.rs |
Adds GPS sanitization and lap detection. |
src/ipc/handler.rs |
Clears map coloring on channel deselection. |
src/colormap.rs |
Adds Viridis and Turbo colormaps. |
src/bin/inject_fake_gps_mlg.rs |
Adds an MLG GPS-generation utility. |
src/app.rs |
Integrates panel rendering and map state. |
src/adapters/registry.rs |
Versions refreshed specification maps. |
src/adapters/mod.rs |
Re-exports specification generation. |
scripts/build-release.sh |
Rebuilds macOS bundles before installation. |
i18n/zh-CN.yaml |
Adds Chinese panel/map translations. |
i18n/ur.yaml |
Adds Urdu panel/map translations. |
i18n/ru.yaml |
Adds Russian panel/map translations. |
i18n/pt-PT.yaml |
Adds Portuguese translations. |
i18n/pt-BR.yaml |
Adds Brazilian Portuguese translations. |
i18n/ja.yaml |
Adds Japanese panel/map translations. |
i18n/it.yaml |
Adds Italian panel/map translations. |
i18n/id.yaml |
Adds Indonesian panel/map translations. |
i18n/hi.yaml |
Adds Hindi panel/map translations. |
i18n/fr.yaml |
Adds French panel/map translations. |
i18n/es.yaml |
Adds Spanish panel/map translations. |
i18n/en.yaml |
Adds English panel/map strings. |
i18n/de.yaml |
Adds German panel/map translations. |
i18n/bn.yaml |
Adds Bengali panel/map translations. |
i18n/ar.yaml |
Adds Arabic panel/map translations. |
examples/inject_fake_gps_mlg.rs |
Adds a track-shaped GPS fixture generator. |
examples/check_gps_lookup.rs |
Diagnoses canonical GPS metadata lookup. |
examples/check_gps_in_log.rs |
Diagnoses GPS detection in parsed logs. |
docs/FORMAT_SPECIFICATIONS.md |
Documents hexadecimal bitfield IDs. |
Cargo.toml |
Registers the utility and JPEG decoding. |
Cargo.lock |
Locks JPEG decoder dependencies. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let is_lat = canonical.as_deref() == Some("gps_latitude") | ||
| || (canonical.is_none() && matches_gps_name(&name, GpsAxis::Lat)); | ||
| let is_lon = canonical.as_deref() == Some("gps_longitude") | ||
| || (canonical.is_none() && matches_gps_name(&name, GpsAxis::Lon)); |
| fn icon_button( | ||
| ui: &mut egui::Ui, | ||
| size: f32, | ||
| draw_icon: fn(&egui::Painter, egui::Rect, egui::Color32), | ||
| ) -> egui::Response { | ||
| let (rect, resp) = ui.allocate_exact_size(egui::vec2(size, size), egui::Sense::click()); |
| if let Some(pointer) = response.hover_pos() | ||
| && let Some(record) = nearest_record(&points_screen, pointer, &project_record) |
| st.color_channel = Some(i); | ||
| st.color_min = None; | ||
| st.color_max = None; |
| #[derive(Debug, Clone)] | ||
| pub struct TrackMapState { | ||
| pub enabled: bool, | ||
| pub height_frac: f32, |
| // `drag_stopped`/`lost_focus` are unreliable on float Sliders | ||
| // (they only fire when keyboard-focused or when egui detects a | ||
| // drag start, neither of which happens for a single-click + | ||
| // hold-and-drag). `changed()` fires reliably on every value | ||
| // change. The settings file is ~300 bytes - even a 60 fps drag | ||
| // costs <1 ms/save on modern SSDs. | ||
| opacity_committed = slider_resp.changed(); |
|
@irudoy could you review the copilot feedback and adjust as needed? |
SomethingNew71
left a comment
SomethingNew71
left a comment
There was a problem hiding this comment.
Thanks for this PR — it's an impressive feature and a genuinely clean implementation. I did a full security pass over the diff (network surface, dependencies, build script, binary fixtures) and found nothing concerning; the tile code in particular is carefully bounded and gated. The inline comments below are supplementary to the Copilot review — mostly cleanup, plus one tile-policy item. Feel free to fold them into the rounds you're already working through.
| /// Number of parallel HTTP workers. Tile servers have per-IP concurrency | ||
| /// limits; 4 is a polite default that still hides per-tile latency on | ||
| /// high-zoom panning. | ||
| const WORKER_COUNT: usize = 4; |
There was a problem hiding this comment.
The OSM tile usage policy caps apps at 2 simultaneous download connections, and all 4 workers in this shared pool can hit tile.openstreetmap.org at once. Apps that exceed the policy get blocked by IP, which would affect every UltraLog user at once. Please add a per-provider concurrency cap (2 for OSM). Esri being the default softens this, but OSM is one click away in the provider dropdown.
| name = "test_parser" | ||
| path = "src/bin/test_parser.rs" | ||
|
|
||
| [[bin]] |
There was a problem hiding this comment.
This registers a dev-only tool as a first-class binary, and it duplicates examples/inject_fake_gps_mlg.rs — the two copies have already diverged (Moscow oval vs. the Vladivostok kart circuit). Please keep just the examples/ version and drop this [[bin]] so the tool isn't built into release artifacts.
Also, the doc comment in the src/bin/ copy references firmware/Lib/demo_gen.c in "the canlogger repo", which isn't part of this project — please remove or clarify that reference in whichever copy survives.
|
|
||
| #[test] | ||
| fn test_megasquirt_gps_log_supports_hex_bitfield_and_consistent_offsets() { | ||
| let file_path = "exampleLogs/megasquirt/2026-04-12_12.49.36_gps.mlg"; |
There was a problem hiding this comment.
This test only reads the 1.5 MB MegaSquirt fixture, but the PR adds ~8.4 MB of binary fixtures overall (rusefilog_gps.mlg alone is 5 MB), and git history keeps them forever. Since the injector tool ships with the PR and the source logs are already in the repo, please drop the fixtures no test exercises — or generate them on demand from the existing logs.
| app.user_settings.tile_opacity = new_tile_opacity; | ||
| dirty = true; | ||
| } | ||
| if dirty && let Err(e) = app.user_settings.save() { |
There was a problem hiding this comment.
The ..self.user_settings.clone() spread you added in eframe::App::save already persists these fields via eframe's auto-save (~30 s) and the shutdown save. With that in place, this explicit save — which runs on every changed() tick of the opacity slider, i.e. every frame of a drag — is redundant disk churn. The in-memory mutation is enough here; same for the save() call in data_panel.rs.
| fn is_available(&self, app: &UltraLogApp) -> bool { | ||
| let available = detect_gps_channels(app).is_some(); | ||
| if !available { | ||
| cancel_tile_requests(); |
There was a problem hiding this comment.
is_available() is a query, but this gives it a side effect — and it runs every frame via data_panel_has_content(). maintain_tile_source already calls cancel_pending() whenever tile_rendering_is_active() is false, so this looks redundant. Removing it keeps the trait method pure.
| let prev_tile_provider = st.tile_provider; | ||
| let prev_tile_grayscale = st.tile_grayscale; | ||
| let prev_tile_opacity = st.tile_opacity; | ||
| ui.checkbox(&mut st.tiles_enabled, t!("track_map.tiles.show")); |
There was a problem hiding this comment.
Nice-to-have, not blocking: enabling tiles sends the track's approximate location (tile coordinates) to Esri/OSM. The opt-in default is exactly the right call. A one-time note or confirm the first time a user enables the satellite background would match UltraLog's privacy posture.
|
@irudoy Actually you know what I have some time I can work on some of these feedback items. I will go ahead and merge into the release branch and help out with fixing some of this feedback. Thank you SO much for this contribution. :-) It will be in the 2.13.0 release |
edeec6e
into
ClassicMiniDIY:release/2.13.0
Copilot review: - GPS alias fallback is now independent of the registry canonical-ID result, so a refreshed spec cannot break detection of exact aliases - data-panel icon buttons register an accessible button label (AccessKit) and remain keyboard-activatable - map hover now scrubs the chart cursor/timeline while playback is stopped, matching the promised hover synchronization - track coloring gains editable min/max range controls with an Auto reset, matching the promised configurable ranges - removed the unused TrackMapState::height_frac - the opacity slider no longer writes settings.json every drag frame Maintainer review: - OSM tile fetches are capped at 2 concurrent connections per the OSM tile usage policy, via per-provider FetchPermits - tile/widget preferences follow the settings persistence contract as live UltraLogApp fields synced by eframe::App::save; the UI never saves to disk directly - removed the duplicate src/bin fake-GPS injector and its [[bin]] registration; the examples/ copy survives with the stray canlogger reference removed - dropped the three binary fixtures no test exercises (~7 MB); the diagnostic example now defaults to the remaining MegaSquirt fixture - TrackMapWidget::is_available is a pure query again - first tile enable shows a one-time privacy toast, localized in all 15 languages, noting the track location is shared with the provider CLAUDE.md documents the new modules, the tile fetching contract, and the expanded settings persistence list.
Copilot review: - GPS alias fallback is now independent of the registry canonical-ID result, so a refreshed spec cannot break detection of exact aliases - data-panel icon buttons register an accessible button label (AccessKit) and remain keyboard-activatable - map hover now scrubs the chart cursor/timeline while playback is stopped, matching the promised hover synchronization - track coloring gains editable min/max range controls with an Auto reset, matching the promised configurable ranges - removed the unused TrackMapState::height_frac - the opacity slider no longer writes settings.json every drag frame Maintainer review: - OSM tile fetches are capped at 2 concurrent connections per the OSM tile usage policy, via per-provider FetchPermits - tile/widget preferences follow the settings persistence contract as live UltraLogApp fields synced by eframe::App::save; the UI never saves to disk directly - removed the duplicate src/bin fake-GPS injector and its [[bin]] registration; the examples/ copy survives with the stray canlogger reference removed - dropped the three binary fixtures no test exercises (~7 MB); the diagnostic example now defaults to the remaining MegaSquirt fixture - TrackMapWidget::is_available is a pure query again - first tile enable shows a one-time privacy toast, localized in all 15 languages, noting the track location is shared with the provider CLAUDE.md documents the new modules, the tile fetching contract, and the expanded settings persistence list.
|
Glad I could help! I built this feature for my own use some time ago and have been using it ever since, so I was happy to contribute it upstream. Thank you for creating and maintaining UltraLog, and for taking the time to address the review feedback, refine the implementation, and include it in the 2.13.0 release. I really appreciate it! |
Description
Adds an extensible right-side data panel and a GPS Track Map that appears when a parsed log contains recognized latitude and longitude channels.
Track visualization
Optional map tiles
Supporting changes
0x10through0x12while retaining compatibility with the previous decimal interpretation.Type of Change
Related Issues
N/A
Testing
cargo test --all-targetscargo test --doccargo clippy --all-targets -- -D warningscargo fmt --all -- --checkgit diff --check./scripts/build-release.sh --install; bundle architecture and ad-hoc signature verified.Screenshots
Checklist