Skip to content

Commit

Permalink
Ensure that the parent is always the expected entity (#4717)
Browse files Browse the repository at this point in the history
# Objective

- Transform propogation could stack overflow when there was a cycle.
- I think #4203 would use all available memory.

## Solution

- Make sure that the child entity's `Parent`s are their parents.

This is also required for when parallelising, although as noted in the comment, the naïve solution would be UB.
(The best way to fix this would probably be an `&mut UnsafeCell<T>` `WorldQuery`, or wrapper type with the same effect)
  • Loading branch information
DJMcNab committed May 16, 2022
1 parent 5422a2b commit 84c783b
Showing 1 changed file with 57 additions and 11 deletions.
68 changes: 57 additions & 11 deletions crates/bevy_transform/src/systems.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,21 @@ pub fn transform_propagate_system(
&Transform,
Changed<Transform>,
&mut GlobalTransform,
Entity,
),
Without<Parent>,
>,
mut transform_query: Query<
(&Transform, Changed<Transform>, &mut GlobalTransform),
With<Parent>,
>,
mut transform_query: Query<(
&Transform,
Changed<Transform>,
&mut GlobalTransform,
&Parent,
)>,
children_query: Query<(&Children, Changed<Children>), (With<Parent>, With<GlobalTransform>)>,
) {
for (children, transform, transform_changed, mut global_transform) in root_query.iter_mut() {
for (children, transform, transform_changed, mut global_transform, entity) in
root_query.iter_mut()
{
let mut changed = transform_changed;
if transform_changed {
*global_transform = GlobalTransform::from(*transform);
Expand All @@ -35,6 +40,7 @@ pub fn transform_propagate_system(
&mut transform_query,
&children_query,
*child,
entity,
changed,
);
}
Expand All @@ -44,19 +50,26 @@ pub fn transform_propagate_system(

fn propagate_recursive(
parent: &GlobalTransform,
transform_query: &mut Query<
(&Transform, Changed<Transform>, &mut GlobalTransform),
With<Parent>,
>,
transform_query: &mut Query<(
&Transform,
Changed<Transform>,
&mut GlobalTransform,
&Parent,
)>,
children_query: &Query<(&Children, Changed<Children>), (With<Parent>, With<GlobalTransform>)>,
entity: Entity,
expected_parent: Entity,
mut changed: bool,
// We use a result here to use the `?` operator. Ideally we'd use a try block instead
) -> Result<(), ()> {
let global_matrix = {
let (transform, transform_changed, mut global_transform) =
let (transform, transform_changed, mut global_transform, child_parent) =
transform_query.get_mut(entity).map_err(drop)?;

// Note that for parallelising, this check cannot occur here, since there is an `&mut GlobalTransform` (in global_transform)
assert_eq!(
child_parent.0, expected_parent,
"Malformed hierarchy. This probably means that your hierarchy has been improperly maintained, or contains a cycle"
);
changed |= transform_changed;
if changed {
*global_transform = parent.mul_transform(*transform);
Expand All @@ -73,6 +86,7 @@ fn propagate_recursive(
transform_query,
children_query,
*child,
entity,
changed,
);
}
Expand Down Expand Up @@ -322,4 +336,36 @@ mod test {
);
}
}
#[test]
#[should_panic]
fn panic_when_hierarchy_cycle() {
let mut app = App::new();

app.add_system(parent_update_system);
app.add_system(transform_propagate_system);

let child = app
.world
.spawn()
.insert_bundle((Transform::identity(), GlobalTransform::default()))
.id();

let grandchild = app
.world
.spawn()
.insert_bundle((
Transform::identity(),
GlobalTransform::default(),
Parent(child),
))
.id();
app.world.spawn().insert_bundle((
Transform::default(),
GlobalTransform::default(),
Children::with(&[child]),
));
app.world.entity_mut(child).insert(Parent(grandchild));

app.update();
}
}

0 comments on commit 84c783b

Please sign in to comment.