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
29 changes: 29 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,35 @@ 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 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!")
/// }
/// }
/// # targeting_system.system();
/// ```
#[inline]
pub fn contains(&self, entity: Entity) -> bool
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if its worth taking a chunk from get_unchecked_manual instead of using the whole function for performance reasons?

for example, something like this in state.rs:

    pub fn contains<'w, 's>(&'s self, world: &'w World, entity: Entity) -> bool {
        let location = world.entities.get(entity);

        if let Some(location) = location {
            return self
                .matched_archetypes
                .contains(location.archetype_id.index());
        }
        false
    }

and then call self.state.contains(self.world, entity) inside query

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as @TheRawMeatball said earlier

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

Copy link
Contributor

Choose a reason for hiding this comment

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

the above code doesn't call any unsafe functions and it would be easier enough to write a test to verify it works.

Copy link
Contributor

Choose a reason for hiding this comment

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

example:

    #[test]
    fn query_contains_entity() {
        fn without_entity(entity: Res<Entity>, empty: Query<&A>) {
            assert!(!empty.contains(*entity));
        }

        fn with_entity(entity: Res<Entity>, not_empty: Query<&A>) {
            assert!(not_empty.contains(*entity));
        }
        let mut world = World::default();
        let entity = world.spawn().insert(B).id();
        world.insert_resource(entity);

        let mut without_entity = without_entity.system();
        without_entity.initialize(&mut world);
        without_entity.run((), &mut world);

        let entity = world.spawn().insert(A).id();
        world.insert_resource(entity);

        let mut with_entity = with_entity.system();
        with_entity.initialize(&mut world);
        with_entity.run((), &mut world);
    }

Copy link
Member

Choose a reason for hiding this comment

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

I thought we would want to handle query filters though?

where
Q::Fetch: ReadOnlyFetch,
{
self.get(entity).is_ok()
}
}

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