Skip to content

Conversation

@mintlu8
Copy link
Contributor

@mintlu8 mintlu8 commented May 31, 2024

Objective

Recursive iterating through the hierarchy while keeping a reference to parent requires unsafe, as seen in propagate_transform in bevy_transform. This PR alleviates this issue by generalizing recursive iteration into a safe interface.

Solution

Added QueryRecursive that contains 2 queries from 5 generic parameters.

root: Query<(Shared, Root), RootFilter>
child: Query<(Shared, Child), ChildFilter>

QueryData Shared gets passed from parent to children as a reference during iteration.

Additionally we support passing evaluation results from parent to children in iter_mut_with.

Example: transform pipeline (naive implementation)

fn propagate_transforms(mut query: QueryRecursive<
    (&Transform, &mut GlobalTransform),
    (),
    (),
    Without<Parent>,
    ()
>) {
    query.for_each_mut(
        |(transform, global_transform), ()|
            **global_transform = (**transform).into(),
        |(_, parent), (transform, global_transform), ()|
            **global_transform = parent.mul_transform(**transform),
    );
}

Testing

Added some tests.


Changelog

Added QueryRecursive.

@mintlu8 mintlu8 changed the title SystemParam QueryRecursive, a generalized version of propagate_transform . SystemParam QueryRecursive for safe recursive iteration. May 31, 2024
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events A-Hierarchy C-Feature A new feature, making something new possible M-Release-Note Work that should be called out in the blog due to impact X-Contentious There are nontrivial implications that should be thought through D-Complex Quite challenging from either a design or technical perspective. Ask for help! labels May 31, 2024
@mintlu8 mintlu8 marked this pull request as ready for review June 1, 2024 06:15
@alice-i-cecile alice-i-cecile added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Jun 2, 2024
@iiYese
Copy link
Contributor

iiYese commented Jun 2, 2024

Aside from the API being IMO a bit rough & needing more design the issue with this is it's a stopgap that can be done via third party. Specifically:

So the question then is what additional value would there be to providing this stopgap as a first party? Bare in mind this is already a kind of niche scenario simply due to the fact that hierarchy right now is very limited.

@mintlu8
Copy link
Contributor Author

mintlu8 commented Jun 3, 2024

Aside from the API being IMO a bit rough & needing more design the issue with this is it's a stopgap that can be done via third party.

I think this PR is very important since

  • Third party unsafe code has less scrutiny and therefore less trust worthy, especially if the author failed to account for changes in new bevy releases.

  • This feature is used in bevy itself, for instance the transform pipeline and visibility pipeline can be rewritten using this PR if proven safe. In the future if we upstream bevy_mod_billboard or add styling inheritance this can be useful as well.

  • This PR is not the full picture. A filter method for DFS and/or parallel iteration can be added to compliment QueryRecursive.

I welcome suggestion to changes in the API.

@BenjaminBrienen BenjaminBrienen added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible D-Complex Quite challenging from either a design or technical perspective. Ask for help! M-Release-Note Work that should be called out in the blog due to impact S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Contentious There are nontrivial implications that should be thought through

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants