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

Ensure GlobalTransform consistency when Parent is spawned without Children #3340

Closed
wants to merge 3 commits into from
Closed

Ensure GlobalTransform consistency when Parent is spawned without Children #3340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6732003
Closes #3329
yilinwei Dec 15, 2021
5adb285
fix clippy warnings
yilinwei Dec 15, 2021
46cead4
address comments.
yilinwei Dec 17, 2021
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
View file
Open in desktop
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
View file
Open in desktop
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;
80 changes: 78 additions & 2 deletions crates/bevy_transform/src/hierarchy/hierarchy_maintenance_system.rs
View file
Open in desktop
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 a `DirtyParent` component so that systems which run
// in the same stage have a way to query for a missed update.
Comment on lines +70 to +71
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this comment is more appropriate on the struct declaration. Right now the only explanation as to what DirtyParent means is here but the struct itself has no documentation.

Copy link
Contributor Author

@yilinwei yilinwei Jan 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not certain the struct declaration is a great place for this particular bit of documentation - DirtyParent is a quirk of both the parent_update_system and how the stages are currently run; the comment is there because it's not obvious why you'd add it in that particular code branch.

As to the wider point, I don't know what the documentation on the declaration should achieve. The problem is quite a niche one - you need to spawn your components in a particular way and even then, you'll only touch DirtyParent in the specific case that you're running a system in the same stage. If you're looking at DirtyParent directly, it's likely that you'll want to read through the code itself.

Arguably it should be part of the documentation on the whole hierarchy system, since that would be where the "entrypoint" to this behaviour would be.

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
View file
Open in desktop
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);
}
}
7 changes: 4 additions & 3 deletions crates/bevy_transform/src/transform_propagate_system.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::components::{Children, GlobalTransform, Parent, Transform};
use crate::components::{Children, DirtyParent, GlobalTransform, Parent, Transform};
use bevy_ecs::{
entity::Entity,
prelude::Or,
query::{Changed, With, Without},
system::Query,
};
Expand All @@ -13,7 +14,7 @@ pub fn transform_propagate_system(
Without<Parent>,
>,
mut transform_query: Query<(&Transform, &mut GlobalTransform), With<Parent>>,
changed_transform_query: Query<Entity, Changed<Transform>>,
changed_transform_query: Query<Entity, Or<(Changed<Transform>, With<DirtyParent>)>>,
Copy link
Member

Choose a reason for hiding this comment

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

I think Changed<Children> would be a better filter here; in that case we could avoid the entire DirtyParent bookkeeping.

I'm not actually sure that (without doing that) any changes to Children which didn't also change the child's parent would result in transforms being updated; unless there is a system similar to update_parents but the other way around.

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 did have a discussion about this when implementing it. The options were:

  1. Add a marker component
  2. Remove the automatic insertion of the Children resource, so that it fails in a way which is obvious to the user (how Transform and GlobalTransform behave)
  3. Recalculate when the Children change.

My preference was 1. or 2. The reasoning was that Children feels like a general Component since it's first-class in the API and using it when there are Parent/Child relationships is encouraged.

IMO, it shouldn't really trigger a Transform update; not merely for optimization purposes, but I don't like the idea of the user receiving the GlobalTransform update events each time a child is added or removed. I think that would be confusing.

Copy link
Member

Choose a reason for hiding this comment

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

I suppose what actually needs to happen is that Changed<Children> needs to be reacted to by re-calculating the children, but 'self' doesn't need to be recalculated (unless it already needed to be). The tree has changed, so the transformations of the subtree need updating.

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 don't quite follow; could you please elaborate?

I think in general Changed<Children> is too coarse a filter; under normal operation if a child is changed, there shouldn't be a reason to recalculate the GlobalTransform of it's siblings. It's only in this specific case, where we "lose" the insertion since we're in the same stage that we need to react to it.

Copy link
Member

Choose a reason for hiding this comment

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

Well my point is: if you add a chilld, how does the child's transform get updated? Neither the parent or child's transforms have changed, so that filter won't activate; so changed will be false, so it won't activate.

If you don't want to update 'self''s transform then, the way to implement this would be to add a changed_children query.
If this solution is implemented, it cleanly sidesteps the need for the messy DirtyParent component.

Copy link
Contributor Author

@yilinwei yilinwei Jan 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

OK I don't understand the solution at all; at least I can't think of a way of doing it without recalculating the siblings which haven't changed.

This is how I think that the algorithm currently works.

  1. Go to the root nodes
  2. Walk down each node checking if the Transform has changed
  3. If the Transform has changed, then update the GlobalTransform and the GlobalTransform of the Children

In the problematic case, the Children has been created in this stage so it doesn't see the child when walking down the tree. In the frame afterwards, neither the parent nor child have an updated Transform but has a Changed<Children> which would evaluate to true.

The Changed<Children> query:

  1. Can't distinguish which child has changed, which means we'd have to update all the siblings since we need to mark the parent. Note, this is also true of DirtyParent, but since it only lasts for a single frame, that doesn't matter since any changes during that single frame needs to be calculated anyway.
  2. Will also evaluate to true when mutating Children later as well.

I cannot see how a single Changed<Children> would be able to sidestep both the issues?

children_query: Query<Option<&Children>, (With<Parent>, With<GlobalTransform>)>,
) {
for (entity, children, transform, mut global_transform) in root_query.iter_mut() {
Expand All @@ -40,7 +41,7 @@ pub fn transform_propagate_system(

fn propagate_recursive(
parent: &GlobalTransform,
changed_transform_query: &Query<Entity, Changed<Transform>>,
changed_transform_query: &Query<Entity, Or<(Changed<Transform>, With<DirtyParent>)>>,
transform_query: &mut Query<(&Transform, &mut GlobalTransform), With<Parent>>,
children_query: &Query<Option<&Children>, (With<Parent>, With<GlobalTransform>)>,
entity: Entity,
Expand Down