Skip to content

Commit

Permalink
Closes #3329
Browse files Browse the repository at this point in the history
`parent_update` and `transform_propagate` run in the same stage but `parent_update` can spawn `Children`. This means that the `system` can enter an inconsistent state where the `GlobalTransform` has not been updated on `Children` when spawning an `Entity` where the `Parent` does not have an existing `Children` component.

Introduce a marker trait, `DirtyParent`, so that the system will revert to the correct state the next time the systems run.
  • Loading branch information
yilinwei committed Dec 15, 2021
1 parent ffecb05 commit 6732003
Show file tree
Hide file tree
Showing 5 changed files with 95 additions and 7 deletions.
2 changes: 1 addition & 1 deletion crates/bevy_transform/src/components/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,5 @@ mod transform;

pub use children::Children;
pub use global_transform::*;
pub use parent::{Parent, PreviousParent};
pub use parent::{DirtyParent, Parent, PreviousParent};
pub use transform::*;
3 changes: 3 additions & 0 deletions crates/bevy_transform/src/components/parent.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,3 +59,6 @@ impl FromWorld for PreviousParent {
PreviousParent(Entity::new(u32::MAX))
}
}

#[derive(Component, Debug, Copy, Clone, Eq, PartialEq, Reflect)]
pub struct DirtyParent;
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use crate::components::*;
use bevy_ecs::{
entity::Entity,
prelude::Changed,
query::Without,
query::{With, Without},
system::{Commands, Query},
};
use bevy_utils::HashMap;
Expand Down Expand Up @@ -67,20 +67,96 @@ pub fn parent_update_system(
// collect multiple new children that point to the same parent into the same
// SmallVec, and to prevent redundant add+remove operations.
children_additions.iter().for_each(|(e, v)| {
commands.entity(*e).insert(Children::with(v));
// Mark the entity with `Dirty` so that systems which run
// in the same stage have a way to `Query` for a missed update
commands
.entity(*e)
.insert_bundle((Children::with(v), DirtyParent));
});
}

pub fn clean_dirty_parents(
mut commands: Commands,
dirty_parent_query: Query<Entity, With<DirtyParent>>,
) {
for entity in dirty_parent_query.iter() {
commands.entity(entity).remove::<DirtyParent>();
}
}

#[cfg(test)]
mod test {
use bevy_ecs::{
schedule::{Schedule, Stage, SystemStage},
system::CommandQueue,
world::World,
};
use bevy_math::vec3;

use super::*;
use crate::{hierarchy::BuildChildren, transform_propagate_system::transform_propagate_system};

#[test]
fn correct_transforms_when_no_children() {
let mut world = World::default();

let mut update_stage = SystemStage::parallel();
update_stage.add_system(parent_update_system);
update_stage.add_system(transform_propagate_system);
update_stage.add_system(clean_dirty_parents);

let mut schedule = Schedule::default();
schedule.add_stage("update", update_stage);

let mut command_queue = CommandQueue::default();
let mut commands = Commands::new(&mut command_queue, &world);

let translation = vec3(1.0, 0.0, 0.0);

let parent = commands
.spawn()
.insert(Transform::from_translation(translation))
.insert(GlobalTransform::default())
.id();

let child = commands
.spawn()
.insert(Transform::default())
.insert(GlobalTransform::default())
.insert(Parent(parent))
.id();

let children = vec![child];

command_queue.apply(&mut world);
schedule.run(&mut world);

// check the `Children` structure is spawned
assert_eq!(
world
.get::<Children>(parent)
.unwrap()
.0
.iter()
.cloned()
.collect::<Vec<_>>(),
children,
);
assert!(world.get::<DirtyParent>(parent).is_some());

schedule.run(&mut world);

assert_eq!(
world.get::<GlobalTransform>(child).unwrap(),
&GlobalTransform {
translation,
..Default::default()
},
);

assert!(world.get::<DirtyParent>(parent).is_none());
}

#[test]
fn correct_children() {
let mut world = World::default();
Expand Down
8 changes: 6 additions & 2 deletions crates/bevy_transform/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@ pub mod prelude {

use bevy_app::prelude::*;
use bevy_ecs::schedule::{ParallelSystemDescriptorCoercion, SystemLabel};
use prelude::{parent_update_system, Children, GlobalTransform, Parent, PreviousParent, Transform};
use prelude::{
clean_dirty_parents, parent_update_system, Children, GlobalTransform, Parent, PreviousParent,
Transform,
};

#[derive(Default)]
pub struct TransformPlugin;
Expand Down Expand Up @@ -47,6 +50,7 @@ impl Plugin for TransformPlugin {
transform_propagate_system::transform_propagate_system
.label(TransformSystem::TransformPropagate)
.after(TransformSystem::ParentUpdate),
);
)
.add_system_to_stage(CoreStage::PostUpdate, clean_dirty_parents);
}
}
9 changes: 7 additions & 2 deletions crates/bevy_transform/src/transform_propagate_system.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::components::{Children, GlobalTransform, Parent, Transform};
use crate::components::{Children, DirtyParent, GlobalTransform, Parent, Transform};
use bevy_ecs::{
entity::Entity,
query::{Changed, With, Without},
Expand All @@ -12,13 +12,14 @@ pub fn transform_propagate_system(
(Entity, Option<&Children>, &Transform, &mut GlobalTransform),
Without<Parent>,
>,
dirty_parent_query: Query<Entity, With<DirtyParent>>,
mut transform_query: Query<(&Transform, &mut GlobalTransform), With<Parent>>,
changed_transform_query: Query<Entity, Changed<Transform>>,
children_query: Query<Option<&Children>, (With<Parent>, With<GlobalTransform>)>,
) {
for (entity, children, transform, mut global_transform) in root_query.iter_mut() {
let mut changed = false;
if changed_transform_query.get(entity).is_ok() {
if changed_transform_query.get(entity).is_ok() || dirty_parent_query.get(entity).is_ok() {
*global_transform = GlobalTransform::from(*transform);
changed = true;
}
Expand All @@ -27,6 +28,7 @@ pub fn transform_propagate_system(
for child in children.0.iter() {
propagate_recursive(
&global_transform,
&dirty_parent_query,
&changed_transform_query,
&mut transform_query,
&children_query,
Expand All @@ -40,13 +42,15 @@ pub fn transform_propagate_system(

fn propagate_recursive(
parent: &GlobalTransform,
dirty_parent_query: &Query<Entity, With<DirtyParent>>,
changed_transform_query: &Query<Entity, Changed<Transform>>,
transform_query: &mut Query<(&Transform, &mut GlobalTransform), With<Parent>>,
children_query: &Query<Option<&Children>, (With<Parent>, With<GlobalTransform>)>,
entity: Entity,
mut changed: bool,
) {
changed |= changed_transform_query.get(entity).is_ok();
changed |= dirty_parent_query.get(entity).is_ok();

let global_matrix = {
if let Ok((transform, mut global_transform)) = transform_query.get_mut(entity) {
Expand All @@ -63,6 +67,7 @@ fn propagate_recursive(
for child in children.0.iter() {
propagate_recursive(
&global_matrix,
&dirty_parent_query,
changed_transform_query,
transform_query,
children_query,
Expand Down

0 comments on commit 6732003

Please sign in to comment.