Populated (query) system param
#15488
Conversation
alice-i-cecile
added
C-Feature
MiniaczQ
added
D-Unsafe
MiniaczQ
force-pushed
the
query-non-empty
branch
from
523f599 to
64b27aa
Compare
alice-i-cecile
added
S-Waiting-on-Author
MiniaczQ
added
S-Needs-Review
|
It looks fine but this is too much unsafe for me to have any idea if it is correct |
| archetype: &Archetype, | ||
| system_meta: &mut SystemMeta, | ||
| ) { | ||
| // SAFETY: Delegate to existing `SystemParam` implementations. |
There was a problem hiding this comment.
SAFETY: Delegate to existing ``Query`` implementation.?
There was a problem hiding this comment.
But then the doc comment says exactly what's written
The point is, we use a different SystemParam implementation which satisfies the same safety restrictions
| world: UnsafeWorldCell<'w>, | ||
| change_tick: Tick, | ||
| ) -> Self::Item<'w, 's> { | ||
| // SAFETY: Delegate to existing `SystemParam` implementations. |
| state.validate_world(world.id()); | ||
| // SAFETY: | ||
| // - We have read-only access to the components accessed by query. | ||
| // - The world has been validated. |
There was a problem hiding this comment.
Might be clearer to say something like, The world matches the one used to create ``state``. (Just means the reader doesn't need to look at validate_world to know what it does)
There was a problem hiding this comment.
it's a common term in ECS safety comments,
I'm pretty sure all of the safety comments here are copy-pasted from other implementations
| let query = unsafe { | ||
| Query::<'world, 'state, D, F>::get_param(state, system_meta, world, change_tick) | ||
| }; | ||
| QueryNonEmpty(query) |
There was a problem hiding this comment.
Is the plan for QueryNonEmpty to have some of its own methods or could it just return Query as its Item as its logic is just during validation?
There was a problem hiding this comment.
It's just a newtyped Query, we can modify it in the future to be it's own thing which provides non-empty-based methods
There was a problem hiding this comment.
But I'd expect this to also require extracting traits for shared methods, bit too much work for this PR and not the focus
|
How about |
I'm afraid that has different meaning from intended |
alice-i-cecile
added
S-Ready-For-Final-Review
Co-authored-by: Alice Cecile <[email protected]>
# Objective Add a `Populated` system parameter that acts like `Query`, but prevents system from running if there are no matching entities. Fixes: bevyengine#15302 ## Solution Implement the system param which newtypes the `Query`. The only change is new validation, which fails if query is empty. The new system param is used in `fallible_params` example. ## Testing Ran `fallible_params` example. --------- Co-authored-by: Alice Cecile <[email protected]>
|
Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1706 if you'd like to help out. |
Objective
Add a
Populatedsystem parameter that acts likeQuery, but prevents system from running if there are no matching entities.Fixes: #15302
Solution
Implement the system param which newtypes the
Query.The only change is new validation, which fails if query is empty.
The new system param is used in
fallible_paramsexample.Testing
Ran
fallible_paramsexample.