make the propagation logic generic

[tguichaoua]

Objective

Bevy now has two component pairs that are propagated through the hierarchy (Transform/GlobalTransform and Visibility/ComputedVisibility).
The objective of this PR is to create a generic system that performs the propagation logic to avoid to write boilerplate code.

Previous PR

This has already been proposed in #5507 and #4216.

What's different ?

• The trait is implemented on the main type (e.g. Transform) instead of the computed one that is generaly a prefixed or a suffixed version of the main type (e.g. GlobalTransform).
• These PR doesn't take in consideration the "recompute if changed" or "always recompute" behaviour. Especially for ComputedVisibility that sets is_visible_in_view to false.

Solution

This is achieved with the Propagate trait and a generic system propagate_system<T: Propagate> in bevy_hierarchy. This trait allow to customize how the propagation is performed.

The Propagate trait is implemented on the local component (see vocabulary section) and defines these methods:

• compute_root to update the computed component of the root entity from it's local one.
• compute_root to update the computed component from it's local one and the payload received from it's parent.
• payload to compute the payload to pass to children from it's computed component.

The propagation system has two possible behaviours:

• always recompute (like Visibility/ComputedVisibility).
• recompute only if the local component or an ancestor component changed (like Transform/GlobalTransform).

The choice of the behaviour can be implemented in two ways:

• with a const in the Propagate trait (current implemetation)
• with two variant of the system : propagate_is_changed_system and always_propagate_system.

Vocabulary

Local component

The component with the state of the entity (e.g. `Transform`, `Visibility`).

Computed component

The component that hold computed state (e.g. `GlobalTrasform`, `ComputedVisibility`).

Payload

The value passed from parent to child to compute it's state.

Questions to resolve

• Is the vocabulary ok ? Espcially is "local component" clear enough and not confusing ?
• Is the "always propagate" behaviour should be controlled via a const value in Propagate trait or by using two different systems propagate_is_changed_system and always_propagate_system ?

---

Changelog

• Added Propagate trait.
• Added generic system propagate_system.
• Removed transform_propagate_system.
• Removed visibility_propagate_system (was not public).
• Moved propagation related tests from bevy_transform to bevy_hierarchy.

Migration Guide

• transform_propagate_system has been removed.
• Added propagate_system and Propagate to create propagable component.

[tguichaoua]

I mark this PR as draft until questions to resolve are resolved.

[DJMcNab]

How does this compare to the other two prs which implement this - #5507 and #4216 ?

I couldn't see it in the main body.

[tguichaoua]

How does this compare to the other two prs which implement this - #5507 and #4216 ?

I couldn't see it in the main body.

I add a "Previous PR" section.

[DJMcNab]

Ah, yeah. Apologies

I was expecting the links to be highlighted in blue, so scanned for that.

[JoJoJet]

I like this approach a lot better than the previous PRs. Feels a lot more natural to express the relationship in this way.

[JoJoJet]

These type aliases are beautiful

[tguichaoua]

Possible implementation to control propagation behaviour

Disclaimer: I may be over thinking 😅
NOTE: this require GAT, so it's currently not possible.

POC : poc/propagate_behaviour_gat

What's different from the current implementation ?

The current implementation use a const boolean to switch from one behaviour to the other but queries are the same and request for types that may not be used (e.g. Changed<T> for the "always propagate" behaviour).

With this implementation, the behaviour is more flexible : control over the queries and the code.

Why not having a system per behaviour ?
This is an actual alternative having always_propagate_system<T> and propagate_if_changed_system<T>.
But with this proposition you can just use propagate_system<T> and let the compiler ✨automagically✨ give you the correct method.

Implementation

// Trait to define the behaviour
trait PropagationKind {
    type RootQuery<'w, 's, 'a, T>;
    type LocalQuery<'w, 's, 'a, T>;
    type ChildrenQuery<'w, 's, 'a, T>;

    fn propagate<'w, 's, 'a, T>(
        root_query: Self::RootQuery<'w, 's, 'a, T>,
        local_query: Self::LocalQuery<'w, 's, 'a, T>,
        children_query: Self::ChildrenQuery<'w, 's, 'a, T>,
    );
}

// Possible behaviour
struct AlwaysPropagate;
struct PropagateIfChanged;

impl PropagateKind for PropagateIfChanged {
    type RootQuery<'w, 's, 'a, T> = Query<
        'w,
        's,
        (
            Option<(&'static Children, Changed<Children>)>,
            &'static T,
            Changed<T>,
            &'static mut T::Computed,
            Entity,
        ),
        Without<Parent>,
    >;

    type LocalQuery<'w, 's, 'a, T> = Query<
        'w,
        's,
        (
            &'a T,
            Changed<T>,
            &'a mut <T as Propagate>::Computed,
            &'a Parent,
        ),
    >;

    type ChildrenQuery<'w, 's, 'a, T> = Query<
        'w,
        's,
        (&'a Children, Changed<Children>),
        (With<Parent>, With<<T as Propagate>::Computed>),
    >;

    fn propagate<'w, 's, 'a, T>(
        mut root_query: Self::RootQuery<'w, 's, 'a, T>,
        mut local_query: Self::LocalQuery<'w, 's, 'a, T>,
        children_query: Self::ChildrenQuery<'w, 's, 'a, T>,
    ) {
         // we use the queries here and perform the propagation.
    }
}

pub trait Propagate: Component {
    type Computed: Component;
    type Payload;

    // The kind is defined by this type.
    type Kind: PropagateKind;

    fn compute_root(computed: &mut Self::Computed, local: &Self);
    fn compute(computed: &mut Self::Computed, payload: &Self::Payload, local: &Self);
    fn payload(computed: &Self::Computed) -> Self::Payload;
}

pub fn propagate_system<'w, 's, 'a, T: Propagate>(
    root_query: <T::Kind as PropagateKind>::RootQuery<'w, 's, 'a, T>,
    local_query: <T::Kind as PropagateKind>::LocalQuery<'w, 's, 'a, T>,
    children_query: <T::Kind as PropagateKind>::ChildrenQuery<'w, 's, 'a, T>,
) {
    T::Kind::propagate(root_query, local_query, children_query);
}

Usage

impl Propagate for Transform {
    type Computed = GlobalTransform;
    type Payload = GlobalTransform;
    type Kind = PropagateIfChanged;

    /* ... */
}

[JoJoJet]

Would it be doable to have a Filter associated type? Something along the lines of

impl Propagate for Transform {
    type Filter = Changed<Self>;
    ...
}

impl Propagate for Visibility {
    type Filter = AlwaysTrue;
    ...
}

[tguichaoua]

Would it be doable to have a Filter associated type? Something along the lines of

impl Propagate for Transform {
    type Filter = Changed<Self>;
    ...
}

impl Propagate for Visibility {
    type Filter = AlwaysTrue;
    ...
}

Yes, it's doable but AFAIK there is no static way to prevent Changed to be used with another type than Self.
It's leads to invalid implementation :

impl Propagate for Foo {
    type Filter = Changed<Goo>;
    ...
}

[JoJoJet]

Yes, it's doable but AFAIK there is no static way to prevent Changed to be used with another type than Self.

Not that it's up to me, but that should be acceptable IMO. If you implement it incorrectly, it'll lead to incorrect behavior. As long as it doesn't cause UB, that's fine.

[tguichaoua]

Yes, it's doable but AFAIK there is no static way to prevent Changed to be used with another type than Self.

Not that it's up to me, but that should be acceptable IMO. If you implement it incorrectly, it'll lead to incorrect behavior. As long as it doesn't cause UB, that's fine.

If the doc is clear enough, I think it's acceptable.
Generally I prefer solution that is statically checked when it's possible.

[Aceeri]

Suggested change

	/// Marks a component as propagatable thrown hierachy alike `Transform`/`GlobalTransorm`
	/// Marks a component as propagatable through hierarchy similar to `Transform`/`GlobalTransform`

[alice-i-cecile]

@tguichaoua I'd like to try and move forward with this, are you around to rebase and work on this?

No worries if not, we'll have someone else tackle it and keep your credit.

[tguichaoua]

@alice-i-cecile I'm sorry, I'm not available to work on this.

[JoJoJet]

I am willing to adopt this, unless @Aceeri would rather.

[mockersf]

I'm not sure there was a consensus on which way forward to go between this, #5507 and #4216.

Also, the propagations don't follow the same logic anymore as transform is now done in parallel and visibility is not. Does it make sense to parallelise visibility too? I'm not sure, it would have to be benchmarked.

[JoJoJet]

There was a discussion about this on discord, linking for posterity: https://discordapp.com/channels/691052431525675048/692572690833473578/1063191826594549760

@james7132:

We may need a few variants: with/without change detection, and single vs multithreaded.
Visibility right now doesn't need to be multithreaded due to how cheap it is to propagate.
But transform propagation needs to be parallel due to how large the structs are and how heavy the operation of propagation is

[Aceeri]

I am willing to adopt this, unless @Aceeri would rather.

Feel free if you'd like, I just wanted to get this stuff in for some future work related to splitting hierarchies

[mockersf]

@Aceeri could you describe in a new issue those future works?

Without more details I would be reluctant to make it generic now that we have only two cases and they are different...

[Aceeri]

@Aceeri could you describe in a new issue those future works?

Without more details I would be reluctant to make it generic now that we have only two cases and they are different...

Ya, will do so early tomorrow, the gist of it is that I'd like a way to opt out of these hierarchies but keep the organization of them. Since for example I would like the arms of my player to still be "children" of the player, but I don't want them to sync to the transform since that will override the physics joints doing their thing. So I was thinking something along the lines of Split::<T> where T: Propagate which would essentially ignore it when traversing downwards into children.

An alternative here would be to have propagation logic be opt in and just have the TransformBundle have a marker component for that but I'm not sure how common this will be.

[bas-ie]

Backlog cleanup: lack of activity (although attempted adoption last year) and ComputedVisibility no longer exists. Consider also discussion in #1680 for further context.