Seemingly undefined behaviour in filtering for changed components

[lyonbeckers]

I am struggling to reproduce this as minimally as possible, but hopefully I can be clear enough. I'm describing how I am coming into this issue through my current use case. This is off of the master branch.

Using legion and rust bindings for godot, I am writing a level editor where "tiles" (or voxel data) are stored as elements in an octree. The map is separated and rendered in chunks, each of which is a component attached to an entity. A query is run on the chunks and draws the tiles, filtered for when the map chunk is changed.

I'll try to illustrate the issue so it is as clear as possible in case the issue is unclear from the test.

Here, the selection box has a width of 2 tiles, and is straddling the border or 2 chunks (one on either side of the cone).

Eventually, one of the chunks fails to draw.

We can tell that the chunk has actually updated, because if we go to draw in that chunk again, it updates with the tile that had previously been inserted but not drawn. Also, everything behaves as expected when the filter is commented out. So it seems that it is the filter that is just not correctly detecting it as changed.

I thought that there may be an issue with how things are scheduled in the game's implementation, but I've written the following test within my project to try and isolate the issue and make sure it isn't something like that.

use legion::prelude::*;
use crate::level_map;
use std::sync::{Arc, Mutex};

type AABB = crate::geometry::aabb::AABB<i32>;
type Point = nalgebra::Vector3<i32>;

#[test]
fn detect_map_chunk_change() {
    let universe = Universe::new();
    let mut world = universe.create_world();

    let mut resources = Resources::default();
    resources.insert(level_map::Map::default());

    let insert_count = Arc::new(Mutex::new(0u32));
    let insert_counter = insert_count.clone();

    // // Same results whether I create the chunks beforehand or not
    // let chunk_pt1 = Point::new(0,0,0);
    // let chunk_pt2 = Point::new(-1,0,0);

    // world.insert((chunk_pt1,), vec![
    //     (level_map::MapChunkData::new(AABB::new(Point::new(5,5,5), Point::new(10,10,10))),)
    // ]);

    // world.insert((chunk_pt2,), vec![
    //     (level_map::MapChunkData::new(AABB::new(Point::new(-5,5,5), Point::new(10,10,10))),)
    // ]);

    let insert_tiles_fn = Box::new(move |world: &mut legion::world::World, resources: &mut Resources|{
        let mut count = insert_counter.lock().unwrap();
        
        let point = Point::new(0,0,*count as i32);

        let map = resources.get::<level_map::Map>().unwrap();
        let tile_data = level_map::TileData::new(Point::zeros());
        let aabb = AABB::new(point, Point::new(2,1,1));

        //This function checks if the map chunks which would fit the aabb exist, creates them if not, and then inserts
        // the tile_data
        map.insert(world, tile_data, aabb);
        println!("Inserting at {:?}", point);

        *count += 1;
    });

    let detect_change_system = SystemBuilder::new("detect_change_system")
        .with_query(<Read<level_map::MapChunkData>>::query()
            //Test passes if filter is commented out
            .filter(changed::<level_map::MapChunkData>())
        )
        .build(move |_, world, _, query| {
            assert_eq!(query.iter(world).count() as u32, 2);
        });

    let mut schedule = Schedule::builder()
        .add_thread_local_fn(insert_tiles_fn)
        .add_system(detect_change_system)
        .build();

    // loop in place of the full game loop
    for _ in 0..9 {
        schedule.execute(&mut world, &mut resources);
    }
}

As you can see from the comments, the test passes if the filter is commented out. Since the map.insert() function creates the chunks, I tested with the chunks premade as well. The components are retrieved and added through world::get_component and world::add_component in the map.insert() function but in my previous attempts I have gotten the same results from writing the data right from the queries. I don't think the issue is with the insertion, since it behaves properly without the filter (and occasionally without), but I can include a snippet for that as well if it will help get to the bottom of this.

The reason I suspect there is some undefined behaviour here is because the test fails at a different point coordinate each time, seemingly sporadically. Same as the implementation in the actual game, the chunks seem to fail in different spots each time.

Note that in both the test and the example from the game, I've demonstrated the issue using a selection that covers more than one chunk, but the issue is not limited to this case, it is just the most consistent at reproducing the issue.

I don't know if somehow the issue is related to my container or something, I'll continue to try to produce a more standalone test that is reproducible.

[lyonbeckers]

Here is a test that can be run in legion. Same thing, getting different results each time. The getting and adding of components might look weird and redundant, but in my original use case there is other stuff going on that necessitated this, I was trying to get as close to my case as possible. I have had similar results with a more straightforward query based approach.

#[test]
fn detect_map_chunk_change() {

    use std::sync::{Arc, Mutex};

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

    let mut resources = Resources::default();

    let insert_count = Arc::new(Mutex::new(0u32));
    let insert_counter = insert_count.clone();

    world.insert((Model(0u32),), vec![
        (Vec::<u32>::new(),)
    ]);

    world.insert((Model(1u32),), vec![
        (Vec::<u32>::new(),)
    ]);

    let insert_values_fn = Box::new(move |world: &mut legion::world::World, _: &mut Resources|{
        let mut count = insert_counter.lock().unwrap();
        
        let mut entities = Vec::<Entity>::new();

        for i in 0..2 {

            println!("Looking for entity tagged {}", i);

            let model = Model(i);

            let query = <Tagged<Model>>::query()
                    .filter(tag_value(&model))
                    ;

            for (entity, tag) in query.iter_entities(world) {
                println!("Looking at entity tagged as {}", tag.0);
                entities.push(entity);
            }
        }

        let mut to_add: HashMap<Entity, Vec<u32>> = HashMap::new();

        for entity in entities {
            let mut component = world.get_component_mut::<Vec<u32>>(entity).unwrap();
            println!("Vec has a length of {}", component.len());
            component.push(*count);

            to_add.insert(entity, component.clone());
        }
        
        for (entity, component) in to_add {
            world.add_component(entity, component).unwrap();
        }

        *count += 1;
    });

    let detect_change_system = SystemBuilder::new("detect_change_system")
        .with_query(<Read<Vec<u32>>>::query()
            //Test passes if filter is commented out
            .filter(changed::<Vec<u32>>())
        )
        .build(move |_, world, _, query| {
            assert_eq!(query.iter(world).count() as u32, 2);
        });

    let mut schedule = Schedule::builder()
        .add_thread_local_fn(insert_values_fn)
        .add_system(detect_change_system)
        .build();

    // loop in place of the full game loop
    for _ in 0..9 {
        schedule.execute(&mut world, &mut resources);
    }
}

[TomGillen]

I believe this is caused by the changed filter updating its version threshold as it iterates through chunks instead of once per query. I have pushed some changes to master which fixes this, although it introduces a new init function to filters for which it is not always obvious when it should or should not be called (user code should never have to do deal with this, though). Some of the changes in #115 fix this potential issue by making a distinction between stateful and stateless filters.