-
Notifications
You must be signed in to change notification settings - Fork 312
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
Map Space View and GPS Coordinates archetype (Mapbox/OSM support) #6561
base: main
Are you sure you want to change the base?
Conversation
Awesome work! Sorry if it's a noob question, but how do I check this out?
|
New rerun needs rust version 1.79 while you have only 1.76. Update your rust toolchain and try it again. |
Ah, needed to update the |
Few changes we discussed:
|
I talked this one over with some folks and the desire is to keep ViewCoordinates separate and somewhat dedicated to interpretation of camera-orientation, as distinct from data semantics. I'm going to put together a proposal for a new archetype, probably named something like CRS (CoordinateReferenceSystem). We can start with this somewhat dedicated to the mapping use-cases, but I think it could be helpful for other things like CAD import where it might be useful to carry context like unit-sizes. |
Here's my proposal for adding a very simple CRS type as an indicator: #6601 |
The new The current cargo tree:
In
|
What is left from what we discussed:
For 1, I'm open to your suggestions. Should the feature be enabled or disabled by default? Either way, how should the CI/CD integration work? Should we test all features by default or test combinations of features? |
1b5568f
to
12d432d
Compare
@tfoldi nice work! Excited to see it merged :) |
Extended the nuScenes example with MapView to have at least one example using the new view: nuscenes_mapview.mp4 |
would this support plotting a scatter of points across the map (instead of a single point/track) ? from the PR seems yes a la |
yes, it does support multiple points |
good to know ! curious if team/egui has evaluated integrating cesium -> its rendering pipeline for large set of N points may be faster |
Hey, still very keen to see this. What's the current blocker? |
The same from my side, will it be continued? |
I’m open to continuing this, but it's largely dependent on the Rerun team's priorities. As you may have seen from the PR history, I tried to keep it up-to-date for a few versions but eventually paused due to the complexity involved. There are many moving parts in Rerun, and with so much auto-generated code and changing internals, maintaining consistency across view-related PRs isn’t straightforward. I received two key comments from @jleibs:
On June 22nd, I asked a few questions regarding these points, but since I didn’t receive any responses, so I paused further updates. That said, I’m happy to rebase the PR to the latest main branch if there’s a realistic chance it could be merged. |
@tfoldi First, thanks again for your contribution, and for putting up with our slow responses so far. With 0.19 out and the huge push on the dataframe and video fronts, we are now ready to focus on other areas—and this one is high up our list. Starting on Monday, I'll make it my priority to help you land this PR. Would you be willing to rebase it to latest main? My plan would then be to dive in, give it a thorough review, and help (including hands-on) to land it sooner than later. As for the "feature flag": yes we want one, that is disabled by default (at least initially). This will give us more confidence to land stuff before they are 100% polished. To be clear, by "feature flag", we don't necessarily mean a Cargo-style feature, but a global application setting which dynamically enable/disable the map view in the viewer. That's often how we signify to users that a particular view is "in beta". (For illustration, I recently removed the feature flag we had on the dataframe view in this PR). |
21cfac9
to
2f4d463
Compare
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.
This generally looks in excellent shape. Thanks for this contribution! I'm not yet completely done with my review, here are already some comments.
crates/store/re_types/definitions/rerun/blueprint/archetypes/map_options.fbs
Outdated
Show resolved
Hide resolved
crates/store/re_types/definitions/rerun/blueprint/components/secrets.fbs
Outdated
Show resolved
Hide resolved
crates/store/re_types/definitions/rerun/blueprint/views/map.fbs
Outdated
Show resolved
Hide resolved
crates/viewer/re_component_ui/src/datatype_uis/singleline_string.rs
Outdated
Show resolved
Hide resolved
@@ -12,6 +12,7 @@ | |||
from nuscenes import nuscenes | |||
|
|||
from .download_dataset import MINISPLIT_SCENES, download_minisplit | |||
from .export_gps import derive_latlon |
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.
Very nice addition to this example. However, please open a separate PR for this, as I'm a bit worried this will create CI issue due to how we're initially going to deal with cargo features.
@@ -135,6 +135,7 @@ Update instructions: | |||
| re_space_view | Types & utilities for defining Space View classes and communicating with the Viewport. | | |||
| re_space_view_bar_chart | A Space View that shows a single bar chart. | | |||
| re_space_view_dataframe | A Space View that shows the data contained in entities in a table. | | |||
| re_space_view_map | A Space View that shows geospatial data on a map. | |
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.
Note to self: update the architecture diagram accordingly
@@ -11,7 +11,7 @@ namespace rerun.archetypes; | |||
table Points3D ( |
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.
As discussed, this is probably no the right archetype to initially support. Either Point2D or some custom type.
tiles: Option<HttpTiles>, | ||
map_memory: MapMemory, | ||
selected_provider: MapProvider, | ||
access_token: String, |
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.
I think selected_provider
and access_token
are not really necessary here. We usually try to keep view states as lean as possible. My suggestion:
- remove these two fields
- make
map_memory
anOption
- update
map_memory
fromMapViewClass::ui()
Also, I'd either inline ensure_and_get_mut_refs
or somehow move it to MapViewClass
.
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.
selected_provider
and access_token
are used to detect if I need to invalidate the tiles cache. what is the best way to check in ui()
what parameters have been changed if not comparing the state with the new blueprint values?
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.
You're right. Then let's just add a comment above selected_provider
to indicate that it is used for invalidation.
); | ||
|
||
map_widget.double_clicked().then(|| { | ||
map_memory.follow_my_position(); |
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.
Our usual semantics for double clicking is zoom to the "bounding box", e.g. whatever area that contains the entire view content.
I haven't looked into it yet. If it is involved, we can keep that for a follow-up PR though.
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.
double click reset the view (re-enable follow mode). We can change the zoom level as well, but if you have only point finding what zoom level the user expects is tricky imu
// --- Optional --- | ||
|
||
/// Zoom level for the map. The default is 16. | ||
zoom: rerun.blueprint.components.ZoomLevel ("attr.rerun.component_optional", order: 2000); |
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.
In addition to zoom level, it would be extremely valuable to store in the blueprint the scroll position (coordinate of center point?). In particular, that would make it settable from code.
If that's too involved, we can keep it for a follow-up PR.
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.
absolutely possible, but the map center can be more than a position:
- it can be in follow mode, which is useful if you have one or interconnected points. here the first logged point is always in the center (like in the embedded video on the PR description)
- we can fix the map at a certain lat, long position
what archetypes should I use to store this? in the lib if the map center is None
then it is in the following mode; otherwise, it is fixed. but I assume this approach is not idiomatic in rerun
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.
According to your last comment, the center_position
should be DVec2D
, and follow_mode
should be a new enum or a boolean with a well-named entry to keep the number of components down.
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.
My gut feeling would be to minimise the feature set for this PR and skip any sort of "auto-scroll" mode. The MVP would be:
CenterPosition
view property (datatype:DVec2D
) to store the center location along with the zoom level- Best effort implementation for the fallback provider of
CenterPosition
based on whatever data is logged
In the future, we indeed need to devise a set of more advanced auto-scroll/follow mode, but I think the hindsight we'll gain from landing this PR will help designing that properly. (Ideally, it'd be somewhat consistent with the 2D view too.)
map_windows::acknowledge(ui, &window_id, &map_pos, tiles.attribution()); | ||
|
||
// update blueprint if zoom level changed from ui | ||
if map_memory.zoom() != *zoom_level { |
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.
see my other note, would be great to store the map position in the blueprint as well
I just discussed the data model aspect of this PR with @jleibs and we reached the following decision. We introduce a new
This PR will have to introduce the For the scope of this PR, the recommended/optional components should be included only so long as they are actually supported by the view. If they are not, then it's fine to drop them—we can add them in a subsequent PR. In the future, we will further add a Motivation for this decision:
|
we can support text labels. However, shouldn't you be able to change their fonts, sizes, and colors? |
No, I don't think this is required for this PR (and the short term future tbh). IWe don't support styling of labels in other views either. |
Let's make sure we reference https://epsg.io/4326 in the |
What
Map Space View for GNSS/GPS data logged through new GPS Coordinates archetype.
rerun_mapview.mp4
re_space_view_map
crate for visualizingPoints3D
with gps coordinates.map_visualizer_system.rs
implements theVisualizerSystem
trait. It has only one meaningful function, that takes thePositions3D
components and turns intovec<MapEntry>
array along with optionalColor
andRadius
components. Thisvec<MapEntry>
is the only field of theMapVisualizerSystem
.map_space_view.rs
is where the magic happens. It useswalkers
slippy map implementation to show an OpenStreetMap or Mapbox map. On theselection_ui
you can change the map providers. To view Mapbox maps, you need to set mapbox tokens (you can get free tokens from Mapbox's site) and pass it via the env variable (MAPBOX_ACCESS_TOKEN
) or via the blueprintmap_windows.rs
is a small helper to show the zoom controls and acknowledgment on the UIs.MapProvider
component (enum with providers+styles), and aMapOptions
archetype.The logging of data is performed as follows:
The blueprint is defined as follows:
The PR is not perfect, but the view is usable.
Please review the changes and evaluate their potential utility for your needs. If you don't like, prefer not to merge just close it - no hard feelings.
To be discussed
f32
instead off64
. This makes it less precise than/NavSatFix
Checklist
To run all checks from
main
, comment on the PR with@rerun-bot full-check
.