Bevy does not distinguish between "has a component" access and "read" access, leading to `OnRemove` + `Has` system param incompatibility

[tnthung]

Bevy version

v0.16.0

What you did

Code:

use bevy::prelude::*;


fn main() {
  App::new()
    .add_plugins(DefaultPlugins)
    .add_observer(foo)
    .add_systems(Startup, setup)
    .add_systems(Update, update)
    .run();
}


#[derive(Component)]
struct A;


fn foo(t: Trigger<OnRemove, A>, q: Query<Has<A>>) {
  println!("foo: {:?}", q.get(t.target()));
}


fn setup(mut commands: Commands) {
  commands.spawn(A);
}


fn update(mut commands: Commands, q: Query<Entity, With<A>>, time: Res<Time>) {
  if time.elapsed_secs() > 2.0 {
    for entity in q.iter() {
      commands.entity(entity).remove::<A>();
    }
  }
}

What went wrong

When the component A is removed from the entity, the observer foo is correctly triggered. However, the Has<A> in query still reports true. This is a MRE, my actual implementation is way complicated, solely relay on OnRemove to tell if the component presents in my case is impossible, unless I totally rewrite that part of the logics (which is probably the only workaround, AFAIK).

[tnthung]

Just tested, with Query<Option<&A>>, Some is also reported. I know this is an expected result for OnRemove, it fired before the component actually removed. But the unavailability to test if an component exists is quite troublesome.

[MrGVSV]

This is expected behavior since OnRemove is fired before the component is actually removed. This is done so that you have access to the component data one last time (namely to update any invariants your components might be trying to uphold).

So for clarification, what are you trying achieve? Is there something stopping you from excluding the target entity in the query.get call? Or using RemovedComponents if you don't need access to the data?

[tnthung]

The thing I'm trying to do is something like follows.

fn main() {
  App::new()
    .add_plugins(DefaultPlugins)
    .add_observer(update_state::<OnRemove, A>)
    .add_observer(update_state::<OnRemove, B>)
    .add_observer(update_state::<OnAdd   , C>)
    .add_observer(update_state::<OnAdd   , D>);
}

#[derive(Component)] struct A;
#[derive(Component)] struct B;
#[derive(Component)] struct C;
#[derive(Component)] struct D;

fn update_state<E: Event, B: Bundle>(
  trigger: Trigger<E, B>,
  query: Query<(Has<A>, Has<B>, Has<C>, Has<D>)>,
  // some other queries
) {
  // some logics based on the presence of A, B, C and D
}

A separate OnRemoved for triggering AFTER the component is actually removed would be handy in this situation. OFC, I can use RemovedComponents to write for removing cases but it complicated things. Following is my current "workaround"

fn update_on_remove<C: Component>(r: RemovedComponents<C>, ...) { update(...); }
fn update_on_added <C: Component>(t: Trigger<OnAdd, C>   , ...) { update(...); }

fn update(
  a: bool,
  b: bool,
  c: bool,
  d: bool,
  mut entity: EntityCommands,
) {
  // actual update logic
}

As you can see, I need to split the actual implementation into 3 which is 3 times of works need to be done for maintaining this update logic. Also, Observers are only ran when the targeting event is triggered, while RemovedComponent would run regardless if any component is actually removed, this inconsistency is also making the debugging harder.

[MrGVSV]

Gotcha, okay that makes more sense. I can't speak to whether the ECS team would want to add a post-remove hook (though it sounds like that could be handy), but I can suggest a possible solution in the meantime. You can use a custom Has-like query data type. Something like this:

#[derive(QueryData)]
#[query_data(mutable)]
struct CustomHas<C: Component, E: Event, B: Bundle> {
    has: Has<C>,
    _phantom: PhantomData<(E, B)>,
}

impl<'w, C: Component, E: Event, B: Bundle> CustomHasReadOnlyItem<'w, C, E, B> {
    pub fn value(&self) -> bool {
        if TypeId::of::<E>() != TypeId::of::<OnRemove>() {
            return self.has;
        }

        // Note: This only works if `B` is a single component.
        // I only kept it as a `Bundle` here to make it clear what goes where
        self.has && TypeId::of::<C>() != TypeId::of::<B>()
    }
}

// ...

fn update_state<E: Event, B: Bundle>(
  trigger: Trigger<E, B>,
  query: Query<(CustomHas<A, E, B>, CustomHas<B, E, B>, CustomHas<C, E, B>, CustomHas<D, E, B>)>,
  // some other queries
) {
  // some logics based on the presence of A, B, C and D
}

[alice-i-cecile]

Oh hey, the distinction between "has a component access" and "reads access" has finally shown up in a bug. See bevyengine/rfcs#30 for some early thoughts on this problem space. I would categorize this as a bug, but fixing this will require a sizable overhaul to the ECS access model.