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

QuerySingle family of system params #15476

Merged
merged 20 commits into from
Sep 28, 2024
Merged

Conversation

MiniaczQ
Copy link
Contributor

@MiniaczQ MiniaczQ commented Sep 27, 2024

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: #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 #15391.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events M-Needs-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 alice-i-cecile added this to the 0.15 milestone Sep 27, 2024
@MiniaczQ MiniaczQ added C-Feature A new feature, making something new possible M-Needs-Release-Note Work that should be called out in the blog due to impact S-Needs-Help The author needs help finishing this PR. and removed C-Feature A new feature, making something new possible M-Needs-Release-Note Work that should be called out in the blog due to impact S-Needs-Help The author needs help finishing this PR. labels Sep 27, 2024
@alice-i-cecile 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
Copy link
Contributor

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.

@MiniaczQ MiniaczQ removed the S-Needs-Help The author needs help finishing this PR. label Sep 27, 2024
@MiniaczQ MiniaczQ marked this pull request as ready for review September 27, 2024 21:55
@MiniaczQ MiniaczQ added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Sep 27, 2024
@alice-i-cecile 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
crates/bevy_ecs/src/system/system_param.rs Outdated Show resolved Hide resolved
Comment on lines +420 to +421
unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam
for Option<QuerySingle<'a, D, F>>
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@MiniaczQ MiniaczQ Sep 28, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, infinitely nested Options

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

state.as_readonly().get_single_unchecked_manual(
world,
system_meta.last_run,
world.change_tick(),
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@MiniaczQ 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 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 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
for QuerySingle<'a, D, F>
{
type State = QueryState<D, F>;
type Item<'w, 's> = QuerySingle<'w, D, F>;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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`

@alice-i-cecile 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
30 checks passed
robtfm pushed a commit to robtfm/bevy that referenced this pull request Oct 4, 2024
# 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.
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 C-Feature A new feature, making something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes D-Unsafe Touches with unsafe code in some way M-Needs-Release-Note Work that should be called out in the blog due to impact S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce QuerySingle<Q, F> family of system parameters
5 participants