-
Notifications
You must be signed in to change notification settings - Fork 373
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
Fix heuristic object properties being broken in some cases / fix DepthMeter being ignored sometimes #4679
Conversation
@@ -186,7 +186,7 @@ fn update_depth_cloud_property_heuristics( | |||
properties.backproject_radius_scale = EditableAutoValue::Auto(1.0); | |||
} | |||
|
|||
entity_properties.update(ent_path.clone(), properties); | |||
entity_properties.overwrite_properties(ent_path.clone(), properties); |
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 seems like the opposite of what we want. If an entity has both a depth heuristic and a pinhole heuristic, only one of these is going to end up being applied.
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 don't think that's true. Each time we first retrieve properties
via entity_properties.get
which will have the updated value. None of the things we set here overwrite each other directly
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.
Ahh -- I needed to expand to see the context that this is a read-edit-write loop and not just a blind update. Still feels fragile, but as I've mentioned, hopefully this is all going away soon enough when we split properties into their own components.
@@ -186,7 +186,7 @@ fn update_depth_cloud_property_heuristics( | |||
properties.backproject_radius_scale = EditableAutoValue::Auto(1.0); | |||
} | |||
|
|||
entity_properties.update(ent_path.clone(), properties); | |||
entity_properties.overwrite_properties(ent_path.clone(), properties); |
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.
Ahh -- I needed to expand to see the context that this is a read-edit-write loop and not just a blind update. Still feels fragile, but as I've mentioned, hopefully this is all going away soon enough when we split properties into their own components.
What
The way we updated properties for auto-property values made it pretty much random (not literally, but feels as-if) when and when not we'd see updated
EditableAutoValue::Auto
values!Checklist
main
build: app.rerun.ionightly
build: app.rerun.io