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

Implement ReadOnlyQueryData for Parent and Children #15049

Conversation

bushrat011899
Copy link
Contributor

Objective

  • Improve the ergonomics of Parent and Children in queries.
  • Act on a conversation had on Discord with @ItsDoot

Solution

Add an implementation of ReadOnlyQueryData (and the prerequisite traits) for Parent and Children. This does not conflict with the derived implementations from Component, allowing the wrapping types to still be accessed via &T and &mut T as currently.

Testing

  • CI passed locally.

Showcase

Before
fn foo(foo_query: Query<(&Foo, &Parent)>, bar_query: Query<&Bar>) {
    for (foo, parent) in foo_query.iter() {
        let parent = parent.get();
        let bar = bar_query.get(parent).unwrap();
        do_something(foo, bar);
    }
}
After
fn foo(foo_query: Query<(&Foo, Parent)>, bar_query: Query<&Bar>) {
    for (foo, parent) in foo_query.iter() {
        let bar = bar_query.get(parent).unwrap();
        do_something(foo, bar);
    }
}

As can be seen above, the change in end-user code is minimal, but positive. It's unfortunate that it requires an unsafe implementation since we're able to delegate all the actually unsafe operations to the known-safe derived implementation from Component. But I think it's worth it for this (mild) improvement.

Uses the derived implementation based on `&T where T: Component`. This allows users to directly receive an `Entity` or a `&[Entity]` rather than needing to call methods on `Parent` or `Children` respectively.
@bushrat011899 bushrat011899 added D-Trivial Nice and easy! A great choice to get started with Bevy A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Hierarchy Parent-child entity hierarchies X-Contentious There are nontrivial implications that should be thought through D-Unsafe Touches with unsafe code in some way S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 5, 2024
@alice-i-cecile alice-i-cecile added D-Straightforward Simple bug fixes and API improvements, docs, test and examples and removed D-Trivial Nice and easy! A great choice to get started with Bevy labels Sep 5, 2024
@alice-i-cecile
Copy link
Member

Hmm. I don't think I like this: it adds user-facing complexity and an inconsistent API for a pretty modest ergonomic gain.

@alice-i-cecile alice-i-cecile added X-Controversial There is active debate or serious implications around merging this PR and removed X-Contentious There are nontrivial implications that should be thought through labels Sep 5, 2024
@bushrat011899
Copy link
Contributor Author

Totally reasonable, I'm not too confident this is something that would be popular enough to get merged. I just wanted to try it out since it was on my mind anyway 😁

@ItsDoot
Copy link
Contributor

ItsDoot commented Sep 8, 2024

To clarify what I meant originally, I intended that it be taken a step further and remove the ability to use &Parent/&mut Parent/&Children/&mut Children, rather than just introducing the non-reference version. This is inevitable anyways once we introduce relations, as all hierarchy management will likely be done through commands instead (i.e. moving the sort functions currently on Children into a command).

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 A-Hierarchy Parent-child entity hierarchies C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples D-Unsafe Touches with unsafe code in some way S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants