FX My Life 3.2: Seek The Treasure!

FX My Life 3.2: Seek The Treasure!

·

22 min read

We can now place walls in our level designer, so let's add the next game piece: treasure. The characteristics of treasure are:

  1. It occupies a space.
  2. When the player bumps into it, he picks it up. (It vanishes.)
  3. Optionally, we can say the exit will not be open until all treasure is picked up.

This seems like it should be pretty easy, but we'll also take the opportunity to embellish the designer, and maybe put in some way to play the level. Oh, so, we'll also probably want to make it possible to place the player and the exit.

That should be enough for one chapter!

Let's start with the concept of "occupies":

    public record Thing(
            GamePiece id,
            Direction blocks,
            Boolean occupies
            ) { }

A piece that "occupies" the board prevents entrance into it from any direction. So, in our movePlayerOneSpace we want to check for occupies:

        for (var entry : things.entrySet()) {
            if (entry.getKey().equals(player) &&
                    entry.getValue().stream().anyMatch(thing -> thing.blocks == d)) return false;
            if (entry.getKey().equals(c) &&
                    entry.getValue().stream().anyMatch(thing -> thing.blocks == directionComplement(d))) return false;
            if (entry.getKey().equals(c) &&
                    entry.getValue().stream().anyMatch(thing -> thing.occupies)) {
                return false;
            }

The order of these checks is important: If he can't exit the square (say, due to a wall) or he can't enter the square (again, due to a wall), he should never have an interaction with the thing that's occupying the square.

Now, going back to our Thing, let's introduce the concept of...hmmm...what should we call it? We could call it "pickupable"..."fragile"..."delicate"? The thing is, we don't want to get stuck with a word that doesn't fit, or it'll get weird later on. Let's say we have a trap trigger that fires and then goes away, well, "fragile" doesn't really fit there.

Sometimes it's best to use an awkward neologism, even if the spellchecker bugs you about it. So how about "onetouch"?

    public record Thing(
            GamePiece id,
            Direction blocks,
            Boolean occupies,
            Boolean onetouch
            ) { }

Yeah, it's not a word, but it's pretty clear in the context of the game. At least it's not misleading, which is what happens when you try to force an English word into a usage like this. Another possibility is removeOnTouch.

    public record Thing(
            GamePiece id,
            Direction blocks,
            Boolean occupies,
            Boolean removeOnTouch
            ) { }

Yeah, it's a little verbose but I like it. Looking it over, we can't use the anyMatch gag we roughed out for movePlayerOneSpace because we need the Thing that was touched.

My first stab at this was:

            Optional<Thing> t = entry.getValue().stream().filter(thing -> entry.getKey().equals(c) && thing.occupies && thing.removeOnTouch).findFirst();
            if(t.isPresent()) {
                remove(entry.getKey().x, entry.getKey().y, t.get());
                return false;
            }

So, we're filtering out everything that both occupies the square AND is removable, and if anything matches, we remove it and return false. But what if we have something that occupies but isn't removable? I'm not sure yet what this would be but we should make each object attribute independent—or literally combine them, like "occupiesAndIsRemovable". Or we could say "removeOnTouch" implies "occupies" and skip the occupies attribute.

That said, I can imagine a deadfall trap that creates a solid block occupying a space and not being removable, and I can imagine a floor trap trigger that is removeOnTouch but doesn't occupy a space.

So, where does this leave us? Let's handle each item separately:

            Optional<Thing> t = entry.getValue().stream().filter(thing -> entry.getKey().equals(c) && thing.removeOnTouch).findFirst();
            t.ifPresent(thing -> remove(entry.getKey().x, entry.getKey().y, thing));
            if (entry.getKey().equals(c) &&
                    entry.getValue().stream().anyMatch(thing -> thing.occupies)) return false;

So, we use the Optional bit for removing the item, then do our usual gag for occupies...but wait! That won't work either, because we'll have removed the thing and then tested to see if anything is occupying the space. But it's already gone!

After Effects

It was part of the plan all along to basically process game moves in turns. Like, I thought we could get away with removing the object in the same code that does the player moves—and really we could, there would be no big issue with checking to see if we had a value for t that included a true occupies value.

But I think if we get into the practice of combining effects into the player move code, we'll actually complicate the rules and run the risk of creating subtle distortions. I figured we would handle player moves this way:

  • As the player moves, we put the effects of that move into a list
  • When the player is done moving, we process the list, which creates its own list
  • We continue to process the effects lists until we get an empty list

Imagine the player moves and hits something that triggers a reaction, which in turn hits something that triggers a reaction, and so on. I envision the list objects looking like:

    public record Effect(
            Coord effectLoc,
            Thing effect,
            Coord causeLoc,
            Thing cause
    ) {
    }
   public ArrayList<Effect> effectList;

The cause at causeLoc did something to the effect at effectLoc. We may want to alter this slightly or enhance it—like, it's conceivable that there could be kinds of effects: touches, pushes, rebounds, I don't know what else, or even if we'll need them, but it's a possibility.

The player probably needs to be a Thing now, which I sort of figured we'd want (as well as the exit) last chapter but let's see what we can do without adding that yet: I'd like to get to the treasure being a removable object first.

So, let's change our previous remove code (in movePlayerOneSpace) to:

            entry.getValue().stream()
                    .filter(thing -> entry.getKey().equals(c) && thing.removeOnTouch)
                    .forEach(thing -> {effectList.add(new Effect(c, thing, player, null));});

And in movePlayer, we'll need to initialize the effect list:

    public boolean movePlayer(Direction d) {
        if (gameState != GameState.inGame) return false;
        var moved = false;
        effectList = new ArrayList<>(); //ADD THIS
        while (movePlayerOneSpace(d)) {

Then, after the move loop:

        }
        processEffects(); //add this
        return moved;
    }

And processEffects will be just this for now:

    public void processEffects() {
        effectList.forEach(e -> {
            if(e.effect.removeOnTouch)
                remove(e.effectLoc.x, e.effectLoc.y, e.effect);
        });

This is a lot of code to write without trying any of it out. So let's try it out! How hard can it be? (Famous last words.) We need to add the game piece:

public enum GamePiece {wall, treasure}

Now we'd like to add some treasure to the map. Right off the bat, we'll see our code for adding walls doesn't work unless we add in the parameters for occupies and removeOnTouch:

        game.add(2,2,  new Thing(wall, DOWN, false, false));
        game.add(2, 0, new Thing(wall, LEFT, false, false));

And here:

        complist.getSelectionModel().selectedItemProperty().addListener((obs, oldValue, newValue) -> {
            switch(newValue) {
                case "Wall (left)" -> dvm.thing = new Thing(wall, LEFT, false, false);
                case "Wall (right)" -> dvm.thing = new Thing(wall, RIGHT, false, false);
                case "Wall (up)" -> dvm.thing = new Thing(wall, UP, false, false);
                case "Wall (down)" -> dvm.thing = new Thing(wall, DOWN, false, false);
                default -> dvm.thing = null;
            }

On the nice side, we don't have to change the view-model at all: It only knows it has a Thing, not anything about the kind of Thing. I think we're going to want to have piece specific calls, like addWall and addTreasure and so on.

After a quick sanity check to see that we haven't broken anything—we haven't—we can add in a Treasure.

game.add(2, 0, new Thing(treasure, null, true, true));

And we'll see—well, we won't see anything because we don't have any code for drawing treasure, but it will be there and it will stop us exactly once!

image.png

Going left stops us once. But if we go left again:

image.png

Nice! Now let's actually draw the loot. In our view-controller draw() we should do this:

        for (var entry : game.things.entrySet()) {
            for(var thing : entry.getValue()) {
                switch(thing.id()) {
                   case wall -> addWall(entry.getKey().x, entry.getKey().y, thing.blocks());
                    case treasure -> {
                        var t = new Circle();
                        setToken(t, tokenCalc(t, new Coord(entry.getKey().x, entry.getKey().y)));
                        t.setFill(Color.GOLD);
                        board.piecePane.getChildren().add(t);
                    }
                }
            }
        }

This will draw the "gold":

image.png

So, that's cool. The gold will stop us, but:

image.png

It won't actually go away. It's gone, and if we go left again:

image.png

You can actually see it gets erased before the player token moves over it. If you've been following along from the beginning, you should be able to figure out why. Well, in the movePlayer() code, we're doing this:

        while (movePlayerOneSpace(d)) {
            var delta = dirToDelta(d);
            player.x += delta.x;
            player.y += delta.y;
            moved = true;
            alertConsumers(); //*******
. . .
        }
        processEffects();
        return moved;
    }

So after we move the player, we call alertConsumers() and that updates the visual representation. We're not doing that after processEffects. But! If we add an alertConsumers():

        processEffects();
        alertConsumers();

The gold will vanish before we bump into it! (Try it out!) Again, this should be fairly clear at this point: Our animation lags the game state. Updating the game state occurs immediately; updating the visual representation does not.

Actually, thinking about it, we don't need the alertConsumers() in the while. If you delete the alertConsumers() above marked with the asterisks, it will still work. Why? Because we're handling the player animation directly, without need to resort to a full redraw.

We may need a more complex mechanism to handle the other effects of the game: In fact, we almost certainly will. But rather than explore that subterranean rodent habitat, let's see if we can't work this out simply for now. First, I think we should refactor our key-handler a little bit:

        board.addEventHandler(KeyEvent.KEY_PRESSED, new EventHandler<KeyEvent>() {
            @Override
            public void handle(KeyEvent keyEvent) {
                var valid =
                        switch (keyEvent.getCode()) {
                            case UP -> game.movePlayer(UP);
                            case RIGHT -> game.movePlayer(RIGHT);
                            case LEFT -> game.movePlayer(LEFT);
                            case DOWN -> game.movePlayer(DOWN);
                            default -> null;
                        };
                if (Boolean.TRUE.equals(valid)) {
                    addTransition(new DsViewModel.Transition(playerToken, new Coord(game.player.x, game.player.y)));
                    if (game.gameState == GameState.postGame) {
                        messageWin.show(Main.me.stage);
                        showMessage("You have escaped!");
                    } else if (game.gameState == GameState.beenEaten) {
                        messageWin.show(Main.me.stage);
                        showMessage("You have been eaten!");
                    }
                } else if (Boolean.FALSE.equals(valid)) Toolkit.getDefaultToolkit().beep();
                keyEvent.consume();
            }
        });
    }

We're capturing the result of the key event returned by game.movePlayer now. So valid will be either true or false. But we also want to be able to tell the difference between an illegal move and a keypress we ignore, so if we're not handling the keypress, we'll just set valid to null.

If valid is true, we'll do all the animation stuff we were doing before, and if it's false we'll beep. Actually, consider that beep() a placeholder. An awful, awful placeholder as it just uses the default system sound.

In any event, we're going to consume the event because if we don't, we'll end up activating the tabs on the notebooks or doing who knows what else.

I stumbled on this refactoring while thinking through the animation issue, though it's not really on point. Sometimes refactorings just ambush you and you might as well go for it. (Other times, however, particularly if you're in the middle of coding a different task, it's vitally important to just carefully note down what you'd like to do and do it after the current step.)

In this case it was helpful because it reminded me that we can't really handle the issue from the keypress, because all the keypress does is queue up animations. What does this mean? Well, when we're done playing transitions, we can just call draw() and the board will end up looking like it should:

    public void playTransition() {
. . .
            timeline.setOnFinished((e) -> {
                removeTransition();
                if (transitionQueue.size() == 0) draw(); //add this
            });
            timeline.play();
        }
    }

Now, theoretically—and very possibly actually—the game board will look a little screwy during the transition times. Like, if we bump the treasure by going left, then immediately go right, it's possible that the treasure won't get removed until after we've finished moving right, which would look odd.

I never was able to view this happening, if it ever did, however. At some point, we'll probably need to make the transitionQueue hold all kinds of different animations or, more likely, we'll have a different effects queue that runs independently of the queue we're using to move the player.

Treasure Maps

Our next step will be to allow the designer to place treasure. Let's do this the easiest way possible and then look at refactoring. Add an item to the list view in the FXML:

<String fx:value="Treasure" />

And now in the controller, we just need to add a case for "Treasure":

                case "Treasure" -> dvm.thing = new Thing(treasure, null, true, true);

And...that's it! Well, that was easy! The only "weird" thing is that the parameters we use for the objects don't have to agree. For example, we could add these lines to the same places to create a "breakable" wall.

<String fx:value="Breakable Wall" />

And:

case "Breakable Wall" -> dvm.thing = new Thing(wall, LEFT, false, true);

Now we have a wall that stops the user from passing through it but vanishes when touched, like treasure. If we wanted to prevent this, we could add specific calls for specific items, so if we created a class method:

    public static Thing Wall(Direction dir) {
        return new Thing(GamePiece.wall, dir, false, false);
    }

then:

case "Wall (left)" -> dvm.thing = new Thing(wall, LEFT, false, false);

might become:

case "Wall (left)" -> dvm.thing = Game.Wall(LEFT);

But do we want to prevent this? After all, we're the ones setting up the list of items, and we're the ones writing the (one) line of code to place the object. Calling new Thing directly gives us a lot of freedom to experiment with different combinations of attributes, so I think we want to keep it.

And this code is literally used in one place. So, generally, no, I don't think we need to come up with specialized Thing constructors for every game piece. I do kind of like the Wall idea, though, since it turns our code from this:

                case "Wall (left)" -> dvm.thing = new Thing(wall, LEFT, false, false);
                case "Wall (right)" -> dvm.thing = new Thing(wall, RIGHT, false, false);
                case "Wall (up)" -> dvm.thing = new Thing(wall, UP, false, false);
                case "Wall (down)" -> dvm.thing = new Thing(wall, DOWN, false, false);

to:

                case "Wall (left)" -> dvm.thing = Dunslip.Wall(LEFT);
                case "Wall (right)" -> dvm.thing = Dunslip.Wall(RIGHT);
                case "Wall (up)" -> dvm.thing = Dunslip.Wall(UP);
                case "Wall (down)" -> dvm.thing = Dunslip.Wall(DOWN);

Oh, is the tab-pane switching driving you crazy, too? Every time we place a piece, the grid loses the focus and the tab-pane decided the arrow keys are how the tabs should be changed.

It seems to me that we should be able to have the grid request the focus after our case statement placing the piece, but that doesn't seem to work. For now, let's just put a:

board.requestFocus();

At the bottom of the draw() in the ViewController. We may find this overly greedy at some point but for now, it serves.

A Bug!?

Being able to place walls has revealed an unexpected behavior, or "feature".

image.png

We tested "grabbing" the treasure from the left, and it worked, but if we build a map like this and try to grab the treasure from underneath, we get the system alert sound and the treasure doesn't go away.

Rather more confusingly, it does go away but isn't re-drawn. If you expand the map after touching it:

image.png

My first thought was that there was something wrong with the "UP" approach, somehow, like we had set the treasure to block, but that's not the case. Then I placed treasures all around the map to see if the approaches worked from all the other angles—

image.png

Including for up! Then I thought...hmmm...what if:

image.png

Sure enough, "alert" sounding in all directions, no re-draw. Figured it out? The key is that movePlayer only returns true if the player has moved, and when he's blocked by treasure, he doesn't move!

So, it's actually WAD: Working as designed! In many ways, these are the worst kinds of "unwanted program features", since it reveals an underlying flaw in how we were thinking of something, and will require some thought to resolve properly.

Before we thought of a successful "move" as one that changed the player's position, but in fact, a successful "move"—or we should perhaps double-up on the scare quotes and say a "successful" "move" is one that changes game state. (The change in state could result in the player's demise, so "successful" deserves some quotes.)

We Interrupt This Program

I (foolishly) upgraded my IntelliJ after this section and discovered that the fxgames project would no longer run because Java version 17 wasn't supported. So, I lowered it to version 16, and that also wasn't supported.

Weird, because you don't expect an upgrade to take support for newer things away. (In fairness, I saw something about this somewhere and clearly did not give it enough attention.) So, kicked it down to 15, somewhat worried that I would end up losing support for records, which is not good. Turns out the only thing not supported that we're using is:

        if (!(o instanceof Coord c)) {

That little bit of sugar c after the Coord. We could live without it, but I opted for using the "--enable preview" switch. It needs to be added both to the compiler section:

image.png

And to the run/debug configuration options:

image.png

After the initial impression that records were still working, it looks like, no, the preview won't save us. But all we have to do is download the JDK 17, which you can do easily from IJ.

image.png

Now Back To Our Regularly Scheduled Distraction

Our current issue (and one which will have some far-flung ramifications for the game) is how to detect a change of game state ex-post-facto. Another way of looking at it is to say "we want to examine the history of the game board to date."

One of the cool features of "Sleepaway Camp" is that it allows you to "re-wind". It works very aesthetically with the conceit of the game being a VHS-era slasher movie series, but the practical effect is that it allows the user to go backward a move when he's either lost or stuck.

In a functional world without mutable state, this would be trivial. (In fact, functional language tutorials are always talking about how you get a complete history of everything ever done in the program for free!) The way it works there is that your entire game state would be encapsulated in a hash-map. Something like this:

{:game-state :in-game :player {:x 5, :y 4}, :things [[2, 1] {:id treasure :occupies true :remove-on-touch true][2 4] {:id wall :blocks south}...]

When the player hits a treasure, the new game state would be returned:

{:game-state :in-game :player {:x 5, :y 4}, :things [[2 4] {:id wall :blocks south}...]

The old state remains untouched and the new state is basically a "diff" (as part of the language design, not due to anything the coder does) so detecting a state change is super-simple. We can't really have the same efficiency, but could we get a similar effect by doing something like:

List<Dunslip> history = new ArrayList<>();

And then in movePlayer():

    public boolean movePlayer(Direction d) {
         recordState();

And then:

    private void recordState() {
        history.add(this.copy());
    }

And I guess copy() would be something like—this approximation is wrong, but:

    private Dunslip copy() {
        var d = new Dunslip(this.width, this.height);
        d.player = this.player;
        d.exit = this.exit;
        d.gameState = this.gameState;
        d.things = this.things;
        d.effectList = this.effectList;
        d.history = this.history;
        d.consumers = this.consumers;
        return d;
    }

Theoretically this should all work (except the copy(), which we'll get to in a moment) and should allow us to "reverse" the flow of time just by going back into the history array.

The problem with it is that the copy() method is just copying references: pointers to existing objects. This line, for example:

        d.player = this.player;

Doesn't create a new Coord, it just copies a pointer to the existing one. So when the player moves in the game, it will also move in the history. This is why we can't just say:

history.add(this);

We'll just get umpty-ump copies of the current object. We need to copy the existing fields to new instances, just like we're doing to create a new Dunslip. Let's take a slightly different approach from the one above. We'll have recordState() look like this:

    private void recordState() {
        history.add(new Dunslip(this));
    }

And we'll create a new Dunslip constructor:

    public Dunslip(Dunslip d) {

And IJ will warn us if we don't copy our fields properly. The basic fields are easy to copy:

    public Dunslip(Dunslip d) {
        this.width = d.width;
        this.height = d.height;
        this.player = new Coord(d.player.x, d.player.y);
        this.exit = new Coord(d.exit.x, d.exit.y);
        this.gameState = d.gameState;

To copy the things from one map to another requires a bit more work:

        for (Map.Entry<Coord, List<Thing>> entry : d.things.entrySet()) {
            for (Thing t : entry.getValue()) {
                this.add(entry.getKey().x, entry.getKey().y, new Thing(t.id, t.blocks, t.occupies, t.removeOnTouch));
            }
        }

We have to go through every entry in the things-map, then pull the list of things from that coordinate and re-create each thing. The record type may have some copy semantics some day but for now we have to make each Thing anew. (Though we can and probably will end up making a specific copy method for Thing.)

Records are immutable, by the way. There might be a way to achieve something like the simpler approach (enabled by immutable, functional languages) I described at the top of the section using records. But for now, let's carry on.

I'm not sure we need to copy effectList at all, but in case we do:

        if(d.effectList!=null) {
            this.effectList = new ArrayList<Effect>();
            for (Effect e : d.effectList) {
                this.effectList.add(new Effect(e.effectLoc, e.effect, e.causeLoc, e.cause));
            }
        }

The history field itself is kind of interesting. We can just do a shallow copy:

        this.history = new ArrayList<>();
        this.history.addAll(d.history);

Why? Because history is inherently immutable. That isn't to say Java can't change it but that, logically, since each item in the history represents a turn, there's never a reason to change it, and we can use the same pointer for (e.g.) Turn 1 and Turn 12 and Turn 40 in every case.

What we'll do, actually, to test this is allow the user to reverse time, and we'll either immediately destroy the "future" (in other words, he can rewind but not fast-forward), or we'll destroy the "future" when he makes a new and different move.

To copy the consumer list, we can genuinely just assign the reference:

        d.consumers = this.consumers;
    }

The consumer list should be the same universally and doesn't really depend on game state. We may not need to copy it at all.

If I Could Turn Back Time

Let's give this a trial. Let's catch the R and rewind:

public void handle(KeyEvent keyEvent) {
                var valid =
                        switch (keyEvent.getCode()) {
. . .
                            case R -> game.rewind(); //add this
                            default -> null;
                        };

Now we need a rewind() in Dunslip, which is kind of a tedious reversal of our constructor:

    public Boolean rewind() {
        if(history.size() == 0) return false;
        var d = history.get(history.size()-1);
        this.width = d.width; //Have to remove "final" from field
        this.height = d.height; //Have to remove "final" from field
        this.player = this.player = new Coord(d.player.x, d.player.y);
        this.exit = d.exit;
        this.gameState = d.gameState;
        this.things = d.things;
        this.effectList = d.effectList;
        this.history = d.history;
        this.consumers = d.consumers;
        return true;
    }

We had gone along with IJ's advice to make our height and width field's final, but that doesn't really work here. I guess we could mandate that height and width could never change and therefore not save them, but this works for now and might have usefulness later.

Both the constructor and this method are essentially variations of copy, the semantics of which have plagued object-oriented programming from the beginning of time (the late '60s): Our constructor does a "deep" copy, recreating each of the objects that make up our game, whereas rewind does a "shallow" copy: We don't need to recreate things because the history contains the actual list of the actual things we want.

Given that this is a simple case and yet has all kinds of exceptions, it's not hard to see why, e.g., Java's clone mechanic is considered broken. (In fact, we're going to discover the code has serious limitations shortly.)

However, for this use case (rewind), the code works! Behold!

DungeonSlippers4.gif

The player goes up, then goes to the left bumping the treasure, then to the right and down. Then I "R"ed my way backwards. The treasure re-appears and everything. Pretty nice!

Back To The Future

Now, what if we want to fast-forward? Let's keep a pointer to our history list:

    private int history_position = 0;

And we'll need to capture a fast-forward keystroke in our view-model:

                            case F -> game.fforward();

Since our fast-forward should be more-or-less the same, as far as recreating state goes, as our rewind, let's refactor out the copy stuff:

    public Boolean rewind() {
        if (history_position == 0) return false;
        history_position--;
        copy_history(history_position);        
        return true;
    }

    public Boolean fforward() {
        if (history_position == history.size() - 1) return false;
        history_position++;
        copy_history(history_position);
        return true;
    }

The fast-forward code revealed so many problems with the shallow-copy approach, I found myself having to build a toString() for Dunslip:

    @Override
    public String toString() {
        return "Dunslip{" +
                "history_position=" + history_position +
                ", player=" + player +
                ", things=" + things +
                ", history=" + history +
                '}';
    }

IJ makes it super-easy to create a toString() with selected fields, so I made a few, then deleted them because they're so specific to particular debugging issues. (Dumping the whole object out, as you would normally in toString(), makes debugging much harder.)

One of the first issues I had was that I didn't have a 0-state. We don't have a method for starting the game yet so the initial board state isn't preserved. This isn't the best solution, but it will do for now:

    public boolean movePlayer(Direction d) {
        if (gameState != GameState.inGame) return false;
        if (history.size() == 0) recordState();
. . .
        processEffects();
        recordState();
        return moved;
    }

OK, so if there's nothing in history, we're at the start state, and we only record the new state after the move is over. recordState() itself needed to be able to say, "If the player rewinds, then moves again, start their history from the current move":

private void recordState() {
        if (history.size() > 0) history = new ArrayList<>(history.subList(0, history_position + 1));
        history.add(new Dunslip(this));
        history_position = history.size() - 1;;
    }

The subList() does that. You can rewind to the beginning and you can fast forward to your last move. But if you rewind and take a different path, the original path is gone.

copy_history() had to change, too: That shallow copy wasn't going to work any more.

public void copy_history(int hp) {
        var d = history.get(hp);
        this.width = d.width;
        this.height = d.height;
        this.player = new Coord(d.player.x, d.player.y);
        this.exit = new Coord(d.exit.x, d.exit.y);
        this.gameState = d.gameState;
        for (Map.Entry<Coord, List<Thing>> entry : d.things.entrySet()) {
            for (Thing t : entry.getValue()) {
                this.add(entry.getKey().x, entry.getKey().y, new Thing(t.id, t.blocks, t.occupies, t.removeOnTouch));
            }
        }
        if (d.effectList != null) {
            this.effectList = new ArrayList<Effect>();
            for (Effect e : d.effectList) {
                this.effectList.add(new Effect(e.effectLoc, e.effect, e.causeLoc, e.cause));
            }
        }
        processEffects();
        this.consumers = d.consumers;
    }

The shallow copy gave us the last-recorded game state references, meaning that the player's move didn't just add to the record, it changed the last record. So we do need to create new instances.

Further, we need to copy the effects and process them, because there's no guarantee that they will be fully processed when we record the history. Phew!

There's a lot to be said for keeping all your game state in an immutable history!

Wrap-Up

Do you remember why we went down this path in the first place? It was because a change in the game state that didn't move the player got the "warning" sound, as if the move had been illegal.

We can now fix that in the view-model:

            public void handle(KeyEvent keyEvent) {
                var oh = game.history.size();
                var moved =
                        switch (keyEvent.getCode()) {
                            case UP -> game.movePlayer(UP);
. . .
                var valid = oh != game.history.size();
                if (Boolean.TRUE.equals(valid)) {

We're once again not really using the return from movePlayer() because we don't need it. We can literally just look at the history size: If it's grown, the move changed the game state and was therefore "legal".

It's a little cheesy, but it'll do.

The code for this chaper is here.