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

[Merged by Bors] - Add Query::contains #3090

Closed
wants to merge 11 commits into from
38 changes: 38 additions & 0 deletions crates/bevy_ecs/src/system/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -975,6 +975,44 @@ where
self.state
.is_empty(self.world, self.last_change_tick, self.change_tick)
}

/// Returns `true` if the given [`Entity`] matches the query.
Copy link
Member

Choose a reason for hiding this comment

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

I don't propose that you take any action on this comment, just noting down my thoughts.

By a strict reading of these docs, a valid implementation of this function would be {true}; we don't specify the behaviour if the condition is false. It is however intended to be read as if and only if.

However, this fits the pattern of documentation in std, e.g. Vec::contains. I tried to find a rust-lang/rust issue on these doc items, but failed to do so.

///
/// # Example
///
/// ```
/// # use bevy_ecs::prelude::*;
/// #
/// # #[derive(Component)]
/// # struct Player;
/// # #[derive(Component)]
/// # struct Alive;
/// fn check_if_alive(
/// player_query: Query<Entity, With<Player>>,
/// alive_query: Query<(), With<Alive>>,
/// ) {
/// if let Ok(player) = player_query.get_single() {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, so I think a more idiomatic version of this would use #3088 (or the option workaroud detailed therein). I think this method is more useful for pseudo-relations, e.g. 'has this pet's owning player been killed', or 'how many neighbours of this entity are alive'.

However, I'm struggling to think of a super succinct version which does this.

Copy link
Member

Choose a reason for hiding this comment

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

Also, replacing the get_single with single would be a welcome change.

Copy link
Member

Choose a reason for hiding this comment

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

Here's an alternate example to steal that's a bit more idiomatic.

#[derive(Component)]
struct InRange;

struct Target {
    entity: Entity,
}

fn targeting_system(in_range_query: Query<&InRange>, target: Res<Target>) {
    if in_range_query.contains(target.entity) {
        println!("Bam!")
    }
}

/// if alive_query.contains(player) {
/// println!("The player is alive.");
/// } else {
/// println!("The player is dead!");
/// }
/// }
/// }
/// # check_if_alive.system();
/// ```
#[inline]
pub fn contains(&self, entity: Entity) -> bool {
unsafe {
self.state.get_unchecked_manual(
Copy link
Member

Choose a reason for hiding this comment

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

At the moment, this unsafe block is identical to the one in get, apart from it's missing the safety comment.

Please just replace the body with self.get(entity).is_ok().

I think that a more optimised version is possible, but it would be challenging to review due to the amount of unsafety required.

self.world,
entity,
self.last_change_tick,
self.change_tick,
)
}
.is_ok()
}
}

/// An error that occurs when retrieving a specific [`Entity`]'s component from a [`Query`]
Expand Down