Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Don't make top-level spaces their own children #1100

Merged
merged 4 commits into from
Feb 6, 2023
Merged

Don't make top-level spaces their own children #1100

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f87f7fa
Don't make top-level spaces their own children
jleibs Feb 5, 2023
5e41501
Allow view coordinates to be logged at the root
jleibs Feb 5, 2023
9849d13
Merge remote-tracking branch 'origin/main' into jleibs/root_transform…
Wumpf Feb 6, 2023
b5f6506
Reintroduce https://github.com/rerun-io/rerun/pull/1097
Wumpf Feb 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion crates/re_viewer/src/misc/space_info.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,15 @@ impl SpaceInfoCollection {
.child_spaces
.insert(tree.path.clone(), transform);

add_children(entity_db, &mut spaces_info, &mut space_info, tree, &query);
for child_tree in tree.children.values() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh I didn't consider that add_children adds to the parent!

Hmm almost wondering now if we should instead do this loop on the root only (instead of having nested loops on children of children of root) plus having add_children something like this internally:

            let transform = query_transform(entity_db, entity_path, &query)
.or_else(|| (entity_path.len() == 1).then(|| Transform::Rigid3(re_log_types::Rigid3::IDENTITY)));

does that make sense?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was thinking the same thing but was trying to keep the fix small.

add_children(
entity_db,
&mut spaces_info,
&mut space_info,
child_tree,
&query,
);
}
spaces_info.spaces.insert(tree.path.clone(), space_info);
}
spaces_info
Expand Down
15 changes: 10 additions & 5 deletions rerun_py/src/python_bridge.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,9 +443,8 @@ fn log_transform(
timeless: bool,
) -> PyResult<()> {
let entity_path = parse_entity_path(entity_path)?;
if entity_path.len() == 1 {
// Stop people from logging a transform to a root-entity, such as "world" (which doesn't have a parent).
return Err(PyTypeError::new_err("Transforms are between a child entity and its parent, so root entities cannot have transforms"));
if entity_path.is_root() {
return Err(PyTypeError::new_err("Transforms are between a child entity and its parent, so the root cannot have a transform"));
}
let mut session = global_session();
let time_point = time(timeless);
Expand Down Expand Up @@ -517,16 +516,22 @@ fn log_view_coordinates_up_handedness(
}

fn log_view_coordinates(
entity_path: &str,
entity_path_str: &str,
coordinates: ViewCoordinates,
timeless: bool,
) -> PyResult<()> {
if coordinates.handedness() == Some(coordinates::Handedness::Left) {
re_log::warn_once!("Left-handed coordinate systems are not yet fully supported by Rerun");
}

// We normally disallow logging to root, but we make an exception for view_coordinates
let entity_path = if entity_path_str == "/" {
EntityPath::root()
} else {
parse_entity_path(entity_path_str)?
};
Comment on lines +527 to +532
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didn't think of this whole thing at all :O


let mut session = global_session();
let entity_path = parse_entity_path(entity_path)?;
let time_point = time(timeless);

// We currently log view coordinates from inside the bridge because the code
Expand Down