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

Ensure GlobalTransform consistency when Parent is spawned without Children #3340

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
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
yilinwei marked this conversation as resolved.
Show resolved Hide resolved
// in the same stage have a way to `Query` for a missed update
yilinwei marked this conversation as resolved.
Show resolved Hide resolved
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() {
Copy link
Member

Choose a reason for hiding this comment

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

Like you suggested on Discord, I think this is going to be cleaner with an Option<&DirtyParent> query parameter, rather than creating a new query entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've switched this to use an Or and the same changed_transform_query, wdyt?

*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