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

Introduce QuerySingle<Q, F> family of system parameters #15264

Closed
MiniaczQ opened this issue Sep 17, 2024 · 8 comments · Fixed by #15476
Closed

Introduce QuerySingle<Q, F> family of system parameters #15264

MiniaczQ opened this issue Sep 17, 2024 · 8 comments · Fixed by #15476
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 S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! X-Uncontroversial This work is generally agreed upon

Comments

@MiniaczQ
Copy link
Contributor

MiniaczQ commented Sep 17, 2024

What problem does this solve or what need does it fill?

Following #15276 we should expose new system params that benefit from no panics.
Specifically, we should support entities that act as resources.
We can provide system parameters that are equivalents of Query::single family of methods.

What solution would you like?

Add the following system parameters:

  • QuerySingle<Q, F> - valid with exactly one result
  • Option<QuerySingle<Q, F>> - valid with zero or one result
@MiniaczQ MiniaczQ added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Sep 17, 2024
@MiniaczQ
Copy link
Contributor Author

I'll be happy to implement it if someone gives me some directions

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events X-Controversial There is active debate or serious implications around merging this PR and removed S-Needs-Triage This issue needs to be labelled labels Sep 17, 2024
@alice-i-cecile
Copy link
Member

So I quite like this idea from an ergonomics standpoint, and would use it in my own personal projects.

I'm loathe to expose more panicking APIs though, see #14275. I'd be interested in seeing Single return an Option. I'd also be nervous to use this in learning materials other than that dedicated to this concept, since teaching users a single blessed tool is really helpful conceptually.

@MiniaczQ
Copy link
Contributor Author

Until #14275 gets resolved I feel like we should stick to the existing convention.
The scope is already massive, one more feature won't make a dent in the breaking changes

@alice-i-cecile
Copy link
Member

I'm fine with that. I still think that this suggestion is controversial, independent of #14275, because of the impact on new learners / style due to introducing a redundant method to do the same thing.

@janhohenheim
Copy link
Member

@alice-i-cecile imo we should not introduce more Options, but only use Results where possible in new API. Otherwise we will have to change it again once we an error handling overhaul happens.

@MiniaczQ
Copy link
Contributor Author

@alice-i-cecile imo we should not introduce more Options, but only use Results where possible in new API. Otherwise we will have to change it again once we an error handling overhaul happens.

This is exactly the problem I have with #14275, keep the discussion there and unless there is a consensus for dealing with this, don't bother other issues, it's very disruptive.

@janhohenheim
Copy link
Member

@MiniaczQ alright, sorry!

@alice-i-cecile alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through and removed X-Controversial There is active debate or serious implications around merging this PR labels Sep 17, 2024
@alice-i-cecile
Copy link
Member

I'm happy to consider this idea unblocked due to the ideas in #15625. Once that's fully implemented, this should be recommended as the default way to work with singleton queries and used consistently through our examples.

@MiniaczQ MiniaczQ changed the title Res-style system params equivalents for Query::single Introduce (query) Single<Q, F> family of system parameters Sep 19, 2024
@MiniaczQ MiniaczQ changed the title Introduce (query) Single<Q, F> family of system parameters Introduce QuerySingle<Q, F> family of system parameters Sep 19, 2024
@alice-i-cecile alice-i-cecile added S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! X-Uncontroversial This work is generally agreed upon D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes and removed X-Contentious There are nontrivial implications that should be thought through labels Sep 23, 2024
github-merge-queue bot pushed a commit that referenced this issue Sep 23, 2024
# Objective

The goal of this PR is to introduce `SystemParam` validation in order to
reduce runtime panics.

Fixes #15265

## Solution

`SystemParam` now has a new method `validate_param(...) -> bool`, which
takes immutable variants of `get_param` arguments. The returned value
indicates whether the parameter can be acquired from the world. If
parameters cannot be acquired for a system, it won't be executed,
similarly to run conditions. This reduces panics when using params like
`Res`, `ResMut`, etc. as well as allows for new, ergonomic params like
#15264 or #15302.

Param validation happens at the level of executors. All validation
happens directly before executing a system, in case of normal systems
they are skipped, in case of conditions they return false.

Warning about system skipping is primitive and subject to change in
subsequent PRs.

## Testing

Two executor tests check that all executors:
- skip systems which have invalid parameters:
  - piped systems get skipped together,
  - dependent systems still run correctly,
- skip systems with invalid run conditions:
  - system conditions have invalid parameters,
  - system set conditions have invalid parameters.
github-merge-queue bot pushed a commit that referenced this issue Sep 28, 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.
robtfm pushed a commit to robtfm/bevy that referenced this issue 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 S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants