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

[Merged by Bors] - Parallelized transform propagation #4775

Closed
3 changes: 3 additions & 0 deletions crates/bevy_transform/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
4 changes: 2 additions & 2 deletions crates/bevy_transform/src/components/global_transform.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_transform/src/components/transform.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
18 changes: 13 additions & 5 deletions crates/bevy_transform/src/lib.rs
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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),
);
}
}
156 changes: 106 additions & 50 deletions crates/bevy_transform/src/systems.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>),
Copy link
Member

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.

Copy link
Member

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.

>,
) {
query.par_for_each_mut(1024, |(transform, mut global_transform)| {
*global_transform = GlobalTransform::from(*transform);
Copy link
Member

Choose a reason for hiding this comment

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

This isn't a problem with this PR, but it's an interesting conundrum:

Currently, if the GlobalTransform has been erroneously changed by users to something other than the correct transform, that will remain until something higher up in the hierarchy or the Transform changes. Do we want this behaviour, or do we want to fix this (or warn?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Simple solution: make GlobalTransform read-only the same way #4197 does with the hierarchy itself. Makes this footgun a lot harder to come across.

});
}

/// 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<Children>)>,
Entity,
&Children,
&Transform,
Changed<Transform>,
Changed<Children>,
&mut GlobalTransform,
Entity,
),
Without<Parent>,
>,
mut transform_query: Query<(
&Transform,
Changed<Transform>,
&mut GlobalTransform,
&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>)>,
Copy link
Member

Choose a reason for hiding this comment

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

Why does this query need those filters? I guess completeness of recording requirements to limit possible collisions?

Copy link
Contributor

Choose a reason for hiding this comment

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

This should allow this system to run in parallel with sync_simple_transforms.

) {
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,
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 let Some((children, changed_children)) = children {
// If our `Children` has changed, we need to recalculate everything below us
changed |= changed_children;
james7132 marked this conversation as resolved.
Show resolved Hide resolved
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<Transform>,
&mut GlobalTransform,
&Parent,
)>,
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>)>,
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);
}
*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)]
Expand All @@ -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};

Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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);
Expand Down