Refactoring cluttered ECS systems in favour of the command pattern

Recently I have came back to work a bit more on the tiny Shifting Chamber game, which has been put on itch.io earlier this year. Before releasing the early version I had to really force myself to finalize things and make it more or less playable. Therefore it is no surprise that the code got bit messy and ill structured at the end. So now, before introduction new features to the game, it became clear that some refactoring is needed.

One of the first things to tackle was game state handling. It started reasonably simple, but grew towards the end and became really hard to manage. I've reached an unreasonably high amount of states the game could find itself in:

pub enum GameState {
    #[default]
    LoadAssets,
    MainMenu,
    GameInit,
    MapInit,
    TurnStart,
    PlayerInput,
    TileShift,
    NPCAction,
    NPCResult,
    TurnEnd,
    MapEnd,
    Upgrade,
    GameOver,
    GameWin
}

This granulated state structure allowed me to organize the code responsible for game data preparation, generating maps etc. As you can imagine, eg. randomizing map's content is performed only once per level; game init only once per run!. Therefore I have used lots of `one-off` systems that were triggered during the state changes - with the help of `OnEnter` and `OnExit` schedules.

The state change itself would be triggered inside one of the `OnUpdate` systems, making them often run just for a single tick. This, however nasty, pattern made it possible to order logic that should be executed in a specific sequence:

// those systems actually come from different plugins, group here only for the example
    .add_systems(OnEnter(GameState::MapInit), spawn_map);
    .add_systems(Update, start_map.run_if(in_state(GameState::MapInit)))
    .add_systems(OnExit(GameState::MapInit), placement::generate_pieces)

You might say that I could've used `chain` to achieve the same effect. Probably, in some cases, yes. However in the example above, I have needed the `spawn_map` commands to be written into the game world before spawning other board pieces - such as NPCs. Possibly, other solutions such as stages or `apply_system_buffers` (depending on the Bevy version) could have helped here - but I did not know about those at the time :)

Game Init

Let's take a look in a greater detail at one more example. This was a system meant to set up the initial game parameters. Executed only once before each play:

fn start_game(
    mut next_state: ResMut<NextState<GameState>>,
    mut res: ResMut<GameRes>
) {
    res.score = 0;
    res.level = 1;
    res.max_ap = 1;
    res.tile_transforms = HashMap::from_iter(
        get_all_transforms().iter().map(|a| (*a, false))
    );
    // at the beggining only the default action is enabled
    res.tile_transforms.insert(TransformKind::default(), true);
    res.possible_upgrades = crate::player::upgrades::get_initial_upgrades();
    next_state.set(GameState::MapInit);
}

// scheduled like:
    .add_systems(Update, start_game.run_if(in_state(GameState::GameInit)))

It runs during the `GameState's` update phase, but makes sure the state doesn't last any longer than a single tick. It schedules the next `MapInit` state immediately.

Now let's see how those state changes were triggered. I've created a custom command event and an accompanying enum to specify the requested command type:

#[derive(Clone, Debug, PartialEq)]
pub enum CommandType {
    TransformTiles(TileTransform),
    PlayerWait,
    AnimationEnd,
    TurnEnd,
    Upgrade(UpgradeKind),
    Start,
    RestartGame,
    RestartLevel
}

#[derive(Event)]
pub struct CommandEvent(pub CommandType);

Then there was a dedicated `update_state` system that would handle those event calls:

pub fn update_state(
    mut ev_command: EventReader<CommandEvent>,
    game_state: Res<State<GameState>>,
    mut next_state: ResMut<NextState<GameState>>,
    npc_query: Query<&Walking>,
    mut res: ResMut<GameRes>,
    mut health_query: Query<&mut Health, With<Player>>
) {
    for ev in ev_command.iter() {
        if let CommandType::TurnEnd = ev.0 {
            next_state.set(GameState::TurnEnd);
            break;
        }
        if let CommandType::Start = ev.0 {
            next_state.set(GameState::GameInit);
            break;
        }
        if let CommandType::RestartLevel = ev.0 {
            res.score -= RESTART_PENALTY;
            if let Ok(mut health) = health_query.get_single_mut() {
                // restart players HP to level's initial
                health.value = res.level_starting_hp;
            }
            next_state.set(GameState::MapInit);
            break;
        }

	// cut - all the other CommandTypes go here

It is clearly a bad design pattern. At the beginning, there were only few of those commands, so this approach was not ideal - but manageable. Towards the end of the development, it became quite lengthy and difficult to work with. The game state flow was quite convoluted.

Another approach

Now that I am working on the game again, I knew that this was one of the first issues I had to address. Before introducing any new features. Fortunately, in the meantime I've done other small game-prototypes (some of them also in Bevy) and found a better solutions for those problems :)

I am a big fun of the command pattern design, but initially couldn't make it work in the ECS realm. In the end it turned out to be not as complicated as I expected.

First of all, I had to create a common command trait (simirarily to the one shown in the Bevy Roguelike tutorial). I've actually called it `Action` in order not to get confused with Bevy's own commands:

pub trait Action: Send + Sync {
    fn execute(&self, world: &mut World);
}

Now I can redefine my `GameInit` process as a single method, that is not a Bevy system any more, and can be called in a more flexible way. (only from an exclusive system though - because of the `&mut World` reference):

pub struct StartGameAction {
    pub level: i32
}
impl Action for StartGameAction {
    fn execute(&self, world: &mut World) {
        if let Some(mut res) = world.get_resource_mut::<GameRes>() {
            res.score = 0;
            res.level = self.level;
            res.max_ap = 1;
            res.tile_transforms = HashMap::from_iter(
                get_all_transforms().iter().map(|a| (*a, false))
            );
            // at the beggining only the default action is enabled
            res.tile_transforms.insert(TransformKind::default(), true);
            res.possible_upgrades = crate::player::upgrades::get_initial_upgrades();
        }

        if let Some(mut state) = world.get_resource_mut::<NextState<GameState>>() {
	    // note that probably I am going to remove the MapInit state as well
	    // once I refactor even more code
            state.set(GameState::MapInit);
        }
    }
}

I am still using events to trigger those actions, but the event handler system is much much cleaner now:

fn handle_event(
    world: &mut World
) {
    let events = if let Some(mut res) = world.get_resource_mut::<Events<ActionEvent>>() {
        res.drain().collect::<Vec<_>>()
    } else { return };
    for ev in events {
        ev.0.execute(world);
    }
}

// registered like:
    .add_systems(Update, handle_event.run_if(on_event::<ActionEvent>()));

The handler doesn't need to know anything about the action types. It just accepts and executes any. So adding new actions won't result in bloating it's code. (regardless of how many I am going to need).

In the first approach, each single piece logic has been split between the one-shot systems and the `update_state` - making it difficult to follow. Now each functionality / stage can grouped nicely in it's own action struct. The behaviour of one does not affect the other. (previously I'd had to constantly modify the shared event handler)

What are the other benefits of this approach? The system scheduling is way cleaner now, without all those single-shot pseudo systems. I can (and will) get rid of some of the game states, which also should make the game code easier to work with. Moreover, if I want to run the functions in a specific order I can simply just fire one action from another (they all share the `&mut World` parameter) - without worrying about the schedule ordering and command flushes. Lastly I do not have to keep track of all the action types in the lengthy enum.

So now I am quite optimistic about the refactor, will se how it works after I finish :)