Dungeon 85

Last night I found a rather embarrassing defect. Fortunately, there are probably lessons to be learned.

As I sometimes do, I downloaded the Dung program to my tv-room iPad, and played with the game. I noticed that the princess could still move sometimes when she shouldn’t, and that sometimes the monsters could attack from further away than should be possible.

A monster can move up to three times. If it is two squares away, that would make move-move-attack possible. But monsters were crowding around from much further than two square away. I found the bug, and I vaguely recall what I did to fix it, but that iPad isn’t connected to my repo, so we’ll have to find the problem again. We’ll fix it, and then see what we have learned from the experience.

In other news, Bryan pointed out that the wall tiles show bricks sometimes that the princess couldn’t possibly see.

wall

In the top right of that room, the white bricks on the far side from the princess should, according to Bryan, not be visible. I grant that that’s the case, and I even tried just using a simple dark texture for all the walls, with no bricks at all. It looked OK, and I thought last night that we’d go with it. That would result in ripping out all the work to figure out which tile to use, that really neat binary thing. We could have a little chat about whether we should have been more clever, so as never to have done all the work, or whether sometimes you try stuff and change your mind.

But I’ve changed my mind upon looking at it this morning. I grant that the walls are technically “wrong”, but I like them. So sometimes you change your mind back. And that’s OK. The point, with thinking, is to keep doing it, to keep allowing new angles and ideas to influence us.

Well, I guess there’s nothing to it but to look for the bug. The bug is:

Monsters are getting more than one turn on a button or keypress (perhaps either, perhaps just one or the other), and the player can move during a battle crawl.

Oh, that reminds me of another thing Bryan said. He suggested that it’s weird having the battle executed step by step by the floater. The floater, he suggested, should be called when you have something to display.

He’s not wrong there either. At the time we did it, I think I had a number of ideas in mind. I wanted the battle results to crawl up the screen, because it would save me creating a console panel that would take up screen space. I wanted player and monster movement to be stopped during the crawl, so that they didn’t run around while the crawl was still all “And the spider’s leg falls off”. And I wanted the movement turned back on when the crawl ended, not when the battle ended.

Somehow that led me to the idea of having a crawl-provider that could respond to a request for “next”. I had tried a finite state machine, and in article 61, I tried the coroutine idea. As I started out, I said this:

Coroutines are cool. We should try never to do things that are cool. Chet Hendrickson says that if a fellow developer calls you over to their desk to look at something cool they’ve just put into the product, you should reach over their shoulder and delete it. Cool code is troubled code, all too often.

Coroutines are deep in the bag of tricks. I’d wager that many readers have never heard of them, and that very few have ever actually written them. Still, to every thing there is a season, and perhaps this is coroutine season.

Probably I should have listened to myself, but it does work, and we did learn some deep code things that are always nice to know.

I noticed just now, reading that article, that Bryan also said something else. The article reports:

[He] suggested tagging the text array with clues as to what to display on the screen, so that behind the scenes we could do visual effects. This seemed like a great idea and I had intended to do it. But I came to realize that while it’s an elegant idea, it wouldn’t work well.

For example, during a battle, the health of the opponents changes as they take damage. The flow of the battle is dependent on their actual health. If we want to defer that display until the relevant line comes out, the Encounter object would have to mirror all the variable attributes of the opponents and then update the real objects later. That seems to me to be unsuitable, even though it would be possible.

So I rationalized doing the thing that I wanted to do. And it works and it went in nicely. But is it too much? We’ll decide.

But enough navel-gazing, let’s find that bug. Defect. Find that defect.

Too Many Moves

When the player moves, monsters can move more times than one turn would allow. This suggests that something’s not right. Let’s find out what it is.

move.mov

In the movie above, the Murder Hornet is way too far away to get to the princess in one turn, but when I pressed the right arrow button on the screen, the hornet and the serpent both made it all the way to the princess and started an attack. This will not do.

First let’s look at where the ability to move is checked:

function Button:touched(aTouch, player)
    if aTouch.state ~= BEGAN then return end
    if not player:itsOurTurn() then return end
    local x = aTouch.pos.x
    local y = aTouch.pos.y
    local ww = self.w/2
    local hh = self.h/2
    if x > self.x - ww and x < self.x + ww and
       y > self.y - hh and y < self.y + hh then
        player[self.name](player)
    end
    player:turnComplete()
end

We see that we’re checking if it’s our turn, which goes like this:

function Player:itsOurTurn()
    return self.runner:itsPlayerTurn()
end

function GameRunner:itsPlayerTurn()
    return self.playerCanMove
end

This looks reasonable. How does that variable get manipulated?

function GameRunner:turnComplete()
    self.playerCanMove = false
    self:moveMonsters()
    self.playerCanMove = true
end

But wasn’t the floater supposed to be doing something? Yes, there are these methods called:

function Floater:startCrawl(listener)
    self.listener = listener or FloaterNullListener()
    self.yOff = self.yOffsetStart
    self.buffer = {}
    self.listener:startFloater()
    self:fetchMessage()
end

function Floater:increment(n)
    self.yOff = self.yOff + (n or self:adjustedIncrement())
    if self:linesToDisplay() > self.lineCount then
        table.remove(self.buffer,1)
        self.yOff = self.yOff - self.lineSize
    end
    if #self.buffer < self:linesToDisplay() then
        self:fetchMessage()
    end
    if #self.buffer == 0 then
        self.listener:stopFloater()
    end
end

Those methods are in player. Why didn’t I see them when I searched for playerCanMove?

function GameRunner:stopFloater()
    self.playerCanRun = true
end

function GameRunner:startFloater()
    self.playerCanRun = false
end

They’re referring to playerCanRun, not playerCanMove. I’m sure we saw that yesterday, and I thought we’d fixed it. Maybe it got reverted out. Move is correct, fix it:

function GameRunner:stopFloater()
    self.playerCanMove = true
end

function GameRunner:startFloater()
    self.playerCanMove = false
end

Commit: reference correct variable playerCanMove.

That’s nice. Does it fix the problem? It does not. A Death Fly just attacked me from four moves away, in one turn. But I believe I see why. Remember this?

function GameRunner:turnComplete()
    self.playerCanMove = false
    self:moveMonsters()
    self.playerCanMove = true
end

That is certainly going to cause our push-down stack problem, where we turn off the movement, move monsters, some of whom may legitimately attack, starting the crawl, sending startFloater, stopping movement, then we finish move monsters and turn movement back on. And it becomes possible for the princess to move during battle.

I think those calls are unnecessary. I’m going to break a personal rule and comment them out, however, because when problems arise in the future, I want to be reminded that they were there. This is not a good sign for my confidence, my tests, or my code, but when things get tough, we use the techniques we have until things get better.

function GameRunner:turnComplete()
    --self.playerCanMove = false
    self:moveMonsters()
    --self.playerCanMove = true
end

I also notice a lot of direct accesses to that variable. That’s a sign that perhaps it should be centralized, or that there’s some other solution going on. Certainly this variable changes at a rate very differently from the rest of GameRunner. Something smells funky here.

But even with those calls in, we had too many moves made by monsters after a single button press. Let’s look further, starting again from here:

function Button:touched(aTouch, player)
    if aTouch.state ~= BEGAN then return end
    if not player:itsOurTurn() then return end
    local x = aTouch.pos.x
    local y = aTouch.pos.y
    local ww = self.w/2
    local hh = self.h/2
    if x > self.x - ww and x < self.x + ww and
       y > self.y - hh and y < self.y + hh then
        player[self.name](player)
    end
    player:turnComplete()
end

The execution of the button’s action is a bit obscure in this code, but is clear when the button is defined:

    table.insert(self.buttons, Button("right",300,200, 64,64, asset.builtin.UI.Blue_Slider_Right))

function Button:init(name, x,y,w,h, img)
    self.name = name
    self.x = x
    self.y = y
    self.w = w
    self.h =  h
    self.img = img
end

We save the name, and execute the method of that name, so for our move right button we do player:right, which is:

function Player:right()
    self:keyPress("d")
end

We use the same code as the WASD key “d”, which is a move right, and is implemented like this:

function Player:keyPress(key)
    if not self:itsOurTurn() then return false end
    local step = PlayerSteps[key]
    if step then
        self:moveBy(step)
    end
    self:turnComplete()
end

Well, well. Look at that, won’t you? Another call to turnComplete. When we press a screen button, we’ll get two calls to turn complete, which will give all the monsters between two and six moves. No wonder they close in so rapidly.

What should we do about this?

It seems clear that we need a turnComplete after an accepted and processed key press, and we need one after an accepted and processed button press. And … not all the buttons correspond to a keypress:

function GameRunner:createCombatButtons(buttons)
    table.insert(buttons, Button:textButton("Flee", 100,350))
    table.insert(buttons, Button:textButton("Fight",200,350))
end

These two buttons aren’t really implemented at all yet, since player has empty methods for flee and fight, but they do “press” and, as the code is now, they call turnComplete only once.

How are we to resolve this? We want to call turnComplete once and only once after a button press or keypress, no matter what it is. Arguably the key pressing and button pressing logic should not be using each other, but should instead use some common, underlying methods that do the various operations. Then it would be legitimate for each of them to call turnComplete when the command is done.

“When the command is done” … that’s an interesting phrase, isn’t it? In my mind, and in yours coming right up, is the notion that each button press, each keypress, is a “command”, and the turn is complete when the command is done.

There’s a concept in our minds–I know it’s in yours now–of a “command”, and that concept is not in the code. Kent Beck’s Rules of Simple Design include “the code contains all the programmer’s ideas about the code”.

The code is asking us to put in the idea of “command”. Unfortunately, the code isn’t telling us just how to do that. Funny how that goes.

Should we fix the bug now, or wait until we have our new command thing to fix it? I want to fix it now, because it makes a real problem in the game. I think I see an easy way.

Let’s refactor this:

function Player:keyPress(key)
    if not self:itsOurTurn() then return false end
    local step = PlayerSteps[key]
    if step then
        self:moveBy(step)
    end
    self:turnComplete()
end

We’ll extract the working bit:

function Player:executeKey(key)
    local step = PlayerSteps[key]
    if step then
        self:moveBy(step)
    end
end

function Player:keyPress(key)
    if not self:itsOurTurn() then return false end
    self:executeKey(key)
    self:turnComplete()
end

Now there’s a function that does the action and doesn’t do turnComplete and we can call it from the functions that the Buttons call:

function Player:left()
    self:executeKey("a")
end

function Player:right()
    self:executeKey("d")
end

function Player:up()
    self:executeKey("w")
end

function Player:down()
    self:executeKey("s")
end

Now there should be just one call to turnComplete per button or key press, which is what we meant all along.

I’ll do some testing to be sure. And it’s still happening!

I just had a serpent jet across at least a half a dozen tiles in one swoop when i touched the go-right button just once.

I see no way out but to do some prints.

While putting in the print, I noticed that this method calls moveMonsters:

function GameRunner:turnComplete()
    print("turn complete")
    --self.playerCanMove = false
    self:moveMonsters()
    --self.playerCanMove = true
end

And just for fun, I looked for references to that, and found this:

function GameRunner:playerHasMoved()
    self:moveMonsters()
end

And looking for references to that we find no references. Well, I can delete that. I was hoping it was used, it would hae explained the problem I’m seeing. Leaving the print in …

I just touched the go-right button ONCE and I see three calls to turnComplete. I see no way that can happen. I’m going to put a print in the touched. Maybe I’m getting more than one.

function Button:touched(aTouch, player)
    if aTouch.state ~= BEGAN then return end
    if not player:itsOurTurn() then return end
    print("touched running")
    local x = aTouch.pos.x
    local y = aTouch.pos.y
    local ww = self.w/2
    local hh = self.h/2
    if x > self.x - ww and x < self.x + ww and
       y > self.y - hh and y < self.y + hh then
        player[self.name](player)
    end
    player:turnComplete()
end

I see “touch running” six times for one button press.

Touch state has four states, of which you only ever see the first three: BEGAN, CHANGED, ENDED, and CANCELLED.

Oh. We have six buttons, and they all get to look at the touch. And each one signals turn complete, whether it processes the touch or not.

There’s our defect. Fix is “easy”:

function Button:touched(aTouch, player)
    if aTouch.state ~= BEGAN then return end
    if not player:itsOurTurn() then return end
    local x = aTouch.pos.x
    local y = aTouch.pos.y
    local ww = self.w/2
    local hh = self.h/2
    if x > self.x - ww and x < self.x + ww and
       y > self.y - hh and y < self.y + hh then
        player[self.name](player)
        player:turnComplete()
    end
end

I’m quite sure that we’ve nailed this one. I even took that print back out. I will test a bit though. Yes, works as intended now.

Commit: fix turnComplete defect causing monsters to move too far.

Let’s do a little retrospective and then decide whether and how to proceed.

Retrospective

We set out to find out why monsters were moving too far, and we found out and fixed it. All good, move on, nothing to see here …

Except, no. We had references to two member variable namess, only one of which was right. The wrong one was set and never referenced. Lua’s compiler isn’t smart enough to detect that, but I do have an idea for something we could do. Let’s get the issues on the table and then circle back.

We had the Floater crawl correctly calling start and stop, which correctly stopped player motion and started it. That part was actually right.

We have a method turnComplete that tells the GameRunner that the player has taken her turn, and that it is time to give the monsters their turn. But that method was also setting and clearing the playerCanMove flag, and that could override the Floater’s control of that flag. It seems to me now that we can probably live with the Floater being the only object that should freeze the player, although in principle she shouldn’t move when the monsters are moving. But in practice, she won’t, because the touches and keystrokes probably won’t interrupt.

Probably. Maybe she should be locked out. But that will require solving the nesting problem. Table that too.

Then we had both the button and the keyboard sending turnComplete, which allowed for more than one monster move per apparent turn. So we moved things around to allow both the button and the keyboard to call turnComplete without the other also doing it.

And then, I finally realized that since all the buttons get a shot at accepting the touch, they were all sending turnComplete, so you got six per button press. That got resolved by moving the call so that it is only made if the button accepts the touch.

That could be handled another way. For example, we could loop over all the buttons, checking whether they want the touch and only then calling their action. Presently it goes like this:

function GameRunner:touched(aTouch)
    for i,b in ipairs(self.buttons) do
        b:touched(aTouch, self.player)
    end
end

I’d hate to ask that question up there, though. That would violate the Friendly Admonition of Demeter. Let’s see if the button code itself can be more clear:

function Button:touched(aTouch, player)
    if aTouch.state ~= BEGAN then return end
    if not player:itsOurTurn() then return end
    local x = aTouch.pos.x
    local y = aTouch.pos.y
    local ww = self.w/2
    local hh = self.h/2
    if x > self.x - ww and x < self.x + ww and
       y > self.y - hh and y < self.y + hh then
        player[self.name](player)
        player:turnComplete()
    end
end

Oh yeah. No doubt. How about this for a first cut:

function Button:touched(aTouch, player)
    if aTouch.state ~= BEGAN then return end
    if not player:itsOurTurn() then return end
    if not self:mine(aTouch) then return end
    player[self.name](player)
    player:turnComplete()
end

function Button:mine(aTouch)
    local x = aTouch.pos.x
    local y = aTouch.pos.y
    local ww = self.w/2
    local hh = self.h/2
    return x > self.x - ww and x < self.x + ww and
    y > self.y - hh and y < self.y + hh
end

That’s somewhat more clear. Do we prefer this?

function Button:touched(aTouch, player)
    if aTouch.state == BEGAN and player:itsOurTurn() and self:mine(aTouch) then
        player[self.name](player)
        player:turnComplete()
    end
end

Perhaps we do. Commit: refactoring Button:touched for clarity.

Do we prefer this?

function Button:touched(aTouch, player)
    if self:shouldAccept(aTouch,player) then
        player[self.name](player)
        player:turnComplete()
    end
end

function Button:shouldAccept(aTouch, player)
    return aTouch.state == BEGAN and player:itsOurTurn() and self:mine(aTouch)
end

Yes, we do. Commit: further refactoring Button:touched for clarity,

We could get truly fanatical with this:

function Button:touched(aTouch, player)
    if self:shouldAccept(aTouch,player) then
        self:performCommand(player)
    end
end

function Button:performCommand(player)
    player[self.name](player)
    player:turnComplete()
end

And you know what? That’s better, because it nicely encapsulates the execution of the command as consisting of doing the operation and signaling turn complete.

I’m tempted to try to get rid of that if, but not today.

Let’s circle back to the tabled operation of using the wrong member variable. There is something that we could do to make Lua help us a bit more on things like that.

Remember back when we did the DeferredTable, the first too clever time, we used two “hidden” lua functions, __index and __newindex.

Lua calls __index when you access a table element that isn’t there. Normally it just returns nil, but you can override the function to do anything. We could make it provide a clear error message if our code ever accessed a member variable that wasn’t there. (We’ll get an error soon, with a nil coming back, but our message could come out sooner and be more clear.)

Similarly, Lua calls __newindex when you store into a table element that has not had contents before. It would have been called when we said self.playerCanRun, intending playerCanMove. Again, we could get a clear message like “attempt to create member variable playerCanRun` or something.

To do that, we’d have to follow some simple rules like this:

All member variables must be initialized, at least to nil, in init. (And nil might not suffice, we’d have to try it.)
At the very end of init, we could define the __index and __newindex functions to give us those messages. Then the object would be locked against those errors.

In a production Lua environment, I might argue that this was a very good practice and we should do it. In the play environment, essentially what we have here, it’s a bit less clear. I’ll leave it on the yellow sticky notes where i keep ideas to work on. It might be educational as well as useful.

One more thing. The “command” concept. We have that idea in our heads, but we don’t really have it in the code. We don’t even have very many, just up down left right that actually function, and fight/flee that don’t even do anything, but already the code is kind of all over the map calling keypress and whatnot.

There should probably be some kind of object that knows the player and that accepts commands like “up” and “flee”, and maps them into calls on the player. And then the buttons would just tell the command object “up”, and the keys would just tell it “up”, and it would do the upping.

Should the player have a command object? Should pressing a key or button create a command object for the player and then call it? Should there be a well-known single command object that key and button presses know to talk to?

I don’t know the answer to that. Perhaps tomorrow I’ll have an answer. Perhaps we’ll evolve to something as yet uncontemplated. And perhaps, we’ll do something else entirely.

See you then!

D2.zip