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.

Sign up for GitHub

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

Jump to bottom

Implement WorldQuery and QueryData on Mut. #13338

Merged
merged 5 commits into from May 14, 2024
Merged

Implement WorldQuery and QueryData on Mut. #13338

merged 5 commits into from May 14, 2024

Conversation

Themayu
Copy link
Contributor

@Themayu Themayu commented May 12, 2024 •
edited

Objective

Provides a WorldQuery implementation on Mut<T> that forwards to the implementation on &mut T, and give users a way to opt-in to change detection in auto-generated QueryData::ReadOnly types.

Fixes #13329.

Solution

I implemented WorldQuery on Mut<'w, T> as a forwarding implementation to &mut T, setting the QueryData::ReadOnly associated type to Ref<'w, T>. This provides users the ability to explicitly opt-in to change detection in the read-only forms of queries.

Testing

A documentation test was added to Mut showcasing the new functionality.

Changelog

Added

  • Added an implementation of WorldQuery and QueryData on bevy_ecs::change_detection::Mut.

@SkiFire13
Copy link
Contributor

This could reuse WriteFetch and forward all the methods to the implementation for &mut T, reducing code and maintenance.

@Themayu
Copy link
Contributor Author

Themayu commented May 12, 2024 •
edited

I was originally thinking of doing that, but was put off of doing so by the fact that the implementation on Ref<'w, T> doesn't forward to &'w T. Because of this, I assumed a re-implementation was preferred over a forwarding implementation.

@SkiFire13
Copy link
Contributor

The implementation for &'_ T doesn't forward to the one for Ref<'_, T> because they are different. However the implementations for &'_ mut T and Mut<'_, T> are the same, the only difference is the ReadOnly associated type.

@Themayu Themayu force-pushed the 13329-worldquery-mut branch 2 times, most recently from 7068559 to 73d2104 Compare May 13, 2024 01:42
@Themayu
Copy link
Contributor Author

Themayu commented May 13, 2024 •
edited

Don't remember what the Bevy project does to deal with out-of-date PRs, so I used the github UI to rebase on top of main, which is why the three commits were suddenly repeated.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use X-Contentious There are nontrivial implications that should be thought through D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels May 13, 2024
@alice-i-cecile
Copy link
Member

Yeah, we don't have strong feelings on rebasing vs. merging: do whatever is easiest for you.

I agree with @SkiFire13's suggestion above: reducing code duplication is important here.

alice-i-cecile
alice-i-cecile requested changes May 13, 2024
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Solid stuff. A couple of improvements to the documentation, then this LGTM.

crates/bevy_ecs/src/query/fetch.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/change_detection.rs Outdated Show resolved Hide resolved
@Themayu
Implement WorldQuery and QueryData on Mut.
9eeecf2 
Provides a `WorldQuery` implementation on `Mut<T>` to mirror the
implementation on `&mut T`, and give users a way to opt-in to change
detection in auto-generated `QueryData::ReadOnly` types.
@Themayu
Forward Mut<T> WorldQuery implementation to &mut T.
449ed67 
This replaces the manual copy of `&mut T`'s WorldQuery implementation
on `Mut<T>` with a version that simply forwards to `&mut T`. Most
trait methods call their respective methods on the forwarding target,
though to avoid erroneous references to `&mut T` in the assert message
for component access conflicts, `update_component_access` has been left
unchanged.
@Themayu
Fix clippy::semicolon-if-nothing-returned
3e85504
@Themayu
Copy link
Contributor Author

Themayu commented May 14, 2024

Oh. #13343 happened. Welp, time to fix my code.

@Themayu
Copy link
Contributor Author

Themayu commented May 14, 2024

There we go.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 14, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 14, 2024
Merged via the queue into bevyengine:main with commit dcf24df May 14, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@SkiFire13 SkiFire13 SkiFire13 approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it 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.

Implement WorldQuery and QueryData for Mut
3 participants
@Themayu @SkiFire13 @alice-i-cecile