Skip to content

Implement WorldQuery for MainWorld and RenderWorld components #15745

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

Merged

Conversation

alice-i-cecile
Copy link
Member

@alice-i-cecile alice-i-cecile commented Oct 8, 2024

Objective

#15320 is a particularly painful breaking change, and the new RenderEntity in particular is very noisy, with a lot of let entity = entity.id() spam.

Solution

Implement WorldQuery, QueryData and ReadOnlyQueryData for RenderEntity and WorldEntity.

These work the same as the Entity impls from a user-facing perspective: they simply return an owned (copied) Entity identifier. This dramatically reduces noise and eases migration.

Under the hood, these impls defer to the implementations for &T for everything other than the "call .id() for the user" bit, as they involve read-only access to component data. Doing it this way (as opposed to implementing a custom fetch, as tried in the first commit) dramatically reduces the maintenance risk of complex unsafe code outside of bevy_ecs.

To make this easier (and encourage users to do this themselves!), I've made ReadFetch and WriteFetch slightly more public: they're no longer doc(hidden). This is a good change, since trying to vendor the logic is much worse than just deferring to the existing tested impls.

Testing

I've run a handful of rendering examples (breakout, alien_cake_addict, auto_exposure, fog_volumes, box_shadow) and nothing broke.

Follow-up

We should lint for the uses of &RenderEntity and &MainEntity in queries: this is just less nice for no reason.

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward S-Needs-Testing Testing must be done before this is safe to merge labels Oct 8, 2024
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Oct 8, 2024
@alice-i-cecile alice-i-cecile added the D-Unsafe Touches with unsafe code in some way label Oct 8, 2024
Co-authored-by: Trashtalk217 <[email protected]>
Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

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

The implementation is valid, but I agree with @chescock on adjusting the implementation to explicitly forward to the underlying implementation in all cases.

Copy link
Contributor

@SpecificProtagonist SpecificProtagonist left a comment

Choose a reason for hiding this comment

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

This doc comment should be adjusted to mention the new impl instead.

@alice-i-cecile alice-i-cecile removed the S-Needs-Testing Testing must be done before this is safe to merge label Oct 13, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 13, 2024
Merged via the queue into bevyengine:main with commit a7e9330 Oct 13, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes D-Unsafe Touches with unsafe code in some way S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants