Deprecated Various Component Methods from Query and QueryState

[bushrat011899]

Objective

• (Partially) Fixes Query::get_component does not respect filters #9904
• Acts on Should we keep `Query::get_component` and friends? #9910

Motivation

These methods were rarely used, less efficient than the alternatives, and a burden on Bevy maintainers, so they have been deprecated. Please see the above issue and discussion for further technical details.

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()

For example:

fn system(query: Query<(&A, &B, &C)>) {
    // Before
    let b = query.get_component::<B>(entity).unwrap();

    // Alternative 1 (using tuple destructuring)
    let (_, b, _) = query.get(entity).unwrap();

    // Alternative 2 (using tuple item indexing)
    let b = query.get(entity).unwrap().1;
}

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.

[james7132]

Small comments here but happy with the overall change. Less confusion and less perf footguns.

Please remove the pub(crate) and private parts from the changelog in the PR description. The changelog and migration guide are supposed to be user facing.

[james7132]

With that said, IMO this final decision should be left to @cart, even with SME approval. This is one of the core functions on Query, and it's one of the primary parts of Bevy's query ergonomics. The poll in #9910 currently is mostly in favor of removal, but I do also want to get a more representative sample from the community before moving forward with this.

[bushrat011899]

With that said, IMO this final decision should be left to @cart [...] I do also want to get a more representative sample from the community before moving forward with this.

Totally reasonable, I thought I'd just mock up a PR for what this change would represent to help with the decision making process.

[james7132]

At @nicopap's suggestion and the poll's results, it does seem like deprecation and eventual removal is the way to go here. If this makes it into 0.12, the deprecation can act as a shout test for us before we remove it completely in a subsequent release.

[cart]

I'm on board for this. get_component is rarely used, less efficient, a safety hazard, and a maintenance risk. If we're going to eat the costs of this, it should be more useful than it currently is.

I do think we have a "query abstraction" problem that makes it hard to write functionality that is reusable across queries. There is also the "needle in the haystack" problem where finding the right component in a large query is ugly / hard:

// This is nasty to write, nasty to read, and nasty adjust as the query changes
let (_, _, _, _, foo, _, _, _) = query.get(entity).unwrap();

// This is easier on the eyes but forces users to manually count
// (both when writing and reading)
// Still nasty
let foo = query.get(entity).unwrap().4;

I think theres space for something like a "query view" system (which would internally look a bit like get_component) that makes it possible to use any query that yields the required components. I think solving that problem would likely also solve the problems that get_component aims to solve.

[cart]

I'm down to deprecate in 0.12 and then remove in 0.13

[nicopap]

afaik #9774 has a nice lead on implementing a query view system.

[james7132]

If we can change this PR for deprecation instead of removal, I'd like to get this in before 0.13 lands so we can move the removal timeline up.

[bushrat011899]

If we can change this PR for deprecation instead of removal, I'd like to get this in before 0.13 lands so we can move the removal timeline up.

I've left all the methods to be removed as-is, instead marking as deprecated. Because I was having an issue with Clippy I couldn't mark QueryComponentError as deprecated as well. I suspect the thiserror Error derive macro expands in such a way that the deprecation still triggers a warning even when used with allow(deprecated).

[nicopap]

In my humble opinion, query transmutes definitively make those component methods out-of-date.

[rparrett]

IMO, this migration guide could use a code block illustrating what is meant by

and select for the exact component based on the structure of the exact query as required

No need to go crazy or cover every case, but something like this would have saved me a minute of digging up this PR and figuring out what was going on.

fn system(query: Query<(&A, &B, &C)>) {
    // Before
    let b = query.get_component::<B>(entity).unwrap();
    // After
    let (_, b, _) = query.get(entity).unwrap();
}

A short passage explaining the motivation for the breakage can be helpful too for a users' sanity.

These methods were rarely used, less efficient than the alternatives, and a burden on Bevy maintainers, so they have been deprecated.

Without this sentence, I'm slightly grumpy because this seems incredibly arbitrary. With this sentence, I am actually happy to be making this change to my code.

[bushrat011899]

IMO, this migration guide could use a code block illustrating what is meant by

and select for the exact component based on the structure of the exact query as required

No need to go crazy or cover every case, but something like this would have saved me a minute of digging up this PR and figuring out what was going on.

fn system(query: Query<(&A, &B, &C)>) {
    // Before
    let b = query.get_component::<B>(entity).unwrap();
    // After
    let (_, b, _) = query.get(entity).unwrap();
}

Totally fair, I've updated the migration guide based on this code example, also including a second alternative which may be preferred by some users.

A short passage explaining the motivation for the breakage can be helpful too for a users' sanity.

These methods were rarely used, less efficient than the alternatives, and a burden on Bevy maintainers, so they have been deprecated.

Without this sentence, I'm slightly grumpy because this seems incredibly arbitrary. With this sentence, I am actually happy to be making this change to my code.

Also completely fair, I've added the suggested sentence verbatim, with an additional note to refer to the linked issue and discussion in a new section labelled Motivation.

Thanks for sharing your experience, hopefully this helps others along the way!

[rparrett]

Thanks! Not a huge deal in the grand scheme of things, but I really appreciate any help with the migration guides I can get!

[haath]

So after the removal, the only way now to get a specific component by its type is to use Query transmutation, correct?

And to be clear, I am referring to use cases where you don't know the exact structure of the query, like in the following example which I have in 0.12:

// with 0.12
fn get_child_component<'a, C, Q, F>(children: &Children, query: &'a Query<'_, '_, Q, F>) -> Option<&'a C>
where
    C: Component,
    Q: WorldQuery,
    F: ReadOnlyWorldQuery,
{
    for &child in children
    {
        if let Ok(component) = query.get_component::<C>(child)
        {
            return Some(component);
        }
    }

    None
}

If I understand correctly, starting with 0.13 this function will need to be rewritten to accept a QueryLens.

However the release notes also mention the following:

One thing to take into consideration is the transmutation is not free. It works by creating a new state and copying a bunch of the cached data inside the original query. It's not a expensive operation, but you should probably avoid doing it inside a hot loop.

So does this mean that the removal of these methods now imposes a performance penalty on these use cases?

But please do correct me if I'm wrong, I am relatively new with Bevy still 🙂