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

Jump to bottom

Adding a component trigger changed filter for other components #84

Closed
IcanDivideBy0 opened this issue Jan 22, 2020 · 7 comments
Closed

Adding a component trigger changed filter for other components #84

IcanDivideBy0 opened this issue Jan 22, 2020 · 7 comments

Comments

@IcanDivideBy0
Copy link

IcanDivideBy0 commented Jan 22, 2020 •
edited

Here is a simple example:

use legion::prelude::*;

#[derive(Debug)]
struct Foo(pub i32);

#[derive(Debug)]
struct Bar(pub i32);

fn main() {
    let universe = Universe::new();
    let mut world = universe.create_world();

    let foo_system = SystemBuilder::new("test")
        .with_query(<Read<Foo>>::query().filter(changed::<Foo>()))
        .build(|_, world, _, query| {
            for foo in query.iter(world) {
                println!("{:?} changed", foo);
            }
        });

    let mut schedule = Schedule::builder()
        .add_system(foo_system)
        .build();
    
    let entity = world.insert((), vec![(Foo(1),)])[0];

    println!("pass #1");
    schedule.execute(&mut world);

    // Adding Bar component will mark entity as if Foo has changed
    world.add_component::<Bar>(entity, Bar(0));

    println!("pass #2");
    schedule.execute(&mut world);

    println!("pass #3");
    schedule.execute(&mut world);
}

output

pass #1
Ref { borrow: Shared { state: 2 }, value: Foo(1) } changed
pass #2
Ref { borrow: Shared { state: 2 }, value: Foo(1) } changed
pass #3

It's not clear to me if it is the intended behavior, but typing .filter(changed::<Foo>()) strongly suggest it will pull only entities with modified Foo components.

Tested against 0.2.1 and master.
@cart
Copy link

cart commented Jan 24, 2020

I'm not a legion expert but I have a theory: the archetype changed when you added Bar, which resulted in a move of the entity's components, which triggered a "changed" event for Foo?

@TomGillen
Copy link
Collaborator

TomGillen commented Jan 24, 2020 •
edited

Cart is right. When you add a component to an entity, it internally moves the entity into a new chunk, which will cause all components to be seen as changed. Composition changes are essentially removing the entity and then inserting a new one with the same Entity ID.

@cart
Copy link

cart commented Jan 24, 2020

Its good that we understand why its happening, but is the behavior of this event desirable? Is there a way to make the event behave the way most people would expect it to?

It makes the event less useful for things like "I only want to update the transform hierarchy when the position component changes".

@cart
Copy link

cart commented Jan 24, 2020

In that case, the value of the component hasnt changed. Only the internal legion representation of the value. From the perspective of the public interface presented nothing has changed.

@IcanDivideBy0
Copy link
Author

IcanDivideBy0 commented Jan 24, 2020 •
edited

Yes, I've come to the same conclusion that archetype change is in cause. Specifically, changed filter checks for the version number of the component slice (not the component).

Some possibilities i see:

  • adding the PartialEq to Component trait would allow the filter to check for equality in case version number changed nevermind, this would require to store store previous components data as well to compare against
  • refactoring ComponentResourceSet to use multiple versions (per component), in such case ComponentResourceSet::data_slice_mut<T> should be in charge of incrementing versions numbers and ComponentResourceSet::data_raw_mut may not be public anymore. This will add some overhead but will provide the "correct" behavior IMO
  • renaming filter to may_have_changed ?

let me know if you have any other ideas

I'm not really familiar with legion nor the archetype storage strategy, if you have some documents or articles around these subject, it might worth putting a link into the readme, took me a lot of time getting my head around the basic principle.

@IcanDivideBy0
Copy link
Author

Any news / feedback on this ?
It seems the changed filter is broken in many ways at the moment, not just in the case of adding a component. Also having a filter for added / deleted components seems essential IMO.
I'd be happy to contribute if you can point me in the right direction @TomGillen

@IcanDivideBy0
Copy link
Author

It seems to be fixed now on master, probably in c99326e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@TomGillen @IcanDivideBy0 @cart