Let’s clean some code. Let’s also try to make combat more interesting. Let’s also try to stay out of trouble. Is that so much to ask? (As usual, the plan changes before long. Also, trouble.)

We have a number of objects that concern me. In particular, the ones with more lines of code. That’s not necessarily a bad thing, but it can be an indication that the object is doing more than is ideal, and that turning it into two or more objects might be desirable.

We have an obvious candidate in GameRunner. GameRunner is just short of 350 lines, and here’s a list of just some of its methods:

  • addTextToCrawl
  • addToCrawl
  • clearTiles
  • connectRooms
  • convertEdgesToWalls
  • createCombatButtons
  • createInitialRoom
  • createLevel
  • createLoots
  • createRandomRooms
  • draw
  • drawButtons
  • drawKeys
  • drawLargeMap
  • drawMap
  • drawMessages
  • drawMonsters
  • drawTinyMap
  • dungeonSize
  • horizontalCorridor
  • initiateCombatBetween
  • itsPlayerTurn
  • keyPress
  • maxScrollValues
  • scaleForLocalMap
  • scaleForTinyMap
  • stopFloater
  • startFloater
  • touched
  • turnComplete
  • verticalCorridor

I hope it’s pretty easy to see that there are some possible classifications of what kind of method a given method is, such as:

  • Dungeon Creation
  • Drawing
  • Managing touch and keypress events
  • Managing entity movement
  • Central communication of information

And at least some of these are probably useless or nearly so. We spoke yesterday about startFloater and stopFloater, which are badly named and have little or no effect. Yesterday we modified the turnComplete logic to put a call to turnComplete into the crawl. We use that to cause the monsters to move:

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

Now, as the game works today, the player can move at any time, even during a battle. In the movie below, I quickly move the princess up to attack the PoisonFrog, then immediately dart back south. The attack and response have already happened, as soon as I move to the frog. The crawl is reporting history. But it looks odd, because the frog’s counter-attack appears to happen after we’re well away from it. Arguably, we should prevent that from happening by preventing player action until both her attack crawl, and any monster attacks in “that same turn” have played out.

frog

I would argue that the current meaning of turnComplete is really “player turn complete”, since it’s sent after player keystrokes or button presses, and it frees the monsters to move. I’d argue that there should be a corresponding turn completion thing for the monsters, that would free the player to move.

Now we are here to improve the code, or at least to consider it, so I am reluctant to try this. On the other hand, it’s fresh in my mind, we’ve had this nice little chat, so let’s go for it.

Player vs Monster Turns

Let’s begin by renaming turnComplete to playerTurnComplete.

function Player:turnComplete()
    local op = OP("extern", self.runner, "playerTurnComplete")
    self.runner:addToCrawl( { op } )
end

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

These two changes should keep everything the same, just a new opcode sent to the crawl and ultimately to the GameRunner. Let’s test.

Arrgh, a couple of tests are failing. And I’m not sure whether this change did the deed or not.

Revert.

Oh great, they were already failing. Better fix them and I really need to make it more obvious when a test fails. I’ll try to do that today.

The first error is:

1: First Attack  -- Actual:   , Expected: Princess attacks Spider!

The test is:

        _:test("First Attack", function()
            local result
            local i,r
            local player = FakeEntity("Princess")
            local monster = FakeEntity("Spider")
            local co = CombatRound(player, monster)
            result = co:attack()
            i,r = next(result,i)
            _:expect(r.op).is("display")
            _:expect(r.text).is("Princess attacks Spider!")
        end)

And CombatOperation:attack is:

function CombatRound:attack()
    if self.attacker:isDead() then return {} end
    local result = {}
    table.insert(result, self:display("  "))
    local msg = string.format("%s attacks %s!", self.attacker:name(), self.defender:name())
    table.insert(result, self:display(msg) )
    local cmds = self:attemptHit()
    table.move(cmds,1,#cmds,#result+1, result)
    return result
end

Oh right, I inserted that blank line, then failed to notice that the test had failed. Fix the tests to check the second line:

        _:test("First Attack", function()
            local result
            local i,r
            local player = FakeEntity("Princess")
            local monster = FakeEntity("Spider")
            local co = CombatRound(player, monster)
            result = co:attack()
            i,r = next(result,i) -- skip blank line
            i,r = next(result,i)
            _:expect(r.op).is("display")
            _:expect(r.text).is("Princess attacks Spider!")
        end)

That runs. Same error in the other test.

        _:test("monster attack", function()
            local result
            local i,r
            local defender = FakeEntity("Princess")
            local attacker = FakeEntity("Spider")
            local co = CombatRound(attacker, defender)
            result = co:attack()
            i,r = next(result,i) -- skip blank line
            i,r = next(result,i)
            _:expect(r.text).is("Spider attacks Princess!")
        end)

Tests run.

Let’s see if we can’t make a sound when a test fails. I’ll insert this at the top of where I show the tests:

function showCodeaUnitTests()
    if not Console:find("0 Failed") then
        sound(asset.downloaded.Game_Sounds_One.Crowd_Boo)
    end
    ...

Ah. There can be one batch with 0 fails and the check only looks for one. Find can do patterns:

function showCodeaUnitTests()
    if  Console:find("[1-9] Failed") then
        sound(asset.downloaded.Game_Sounds_One.Bell_2)
    end

OK, now I get noises, and an irritating popup of the tests. That’ll do for now. At least I’ll notice that the tests failed.

Let’s improve the display as well, using the patterns. Once I do that, the bell is redundant, because I get a big red overlay. I’ll leave it for now. Commit: fixed combat tests; CodeaUnit displays properly.

Now to put my playerTurnComplete back. But first, a comment.

Gumption Trap

I was really gung-ho to do the playerTurnComplete, monsterTurnComplete thing, and get a bit of improvement to game play. Then I found that test problem. Fixing the test was easy and didn’t cost me too much momentum–at least I think it didn’t. But then going on to improve the tests has taken the wind from my sails.

I’m going to take a break, make an iced chai, and come back in just as long as that takes. I am hopeful that the wind will come back up by then.

OK that was the work of moments, and I think I’m a bit refreshed. Let’s put that player turn stuff back:

function Player:turnComplete()
    local op = OP("extern", self.runner, "playerTurnComplete")
    self.runner:addToCrawl( { op } )
end

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

I typed that back in, which is usually my fashion. You might have stashed and cherry picked or something. Horses for courses. Now I expect game play to be the same and the tests as well.

Commit: rename turnComplete to playerTurnComplete

I have an ignored test in the old code for testing the coroutine version of encounters. I’m going to remove the tab. That will get my big display to green instead of yellow.

I should also be able to delete the EncounterCoroutines tab: we don’t use it any more as far as I know. If we do, we shouldn’t. Tests and game run. Commit: removed coroutine encounter and its tests.

The minute I made the tests display again, the yellow started to irritate me and I removed some code that should have been removed long ago. What does that tell us about making our test results visible?

Oh, nothing. Nothing at all.

Now, where were we? Oh yes, improving the playerCanMove logic. We do check the flag:

function GameRunner:itsPlayerTurn()
    return self.playerCanMove
end

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

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

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

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

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

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

However, the startFloater and stopFloater are useless. They are sent to the Floater’s listener, and it is never sent a listener any more. It only listens to itself, and it ignores itself anyway.

Let’s remove the listener stuff. I’ll start by removing the GameRunner methods, which will then trap any calls if I’m mistaken about there being no listeners.

All is well Remove listener logic from Floater.

function Floater:runCrawl(array, listener)
    self.provider:addItems(array)
    self:startCrawl(listener)
end

Becomes:

function Floater:runCrawl(array)
    self.provider:addItems(array)
    self:startCrawl()
end

And this:

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

Becomes this:

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

This gets deleted:

FloaterNullListener = class()

function FloaterNullListener:stopFloater()
end

function FloaterNullListener:startFloater()
end

This:

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

We have a new rule, which is that at that final if statement, the #buffer can never be 0. I can imagine accidentally breaking that rule. Let’s put a fatal assert there instead of our listener call:

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
    assert(#self.buffer~=0, "Floater buffer is empty!")
end

The editor tells me that I forgot to remove the listener parm here:

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

So …

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

I expect all to work fine now.

Almost all works fine, but I saw a monster attack after it was dead. I thought that was supposed to be fixed. Indeed, we have this commit message a few back:

`turnComplete` now issued at end of crawl. Resolves monsters attacking when dead.

Further testing convinces me that it can happen.

Another Gumption Trap

It seems certain that this is not a new effect. I’ve really only renamed some methods, and I’m (nearly) certain that there were never any listeners to the Floater, and that it would have never sent its stop message anyway.

I’m going to proceed as if this problem, whatever it is, is not a function of our work this morning. As such, let’s see if we can commit what we have. Looks OK to me, so commit: remove listener capability from Floater.

It’s a gumption trap, or even worse, because I was just about to complicate the player-monster handoff further by stopping player motion when there’s still action in the crawl. At least “important” action. Now, with a defect already present in there, it seems unwise to complicate it further.

I think I’ll make a visible “turn complete” in the crawl, right ahead of the working one, to see whether that tells me anything.

function Player:turnComplete()
    self.runner:addTextToCrawl("playerTurnComplete")
    local op = OP("extern", self.runner, "playerTurnComplete")
    self.runner:addToCrawl( { op } )
end

Ah. I see the problem, or at least a problem. Whenever the player moves, a ‘playerTurnComplete` message is enqueued to the GameRunner. But the princess can move while the scroll is running, so she can move and enqueue another and another. If she moves rapidly enough over to a monster, she can get there while three or four of those messages are still queued up. Then they unroll, and the monster can attack multiple times. Interesting.

I think there is another issue here:

function CombatRound:attack()
    if self.attacker:isDead() then return {} end
    local result = {}
    table.insert(result, self:display("  "))
    local msg = string.format("%s attacks %s!", self.attacker:name(), self.defender:name())
    table.insert(result, self:display(msg) )
    local cmds = self:attemptHit()
    table.move(cmds,1,#cmds,#result+1, result)
    return result
end

I’ve noticed this before. We don’t want to beat a dead princess, so we should check for defender dead as well.

    if self.attacker:isDead() or self.defender:isDead() then return {} end

Commit: attack won’t start if either entity is dead. makes sense.

I just commit the combat change, not the turn complete in the crawl change.

Trouble Again

I really had hoped to stay out of trouble today. But that was not to be. I do think that adding the “turn complete” text to the crawl has exacerbated the problem, but the fact remains that there’s something going wrong in the latching between monsters and player, mediated by the Crawl.

We really love that feature: and it seems to be the source of some serious timing problems. This is not surprising, it can be a bit like trying to put breaking news into the middle of a prepared news program, but it’s a problem nonetheless.

It does seem, though, that the issue can be said to be something like this:

After the player moves, the monsters can move. But the player is not constrained from moving, so she can move again while the monsters are still in motion. If this happens fast enough, the player can be one or more moves ahead of the monsters. When the player stops moving, the monsters then appear to take multiple moves in one turn.

I’m not sure how, or even whether, this concern allows a dead monster to attack or a live monster to attack a dead princess. But it can’t be helping. So I think we need to do a monsterMoveComplete operation, much like the player one, and that we need to prevent the player from moving until we see that message.

Therefore:

function GameRunner:playerTurnComplete()
    self.playerCanMove = false
    self:moveMonsters()
    local op = OP("extern", self, "monsterTurnComplete")
    self:addToCrawl( { op } )
end

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

This should be interesting. It might just work. Let’s see.

Well, it’s no worse. However, I’m still seeing some anomalous behavior.

dual

If I run up to a monster rapidly, or attempt to move onto its square rapidly, I can initiate two princess attacks before I see a monster attack. The movie below shows two effects that we don’t like. First, the princess makes multiple attacks before the Toothhead does. It’s possible that it didn’t try, but then why does it make two attacks after her two? And then the second cycle seems to show it dying long before the crawl comes out, and that’s not supposed to happen either.

I want to try a simpler test, to make sure that my latching is working at all. I’ll start a fight and try to run away while the crawl is running. That’s supposed to be impossible now.

It isn’t. I can still start a fight and run away. I think we need to set that the player can’t move when she has moved. But I thought we did that??

It turns out that we don’t. We do this as soon as we’ve completed our button press:

function Player:turnComplete()
    --self.runner:addTextToCrawl("playerTurnComplete")
    local op = OP("extern", self.runner, "playerTurnComplete")
    self.runner:addToCrawl( { op } )
end

But that doesn’t lock the player until the playerTurnComplete unwinds from the crawl. We should actually lock our movement immediately, but not unlock the monsters until our crawl completes.

Maybe the externs should be playerCrawlCompletes and monsterCrawlCompletes? For now, I’ll do something a bit more direct:

function Player:turnComplete()
    self.runner:stopPlayerMovement()
    local op = OP("extern", self.runner, "playerTurnComplete")
    self.runner:addToCrawl( { op } )
end

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

Let’s see how this plays. The good news is that it has probably made the player-monster handoffs more nearly correct. The bad news is that it puts a guaranteed delay between player moves, while we wait for the blanks in the crawl to scroll off.

Recall that we have the crawl running all the time, displaying blank lines if there’s nothing better to do. That seemed like an easy solution, better than stopping and restarting it. But it does mean that our magic synchronizing messages can experience an awkward delay while the blanks scroll up.

Let me change the default line and we’ll see the effect more clearly.

function Floater:init(runner, yOffsetStart, lineSize, lineCount)
    self.runner = runner
    self.provider = Provider("...")
    self.yOffsetStart = yOffsetStart
    self.lineSize = lineSize
    self.lineCount = lineCount
end

I’ll also make the turn complete visible again.

Much of the time in the video below, I was trying to run from side to side, but because the turn complete had not appeared, I couldn’t move. It’s fine not to move when there’s action, but it’s not fine to wait for the “…” to scroll off.

moving

Watching the display and feeling the keys makes me wonder if this is a problem:

function Provider:getItem()
    if #self.items < 1 then return self.default end
    local item = table.remove(self.items,1)
    if item.op == "display" then 
        return item.text
    elseif item.op == "extern" then
        self:execute(item)
    elseif item.op == "op" then
        self:addItems(self:execute(item))
    else
        assert(false, "unexpected item in Provider array "..(item.op or "no op"))
    end
    return self:getItem()
end

This method is called when the floater wants something to display. If, at that time, the next item is an OP, the OP is executed and we fetch again. If it is text, we return it. We’ll be called again later, when that line has scrolled up one ful line height.

That means that if the items after a display line are ops, they will not execute until the display line scrolls up one line height.

We do execute all the commands ahead of any display line. We could, in principle, execute all the commands following it, up until the next display line. That seems possible but odd.

Hm. In what order do we call things now? Here’s the only case that matters now:

function CombatRound:rollDamage()
    local result = {}
    local damage = self.random(1,6)
    local op = { op="extern", receiver=self.defender, method="damageFrom", arg1=nil, arg2=damage }
    table.insert(result,op)
    table.insert(result, self:display(self.defender:name().." takes "..damage.." damage!"))
    return result
end

We apply the damage with damageFrom, then we display the damage amount, and then we return from the combat operation entirely. The damageFrom, when it executes, will do this:

function Entity:damageFrom(aTile,amount)
    if not self:isAlive() then return end
    self.healthPoints = self.healthPoints - amount
    if self.healthPoints <= 0 then
        self.runner:addTextToCrawl(self:name().." is dead!")
        sound(self.deathSound, 1, self.pitch)
        self:die()
    else
        sound(self.hurtSound, 1, self.pitch)
    end
end

That possibly adds “Foobar is dead!”, if in fact the Foobar is dead.

Oh my. There’s the bug, I think.

The CombatRound has returned. Therefore the code run on behalf of the player or monster, running the combat, has completed.

So we were here:

function GameRunner:playerTurnComplete()
    self.playerCanMove = false
    self:moveMonsters()
    self:addTextToCrawl("monster turn complete")
    local op = OP("extern", self, "monsterTurnComplete")
    self:addToCrawl( { op } )
end

We moved the monsters. Each one may have initiated an attack, and the results of that attack are in the queue, down to but not including the damageFrom, which is deferred. At that point, we enqueue monsterTurnComplete, before the damageFrom can have been dequeued. So we report turn complete and another move can find the player or monster still alive when trying to start a combat round.

In addition, if two monsters attack in one turn, and the first kills the princess, the second is guaranteed to attack her anyway,

Yep. That’s the bug. As to the fix, I do not know.

It’s past time to be done for the day. We’ve got the code a bit better, but we’re again staring down the barrel of the Crawl’s need to know the future before it happens.

That will have to wait for next time. But I’ll sum up before I go. Also let’s see what’s in the commit queue.

  • Floater changed default. Don’t want that.
  • Display of turn complete. Don’t want those.
  • New player movement stopper. I think yes.

Commit accordingly: New player movement stopper. Crawler timing bugs still exist.

Let’s sum up.

Summary

It seems lately like the crawl idea is biting me about two out of every three days. The issues always arise in the same situation: sometimes we defer an action until the crawl gets around to reporting it, so that we can synchronize game play and the crawl output.

I thought that appending a “turn complete” flag to the buffer would ensure that everything was unwound, but that’s not the case if we append it from inside GameRunner. All the initial crawl loading will have taken place when a monster move is made, but any deferred operations, which almost always occur, will not have had their chance to load things. So our marker comes out before the combat results, and odd things ensue.

At this moment I see a few possibilities:

First, we could change the combat round code so that it enqueues a turn complete indicator at the true end of the round, that is, after the damage is applied. That seems possible.

Second, we could bite the bullet and predict the outcome of an attack, so that we can report in one go whether the defender is alive or dead. That seems possible but I think it would be messy.

Third, we could replace our current crawl with something different, something that might be easier to get right. That seems difficult, because I don’t have another idea.

Fourth, we could give up on the crawl idea. I am reluctant to do that, because I rather enjoy letting you see this seemingly simple problem kicking my tail, because I figure you’ll find yourself in a similar situation someday, and my plight may give you some kind of relief, even if it’s only “Well, I’m not as stupid as Ron was that time …”

For now, this is a wrap, and we’ll see what comes to mind tomorrow. See you then, I hope!

D2.zip