From 4e5ae272c67ba7f52c3c25e4e8f2943d8dd9a4b5 Mon Sep 17 00:00:00 2001 From: james7132 Date: Mon, 16 May 2022 14:45:43 -0700 Subject: [PATCH 1/9] Parallelize transform propagation --- crates/bevy_transform/Cargo.toml | 1 + crates/bevy_transform/src/systems.rs | 97 ++++++++++++++++------------ 2 files changed, 57 insertions(+), 41 deletions(-) diff --git a/crates/bevy_transform/Cargo.toml b/crates/bevy_transform/Cargo.toml index 6a85ed46064e5..aed0a7bcc0d60 100644 --- a/crates/bevy_transform/Cargo.toml +++ b/crates/bevy_transform/Cargo.toml @@ -15,3 +15,4 @@ bevy_ecs = { path = "../bevy_ecs", version = "0.8.0-dev", features = ["bevy_refl bevy_hierarchy = { path = "../bevy_hierarchy", version = "0.8.0-dev"} bevy_math = { path = "../bevy_math", version = "0.8.0-dev" } bevy_reflect = { path = "../bevy_reflect", version = "0.8.0-dev", features = ["bevy"] } +bevy_tasks = { path = "../bevy_tasks", version = "0.8.0-dev" } diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index e7bd9789d3df7..05d5e530a374f 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -1,12 +1,15 @@ use crate::components::{GlobalTransform, Transform}; -use bevy_ecs::prelude::{Changed, Entity, Query, With, Without}; +use bevy_ecs::prelude::{Changed, Entity, Query, With, Without, Res}; +use bevy_tasks::prelude::ComputeTaskPool; use bevy_hierarchy::{Children, Parent}; /// Update [`GlobalTransform`] component of entities based on entity hierarchy and /// [`Transform`] component. pub fn transform_propagate_system( + task_pool: Res, mut root_query: Query< ( + Entity, Option<(&Children, Changed)>, &Transform, Changed, @@ -15,61 +18,71 @@ pub fn transform_propagate_system( ), Without, >, - mut transform_query: Query<( - &Transform, - Changed, - &mut GlobalTransform, - &Parent, - )>, + transform_query: Query< + (&Transform, Changed, &mut GlobalTransform), + With, + >, + parent_query: Query<&Parent>, children_query: Query<(&Children, Changed), (With, With)>, ) { - 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); - } + root_query.par_for_each_mut( + &*task_pool, + 64, + |(entity, children, transform, transform_changed, mut global_transform)| { + let mut changed = transform_changed; + if transform_changed { + *global_transform = GlobalTransform::from(*transform); + } - if let Some((children, changed_children)) = children { - // If our `Children` has changed, we need to recalculate everything below us - changed |= changed_children; - for child in children.iter() { - let _ = propagate_recursive( - &global_transform, - &mut transform_query, - &children_query, - *child, - entity, - changed, - ); + if let Some((children, changed_children)) = children { + // If our `Children` has changed, we need to recalculate everything below us + changed |= changed_children; + for child in children.iter() { + let _ = propagate_recursive( + &global_transform, + &transform_query, + &parent_query, + &children_query, + entity, + *child, + changed, + ); + } } - } - } + }, + ); } fn propagate_recursive( parent: &GlobalTransform, - transform_query: &mut Query<( - &Transform, - Changed, - &mut GlobalTransform, - &Parent, - )>, + transform_query: &Query< + (&Transform, Changed, &mut GlobalTransform), + With, + >, + parent_query: &Query<&Parent>, children_query: &Query<(&Children, Changed), (With, With)>, + expected_parent: Entity, 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, 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) + if let Ok(actual_parent) = parent_query.get(entity) { assert_eq!( - child_parent.0, expected_parent, + actual_parent.0, expected_parent, "Malformed hierarchy. This probably means that your hierarchy has been improperly maintained, or contains a cycle" ); + } else { + panic!("Propagated child for {:?} has no Parent component!", entity); + } + + // SAFE: With the check that each child has one and only one parent, each child must be globally unique within the + // hierarchy. Because of this, it is impossible for this query to have aliased mutable access to the same GlobalTransform. + // Any malformed hierarchy will cause a panic due to the above check. + let global_matrix = unsafe { + let (transform, transform_changed, mut global_transform) = + transform_query.get_unchecked(entity).map_err(drop)?; + changed |= transform_changed; if changed { *global_transform = parent.mul_transform(*transform); @@ -83,8 +96,10 @@ fn propagate_recursive( for child in children.iter() { let _ = propagate_recursive( &global_matrix, - transform_query, - children_query, + &transform_query, + &parent_query, + &children_query, + entity, *child, entity, changed, From ff0f0bc287f9758bcf181773235fa78274ae7589 Mon Sep 17 00:00:00 2001 From: james7132 Date: Mon, 16 May 2022 15:06:49 -0700 Subject: [PATCH 2/9] build fixes --- crates/bevy_transform/src/lib.rs | 12 +++++ crates/bevy_transform/src/systems.rs | 77 +++++++++++++++------------- 2 files changed, 52 insertions(+), 37 deletions(-) diff --git a/crates/bevy_transform/src/lib.rs b/crates/bevy_transform/src/lib.rs index a88017c7d7d60..02d6328ed2013 100644 --- a/crates/bevy_transform/src/lib.rs +++ b/crates/bevy_transform/src/lib.rs @@ -93,12 +93,24 @@ impl Plugin for TransformPlugin { app.register_type::() .register_type::() // Adding these to startup ensures the first update is "correct" + .add_startup_system_to_stage( + StartupStage::PostStartup, + systems::transform_propagate_system_flat + .label(TransformSystem::TransformPropagate) + .after(HierarchySystem::ParentUpdate), + ) .add_startup_system_to_stage( StartupStage::PostStartup, systems::transform_propagate_system .label(TransformSystem::TransformPropagate) .after(HierarchySystem::ParentUpdate), ) + .add_system_to_stage( + CoreStage::PostUpdate, + systems::transform_propagate_system_flat + .label(TransformSystem::TransformPropagate) + .after(HierarchySystem::ParentUpdate), + ) .add_system_to_stage( CoreStage::PostUpdate, systems::transform_propagate_system diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index 05d5e530a374f..46a4642cc69e3 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -1,7 +1,19 @@ use crate::components::{GlobalTransform, Transform}; -use bevy_ecs::prelude::{Changed, Entity, Query, With, Without, Res}; -use bevy_tasks::prelude::ComputeTaskPool; +use bevy_ecs::prelude::{Changed, Entity, Or, Query, Res, With, Without}; use bevy_hierarchy::{Children, Parent}; +use bevy_tasks::prelude::ComputeTaskPool; + +/// Update [`GlobalTransform`] component of entities that aren't in the hierarchy +pub fn transform_propagate_system_flat( + mut query: Query< + (&Transform, &mut GlobalTransform), + (Changed, Without, Without), + >, +) { + for (transform, mut global_transform) in query.iter_mut() { + *global_transform = GlobalTransform::from(*transform); + } +} /// Update [`GlobalTransform`] component of entities based on entity hierarchy and /// [`Transform`] component. @@ -10,44 +22,35 @@ pub fn transform_propagate_system( mut root_query: Query< ( Entity, - Option<(&Children, Changed)>, + &Children, &Transform, - Changed, + Or<(Changed, Changed)>, &mut GlobalTransform, - Entity, ), Without, >, - transform_query: Query< - (&Transform, Changed, &mut GlobalTransform), - With, - >, + transform_query: Query<(&Transform, Changed, &mut GlobalTransform), With>, parent_query: Query<&Parent>, children_query: Query<(&Children, Changed), (With, With)>, ) { root_query.par_for_each_mut( &*task_pool, 64, - |(entity, children, transform, transform_changed, mut global_transform)| { - let mut changed = transform_changed; - if transform_changed { + |(entity, children, transform, changed, mut global_transform)| { + if changed { *global_transform = GlobalTransform::from(*transform); } - if let Some((children, changed_children)) = children { - // If our `Children` has changed, we need to recalculate everything below us - changed |= changed_children; - for child in children.iter() { - let _ = propagate_recursive( - &global_transform, - &transform_query, - &parent_query, - &children_query, - entity, - *child, - changed, - ); - } + for child in children.iter() { + let _ = propagate_recursive( + &global_transform, + &transform_query, + &parent_query, + &children_query, + entity, + *child, + changed, + ); } }, ); @@ -55,15 +58,11 @@ pub fn transform_propagate_system( fn propagate_recursive( parent: &GlobalTransform, - transform_query: &Query< - (&Transform, Changed, &mut GlobalTransform), - With, - >, + transform_query: &Query<(&Transform, Changed, &mut GlobalTransform), With>, parent_query: &Query<&Parent>, children_query: &Query<(&Children, Changed), (With, With)>, expected_parent: Entity, 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<(), ()> { @@ -76,7 +75,7 @@ fn propagate_recursive( panic!("Propagated child for {:?} has no Parent component!", entity); } - // SAFE: With the check that each child has one and only one parent, each child must be globally unique within the + // SAFE: With the check that each child has one and only one parent, each child must be globally unique within the // hierarchy. Because of this, it is impossible for this query to have aliased mutable access to the same GlobalTransform. // Any malformed hierarchy will cause a panic due to the above check. let global_matrix = unsafe { @@ -96,12 +95,11 @@ fn propagate_recursive( for child in children.iter() { let _ = propagate_recursive( &global_matrix, - &transform_query, - &parent_query, - &children_query, + transform_query, + parent_query, + children_query, entity, *child, - entity, changed, ); } @@ -116,7 +114,7 @@ mod test { use bevy_math::vec3; use crate::components::{GlobalTransform, Transform}; - use crate::systems::transform_propagate_system; + use crate::systems::*; use crate::TransformBundle; use bevy_hierarchy::{ parent_update_system, BuildChildren, BuildWorldChildren, Children, Parent, @@ -128,6 +126,7 @@ mod test { let mut update_stage = SystemStage::parallel(); update_stage.add_system(parent_update_system); + update_stage.add_system(transform_propagate_system_flat); update_stage.add_system(transform_propagate_system); let mut schedule = Schedule::default(); @@ -173,6 +172,7 @@ mod test { let mut update_stage = SystemStage::parallel(); update_stage.add_system(parent_update_system); + update_stage.add_system(transform_propagate_system_flat); update_stage.add_system(transform_propagate_system); let mut schedule = Schedule::default(); @@ -216,6 +216,7 @@ mod test { let mut update_stage = SystemStage::parallel(); update_stage.add_system(parent_update_system); + update_stage.add_system(transform_propagate_system_flat); update_stage.add_system(transform_propagate_system); let mut schedule = Schedule::default(); @@ -301,6 +302,7 @@ mod test { let mut app = App::new(); app.add_system(parent_update_system); + app.add_system(transform_propagate_system_flat); app.add_system(transform_propagate_system); let translation = vec3(1.0, 0.0, 0.0); @@ -357,6 +359,7 @@ mod test { let mut app = App::new(); app.add_system(parent_update_system); + app.add_system(transform_propagate_system_flat); app.add_system(transform_propagate_system); let child = app From 80b2a33ff553b0bbf92ed8835e1d1057b2116fb4 Mon Sep 17 00:00:00 2001 From: james7132 Date: Thu, 2 Jun 2022 11:45:06 -0700 Subject: [PATCH 3/9] Change batch size to 1 --- crates/bevy_transform/src/systems.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index 46a4642cc69e3..b90a21e4bd5ea 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -1,7 +1,6 @@ use crate::components::{GlobalTransform, Transform}; -use bevy_ecs::prelude::{Changed, Entity, Or, Query, Res, With, Without}; +use bevy_ecs::prelude::{Changed, Entity, Or, Query, With, Without}; use bevy_hierarchy::{Children, Parent}; -use bevy_tasks::prelude::ComputeTaskPool; /// Update [`GlobalTransform`] component of entities that aren't in the hierarchy pub fn transform_propagate_system_flat( @@ -18,7 +17,6 @@ pub fn transform_propagate_system_flat( /// Update [`GlobalTransform`] component of entities based on entity hierarchy and /// [`Transform`] component. pub fn transform_propagate_system( - task_pool: Res, mut root_query: Query< ( Entity, @@ -34,8 +32,7 @@ pub fn transform_propagate_system( children_query: Query<(&Children, Changed), (With, With)>, ) { root_query.par_for_each_mut( - &*task_pool, - 64, + 1, |(entity, children, transform, changed, mut global_transform)| { if changed { *global_transform = GlobalTransform::from(*transform); From 8f28b3dd20637eaf924522637f62844904a2b080 Mon Sep 17 00:00:00 2001 From: james7132 Date: Fri, 3 Jun 2022 01:24:37 -0700 Subject: [PATCH 4/9] Review changes and Fix CI --- .../src/components/global_transform.rs | 4 +- .../src/components/transform.rs | 4 +- crates/bevy_transform/src/lib.rs | 13 +++-- crates/bevy_transform/src/systems.rs | 51 ++++++++++++------- 4 files changed, 43 insertions(+), 29 deletions(-) diff --git a/crates/bevy_transform/src/components/global_transform.rs b/crates/bevy_transform/src/components/global_transform.rs index be1d75cfcb46b..fa858e9369d61 100644 --- a/crates/bevy_transform/src/components/global_transform.rs +++ b/crates/bevy_transform/src/components/global_transform.rs @@ -18,8 +18,8 @@ use std::ops::Mul; /// /// [`GlobalTransform`] is the position of an entity relative to the reference frame. /// -/// [`GlobalTransform`] is updated from [`Transform`] in the system -/// [`transform_propagate_system`](crate::transform_propagate_system). +/// [`GlobalTransform`] is updated from [`Transform`] in the systems labeled +/// [`TransformPropagate`](crate::TransformSystem::TransformPropagate). /// /// This system runs in stage [`CoreStage::PostUpdate`](crate::CoreStage::PostUpdate). If you /// update the[`Transform`] of an entity in this stage or after, you will notice a 1 frame lag diff --git a/crates/bevy_transform/src/components/transform.rs b/crates/bevy_transform/src/components/transform.rs index bbca98e76961d..ea3dd59940002 100644 --- a/crates/bevy_transform/src/components/transform.rs +++ b/crates/bevy_transform/src/components/transform.rs @@ -20,8 +20,8 @@ use std::ops::Mul; /// /// [`GlobalTransform`] is the position of an entity relative to the reference frame. /// -/// [`GlobalTransform`] is updated from [`Transform`] in the system -/// [`transform_propagate_system`](crate::transform_propagate_system). +/// [`GlobalTransform`] is updated from [`Transform`] in the systems labeled +/// [`TransformPropagate`](crate::TransformSystem::TransformPropagate). /// /// This system runs in stage [`CoreStage::PostUpdate`](crate::CoreStage::PostUpdate). If you /// update the[`Transform`] of an entity in this stage or after, you will notice a 1 frame lag diff --git a/crates/bevy_transform/src/lib.rs b/crates/bevy_transform/src/lib.rs index 02d6328ed2013..57a266f8e74c9 100644 --- a/crates/bevy_transform/src/lib.rs +++ b/crates/bevy_transform/src/lib.rs @@ -4,7 +4,6 @@ /// The basic components of the transform crate pub mod components; mod systems; -pub use crate::systems::transform_propagate_system; #[doc(hidden)] pub mod prelude { @@ -32,8 +31,8 @@ use prelude::{GlobalTransform, Transform}; /// /// [`GlobalTransform`] is the position of an entity relative to the reference frame. /// -/// [`GlobalTransform`] is updated from [`Transform`] in the system -/// [`transform_propagate_system`]. +/// [`GlobalTransform`] is updated from [`Transform`] in the systems labeled +/// [`TransformPropagate`](crate::TransformSystem::TransformPropagate). /// /// This system runs in stage [`CoreStage::PostUpdate`](crate::CoreStage::PostUpdate). If you /// update the[`Transform`] of an entity in this stage or after, you will notice a 1 frame lag @@ -95,25 +94,25 @@ impl Plugin for TransformPlugin { // Adding these to startup ensures the first update is "correct" .add_startup_system_to_stage( StartupStage::PostStartup, - systems::transform_propagate_system_flat + systems::sync_simple_transforms .label(TransformSystem::TransformPropagate) .after(HierarchySystem::ParentUpdate), ) .add_startup_system_to_stage( StartupStage::PostStartup, - systems::transform_propagate_system + systems::propagate_transforms .label(TransformSystem::TransformPropagate) .after(HierarchySystem::ParentUpdate), ) .add_system_to_stage( CoreStage::PostUpdate, - systems::transform_propagate_system_flat + systems::sync_simple_transforms .label(TransformSystem::TransformPropagate) .after(HierarchySystem::ParentUpdate), ) .add_system_to_stage( CoreStage::PostUpdate, - systems::transform_propagate_system + systems::propagate_transforms .label(TransformSystem::TransformPropagate) .after(HierarchySystem::ParentUpdate), ); diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index b90a21e4bd5ea..5c0bf0ce03a1d 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -1,9 +1,9 @@ use crate::components::{GlobalTransform, Transform}; -use bevy_ecs::prelude::{Changed, Entity, Or, Query, With, Without}; +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 transform_propagate_system_flat( +pub fn sync_simple_transforms( mut query: Query< (&Transform, &mut GlobalTransform), (Changed, Without, Without), @@ -16,13 +16,14 @@ pub fn transform_propagate_system_flat( /// Update [`GlobalTransform`] component of entities based on entity hierarchy and /// [`Transform`] component. -pub fn transform_propagate_system( +pub fn propagate_transforms( mut root_query: Query< ( Entity, &Children, &Transform, - Or<(Changed, Changed)>, + Changed, + Changed, &mut GlobalTransform, ), Without, @@ -32,12 +33,17 @@ pub fn transform_propagate_system( children_query: Query<(&Children, Changed), (With, 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, - |(entity, children, transform, changed, mut global_transform)| { + |(entity, children, transform, mut changed, children_changed, mut global_transform)| { if changed { *global_transform = GlobalTransform::from(*transform); } + changed |= children_changed; + for child in children.iter() { let _ = propagate_recursive( &global_transform, @@ -55,7 +61,10 @@ pub fn transform_propagate_system( fn propagate_recursive( parent: &GlobalTransform, - transform_query: &Query<(&Transform, Changed, &mut GlobalTransform), With>, + unsafe_transform_query: &Query< + (&Transform, Changed, &mut GlobalTransform), + With, + >, parent_query: &Query<&Parent>, children_query: &Query<(&Children, Changed), (With, With)>, expected_parent: Entity, @@ -77,7 +86,7 @@ fn propagate_recursive( // Any malformed hierarchy will cause a panic due to the above check. let global_matrix = unsafe { let (transform, transform_changed, mut global_transform) = - transform_query.get_unchecked(entity).map_err(drop)?; + unsafe_transform_query.get_unchecked(entity).map_err(drop)?; changed |= transform_changed; if changed { @@ -92,7 +101,7 @@ fn propagate_recursive( for child in children.iter() { let _ = propagate_recursive( &global_matrix, - transform_query, + unsafe_transform_query, parent_query, children_query, entity, @@ -109,6 +118,7 @@ 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::*; @@ -120,11 +130,12 @@ mod test { #[test] fn did_propagate() { let mut world = World::default(); + world.insert_resource(ComputeTaskPool(TaskPool::default())); let mut update_stage = SystemStage::parallel(); update_stage.add_system(parent_update_system); - update_stage.add_system(transform_propagate_system_flat); - update_stage.add_system(transform_propagate_system); + update_stage.add_system(sync_simple_transforms); + update_stage.add_system(propagate_transforms); let mut schedule = Schedule::default(); schedule.add_stage("update", update_stage); @@ -166,11 +177,12 @@ mod test { #[test] fn did_propagate_command_buffer() { let mut world = World::default(); + world.insert_resource(ComputeTaskPool(TaskPool::default())); let mut update_stage = SystemStage::parallel(); update_stage.add_system(parent_update_system); - update_stage.add_system(transform_propagate_system_flat); - update_stage.add_system(transform_propagate_system); + update_stage.add_system(sync_simple_transforms); + update_stage.add_system(propagate_transforms); let mut schedule = Schedule::default(); schedule.add_stage("update", update_stage); @@ -210,11 +222,12 @@ mod test { #[test] fn correct_children() { let mut world = World::default(); + world.insert_resource(ComputeTaskPool(TaskPool::default())); let mut update_stage = SystemStage::parallel(); update_stage.add_system(parent_update_system); - update_stage.add_system(transform_propagate_system_flat); - update_stage.add_system(transform_propagate_system); + update_stage.add_system(sync_simple_transforms); + update_stage.add_system(propagate_transforms); let mut schedule = Schedule::default(); schedule.add_stage("update", update_stage); @@ -297,10 +310,11 @@ mod test { #[test] fn correct_transforms_when_no_children() { let mut app = App::new(); + app.insert_resource(ComputeTaskPool(TaskPool::default())); app.add_system(parent_update_system); - app.add_system(transform_propagate_system_flat); - app.add_system(transform_propagate_system); + app.add_system(sync_simple_transforms); + app.add_system(propagate_transforms); let translation = vec3(1.0, 0.0, 0.0); @@ -354,10 +368,11 @@ mod test { #[should_panic] fn panic_when_hierarchy_cycle() { let mut app = App::new(); + app.insert_resource(ComputeTaskPool(TaskPool::default())); app.add_system(parent_update_system); - app.add_system(transform_propagate_system_flat); - app.add_system(transform_propagate_system); + app.add_system(sync_simple_transforms); + app.add_system(propagate_transforms); let child = app .world From fd483d02e18ae81f31c91cc68fea2a441ecb53e7 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 6 Nov 2022 18:47:04 -0800 Subject: [PATCH 5/9] Fix CI --- crates/bevy_transform/Cargo.toml | 3 +++ crates/bevy_transform/src/lib.rs | 12 ++++-------- crates/bevy_transform/src/systems.rs | 8 ++++---- 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/crates/bevy_transform/Cargo.toml b/crates/bevy_transform/Cargo.toml index 5fcc1bdaf2283..63d9481d297e6 100644 --- a/crates/bevy_transform/Cargo.toml +++ b/crates/bevy_transform/Cargo.toml @@ -17,5 +17,8 @@ bevy_math = { path = "../bevy_math", version = "0.9.0-dev" } bevy_reflect = { path = "../bevy_reflect", version = "0.9.0-dev", features = ["bevy"] } serde = { version = "1", features = ["derive"], optional = true } +[dev_dependencies] +bevy_tasks = { path = "../bevy_tasks", version = "0.9.0-dev" } + [features] serialize = ["dep:serde", "bevy_math/serialize"] diff --git a/crates/bevy_transform/src/lib.rs b/crates/bevy_transform/src/lib.rs index 7d9c9f9a53dac..34fc409dadd2a 100644 --- a/crates/bevy_transform/src/lib.rs +++ b/crates/bevy_transform/src/lib.rs @@ -90,23 +90,19 @@ impl Plugin for TransformPlugin { // add transform systems to startup so the first update is "correct" .add_startup_system_to_stage( StartupStage::PostStartup, - systems::sync_simple_transforms - .label(TransformSystem::TransformPropagate), + systems::sync_simple_transforms.label(TransformSystem::TransformPropagate), ) .add_startup_system_to_stage( StartupStage::PostStartup, - systems::propagate_transforms - .label(TransformSystem::TransformPropagate), + systems::propagate_transforms.label(TransformSystem::TransformPropagate), ) .add_system_to_stage( CoreStage::PostUpdate, - systems::sync_simple_transforms - .label(TransformSystem::TransformPropagate), + systems::sync_simple_transforms.label(TransformSystem::TransformPropagate), ) .add_system_to_stage( CoreStage::PostUpdate, - systems::propagate_transforms - .label(TransformSystem::TransformPropagate), + systems::propagate_transforms.label(TransformSystem::TransformPropagate), ); } } diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index 4d47d19afe02a..347bdaa3c160b 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -131,8 +131,8 @@ mod test { #[test] fn did_propagate() { + ComputeTaskPool::init(TaskPool::default); let mut world = World::default(); - world.insert_resource(ComputeTaskPool(TaskPool::default())); let mut update_stage = SystemStage::parallel(); update_stage.add_system(sync_simple_transforms); @@ -217,8 +217,8 @@ mod test { #[test] fn correct_children() { + ComputeTaskPool::init(TaskPool::default); let mut world = World::default(); - world.insert_resource(ComputeTaskPool(TaskPool::default())); let mut update_stage = SystemStage::parallel(); update_stage.add_system(sync_simple_transforms); @@ -299,7 +299,7 @@ mod test { #[test] fn correct_transforms_when_no_children() { let mut app = App::new(); - app.insert_resource(ComputeTaskPool(TaskPool::default())); + ComputeTaskPool::init(TaskPool::default); app.add_system(sync_simple_transforms); app.add_system(propagate_transforms); @@ -342,11 +342,11 @@ 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(); let mut app = App::new(); - app.insert_resource(ComputeTaskPool(TaskPool::default())); // 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. From 73ec56356a1953244bb21e26852f48dfbdce6c88 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 20 Nov 2022 15:49:12 -0800 Subject: [PATCH 6/9] Use let else where possible --- crates/bevy_transform/src/systems.rs | 38 +++++++++++++++------------- 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index 347bdaa3c160b..676e83a27e8e8 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -46,7 +46,7 @@ pub fn propagate_transforms( changed |= children_changed; for child in children.iter() { - let _ = propagate_recursive( + propagate_recursive( &global_transform, &transform_query, &parent_query, @@ -72,22 +72,23 @@ fn propagate_recursive( entity: Entity, mut changed: bool, // We use a result here to use the `?` operator. Ideally we'd use a try block instead -) -> Result<(), ()> { - if let Ok(actual_parent) = parent_query.get(entity) { - assert_eq!( - actual_parent.get(), expected_parent, - "Malformed hierarchy. This probably means that your hierarchy has been improperly maintained, or contains a cycle" - ); - } else { +) { + 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" + ); - // SAFE: With the check that each child has one and only one parent, each child must be globally unique within the - // hierarchy. Because of this, it is impossible for this query to have aliased mutable access to the same GlobalTransform. - // Any malformed hierarchy will cause a panic due to the above check. - let global_matrix = unsafe { - let (transform, transform_changed, mut global_transform) = - unsafe_transform_query.get_unchecked(entity).map_err(drop)?; + let global_matrix = { + let Ok((transform, transform_changed, mut global_transform)) = + // SAFE: With the check that each child has one and only one parent, each child must be globally unique within the + // hierarchy. Because of this, it is impossible for this query to have aliased mutable access to the same GlobalTransform. + // Any malformed hierarchy will cause a panic due to the above check. + (unsafe { unsafe_transform_query.get_unchecked(entity) }) else { + return; + }; changed |= transform_changed; if changed { @@ -96,11 +97,13 @@ fn propagate_recursive( *global_transform }; - let (children, changed_children) = children_query.get(entity).map_err(drop)?; + 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 { - let _ = propagate_recursive( + propagate_recursive( &global_matrix, unsafe_transform_query, parent_query, @@ -110,7 +113,6 @@ fn propagate_recursive( changed, ); } - Ok(()) } #[cfg(test)] From efbbae283864483c7625b2291f5816640615c05f Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 20 Nov 2022 15:51:08 -0800 Subject: [PATCH 7/9] Parallelize the parentless case --- crates/bevy_transform/src/systems.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index 676e83a27e8e8..b042f60428ec2 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -9,9 +9,9 @@ pub fn sync_simple_transforms( (Changed, Without, Without), >, ) { - for (transform, mut global_transform) in query.iter_mut() { + query.par_for_each_mut(1024, |(transform, mut global_transform)| { *global_transform = GlobalTransform::from(*transform); - } + }); } /// Update [`GlobalTransform`] component of entities based on entity hierarchy and From 4cec5816eaa31fd716fb1c1a819a81716848f102 Mon Sep 17 00:00:00 2001 From: james7132 Date: Sun, 20 Nov 2022 16:02:31 -0800 Subject: [PATCH 8/9] Provide much more justification for the get_unchecked_mut --- crates/bevy_transform/src/systems.rs | 29 +++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index b042f60428ec2..ed172a743991a 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -83,9 +83,32 @@ fn propagate_recursive( let global_matrix = { let Ok((transform, transform_changed, mut global_transform)) = - // SAFE: With the check that each child has one and only one parent, each child must be globally unique within the - // hierarchy. Because of this, it is impossible for this query to have aliased mutable access to the same GlobalTransform. - // Any malformed hierarchy will cause a panic due to the above check. + // 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; }; From e032f9d08e2a84806bfe9dbb83bb304b146ab733 Mon Sep 17 00:00:00 2001 From: james7132 Date: Mon, 21 Nov 2022 10:16:40 -0800 Subject: [PATCH 9/9] Enable clippy warning --- crates/bevy_transform/src/lib.rs | 1 + crates/bevy_transform/src/systems.rs | 12 ++++++------ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/crates/bevy_transform/src/lib.rs b/crates/bevy_transform/src/lib.rs index 34fc409dadd2a..0c09757573aeb 100644 --- a/crates/bevy_transform/src/lib.rs +++ b/crates/bevy_transform/src/lib.rs @@ -1,4 +1,5 @@ #![warn(missing_docs)] +#![warn(clippy::undocumented_unsafe_blocks)] #![doc = include_str!("../README.md")] /// The basic components of the transform crate diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index 4c4324daa0e0b..2fb9006076012 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -84,23 +84,23 @@ fn propagate_recursive( 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 + // - 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