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

[Merged by Bors] - Parallelized transform propagation #4775

Closed
wants to merge 13 commits into from
Closed

[Merged by Bors] - Parallelized transform propagation #4775

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
4e5ae27
Parallelize transform propagation
james7132 May 16, 2022
ff0f0bc
build fixes
james7132 May 16, 2022
925c360
Merge branch 'main' into parallelized-transform-propagation
james7132 Jun 2, 2022
80b2a33
Change batch size to 1
james7132 Jun 2, 2022
8f28b3d
Review changes and Fix CI
james7132 Jun 3, 2022
40c9a4e
Merge branch 'main' into parallelized-transform-propagation
james7132 Nov 6, 2022
fd483d0
Fix CI
james7132 Nov 7, 2022
73fee07
Merge branch 'main' into parallelized-transform-propagation
james7132 Nov 20, 2022
73ec563
Use let else where possible
james7132 Nov 20, 2022
efbbae2
Parallelize the parentless case
james7132 Nov 20, 2022
4cec581
Provide much more justification for the get_unchecked_mut
james7132 Nov 21, 2022
691f99a
Merge branch 'main' into parallelized-transform-propagation
james7132 Nov 21, 2022
e032f9d
Enable clippy warning
james7132 Nov 21, 2022
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
1 change: 1 addition & 0 deletions crates/bevy_transform/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" }
12 changes: 12 additions & 0 deletions crates/bevy_transform/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,12 +93,24 @@ impl Plugin for TransformPlugin {
app.register_type::<Transform>()
.register_type::<GlobalTransform>()
// 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
Expand Down
91 changes: 53 additions & 38 deletions crates/bevy_transform/src/systems.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,75 +1,84 @@
use crate::components::{GlobalTransform, Transform};
use bevy_ecs::prelude::{Changed, Entity, Query, With, Without};
use bevy_ecs::prelude::{Changed, Entity, Or, 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(
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.

>,
) {
for (transform, mut global_transform) in query.iter_mut() {
*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(
mut root_query: Query<
(
Option<(&Children, Changed<Children>)>,
Entity,
&Children,
&Transform,
Changed<Transform>,
Or<(Changed<Children>, Changed<Transform>)>,
&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(
1,
|(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,
&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,
)>,
transform_query: &Query<(&Transform, Changed<Transform>, &mut GlobalTransform), With<Parent>>,
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should rename this to something like unsafe_transform_query here (and above). The requirements for this to be safely accessed are quite subtle - even Query::get is probably UB.
This seems like the kind of code which would be blindly copied for user's custom hierarchy, so we can at least nudge the gun away from the foot-direction with wording here.

We really need UnsafeCell equivalent queries - they're on the list.

Copy link
Member

Choose a reason for hiding this comment

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

Could/should we make this system generic? - that would avoid the copy/paste problem

We couldn't allow arbitrary hierarchies, since the Parent equivalent component could have a malicious Deref impl to make the check always pass. However, I see no reason that this couldn't access arbitrary other components.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is done in #4216, which we can merge before or after this PR.

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 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);
Expand All @@ -84,9 +93,10 @@ fn propagate_recursive(
let _ = propagate_recursive(
&global_matrix,
transform_query,
parent_query,
children_query,
*child,
entity,
*child,
changed,
);
}
Expand All @@ -101,7 +111,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,
Expand All @@ -113,6 +123,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();
Expand Down Expand Up @@ -158,6 +169,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();
Expand Down Expand Up @@ -201,6 +213,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();
Expand Down Expand Up @@ -286,6 +299,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);
Expand Down Expand Up @@ -342,6 +356,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
Expand Down