[Merged by Bors] - Parallelized transform propagation #4775
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,95 +2,140 @@ use crate::components::{GlobalTransform, Transform}; | |
| use bevy_ecs::prelude::{Changed, Entity, Query, With, Without}; | ||
| use bevy_hierarchy::{Children, Parent}; | ||
|
|
||
| /// Update [`GlobalTransform`] component of entities that aren't in the hierarchy | ||
| pub fn sync_simple_transforms( | ||
| mut query: Query< | ||
| (&Transform, &mut GlobalTransform), | ||
| (Changed<Transform>, Without<Parent>, Without<Children>), | ||
| >, | ||
| ) { | ||
| query.par_for_each_mut(1024, |(transform, mut global_transform)| { | ||
| *global_transform = GlobalTransform::from(*transform); | ||
|
There was a problem hiding this comment. This isn't a problem with this PR, but it's an interesting conundrum: Currently, if the There was a problem hiding this comment. Simple solution: make |
||
| }); | ||
| } | ||
|
|
||
| /// Update [`GlobalTransform`] component of entities based on entity hierarchy and | ||
| /// [`Transform`] component. | ||
| pub fn propagate_transforms( | ||
| mut root_query: Query< | ||
| ( | ||
| Entity, | ||
| &Children, | ||
| &Transform, | ||
| Changed<Transform>, | ||
| Changed<Children>, | ||
| &mut GlobalTransform, | ||
| ), | ||
| Without<Parent>, | ||
| >, | ||
| transform_query: Query<(&Transform, Changed<Transform>, &mut GlobalTransform), With<Parent>>, | ||
| parent_query: Query<&Parent>, | ||
| children_query: Query<(&Children, Changed<Children>), (With<Parent>, With<GlobalTransform>)>, | ||
|
There was a problem hiding this comment. Why does this query need those filters? I guess completeness of recording requirements to limit possible collisions? There was a problem hiding this comment. This should allow this system to run in parallel with |
||
| ) { | ||
| root_query.par_for_each_mut( | ||
| // The differing depths and sizes of hierarchy trees causes the work for each root to be | ||
| // different. A batch size of 1 ensures that each tree gets it's own task and multiple | ||
| // large trees are not clumped together. | ||
| 1, | ||
james7132 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| |(entity, children, transform, mut changed, children_changed, mut global_transform)| { | ||
| if changed { | ||
| *global_transform = GlobalTransform::from(*transform); | ||
| } | ||
|
|
||
| // If our `Children` has changed, we need to recalculate everything below us | ||
james7132 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| changed |= children_changed; | ||
|
|
||
| for child in children.iter() { | ||
| propagate_recursive( | ||
| &global_transform, | ||
| &transform_query, | ||
| &parent_query, | ||
| &children_query, | ||
| entity, | ||
| *child, | ||
| changed, | ||
| ); | ||
| } | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| fn propagate_recursive( | ||
| parent: &GlobalTransform, | ||
| unsafe_transform_query: &Query< | ||
| (&Transform, Changed<Transform>, &mut GlobalTransform), | ||
| With<Parent>, | ||
| >, | ||
| parent_query: &Query<&Parent>, | ||
| children_query: &Query<(&Children, Changed<Children>), (With<Parent>, With<GlobalTransform>)>, | ||
| 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); | ||
| }; | ||
| assert_eq!( | ||
| actual_parent.get(), expected_parent, | ||
| "Malformed hierarchy. This probably means that your hierarchy has been improperly maintained, or contains a cycle" | ||
| ); | ||
|
|
||
| let global_matrix = { | ||
| let Ok((transform, transform_changed, mut global_transform)) = | ||
| // SAFETY: This call cannot create aliased mutable references. | ||
| // - The top level iteration parallelizes on the roots of the hierarchy. | ||
| // - The above assertion ensures that each child has one and only one unique parent throughout the entire | ||
| // hierarchy. | ||
| // | ||
| // For example, consider the following malformed hierarchy: | ||
| // | ||
| // A | ||
| // / \ | ||
| // B C | ||
| // \ / | ||
| // D | ||
| // | ||
| // D has two parents, B and C. If the propagation passes through C, but the Parent component on D points to B, | ||
| // the above check will panic as the origin parent does match the recorded parent. | ||
| // | ||
| // Also consider the following case, where A and B are roots: | ||
| // | ||
| // A B | ||
| // \ / | ||
| // C D | ||
| // \ / | ||
| // E | ||
| // | ||
| // Even if these A and B start two separate tasks running in parallel, one of them will panic before attempting | ||
| // to mutably access E. | ||
| (unsafe { unsafe_transform_query.get_unchecked(entity) }) else { | ||
| return; | ||
| }; | ||
|
|
||
| changed |= transform_changed; | ||
| if changed { | ||
| *global_transform = parent.mul_transform(*transform); | ||
| } | ||
| *global_transform | ||
| }; | ||
|
|
||
| let Ok((children, changed_children)) = children_query.get(entity) else { | ||
| return | ||
| }; | ||
| // 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, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
|
|
@@ -99,9 +144,10 @@ mod test { | |
| use bevy_ecs::prelude::*; | ||
| use bevy_ecs::system::CommandQueue; | ||
| use bevy_math::vec3; | ||
| use bevy_tasks::{ComputeTaskPool, TaskPool}; | ||
|
|
||
| use crate::components::{GlobalTransform, Transform}; | ||
| use crate::systems::*; | ||
| use crate::TransformBundle; | ||
| use bevy_hierarchy::{BuildChildren, BuildWorldChildren, Children, Parent}; | ||
|
|
||
|
|
@@ -110,10 +156,12 @@ mod test { | |
|
|
||
| #[test] | ||
| fn did_propagate() { | ||
| ComputeTaskPool::init(TaskPool::default); | ||
| let mut world = World::default(); | ||
|
|
||
| let mut update_stage = SystemStage::parallel(); | ||
| update_stage.add_system(sync_simple_transforms); | ||
| update_stage.add_system(propagate_transforms); | ||
|
|
||
| let mut schedule = Schedule::default(); | ||
| schedule.add_stage(Update, update_stage); | ||
|
|
@@ -152,8 +200,10 @@ mod test { | |
| #[test] | ||
| fn did_propagate_command_buffer() { | ||
| let mut world = World::default(); | ||
|
|
||
| let mut update_stage = SystemStage::parallel(); | ||
| update_stage.add_system(sync_simple_transforms); | ||
| update_stage.add_system(propagate_transforms); | ||
|
|
||
| let mut schedule = Schedule::default(); | ||
| schedule.add_stage(Update, update_stage); | ||
|
|
@@ -192,10 +242,12 @@ mod test { | |
|
|
||
| #[test] | ||
| fn correct_children() { | ||
| ComputeTaskPool::init(TaskPool::default); | ||
| let mut world = World::default(); | ||
|
|
||
| let mut update_stage = SystemStage::parallel(); | ||
| update_stage.add_system(sync_simple_transforms); | ||
| update_stage.add_system(propagate_transforms); | ||
|
|
||
| let mut schedule = Schedule::default(); | ||
| schedule.add_stage(Update, update_stage); | ||
|
|
@@ -272,8 +324,10 @@ mod test { | |
| #[test] | ||
| fn correct_transforms_when_no_children() { | ||
| let mut app = App::new(); | ||
| ComputeTaskPool::init(TaskPool::default); | ||
|
|
||
| app.add_system(sync_simple_transforms); | ||
| app.add_system(propagate_transforms); | ||
|
|
||
| let translation = vec3(1.0, 0.0, 0.0); | ||
|
|
||
|
|
@@ -313,6 +367,7 @@ mod test { | |
| #[test] | ||
| #[should_panic] | ||
| fn panic_when_hierarchy_cycle() { | ||
| ComputeTaskPool::init(TaskPool::default); | ||
| // We cannot directly edit Parent and Children, so we use a temp world to break | ||
| // the hierarchy's invariants. | ||
| let mut temp = World::new(); | ||
|
|
@@ -321,7 +376,8 @@ mod test { | |
| // FIXME: Parallel executors seem to have some odd interaction with the other | ||
| // tests in this crate. Using single_threaded until a root cause can be found. | ||
| app.add_stage("single", SystemStage::single_threaded()) | ||
| .add_system_to_stage("single", sync_simple_transforms) | ||
| .add_system_to_stage("single", propagate_transforms); | ||
|
|
||
| fn setup_world(world: &mut World) -> (Entity, Entity) { | ||
| let mut grandchild = Entity::from_raw(0); | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm skeptical that change detection actually helps here. Branching and checking only saves us a very simple operation, while introducing the potential for weird behavior.
Avoids false positives on change detection for GlobalTransform, but IMO we should consider just opting out of change detection for that anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing change detection bumps me from 180k to 190k birds at 60FPS on BevyMark. So, a modest improvement when everything is changed. Probably worth doing then, as in most scenes the overwhelming majority of things with transforms are static.
This PR seems to help that benchmark significantly in general, even comparing to latest main I'm getting +20k birbs.