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.

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

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

Closed
62 changes: 42 additions & 20 deletions crates/bevy_transform/src/systems.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,21 +46,40 @@ 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:
// - We may operate as if the hierarchy is consistent, since `propagate_recursive` will panic before continuing
// to propagate if it encounters an entity with inconsistent parentage.
// - Since each root entity is unique and the hierarchy is consistent and forest-like,
// other root entities' `propagate_recursive` calls will not conflict with this one.
// - Since this is the only place where `transform_query` gets used, there will be no conflicting fetches elsewhere.
unsafe {
propagate_recursive(
&global_transform,
&transform_query,
&parent_query,
&children_query,
entity,
*child,
changed,
);
}
}
},
);
}

fn propagate_recursive(
/// Recursively propagates the transforms for `entity` and all of its descendants.
///
/// # Panics
///
/// If `entity` or any of its descendants have a malformed hierarchy.
/// The panic will occur before propagating the transforms of any malformed entities and their descendants.
///
/// # Safety
///
/// While this function is running, `unsafe_transform_query` must not have any fetches for `entity`,
/// nor any of its descendants.
unsafe fn propagate_recursive(
parent: &GlobalTransform,
unsafe_transform_query: &Query<
(&Transform, Changed<Transform>, &mut GlobalTransform),
Expand All @@ -71,7 +90,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 +144,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 fetched
// 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