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

[Merged by Bors] - Fix unsoundness for propagate_recursive #7003

Closed
wants to merge 12 commits into from
Closed

[Merged by Bors] - Fix unsoundness for propagate_recursive #7003

Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
90e35bc
mark `propagate_recursive` as unsafe and add safety comments
JoJoJet Dec 21, 2022
5fe198c
remove an outdated comment
JoJoJet Dec 21, 2022
357efbb
be more specific in a comment
JoJoJet Dec 21, 2022
5b7d4bd
add another safety comment
JoJoJet Dec 21, 2022
448f16d
clippy
JoJoJet Dec 21, 2022
9020bff
Update systems.rs
JoJoJet Dec 21, 2022
de3cd69
'children' -> 'descendants'
JoJoJet Dec 21, 2022
468da76
fix a comment
JoJoJet Dec 21, 2022
bf2682d
change another 'children' to 'descendants'
JoJoJet Dec 21, 2022
ef337e0
'borrow' -> 'fetch'
JoJoJet Dec 22, 2022
7c17e2f
rewrite docs and comment
JoJoJet Dec 24, 2022
189e1bd
add a word
JoJoJet Dec 24, 2022
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
51 changes: 31 additions & 20 deletions crates/bevy_transform/src/systems.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,21 +46,29 @@ pub fn propagate_transforms(
changed |= children_changed;

for child in children.iter() {
propagate_recursive(
&global_transform,
&transform_query,
&parent_query,
&children_query,
entity,
*child,
changed,
);
// SAFETY: Assuming the hierarchy is consistent, we can be sure that each `child` entity
JoJoJet marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

@james7132 james7132 Dec 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thinking on this a bit more. I'm still not satisfied with this safety comment. We can't assume that the hierarchy is consistent and tree/forest-like. That's what the assertion in the recursive function is for. Not sure what the best wording here is, since the validation for the safety invariant is asserted (recursively) in the called function itself, but is only safe under the assumption that individual entire trees are not aliased from the roots.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could document propagate_recursive with the invariant that it will panic if the hierarchy is malformed, and then cite that invariant in this safety comment. Does that seem right?

Copy link
Member

Choose a reason for hiding this comment

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

We should definitely document the panic on propagate_recursive, but the safety justification here is only valid if both the uniqueness of the roots AND the panic are included. The safety guarantee requires both the hierarchy to be not malformed (or panic if it is), and for the tree from the root down to be uniquely accessed from a single thread for it to be valid.

Copy link
Member Author

@JoJoJet JoJoJet Dec 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I ended up rewriting this comment and the docs for propagate_recursive entirely. Lmk if thats an improvement.

// will be unique across each invocation of this loop body.
// Since this is the only place where `transform_query` gets used, `child` will not be borrowed anywhere else.
// Also, since `child` only has one ancestor, none of its descendants will be borrowed by other invocations of the loop.
unsafe {
propagate_recursive(
&global_transform,
&transform_query,
&parent_query,
&children_query,
entity,
*child,
changed,
);
}
}
},
);
}

fn propagate_recursive(
/// # Safety
/// `unsafe_transform_query` must not be borrowed for `entity`, nor any of its descendants.
JoJoJet marked this conversation as resolved.
Show resolved Hide resolved
unsafe fn propagate_recursive(
parent: &GlobalTransform,
unsafe_transform_query: &Query<
(&Transform, Changed<Transform>, &mut GlobalTransform),
Expand All @@ -71,7 +79,6 @@ fn propagate_recursive(
expected_parent: Entity,
entity: Entity,
mut changed: bool,
// We use a result here to use the `?` operator. Ideally we'd use a try block instead
) {
let Ok(actual_parent) = parent_query.get(entity) else {
panic!("Propagated child for {:?} has no Parent component!", entity);
Expand Down Expand Up @@ -126,15 +133,19 @@ fn propagate_recursive(
// If our `Children` has changed, we need to recalculate everything below us
changed |= changed_children;
for child in children {
propagate_recursive(
&global_matrix,
unsafe_transform_query,
parent_query,
children_query,
entity,
*child,
changed,
);
// SAFETY: The caller guarantees that `unsafe_transform_query` will not be borrowed
JoJoJet marked this conversation as resolved.
Show resolved Hide resolved
// for any descendants of `entity`, so it is safe to call `propagate_recursive` for each child.
unsafe {
propagate_recursive(
&global_matrix,
unsafe_transform_query,
parent_query,
children_query,
entity,
*child,
changed,
);
}
}
}

Expand Down