-
Notifications
You must be signed in to change notification settings - Fork 585
Add option to specify target_frame to SpatialInformation
#12040
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
|
Latest documentation preview deployed successfully.
Note: This comment is updated whenever you push a commit. |
|
Web viewer built successfully.
View image diff on kitdiff. Note: This comment is updated whenever you push a commit. |
9a6782d to
1843e70
Compare
|
screenshot plz :) |
1843e70 to
01d0063
Compare
Wumpf
left a comment
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 we can make this fallback situation still better. If not then we should at least document here why we can't and why this is the best non-insane solution :)
| ) { | ||
| /// The target reference frame for all transformations. | ||
| /// | ||
| /// Defaults to the frame derived from the space origin. |
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.
nit: trying to be even more clear :)
| /// Defaults to the frame derived from the space origin. | |
| /// Defaults to the coordinate frame used by the space origin entity. |
| match target_frame_component { | ||
| Ok(target_frame) => { | ||
| let target_frame_str = target_frame.as_str(); | ||
| if !target_frame_str.is_empty() { | ||
| TransformFrameIdHash::from_str(target_frame.as_str()) | ||
| } else { | ||
| self.transform_frame_id_for(query.space_origin.hash()) | ||
| } | ||
| } | ||
| Err(err) => { | ||
| re_log::error_once!("Failed to query target frame: {err}"); | ||
| self.transform_frame_id_for(query.space_origin.hash()) | ||
| } | ||
| } |
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.
Took me a bit that your fallback tries to use a cached value and this is ONLY used when we didn't have a chance yet to do that caching. Should document that better, because my knee-jerk reaction was that the fallback is incomplete.
That said, I do think we can still have all the logic in the fallback and never have the fallback return empty string! We should be able to just do an adhoc query latest_at_with_blueprint_resolved_data on the origin entity
Also, status quo has very surprising behavior when intentionally setting an empty string (which ... is actually a valid name for a transform frame.. interesting 🤔 )
|
|
||
| // Target reference frame - fallback to the computed value from view state | ||
| // | ||
| // An empty string signals that nothing has been set yet. |
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 comment above, this is pretty ugly since we generally don't want the receiving end to be able to know that something is a fallback
01d0063 to
95c63b5
Compare
Related
What
This PR adds
target_frameview property to theSpatialInformationarchetype.To this this PR open the DNA example and enter
tf#/helix/structureinto thetarget_framefield of the view. Now only the beads should be animated when pressing play.