Filter out multiple enemy messages moving to the same location

[tmilker]

This pull request is in regards to the code in Chapter 9: Victory and Defeat. At least, it's where I discovered it.

This fixes an issue with enemies being able to move into the same location at the same time. If you have two enemies like this:

.g.
g..
...

And manipulate them into moving into the center position, they'll both end up occupying the same space. There's no actual check at the time of processing the message that the space is free. I think Legion's parallelism is getting in the way but I'm not sure*.

Also would like to know if I wrote this correctly or if there is a better/more efficient way. Creating that Vec and throwing it away every frame for positions seen seems wasteful but I'm just getting started with rust so maybe not?

*I tried following your very recent article to disable Legion's parallelism and test without this fix, but it still occurs. I still see the program using multiple threads on my computer though so I don't trust that it's actually disabled.

[Laharah]

I found this same bug. The issue is that the movements are executed in batch rather than one at a time. Rather than implementing a filter system or some kind of reservation system, you can just change the system to execute the movements one at a time and preform a check that the space hasn't become occupied.

#[system(for_each)]
#[read_component(Player)]
#[write_component(Point)]
pub fn movement(
    message: &Entity,
    want_move: &WantsToMove,
    #[resource] map: &Map,
    #[resource] camera: &mut Camera,
    ecs: &mut SubWorld,
    commands: &mut CommandBuffer,
) {
    if map.can_enter_tile(want_move.destination) {
        let unoccupied = <&Point>::query()
            .iter(ecs)
            .all(|pos| *pos != want_move.destination);
        if unoccupied {
            *ecs.entry_mut(want_move.entity)
                .unwrap()
                .get_component_mut::<Point>()
                .unwrap() = want_move.destination;
        }

        if ecs
            .entry_ref(want_move.entity)
            .unwrap()
            .get_component::<Player>()
            .is_ok()
        {
            camera.on_player_move(want_move.destination);
        }
    }
    commands.remove(*message);
}

you duplicate some work from the player_input and the random_movement systems but it's the most minimal change I could think of.

I think to do it properly you should

1. remove the for_each tag on the system so it only runs once not once for every message
2. build a set of all the want_to_move.destinations, filtering out any conflicting messages
3. batch the filtered movement commands
4. delete all the message entities.

The only problem is that it would seriously change the system and might interrupt/overcomplicate what you're trying to teach the reader.

[jorge-gonzalez-lexia]

@Laharah and @tmilker , I'm a couple of years late to the party, but I had noticed the same issue. I solved it in the chasing system instead of the movement one, by tracking the changing positions of the monsters. In other words, I create a HashMap<Entity, Point> initialized to the starting mover positions and use it to block any move to a position already taken AND I update the HashMap as each mover plans its move. I agree with @Laharah that this may overcomplicate things, but then again, I find that refactoring and solving bugs like this are excellent "exercises for the reader".