Let's code with the Roguelike tutorial - Part 9 - More spell variety

Adding different spell types

When I first added in the ItemComponent for usable items, I didn't add any data for the type of item effect. That was because I generally follow the principle of add it when you need it. Well, now I need it.

Before I can add additional spell effects, I needed to refactor my code to have an item effect variable in the component. I made this an enum, so all the possible spell effects will be centralized in one place. One of the trade offs of this is while it makes it easier to see all the effects in one place, it makes it harder for a player to mod the game, since there will probably need to be a two level dispatch for native effects and player implemented effects.

There is also the argument that every time someone wants to add a new spell effect, they need to modify the original source code. If I were shipping a game engine, then I think this argument would have merit, but on a relatively small team where everyone is working on the same source base, I find that as long as the code is structured well, adding another enum case and match clause isn't worse than having a bunch of callbacks or inherited game behavior.

Anyway, I made a new ItemEffect enum, with the first case being a Heal effect. Rust enums can have associated data, so I attached the amount to heal for, meaning I can cover heal effects of all HP amounts with just this one case. After that, it was straightforward to add a match clause to the use_item method. There was only one case to match but I'll be adding more shortly. I tested this refactoring and saw no issues.

Then, it's time to add the next spell effect: a lightning bolt that will target the closest enemy and deal a good amount of damage.

To start, I added an ItemEffect that targets the closest enemy. I used associated data once again to attach a spell range and spell damage value, so this effect can be used for any item that damages the closest enemy.

Most of the code to implement this effect has already been written in the program. I just needed to reorganize it a bit to make it more logically structured:

Finally, I added this enum to the match clause in use_item, which iterates through all the entities, finds the closest one in range, and calls the FighterSystem to damage the target. Then I tested this by adding a new clause to the map generator to add some lightning bolt scrolls.

Adding a confusion spell

Next, I added a spell to cause enemies to get confused.

To implement this effect, I had to add a new AI type. On the design side, this basically followed the same procedure as adding a new ItemEffect.

On the other hand, there is one interesting problem that came up. My first version of the code to add the AI looked like:


for i in 0..game.entities.len() {
  if let Some(ref ai) = game.entities[i].ai {
    match ai.strategy {
      AIStrategy::Basic => {
        AISystem::basic_ai(i, game, player_x, player_y);
      }
    }
  }
}

In words: iterate through all the entities in the game. If an entity has an AI component, check to see what kind of strategy it is, and then call the function associated with that strategy, passing in the index and game state, and the player coordinates for convenience.

This actually fails the borrow checker. The problem is, during the let match, I get a reference to the ai through game.entities. That makes one reference. Later on, when I try to call basic_ai, I need to pass in a mutable reference to the game state. That mutable reference can't coexist with the previous immutable reference on game.entities. Theoretically, the callee could remove elements from the entities array, thus creating dangling references.

To work around the borrow checker, I converted this code to use an index instead:


for i in 0..game.entities.len() {
  let strategy = game.entities[i].ai.as_ref().map(|ai| ai.strategy);
  if let Some(strategy) = strategy {
    match strategy {
      AIStrategy::Basic => {
        AISystem::basic_ai(i, game, player_x, player_y);
      }
    }
  }
}

This workaround makes a copy of the strategy, and matches on that, instead of matching on a reference to the strategy. (The map is a convenience function on optional values. It returns the result of ai.strategy if ai isn't None, otherwise it returns None).

So does this mean I've worked around the original bug the borrow checker was complaining about?

No! The borrow checker no longer fails, but this code is potentially just as buggy if I try to mutate the elements of the array. It just fails in a way that isn't a dangling pointer, which, to be fair, is one of the hardest kind of bugs to debug. To see how this is broken, imagine an enemy which is a "leader" kind of character. It modifies the AI of any orcs within a certain range. Now there's subtle bugs that can result from the order of elements in the array. Suppose the leader moves and some orcs are now out of range of leadership. When we run our single AI pass through the entities and run the leader's AI, it will reach out to the orc entities and modify some of their AIs. Well if we've already processed that orc's AI because it came earlier in the array, we get different behavior than if we processed the orc's AI later. These kinds of hidden dependencies can be very difficult to reproduce and can lead to subtle erroneous behavior, so they should be avoided as much as possible.

In my code, I will follow the convention: never update some other entity's AI. This rule works for our current design, but if I eventually want to add in group behavior, I need to restructure this code a bit.

With the code refactored, I added a new enum for the AI type for a "confused" enemy. This needed an associated data, since the confusion only lasts for a set number of turns, and then the monster should revert back to its old AI, so I have to keep a reference to the old AI to restore. One wrinkle here is this is self referential - in other words, the AI type has another reference to the AI type. To make this work, Rust requires that I wrap the reference. There are various forms of wrapping (and I'm not quite sure I understand all the subtle differences yet), but I think the simplest one - Box - will suffice for this use.

I did eventually get this code to work, but I wasn't super happy with it:


let target_aic = target.ai.as_mut.unwrap();
let previous_ai = Box::new(target_aic.strategy.clone());
target_aic.strategy = AIStrategy::Confused { remaining_turns: SCROLL_CONFUSE_TURNS, previous_ai: previous_ai };

The problem is, I want to follow these steps:

  1. Take old AI and wrap that in a box.
  2. Create new AI that keeps the box of old AI as a parameter.
  3. Assign new AI to AI component.

When I carefully broke down the above steps, I ran into a problem: Between the first and second step, there's no AI! Because of Rust's move semantics, either the component owns the AI or the box owns the AI, but not both. Now in this particular use case I know this will eventually be fine, because I immediately assign the newly created AI to the component. But there's no way to express this to the compiler. It doesn't seem right to make the strategy an Option, because it isn't really an optional; it's just for a few statements while we move things around, we have nothing to assign to the component. In the end, I had to make the strategy cloneable, and thus create a new copy of the AIStrategy to box before dropping the old one. This feels a little undesirable, especially if there's a complex AI that needs to clone a lot of state... but it does work.

There are also a couple of things I extract out into their own functions since it's used by multiple AIs.

The first is to make checking if a tile is blocked into its own function. Previously, I had tried to refactor this because different subsystems like item placement, character movement, etc. needed to determine if a tile was blocked. I eventually had to abort the refactor since each of the different subsystems had a slightly different notion of what was considered blocked, so even though in English they all sound like they're computing the same thing, in the programming semantics, they are different. On the other hand, "blocking" in an AI is much more straightforward, and all the different AIs have the same notion of what blocking means, so in this case, they are in fact semantically the same. Extracting this is therefore straightforward.

The second is to refactor finding the closest monster. This is also conceptually straightforward. However, the language twist here is that we want the new function to return a reference to the closest monster, and we need to tell the compiler that the reference will continue to be live. For example, if we create a local monster in the search function and try to return it, we'll end up with a dangling reference - a very serious error. In our case, we know we'll always return a reference to something in the entities array, so we need to pass that in to the function:


fn closest_enemy<'a>(map: &mut Map, entities: &'a mut [Entity], range: i32) -> Option<&'a mut Entity> {
  // ...
}

The exact details are in the Rust book, but in short this says this returns an optional entity, which has the same lifetime as the entities array, which will be true if we return a reference to something in the array.

Lastly, I finished by doing pretty much the same steps as I did before. I added confuse scrolls to the list of possible items, and I needed to add a match clause to switch AIs when the spell is used.

Adding a fireball spell

I added another spell, with another twist. This will be a fireball spell, but instead of targeting the closest enemy, the player will get to select a target tile, and it will do damage in a radius around the target.

To implement this feature, I needed to add a new UI state which allows the player to do the targeting. This is similar to the previous section, where I added a new UI state to allow the player to select an inventory item to use. I generally followed the same procedure. First, add in the machinery to handle the state changes, and don't worry about how to handle the actual damage yet. Instead, I put in a stub function that printed out which tile had been targeted. When the user wants to cast a fireball, I moved them into GameState::Targeting. This will automatically drop the inventory menu because the player is no longer in the Inventory state. Then, when the user presses the mouse button on a tile, if the game is in the targeting state, it will call use_targeted_item in the ItemSystem with the (x,y) coordinates the player selected.

Once that part was working, handling the actual item effect was a just a matter of iterating through all the entities, seeing which ones of them were within a certain distance, and dealing damage to each of them.

This same mechanism can be adapted to implement spells that target an enemy, instead of targeting a tile. I opted to make them two separate states, since they seemed fairly different to me. I do think it's possible to make them a single state, but when doing the initial implementation of a feature, I like to first keep it simple and separate, get it working, then assess how much of it is shared and then refactor.

To add a targeted enemy spell, I added a new item effect which targets an enemy, and a new game state that lets the UI target an enemy. This is the same thing I do every time I add a new spell.

The only new design question here is where to check for a valid tile. In the "target enemy" mode, the player shouldn't be able to click on a tile which doesn't have an enemy. There are two places I can do the check.

Both of these are reasonable, but the most important thing is to stay consistent as much as possible. Don't have half your item effects check their inputs, and the other half expect the caller to check inputs. That way leads to darkness.

In retrospect, I think having the ItemSystem fiddle with the current game state is a bad idea. It should probably return what the new game state should be, or possibly the game loop should do that based on the type of item. The game state is pretty much the moral equivalent of a global here, and so I should really limit its scope as much as possible. In other words, I should try to have as few functions as possible need or use this data. The entities already need to be used everywhere, but that shouldn't be the case for some of the other state.

Summary

This functionality actually took a bit longer than I expected, for a couple of reasons. One is that with the additional infrastructure, there are more places I need to modify when adding functionality. While the steps are pretty straightforward, which is good (ie, add another enum to the component, add a function to handle the effect, etc.), it does make it a bit longer to add in more features. The other is that as the code gets further broken down into more functions, I'm running into more subtleties with the borrow checker that didn't manifest when everything could be contained to a single function or scope.

I think this is also an interesting look at the kinds of bugs that Rust can and can't help with. For example, if I had written the AI code in another language, I potentially could have had a memory smasher. On the other hand, the Rust version still has the same limitations - it's just that bugs manifest themselves as different (and arguably more debuggable) behavior.


In this series: Contents, previous


by Yosen
I'm a software developer for both mobile and web programming. Also: tech consultant for fiction, writer, pop culture aficionado, STEM education, music and digital arts.