diff --git a/crates/bevy_transform/Cargo.toml b/crates/bevy_transform/Cargo.toml index a39b47c4de85a..e26f887d10264 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" } bevy_reflect = { path = "../bevy_reflect", version = "0.9.0", 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/components/global_transform.rs b/crates/bevy_transform/src/components/global_transform.rs index 9abfa1413e825..8fae0724f679a 100644 --- a/crates/bevy_transform/src/components/global_transform.rs +++ b/crates/bevy_transform/src/components/global_transform.rs @@ -19,8 +19,8 @@ use bevy_reflect::{std_traits::ReflectDefault, FromReflect, Reflect}; /// /// [`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 a94ff06bda17a..1a437788ebfb1 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 e5712b19068bd..0c09757573aeb 100644 --- a/crates/bevy_transform/src/lib.rs +++ b/crates/bevy_transform/src/lib.rs @@ -1,10 +1,10 @@ #![warn(missing_docs)] +#![warn(clippy::undocumented_unsafe_blocks)] #![doc = include_str!("../README.md")] /// 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 +32,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 @@ -91,11 +91,19 @@ impl Plugin for TransformPlugin { // add transform systems to startup so the first update is "correct" .add_startup_system_to_stage( StartupStage::PostStartup, - systems::transform_propagate_system.label(TransformSystem::TransformPropagate), + systems::sync_simple_transforms.label(TransformSystem::TransformPropagate), + ) + .add_startup_system_to_stage( + StartupStage::PostStartup, + systems::propagate_transforms.label(TransformSystem::TransformPropagate), + ) + .add_system_to_stage( + CoreStage::PostUpdate, + systems::sync_simple_transforms.label(TransformSystem::TransformPropagate), ) .add_system_to_stage( CoreStage::PostUpdate, - systems::transform_propagate_system.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 6b802f585bee9..2fb9006076012 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -2,74 +2,117 @@ 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, Without, Without), + >, +) { + 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 /// [`Transform`] component. -pub fn transform_propagate_system( +pub fn propagate_transforms( mut root_query: Query< ( - Option<(&Children, Changed)>, + Entity, + &Children, &Transform, Changed, + Changed, &mut GlobalTransform, - Entity, ), 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( + // 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, mut changed, children_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 { - let _ = propagate_recursive( + changed |= children_changed; + + for child in children.iter() { + propagate_recursive( &global_transform, - &mut transform_query, + &transform_query, + &parent_query, &children_query, - *child, entity, + *child, changed, ); } - } - } + }, + ); } fn propagate_recursive( parent: &GlobalTransform, - transform_query: &mut Query<( - &Transform, - Changed, - &mut GlobalTransform, - &Parent, - )>, + unsafe_transform_query: &Query< + (&Transform, Changed, &mut GlobalTransform), + With, + >, + parent_query: &Query<&Parent>, children_query: &Query<(&Children, Changed), (With, With)>, - entity: Entity, 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 -) -> Result<(), ()> { +) { + 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 (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) - assert_eq!( - child_parent.get(), expected_parent, - "Malformed hierarchy. This probably means that your hierarchy has been improperly maintained, or contains a cycle" - ); + 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); @@ -77,20 +120,22 @@ 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, - transform_query, + unsafe_transform_query, + parent_query, children_query, - *child, entity, + *child, changed, ); } - Ok(()) } #[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::transform_propagate_system; + 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(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); @@ -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(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); @@ -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(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); @@ -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(transform_propagate_system); + app.add_system(sync_simple_transforms); + app.add_system(propagate_transforms); let translation = vec3(1.0, 0.0, 0.0); @@ -313,12 +367,14 @@ 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.add_system(transform_propagate_system); + app.add_system(propagate_transforms) + .add_system(sync_simple_transforms); fn setup_world(world: &mut World) -> (Entity, Entity) { let mut grandchild = Entity::from_raw(0);