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

implement proof of concept query filter Predicate #14715

Closed
wants to merge 1 commit into from

Conversation

RobWalt
Copy link
Contributor

@RobWalt RobWalt commented Aug 12, 2024

Objective

Honestly, I just wanted to capture the idea "Predicate Filters" that came up after some small discussions in the ecs-dev channel. This is just a draft PR so other people can see how the implementation of this idea looks in real code.

While we do have tools to solve this problem here in other ways, I always think it's nice to have more tools in general for people to play with. Nevertheless there's also some valid counter arguments since this feature just adds another layer of trait complexity. Citing Alice

You're still doing a linear time check
It's just now no longer local, and involves an extra layer of trait machinery
And if you really want custom methods, you can define that on your components, a QueryData type or a SystemParam type

So feel free to see this more as an RFC than a PR.

Citing discord user `.nathanf`s original idea, request and reasoning

It would be convenient to have a way to define custom predicates for Query filters with a low-unsafe API surface... I was thinking something along the lines of:

trait HavingPredicate {
    type Component: bevy::prelude::Component;
    fn filter_predicate(item: &Self::Component) -> bool;
}

pub struct Having<P>(std::marker::PhantomData<P>);

for example,

struct IsAlive;
impl HavingPredicate for IsAlive {
    type Component = Health;
    fn filter_predicate(health: &Health) -> bool {
        health.hit_points >= 0
    }
}

fn print_living_entities(query: Query<Entity, Having<IsAlive>>) {
   for entity in query.iter() {
      println!("is alive: {entity}");
   }
}

I would open a PR to implement this myself... But I don't understand the internals for QueryFilter / WorldQuery at all. Or maybe this is already available and I just haven't seen it?

Well, it's more convenient, especially when you're evolving existing code - if I already have a Query<(Entity, &C1, &C2, &C3)>, but I want to filter using &C4 and &C5, I have to go update

  • The signature, to include &C4 and &C5
  • All of the iter_ pattern destructuring sites, to pattern match on the new C4 and C5 components
  • All of the iter_ sites, to add the filter code

In particular, if I'm moving from With<SomeMarker> to checking some property on a larger component, I have to do all of these changes through all of my code, even though it has nothing to do with the problem I'm trying to solve, which is just filtering.

It's not blazing fast, but it's not any slower than the "manual" way of doing it, which is a lot more verbose with little upside from my perspective

In particular, it can't just be an extension method on Query<_, _>, like .filter(|x| ...) because it needs to access data not already requested by the Query, and that data must be available in the system's signature

Solution

This PR is just implementing an idea from the discord ecs-dev channel.

Citing discord user `quartermeister`

I think you can implement it by doing something like this, although I haven't tested it:

trait HavingPredicate {
    // Might as well generalize from just one component to any QueryData
    type Data: QueryData;
    fn filter_predicate(item: QueryItem<Self::Data>) -> bool;
}

pub struct Having<P>(PhantomData<P>);

unsafe impl<D: QueryData, P: HavingPredicate<Data = D>> WorldQuery for Having<P> {
    // Delegate each item to D, so that we can get QueryItem<D> from Self::fetch while relying on D for safety invariants
    type Foo = D::Foo;
    fn foo(params) { D::foo(params) }
}

impl<D: QueryData, P: HavingPredicate<Data = D>> QueryFilter for Having<P> {
    // This must be set to false because we look at the data in each row 
    // (as opposed to a With or Without filter that just looks at the archetype)
    const IS_ARCHETYPAL: bool = false;

    unsafe fn filter_fetch(
        fetch: &mut Self::Fetch<'_>,
        entity: crate::prelude::Entity,
        table_row: crate::storage::TableRow,
    ) -> bool {
        // This is where the actual filtering happens!
        P::filter_predicate(Self::fetch(fetch, entity, table_row))
    }
}

impl HavingPredicate for IsAlive {
    type Data = &'static Health;
    fn filter_predicate(health: &Health) -> bool {
        health.hit_points >= 0
    }
}

Testing & Showcase

I added this passing test in bevy_ecs/query/mod.rs

    #[derive(Component, Debug, Hash, Eq, PartialEq, Clone, Copy)]
    struct A(usize);

    #[derive(Component, Debug, Clone, Copy)]
    pub struct E(usize);

    #[derive(Debug, Clone, Component, Default)]
    pub struct NonZeroE;

    impl PredicateFilter for NonZeroE {
        type Data = &'static E;
        fn filter_predicate(item: crate::query::QueryItem<Self::Data>) -> bool {
            item.0 != 0
        }
    }

    #[test]
    fn query_predicate() {
        let mut world = World::new();
        world.spawn((A(1), E(0)));
        world.spawn((A(2), E(1)));
        world.spawn((A(3), E(2)));

        let values = world
            .query_filtered::<&A, Predicate<NonZeroE>>()
            .iter(&mut world)
            .collect::<Vec<_>>();
        assert_eq!(values, vec![&A(2), &A(3)]);
    }

To-Do

If we would really want to include this:

  • write proper in depth docs
  • more tests?
  • the current method has the drawback that it needs all the involved components to be public. I don't know if that would cause problems somewhere. We should investigate this further
  • Answer: Is the predicate usable in the data position?

@ItsDoot ItsDoot added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events X-Controversial There is active debate or serious implications around merging this PR labels Aug 12, 2024
@ItsDoot
Copy link
Contributor

ItsDoot commented Aug 12, 2024

Related issue: #5639

@Victoronz
Copy link
Contributor

Victoronz commented Aug 12, 2024

If this is based on a safe PredicateFilter trait, it is not sound. A user could write a non-deterministic predicate, which would result in subtle unsoundness where filter determinism is assumed, like f.e. the QueryIter::sort_* methods.

@alice-i-cecile alice-i-cecile added the S-Needs-SME Decision or review from an SME is required label Aug 12, 2024
@alice-i-cecile
Copy link
Member

Between the soundness issues and the complexity incurred, I don't think this is worth adding to the engine itself <3 Thanks for pulling together a prototype to look at though: I hope it helps inspire a 3rd party solution.

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-Feature A new feature, making something new possible S-Needs-SME Decision or review from an SME is required X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants