Skip to content

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

Open

Description

@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).

Activity

  1. added
    C-BugAn unexpected or incorrect behavior
    S-Needs-TriageThis issue needs to be labelled
    on Apr 28, 2025
  2. tnthung commented on Apr 28, 2025

    @tnthung
    Author

    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.

  3. MrGVSV commented on Apr 28, 2025

    @MrGVSV
    Member

    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?

  4. tnthung commented on Apr 29, 2025

    @tnthung
    Author

    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.

  5. MrGVSV commented on Apr 29, 2025

    @MrGVSV
    Member

    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
    }
  6. added
    A-ECSEntities, components, systems, and events
    S-Needs-ReviewNeeds reviewer attention (from anyone!) to move forward
    S-Needs-DesignThis issue requires design work to think about how it would best be accomplished
    and removed
    S-Needs-TriageThis issue needs to be labelled
    S-Needs-ReviewNeeds reviewer attention (from anyone!) to move forward
    on Apr 29, 2025
  7. alice-i-cecile commented on Apr 29, 2025

    @alice-i-cecile
    Member

    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.

  8. added
    D-ComplexQuite challenging from either a design or technical perspective. Ask for help!
    on Apr 29, 2025
  9. changed the title [-]Trigger `OnRemove` has conflict result with `Has`[/-] [+]Bevy does not distinguish between "has a component" access and "read" access, leading to `OnRemove` + `Has` system param incompatibility[/+] on Apr 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    A-ECSEntities, components, systems, and eventsC-BugAn unexpected or incorrect behaviorD-ComplexQuite challenging from either a design or technical perspective. Ask for help!S-Needs-DesignThis issue requires design work to think about how it would best be accomplished

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions