-
Notifications
You must be signed in to change notification settings - Fork 447
Use ComponentDescriptor
in HybridLatestAtResult
#9938
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
base: main
Are you sure you want to change the base?
Conversation
Actually, `re_view_dataframe` still uses `ComponentNameSet`
Web viewer built successfully. If applicable, you should also test it:
Note: This comment is updated whenever you push a commit. |
@@ -79,15 +79,15 @@ impl ContainerBlueprint { | |||
// This is a required component. Note that when loading containers we crawl the subtree and so | |||
// cleared empty container paths may exist transiently. The fact that they have an empty container_kind | |||
// is the marker that the have been cleared and not an error. | |||
let container_kind = results.component_instance::<ContainerKind>(0)?; | |||
let container_kind = results.component_mono::<ContainerKind>()?; |
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.
Can we use component_instance
and component_mono
interchangeably?
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.
IIRC the difference is that some (.. not the ones here I figure) component_mono
implementations also will check whether a component was truly mono and allow you to issue a warning
@@ -41,14 +41,19 @@ pub struct HybridRangeResults<'a> { | |||
impl HybridLatestAtResults<'_> { | |||
/// Returns the [`UnitChunkShared`] for the specified [`re_types_core::Component`]. | |||
#[inline] | |||
pub fn get(&self, component_name: impl Into<ComponentName>) -> Option<&UnitChunkShared> { | |||
// TODO(#6889): This method seems to be unused? |
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.
Should we remove it?
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.
yes, please! :)
the get_by_name
on LatestAtResults
says it wants to be removed -> you have to remove this one ;-)
90a4af1
to
99bd5f8
Compare
let component_name = component_name.into(); | ||
self.overrides | ||
.get_by_name(&component_name) | ||
.or_else(|| self.results.get_by_name(&component_name)) | ||
.or_else(|| self.defaults.get_by_name(&component_name)) | ||
} | ||
|
||
// TODO(#6889): Right now, fallbacks are on a per-component basis, so it's fine to pass the component name here. |
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.
added to the list of things to do on that ticket
@@ -79,15 +79,15 @@ impl ContainerBlueprint { | |||
// This is a required component. Note that when loading containers we crawl the subtree and so | |||
// cleared empty container paths may exist transiently. The fact that they have an empty container_kind | |||
// is the marker that the have been cleared and not an error. | |||
let container_kind = results.component_instance::<ContainerKind>(0)?; | |||
let container_kind = results.component_mono::<ContainerKind>()?; |
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.
IIRC the difference is that some (.. not the ones here I figure) component_mono
implementations also will check whether a component was truly mono and allow you to issue a warning
Related
MaybeTagged
accessor utility #9929.What
Title.