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

Query::get_component does not respect filters #9904

Closed
cfognom opened this issue Sep 22, 2023 · 6 comments · Fixed by #9920
Closed

Query::get_component does not respect filters #9904

cfognom opened this issue Sep 22, 2023 · 6 comments · Fixed by #9920
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior

Comments

@cfognom
Copy link

cfognom commented Sep 22, 2023

Bevy version

0.11.2

What you did

Created a system query: query: Query<&Transform, Changed<Transform>> and then used query.get_component(entity_with_unchanging_transform).

What went wrong

  • what were you expecting?
    I expected to get no result since the query has the Changed filter.
  • what actually happened?
    I got a result.

Additional information

@cfognom cfognom added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Sep 22, 2023
@iiYese iiYese added A-ECS Entities, components, systems, and events and removed S-Needs-Triage This issue needs to be labelled labels Sep 22, 2023
@james7132 james7132 added P-High This is particularly urgent, and deserves immediate attention P-Unsound A bug that results in undefined compiler behavior labels Sep 23, 2023
@james7132
Copy link
Member

As the filters can be used to ensure mutual exclusion, this is a soundness bug, and we should address this as soon as possible.

I'm actually strongly tempted to say we should remove Query::get_component. It's a very prominent source of unsoundness, it's nowhere near the same performance as Query::get, and the effort to patch up its soundness issues is immense compared to the utility it provides.

@cfognom
Copy link
Author

cfognom commented Sep 23, 2023

How does Query::get() + discarding of some of the results compare to Query::get_component(). If the performance of Query::get() is still better in such a case. Why not just let Query::get_component() call Query::get() internally?

@iiYese
Copy link
Contributor

iiYese commented Sep 23, 2023

How does Query::get() + discarding of some of the results compare to Query::get_component(). If the performance of Query::get() is still better in such a case. Why not just let Query::get_component() call Query::get() internally?

The component can be in an arbitrary position or nested deep in a worldquery. You can't solve this with metaprogramming either in the case where Q is a generic.

@alice-i-cecile
Copy link
Member

I think we should remove it too. It's rarely useful as well.

@james7132 james7132 removed P-High This is particularly urgent, and deserves immediate attention P-Unsound A bug that results in undefined compiler behavior labels Sep 23, 2023
@james7132
Copy link
Member

Removing the unsoundness tag as it's only unsound if this ignores With/Without or other access based filters, where it can potentially cause aliased mutable access, which it doesn't. It simply just doesn't respect the return value on F::filter_fetch.

Even with this in mind, I'm still in support of removing the API since it's often a performance footgun to use over Query::get and discarding the unused results.

@alice-i-cecile
Copy link
Member

Removal is discussed more in #9910

github-merge-queue bot pushed a commit that referenced this issue Feb 4, 2024
…9920)

# Objective

- (Partially) Fixes #9904
- Acts on #9910

## Solution

- Deprecated the relevant methods from `Query`, cascading changes as
required across Bevy.

---

## Changelog

- Deprecated `QueryState::get_component_unchecked_mut` method
- Deprecated `Query::get_component` method
- Deprecated `Query::get_component_mut` method
- Deprecated `Query::component` method
- Deprecated `Query::component_mut` method
- Deprecated `Query::get_component_unchecked_mut` method

## Migration Guide

### `QueryState::get_component_unchecked_mut`

Use `QueryState::get_unchecked_manual` and select for the exact
component based on the structure of the exact query as required.

### `Query::(get_)component(_unchecked)(_mut)`

Use `Query::get` and select for the exact component based on the
structure of the exact query as required.

- For mutable access (`_mut`), use `Query::get_mut`
- For unchecked access (`_unchecked`), use `Query::get_unchecked`
- For panic variants (non-`get_`), add `.unwrap()`

## Notes

- `QueryComponentError` can be removed once these deprecated methods are
also removed. Due to an interaction with `thiserror`'s derive macro, it
is not marked as deprecated.
tjamaan pushed a commit to tjamaan/bevy that referenced this issue Feb 6, 2024
…evyengine#9920)

# Objective

- (Partially) Fixes bevyengine#9904
- Acts on bevyengine#9910

## Solution

- Deprecated the relevant methods from `Query`, cascading changes as
required across Bevy.

---

## Changelog

- Deprecated `QueryState::get_component_unchecked_mut` method
- Deprecated `Query::get_component` method
- Deprecated `Query::get_component_mut` method
- Deprecated `Query::component` method
- Deprecated `Query::component_mut` method
- Deprecated `Query::get_component_unchecked_mut` method

## Migration Guide

### `QueryState::get_component_unchecked_mut`

Use `QueryState::get_unchecked_manual` and select for the exact
component based on the structure of the exact query as required.

### `Query::(get_)component(_unchecked)(_mut)`

Use `Query::get` and select for the exact component based on the
structure of the exact query as required.

- For mutable access (`_mut`), use `Query::get_mut`
- For unchecked access (`_unchecked`), use `Query::get_unchecked`
- For panic variants (non-`get_`), add `.unwrap()`

## Notes

- `QueryComponentError` can be removed once these deprecated methods are
also removed. Due to an interaction with `thiserror`'s derive macro, it
is not marked as deprecated.
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-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants