`QuerySingle` family of system params by MiniaczQ · Pull Request #15476 · bevyengine/bevy

MiniaczQ · 2024-09-27T19:46:39Z

Objective

Add the following system params:

QuerySingle<D, F> - Valid if only one matching entity exists,
Option<QuerySingle<D, F>> - Valid if zero or one matching entity exists.

As @chescock pointed out, we don't need Mut variants.

Solution

Implement the type and both variants of system params.
Also implement ReadOnlySystemParam for readonly queries.

Added a new ECS example fallible_params which showcases SingleQuery usage.
In the future we might want to add NonEmptyQuery, NonEmptyEventReader and Res to it (or maybe just stop at mentioning it).

Testing

Tested with the example.
There is a lot of warning spam so we might want to implement #15391.

examples/ecs/falliable_params.rs

github-actions · 2024-09-27T19:59:25Z

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

crates/bevy_ecs/src/system/system_param.rs

SkiFire13 · 2024-09-28T13:25:39Z

crates/bevy_ecs/src/system/system_param.rs

+unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam
+    for Option<QuerySingle<'a, D, F>>


I wonder if now that we have validate_param we could use it to write a generic impl<S: SystemParam> SystemParam for Option<S>. This could be tackled in a separate PR though.

Not every Option<P> makes sense, like Option<Query>

Also, infinitely nested Options

Right, though maybe this could be solved with a marker trait for those system parameters for which Option can make sense.

If there is a decent way to derive it I'm down.
There are some cases where Option needs some specialized code, like in QuerySingle

SkiFire13 · 2024-09-28T13:26:39Z

crates/bevy_ecs/src/system/system_param.rs

+            state.as_readonly().get_single_unchecked_manual(
+                world,
+                system_meta.last_run,
+                world.change_tick(),


Are we sure this is the change tick that will be used for calling get_param, or an equivalent one for what matters to it? If so could you write a comment explaining why that's the case?

Practically yes, this happens to be the exact tick that is received by get_param, but there are not validation mechanisms/contract for that.

Currently only executors use validate_param and since the validation and running happens one after another, this is the correct change tick.
For run_systems and observers this also shouldn't be a problem.

While I can make validate_param accept a change_tick, the problem is that the tick used by get_param is generated inside run_unsafe, so we'd still need to uphold this "contract" of calling them back-to-back.

I'm open for docs/mechanisms on making this better, I'm not sure how to go about it

It won't always be exactly the same tick, because the multi-threaded executor will call validate_param and then spawn a task to run the system and call get_param at some later point. Another system could start in between and increment the current tick.

I think it will be equivalent, though. Any components read by this param can't be written by another system in between the calls, so they can't have an added or changed tick in between the two change_tick values.

Okay, I'll try to thoroughly document this in validate_param

alice-i-cecile · 2024-09-28T19:16:05Z

crates/bevy_ecs/src/system/system_param.rs

+    for QuerySingle<'a, D, F>
+{
+    type State = QueryState<D, F>;
+    type Item<'w, 's> = QuerySingle<'w, D, F>;


Why isn't this just <D as QueryData>::Item? I think that would improve the ergonomics and complexity quite a bit?

The ergonomics comment got removed, CI told me to remove unnecessary syntax which made it way better.

<D as WorldQuery>::Item is not a SystemParam so that's one issue with this.
Another would be, if SystemParam returns anything else than itself with changed lifetimes, it won't fit as the system argument.

fn my_system(q: QuerySingle<Entity>) { ... } // imaginary executor code let q: <Entity as WorldQuery>::Item = QuerySingle<Entity>::get_param(world); system(q); // Expected `QuerySingle` got `<Entity as WorldQuery>::Item`

@chescock

# Objective Add the following system params: - `QuerySingle<D, F>` - Valid if only one matching entity exists, - `Option<QuerySingle<D, F>>` - Valid if zero or one matching entity exists. As @chescock pointed out, we don't need `Mut` variants. Fixes: bevyengine#15264 ## Solution Implement the type and both variants of system params. Also implement `ReadOnlySystemParam` for readonly queries. Added a new ECS example `fallible_params` which showcases `SingleQuery` usage. In the future we might want to add `NonEmptyQuery`, `NonEmptyEventReader` and `Res` to it (or maybe just stop at mentioning it). ## Testing Tested with the example. There is a lot of warning spam so we might want to implement bevyengine#15391.

alice-i-cecile · 2024-10-20T14:34:32Z

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#1701 if you'd like to help out.

impl and example drafts

74d8f3d

alice-i-cecile added A-ECS Entities, components, systems, and events M-Release-Note Work that should be called out in the blog due to impact S-Needs-Help The author needs help finishing this PR. C-Feature A new feature, making something new possible labels Sep 27, 2024

alice-i-cecile added this to the 0.15 milestone Sep 27, 2024

alice-i-cecile added X-Uncontroversial This work is generally agreed upon D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes D-Unsafe Touches with unsafe code in some way labels Sep 27, 2024

alice-i-cecile reviewed Sep 27, 2024

View reviewed changes

examples/ecs/falliable_params.rs Outdated Show resolved Hide resolved

MiniaczQ added 4 commits September 27, 2024 23:01

its called fallible

45a61ec

fix lint

0ae06f4

fix warnings

8010a0b

update safety comments

859969a

MiniaczQ removed the S-Needs-Help The author needs help finishing this PR. label Sep 27, 2024

MiniaczQ added 2 commits September 27, 2024 23:43

document example

8d0af32

refine docs

125002a

MiniaczQ force-pushed the query_single branch from 7896012 to 125002a Compare September 27, 2024 21:55

MiniaczQ marked this pull request as ready for review September 27, 2024 21:55

MiniaczQ added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Sep 27, 2024

MiniaczQ added 3 commits September 27, 2024 23:56

Merge branch 'main' into query_single

81a3dbd

fixup docs

1cd8a20

fix doc

46226aa

fix docs

7ff336f

chescock approved these changes Sep 28, 2024

View reviewed changes

ItsDoot reviewed Sep 28, 2024

View reviewed changes

crates/bevy_ecs/src/system/system_param.rs Outdated Show resolved Hide resolved

minor doc change

1b84446

MiniaczQ mentioned this pull request Sep 28, 2024

Populated (query) system param #15488

Merged

ItsDoot reviewed Sep 28, 2024

View reviewed changes

crates/bevy_ecs/src/system/system_param.rs Outdated Show resolved Hide resolved

MiniaczQ force-pushed the query_single branch from 4bd9cd3 to 1b84446 Compare September 28, 2024 03:01

ItsDoot approved these changes Sep 28, 2024

View reviewed changes

alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 28, 2024

SkiFire13 reviewed Sep 28, 2024

View reviewed changes

MiniaczQ removed 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 Sep 28, 2024

alice-i-cecile added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Sep 28, 2024

MiniaczQ added 3 commits September 28, 2024 17:05

Merge branch 'main' into query_single

e48b003

document edge case and expect

8df8521

fix docs

c834921

MiniaczQ added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Sep 28, 2024

better doc for change tick

9d0ab0f

MiniaczQ force-pushed the query_single branch from 7509d02 to 9d0ab0f Compare September 28, 2024 15:42

Merge branch 'main' into query_single

4c0ccb2

MiniaczQ mentioned this pull request Sep 28, 2024

Better warnings about invalid parameters #15500

Merged

alice-i-cecile reviewed Sep 28, 2024

View reviewed changes

alice-i-cecile approved these changes Sep 28, 2024

View reviewed changes

alice-i-cecile added this pull request to the merge queue Sep 28, 2024

Merged via the queue into bevyengine:main with commit c148665 Sep 28, 2024

alice-i-cecile mentioned this pull request Oct 20, 2024

Write release notes for PR #15476: QuerySingle family of system params bevyengine/bevy-website#1701

Closed

		unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam
		for Option<QuerySingle<'a, D, F>>

Uh oh!

Conversation

MiniaczQ commented Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Testing

Uh oh!

Uh oh!

github-actions bot commented Sep 27, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MiniaczQ Sep 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile commented Oct 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MiniaczQ commented Sep 27, 2024 •

edited

Loading

MiniaczQ Sep 28, 2024 •

edited

Loading