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 should mention that the compenent must be part of the query #9011

Closed
nicopap opened this issue Jul 1, 2023 · 2 comments · Fixed by #11974 · May be fixed by #9041
Closed

Query::get_component should mention that the compenent must be part of the query #9011

nicopap opened this issue Jul 1, 2023 · 2 comments · Fixed by #11974 · May be fixed by #9041
Labels
A-ECS Entities, components, systems, and events C-Docs An addition or correction to our documentation

Comments

@nicopap
Copy link
Contributor

nicopap commented Jul 1, 2023

How can Bevy's documentation be improved?

Query::get_component doesn't mention that it can only return components that are part of the query itself.

The method itself is very niche, as it's mostly an optimization to avoid accessing several components when only one is necessary.

But the documentation mentions none of this. The only caveat is the following paragraph:

In case of a nonexisting entity or mismatched component, a QueryEntityError is returned instead.

It's not enough!

Code seen in the wild (discord conversation):

fn prepare_player(
    mut commands: Commands,
    players: Query<Entity, With<backend::Player>>,
) {
    for player in &players {      
        let transform = *players.get_component::<Transform>(player).expect("Entity does not contain transform!");
    }
}

This code will always panic! Independently of whether the Entity in question has a Transform component or not. Since players doesn't have access to the Transform component, it cannot return a reference to it.

What should be done

The method should get additional wording saying "can only return components present in the query" (better phrasing pending). This should be slapped at the front of the doc, maybe part of the first paragraph.

@nicopap nicopap added C-Docs An addition or correction to our documentation A-ECS Entities, components, systems, and events labels Jul 1, 2023
@mockersf
Copy link
Member

mockersf commented Jul 3, 2023

It seems the doc is wrong, get_component returns a QueryComponentError, not a QueryEntityError.

In the case mentioned in this issue, it should return a QueryComponentError::MissingReadAccess which has quite a good documentation already (in main only for now)

@Selene-Amanita
Copy link
Member

Looks like I forgot to fix that when implementing #8692 yeah.

I have changes right now to fix this alongside #8917. I'll create a pull request tomorrow to fix both.

github-merge-queue bot pushed a commit that referenced this issue Feb 19, 2024
# Objective
We deprecated quite a few APIs in 0.13. 0.13 has shipped already. It
should be OK to remove them in 0.14's release. Fixes #4059. Fixes #9011.

## Solution
Remove them.
msvbg pushed a commit to msvbg/bevy that referenced this issue Feb 26, 2024
# Objective
We deprecated quite a few APIs in 0.13. 0.13 has shipped already. It
should be OK to remove them in 0.14's release. Fixes bevyengine#4059. Fixes bevyengine#9011.

## Solution
Remove them.
msvbg pushed a commit to msvbg/bevy that referenced this issue Feb 26, 2024
# Objective
We deprecated quite a few APIs in 0.13. 0.13 has shipped already. It
should be OK to remove them in 0.14's release. Fixes bevyengine#4059. Fixes bevyengine#9011.

## Solution
Remove them.
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-Docs An addition or correction to our documentation
Projects
None yet
3 participants