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

Iterative Transform Propagation #4203

Closed
Closed
Show file tree
Hide file tree
Changes from 14 commits
Commits
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
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 @@ -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 labelled
/// [`TransformSystem::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 @@ -19,8 +19,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 labelled
/// [`TransformSystem::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
17 changes: 14 additions & 3 deletions crates/bevy_transform/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 labelled
/// [`TransformSystem::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 @@ -99,11 +98,23 @@ impl Plugin for TransformPlugin {
.label(TransformSystem::TransformPropagate)
.after(HierarchySystem::ParentUpdate),
)
.add_startup_system_to_stage(
StartupStage::PostStartup,
systems::transform_propagate_flat_system
.label(TransformSystem::TransformPropagate)
.after(HierarchySystem::ParentUpdate),
)
.add_system_to_stage(
CoreStage::PostUpdate,
systems::transform_propagate_system
.label(TransformSystem::TransformPropagate)
.after(HierarchySystem::ParentUpdate),
)
.add_system_to_stage(
CoreStage::PostUpdate,
systems::transform_propagate_flat_system
.label(TransformSystem::TransformPropagate)
.after(HierarchySystem::ParentUpdate),
);
}
}
133 changes: 77 additions & 56 deletions crates/bevy_transform/src/systems.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,76 +2,97 @@ use crate::components::{GlobalTransform, Transform};
use bevy_ecs::{
entity::Entity,
query::{Changed, With, Without},
system::Query,
system::{Local, Query},
};
use bevy_hierarchy::{Children, Parent};

pub(crate) struct Pending {
parent: *const GlobalTransform,
james7132 marked this conversation as resolved.
Show resolved Hide resolved
changed: bool,
child: Entity,
}

// SAFE: Access to the parent pointer is only usable in this module, the values
// are cleared after every system execution, and the system only uses one
// thread. There is no way to move this type across multiple threads.
unsafe impl Send for Pending {}
// SAFE: Access to the parent pointer is only usable in this module, the values
// are cleared after every system execution, and the system only uses one
// thread. There is no way to access this type across multiple threads.
unsafe impl Sync for Pending {}

/// Update [`GlobalTransform`] component of entities without children or parents
/// based on the [`Transform`] component.
pub(crate) fn transform_propagate_flat_system(
mut root_query: Query<
(&Transform, &mut GlobalTransform),
(Without<Parent>, Without<Children>, Changed<Transform>),
>,
) {
root_query.for_each_mut(|(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(crate) fn transform_propagate_system(
mut root_query: Query<
(Entity, Option<&Children>, &Transform, &mut GlobalTransform),
(
&Transform,
Changed<Transform>,
&Children,
&mut GlobalTransform,
),
Without<Parent>,
>,
mut transform_query: Query<(&Transform, &mut GlobalTransform), With<Parent>>,
changed_transform_query: Query<Entity, Changed<Transform>>,
children_query: Query<Option<&Children>, (With<Parent>, With<GlobalTransform>)>,
mut transform_query: Query<
(
&Transform,
Changed<Transform>,
Option<&Children>,
&mut GlobalTransform,
),
With<Parent>,
>,
// Stack space for the depth-first search of a given hierarchy. Used as a Local to
// avoid reallocating the stack space used here.
Copy link
Contributor

Choose a reason for hiding this comment

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

Heap space?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's technically used as a stack, just not the stack.

mut pending: Local<Vec<Pending>>,
) {
for (entity, children, transform, mut global_transform) in root_query.iter_mut() {
let mut changed = false;
if changed_transform_query.get(entity).is_ok() {
root_query.for_each_mut(|(transform, changed, children, mut global_transform)| {
if changed {
*global_transform = GlobalTransform::from(*transform);
changed = true;
}

if let Some(children) = children {
for child in children.iter() {
propagate_recursive(
&global_transform,
&changed_transform_query,
&mut transform_query,
&children_query,
*child,
changed,
);
pending.extend(children.iter().map(|child| Pending {
parent: &*global_transform as *const GlobalTransform,
changed,
child: *child,
}));

while let Some(current) = pending.pop() {
if let Ok((transform, mut changed, children, mut global_transform)) =
transform_query.get_mut(current.child)
{
changed |= current.changed;
if changed {
// SAFE: The pointers here are generated only during this one traversal
// from one given run of the system.
unsafe {
*global_transform = current.parent.read().mul_transform(*transform);
Copy link
Member

Choose a reason for hiding this comment

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

I believe this may be unsound. Consider a (malformed) entity whose parent is itself:
You'd read the current.parent, which would be pointing to the same GlobalTransform as global_transform
But global_transform holds a mutable reference to that GlobalTransform, which means you've read something to which a mutable reference already exists.
(Note that in the old code, this case would merely stack overflow)

};
}
if let Some(children) = children {
pending.extend(children.iter().map(|child| Pending {
parent: &*global_transform as *const GlobalTransform,
changed: current.changed,
child: *child,
}));
}
}
}
}
}

fn propagate_recursive(
parent: &GlobalTransform,
changed_transform_query: &Query<Entity, Changed<Transform>>,
transform_query: &mut Query<(&Transform, &mut GlobalTransform), With<Parent>>,
children_query: &Query<Option<&Children>, (With<Parent>, With<GlobalTransform>)>,
entity: Entity,
mut changed: bool,
) {
changed |= changed_transform_query.get(entity).is_ok();

let global_matrix = {
if let Ok((transform, mut global_transform)) = transform_query.get_mut(entity) {
if changed {
*global_transform = parent.mul_transform(*transform);
}
*global_transform
} else {
return;
}
};

if let Ok(Some(children)) = children_query.get(entity) {
for child in children.iter() {
propagate_recursive(
&global_matrix,
changed_transform_query,
transform_query,
children_query,
*child,
changed,
);
}
}
});
debug_assert!(pending.is_empty());
}

#[cfg(test)]
Expand Down