Fix handling of components that only vary by descriptors#11593
Conversation
|
Web viewer built successfully.
View image diff on kitdiff. Note: This comment is updated whenever you push a commit. |
434e1c9 to
6bb4465
Compare
it is still keeping the information of the descriptor around for various practical considerations like conversion to sorbet/record-batches
6bb4465 to
d1fd07a
Compare
|
The repro case for the original example has become a bit more pathological. but it has handled correctly. import rerun as rr
rr.init("duplicate_tagged_components", spawn=True)
# These are two groupings of content with a variety of fields but at least one intersects
schema = rr.DynamicArchetype("mcap.Schema")._with_descriptor_internal(
rr.ComponentDescriptor("data", "mcap.Schema"), 1.0
)
image = rr.DynamicArchetype("SomeCustomImage")._with_descriptor_internal(
rr.ComponentDescriptor("data", "SomeCustomImage"), 2.0
)
rr.log("/camera/data", schema)
rr.log("/camera/data", image)The first descriptor wins - we viewer only shows/receives |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses handling of components that differ only by their descriptors by transitioning the codebase from indexing on ComponentDescriptor to indexing on ComponentIdentifier. The change affects query APIs, cache keys, storage structures, and UI components throughout the viewer and SDK codebases.
Key changes:
- Query APIs now accept
ComponentIdentifierinstead ofComponentDescriptorfor lookups - Internal storage structures index by
ComponentIdentifierwith descriptors stored as values - Cache keys updated to use
ComponentIdentifier
Reviewed Changes
Copilot reviewed 152 out of 153 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
rerun_py/src/arrow.rs |
Updates PendingRow component storage to use identifier-based indexing |
examples/rust/extend_viewer_ui/src/main.rs |
Updates component UI queries to pass identifiers |
examples/rust/custom_view/src/points3d_color_visualizer.rs |
Extracts component identifiers from descriptors for queries |
| Multiple descriptor snippet files | Updates test code to use new component access patterns |
| Viewport/view blueprint files | Converts descriptor-based lookups to identifier-based |
| Query cache and storage files | Major refactoring of indexing from descriptors to identifiers |
Comments suppressed due to low confidence (3)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
grtlr
left a comment
There was a problem hiding this comment.
Wow, this is already looking much nicer 🤩!
All of my comments are minor and only suggestions / notes to myself. Thank you for going through the effort of starting to make these changes.
crates/store/re_chunk/src/batcher.rs
Outdated
| pub fn new(timepoint: TimePoint, components: IntMap<ComponentDescriptor, ArrayRef>) -> Self { | ||
| pub fn new( | ||
| timepoint: TimePoint, | ||
| components: IntMap<ComponentIdentifier, (ComponentDescriptor, ArrayRef)>, |
There was a problem hiding this comment.
Now I wonder if rustc will monomorphize the from_iter for IntMap into the same function as this (probably not).
There was a problem hiding this comment.
Don't think so either. You're wondering whether the from_iter shortcut might be harmful?
|
|
||
| let results = | ||
| cache.latest_at(&query, &descr.entity_path, [&component_descriptor]); | ||
| cache.latest_at(&query, &descr.entity_path, [descr.component]); |
There was a problem hiding this comment.
The deletion above is haunting me, but I guess that was just an early exit? Looking at the diff, we could have always just passed descr along.
Or did that line just become trivial because of @IsseW's recent refactor?
There was a problem hiding this comment.
I believe recently we had to add a brunch of descriptor lookups because of the chnage to component target, and those are gone now again because the descriptor is available in a more obvious fashion
don't see why it would be needed since we're asking the same store on the latest_at query then for the existance
| } | ||
| } | ||
|
|
||
| // TODO(andreas): Remove this variant, we should let users construct `SerializedComponentColumn` directly to sharpen semantics. |
There was a problem hiding this comment.
Yes please (but not now, let's get this merged)!
Related
ComponentIdentifierinstead ofComponentDescriptorindexing into chunks #10460What
... by biting the bullet and pretty much everything that so far indexes of
ComponentDescriptorinstead index overComponentIdentifier.I'd like to believe this also improves performance, but I haven't proven it.
There's a lot of access that goes
descriptor_THING().componentnow which strongly implies that we should generatecomponent_THING()instead. But I've left that as an exercise for the reader.Commit by commit if you're into this thing - every successive commit is curated to walk up the stack of crates
TODO:
Needed follow-ups:
component_THING()methods (use them inside ofdescriptor_THING()).componentsillynesscomponent_THINGfor queries in visualizers -> solve UseComponentIdentifierinstead ofComponentDescriptorindexing into chunks #10460 for good