Skip to content

SystemParam QueryRecursive for safe recursive iteration.#13607

Open
mintlu8 wants to merge 14 commits intobevyengine:mainfrom
mintlu8:recursive_query
Open

SystemParam QueryRecursive for safe recursive iteration.#13607
mintlu8 wants to merge 14 commits intobevyengine:mainfrom
mintlu8:recursive_query

Conversation

@mintlu8
Copy link
Copy Markdown
Contributor

@mintlu8 mintlu8 commented May 31, 2024 •
edited
Loading

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
Copy Markdown
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
Copy Markdown
Contributor Author

mintlu8 commented Jun 3, 2024 •
edited
Loading

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
@Victoronz Victoronz mentioned this pull request Feb 12, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mintlu8 @iiYese @alice-i-cecile @BenjaminBrienen