Dungeon 43

If at first you don’t succeed, try something different. Sorry, no pics or movies today. Just code.

Maybe we should try something different right away, if we don’t succeed, but that’s hard to do if we seemed almost to have it right. But over my many years it seems to me that I’ve generally done better trying something different–but not too different.

Today, however, I’m going to try something quite different.

The code that manages the interaction between the various things that can be in a room, and that can want to enter a room, is too complex and too distributed. Each of the interacting objects is required to implement a growing number of methods, most of which serve merely to say very little. And it’s an n-squared kind of thing, where each added entity adds at least one other method to all the others. Not good.

So I have a new plan. I wrote yesterday that I could imagine drawing a table of entities vs entities, and filling in whether moves were allowed. I even said that maybe we’d turn that table into an object. Yesterday, I was still hanging on to the notion of messages bouncing back and forth. Today, I want to try the table.

I’m envisioning a table indexed by resident type and would-be entering type, with table entries being something, I don’t know, that tell us what to do. We’ll find out as we get closer to it. My first mission is to see whether the type function in Codea works the way I think it does.

Begin with a test.

        _:test("type returns the class", function()
            local runner = GameRunner()
            local tile = Tile:room(10,10,runner)
            local chest = Chest(tile,runner)
            _:expect(type(chest)).is(Chest)
        end)

My plan was that the Codea Lua type function would return the class of the input table. It does not. It just returns the string “table”. Bummer. But where there’s a will, there’s a way. I know at least two more ways to do this.

This test passes:

        _:test("getmetatable returns the class", function()
            local runner = GameRunner()
            local tile = Tile:room(10,10,runner)
            local chest = Chest(tile,runner)
            _:expect(getmetatable(chest)).is(Chest)
        end)

So getmetatable returns the class. We’ll add in some others for clarity of what’s going on.

        _:test("getmetatable returns the class", function()
            local runner = GameRunner()
            local tile = Tile:room(10,10,runner)
            local chest = Chest(tile,runner)
            _:expect(getmetatable(chest)).is(Chest)
            local monster = Monster(tile,runner)
            _:expect(getmetatable(monster)).is(Monster)
            local player = Player(tile,runner)
            runner.player = player
            _:expect(getmetatable(player)).is(Player)
        end)

OK, this tells me that I can build a table indexed by the class (metatable) of my entities, and use it for whatever nefarious purposes I may have in mind. Now let’s figure out what those nefarious purposes might want to be.

Table of Entities

Res \ Ent	Monster	Player
Chest	no	no
Key	yes	yes-give
Monster	no-sep	no-attack
Player	no-attack	n/a

In the table above, the columns are entities trying to enter. The rows are the current residents of the cell being entered. The abbreviations are:

attack - the entering party attacks with initiative
give - the key is given to the player and removed from the cell
sep - some action is taken to separate the two monsters. Possibly just refusing entrance is enough.

If there’s more than one resident in the cell, which can only happen if a monster steps on a key (and maybe we should fix that), then all residents are checked against this table.

There may, in future, be multiple monster types. They might have different behaviors. The “rule” will be that this table has to have an entry for any encounter that can occur, unless that encounter is to default. We’ll define the default later, if ever. For now, we’ll ensure that the table is filled in.

Where should this table be accessed? (We’ll worry about exactly what and where it is shortly.) It seems that it might as well be accessed immediately upon attempted entry into a room, somewhere in these two methods.

function Tile:legalNeighbor(anEntity, aStepVector)
    local newTile = self:getNeighbor(aStepVector)
    return self:validateMoveTo(anEntity,newTile)
end

function Tile:validateMoveTo(anEntity, newTile)
    if newTile:isRoom() then
        local tile = newTile:attemptedEntranceBy(anEntity, self)
        self:moveEntrant(anEntity,tile)
        return tile
    else
        return self
    end
end

Test-Driving

Let’s imagine that we’re going to have a new method, and see if we can test-drive it. I have my doubts, but it seems wise to try.

Let’s further imagine that our new entrance checking method will still be on Tile and will be sent to the proposed new Tile, because that’s the one that knows the residents. And let’s say that the overall responsibility is to go through all the residents, allowing for more than one, to look up the action (or actions) in the table, and execute them. The actions can do whatever they want, but somehow, either those methods, or the table, must tell us yes/no whether the entry is permitted. We might want to allow for “don’t care” as a possibility, since we could imagine that either yes or no might be a definitive answer. (I’m not sure what we’d do with a conflict. Last one wins, probably.)

Let’s do entry first. I’m going to posit a new object with this table in it, RoomArbiter.

        _:test("monster cannot step on chest", function()
            local runner = GameRunner()
            local ct = Tile:room(10,10, runner)
            local chest = Chest(ct,runner)
            local pt = Tile:room(11,10,runner)
            local player = Player(pt,runner)
            local arb = RoomArbiter(chest, player)
            local moveTo = arb:moveTo()
            _:expect(moveTo).is(pt)
        end)

It sure takes a lot of messing about to set up a couple of objects. Anyway, this code has a tile with a chest in it and a tile with a player in it and is asked what the moveTo should be, and it should be to return the player tile unchanged,

I’m not sure that the contents are set up correctly in these two tiles, but I also think it won’t matter. Let’s find out.

Test is red:

16: monster cannot step on chest -- Tests:224: attempt to call a nil value (global 'RoomArbiter')

No surprise there.

RoomArbiter = class()

function RoomArbiter:init(resident, mover)
    self.resident = resident
    self.mover = mover
end

16: monster cannot step on chest -- Tests:225: attempt to call a nil value (method 'moveTo')

I should use “fake it till you make it” here, but I’m happy to drive out real behavior at this point:

function RoomArbiter:moveTo()
    return self.resident.getTile()
end

I’m going to require that all entities can return their tile, and do it via a method call. I could, in principle, rip their guts out with resident.tile but that’s even more wrong than asking. We’ll try to see if there’s something to do with tell-don’t-ask when we get this going.

I think this test will fail looking for getTile

16: monster cannot step on chest -- RoomArbiter:12: attempt to call a nil value (field 'getTile')

Implementing that:

function Chest:getTile()
    return self.tile
end

Test should run. But no:

16: monster cannot step on chest -- Chest:31: attempt to index a nil value (local 'self')

What have I done? Ah. Dot for colon mistake. Should be:

function RoomArbiter:moveTo()
    return self.resident:getTile()
end

Ah a real mistake:

16: monster cannot step on chest  -- Actual: Tile[10][10]: room, Expected: Tile[11][10]: room

That’s the wrong answer! Better fake it right. We want to refuse the move:

function RoomArbiter:moveTo()
    return self.mover:getTile()
end

But that doesn’t communicate. Test runs now, let’s refactor.

function RoomArbiter:moveTo()
    return self:refuseMove()
end

function RoomArbiter:acceptMove()
    return self.resident:getTile()
end

function RoomArbiter:refuseMove()
    return self.mover:getTile()
end

We still haven’t driven out any table stuff. Let’s add in some more checks, and at some point maybe even a separate test method.

I just noticed that the test is called “monster cannot” but checks player. Renaming. See below. Let’s check both player and monster against chest here. That won’t drive out any new behavior though. Interesting.

This revised test runs:

        _:test("player and monster cannot step on chest", function()
            local runner = GameRunner()
            local pt = Tile:room(11,10,runner)
            local player = Player(pt,runner)
            runner.player = player
            local ct = Tile:room(10,10, runner)
            local chest = Chest(ct,runner)
            local arb = RoomArbiter(chest, player)
            local moveTo = arb:moveTo()
            _:expect(moveTo).is(pt)
            local mt = Tile(9,10,runner)
            local monster = Monster(mt,runner)
            arb = RoomArbiter(chest,monster)
            moveTo = arb:moveTo()
            _:expect(moveTo).is(mt)
        end)

But yet again, I get the monster blowing up running its tween. I’m not sure where the problem is coming from, but I know it’s because the monster in question has no runner. Let’s fix that at its source:

function Monster:setTimer(action, time, deltaTime)
    local t = time + math.random()*deltaTime
    return tween.delay(t, action, self)
end

Let’s just not set the tween if there is no runner.

function Monster:setTimer(action, time, deltaTime)
    if not self.runner then return end
    local t = time + math.random()*deltaTime
    return tween.delay(t, action, self)
end

Was that a legit change? Yes. The crash during testing surely counts as a red bar, so this is a legit fix at this stage. (Of course, fixing a defect is always legitimate.)

Hahaha, legit you say? Well, it would be if it fixed the problem but the problem occurs here:

Tile:103: attempt to index a nil value (field 'runner')
stack traceback:
	Tile:103: in method 'getNeighbor'
	Tile:208: in method 'legalNeighbor'
	Monster:83: in method 'moveTowardAvatar'
	Monster:40: in field 'callback'
...

This is still in a tween, and I don’t see how we managed to set one up with that code in setTimer but it sure did happen.

This is a Tile with no runner. Maybe this isn’t in the current test? Or is there a typo, another surprise global, somewhere? Ignoring our latest test makes the crash go away, so it is in there somewhere.

Oh. We haven’t initialized the whole array of tiles. So probably the monster looked around for a tile he shouldn’t have. Let’s put these guys in a room:

        _:test("player and monster cannot step on chest", function()
            local runner = GameRunner()
            local room = Room(1,1,20,20, runner)
            local pt = Tile:room(11,10,runner)
            local player = Player(pt,runner)
            runner.player = player
            local ct = Tile:room(10,10, runner)
            local chest = Chest(ct,runner)
            local arb = RoomArbiter(chest, player)
            local moveTo = arb:moveTo()
            _:expect(moveTo).is(pt)
            local mt = Tile(9,10,runner)
            local monster = Monster(mt,runner)
            arb = RoomArbiter(chest,monster)
            moveTo = arb:moveTo()
            _:expect(moveTo).is(mt)
        end)

Irritatingly, the error is still occurring. I’m going to have to divert my attention to sorting this out, because it’s clear that something isn’t quite right. It’s telling me something about the testability of these objects (and, of course, something about the fact that these objects are a bit too autonomous during testing).

The error is coming from moveTowardAvatar, so I’ll instrument that method:

function Monster:moveTowardAvatar()
    local dxdy = self.runner:playerDirection(self.tile)
    print(self.tile, self.tile.runner, dxdy)
    if math.random() < 0.5 then
        self.tile = self.tile:legalNeighbor(self,vec2(0,dxdy.y))
    else 
        self.tile = self.tile:legalNeighbor(self,vec2(dxdy.x,0))
    end
end

This Monster’s Tile has no runner. How is that possible?

Ahh. This statement:

            local mt = Tile(9,10,runner)

Is, well, not to put too find a point on it, flat wrong.

            local mt = Tile:room(9,10,runner)

Now the test runs and the crash does not occur.

These objects are too hard to test. I have no current good idea for how to make them better. Let’s try to get our head back in the game of dealing with the RoomArbiter.

I noticed that it has a bad name. It is a TileArbiter or an EntranceArbiter, not a Room anything. Lets rename to TileArbiter. Done.

Now we need to drive out new behavior. Let’s do Key, it should be relatively easy.

17: monster can step on key  -- Actual: Tile[9][10]: room, Expected: Tile[10][10]: room

It does. Also I get the crash again. Darn it.

Tile:103: attempt to index a nil value (field 'runner')
stack traceback:
	Tile:103: in method 'getNeighbor'
	Tile:208: in method 'legalNeighbor'
	Monster:83: in method 'moveTowardAvatar'
	Monster:40: in field 'callback'
	...in pairs(tweens) do
    c = c + 1
...

This yak just won’t die.

It’s worth noting, I think, that even when I’m doing the best I can, weird stuff does sometimes happen and it gets in the way. I’m sure by now that this is a test misconfiguration issue, and that if I get the test configured right, it will go away. But the real bug is that we’re starting the tweens in Monster when it’s created, and shouldn’t really start them until the game is actually running.

I rather hate fixing this on a red bar, but I can argue that the crash is also part of a red bar, so I am still authorized by my rules to go after it. So now I’m going to remove the startup of monster motion from creation and make it happen elsewhere.

function Monster:init(tile, runner)
    self.alive = true
    self.tile = tile
    self.tile:addContents(self)
    self.runner = runner
    self.standing = asset.builtin.Platformer_Art.Monster_Standing
    self.moving = asset.builtin.Platformer_Art.Monster_Moving
    self.dead = asset.builtin.Platformer_Art.Monster_Squished
    self.sprite1 = self.standing
    self.sprite2 = self.dead
    self.swap = 0
    self.move = 0
    self:setAnimationTimer()
    self:setMotionTimer()
end

Extract the two bottom methods to a new method startAllTimers:

function Monster:startAllTimers()
    self:setAnimationTimer()
    self:setMotionTimer()
end

Call that on all the monsters during real game setup:

function GameRunner:createLevel(count)
    self:createRandomRooms(count)
    self:connectRooms()
    self:convertEdgesToWalls()
    local r1 = self.rooms[1]
    local rcx,rcy = r1:center()
    local tile = self:getTile(vec2(rcx,rcy))
    self.player = Player(tile,self)
    self.monsters = self:createThings(Monster,5)
    for i,monster in ipairs(self.monsters) do
        monster:startAllTimers()
    end
    self.keys = self:createThings(Key,10)
    self:createThings(Chest,10)
end

Now I think I should be left alone to work on my moving stuff. Test fails as expected:

17: monster can step on key  -- Actual: Tile[9][10]: room, Expected: Tile[10][10]: room

TileArbiter:moveTo is a bit less permissive than it needs to be:

function TileArbiter:moveTo()
    return self:refuseMove()
end

Now to do this job right, we have to consider who’s in the tiles we’re arbitrating:

function TileArbiter:init(resident, mover)
    self.resident = resident
    self.mover = mover
end

Let’s see if we can start driving out a table:

function TileArbiter:moveTo()
    local entry = self:table()[self:residentType()][self:moverType)]
    local result = entry:moveTo(resident,mover)
    return result
end

I’m not altogether sure about this sequence. I could test-drive at a lower level, but for now let’s see what happens here.

I need entries for chest vs player and monster, and key vs monster. I immediately think of “what if there is none”, and that tells me not to access that table so directly:

function TileArbiter:moveTo()
    local entry = self:tableMoverAction(self.resident,self.mover)
    local result = entry:moveTo(self.resident,self.mover)
    return result
end

This makes more sense to me. I’m still just working out the protocol.

function TileArbiter:tableMoverAction(resident,mover)
    local rclass = getmetatable(resident)
    local mclass = getmetatable(mover)
    local entry = self:table()[rclass][mclass]
    assert(entry, "missing entry")
    return entry.moveTo
end

Here, I’m supposing a double-indexed table, containing entries, and that the entries have an element moveTo that is a function.

Let’s see if we can make that happen. Here’s my cut at the table:

function TileArbiter:table()
    local t = {}
    t[Chest][Monster] = {moveTo=TileArbiter.refuseMove}
    t[Chest][Player] = {moveTo=TileArbiter.refuseMove}
    t[Key][Monster] = {moveTo=TileArbiter.acceptMove}
    return t
end

That tells me that I don’t need to pass the parameters in the call, so we change to this:

function TileArbiter:moveTo()
    local entry = self:tableMoverAction(self.resident,self.mover)
    local result = entry.moveTo(self)
    return result
end

Note that I realized I can’t call it with : sytax, or at least I think I can’t. Once this works I’ll try it: it might be OK. It’s a table with a symbol in it.

Let’s see why this explodes.

16: player and monster cannot step on chest -- TileArbiter:27: attempt to index a nil value (field '?')

function TileArbiter:table()
    local t = {}
    t[Chest][Monster] = {moveTo=TileArbiter.refuseMove}
    t[Chest][Player] = {moveTo=TileArbiter.refuseMove}
    t[Key][Monster] = {moveTo=TileArbiter.acceptMove}
    return t
end

Line 27 is the first assignment into t. That has to be the reference to refuseMove. Do I have to say self.refuseMove? I don’t see why that would be the case. And it doesn’t help. Can I assign, oh, 2?

Oh. It’s the double fetch isn’t it?

Code’s wrong. Should be:

function TileArbiter:table()
    local t = {}
    t[Chest] = {}
    t[Key] = {}
    t[Chest][Monster] = {moveTo=TileArbiter.refuseMove}
    t[Chest][Player] = {moveTo=TileArbiter.refuseMove}
    t[Key][Monster] = {moveTo=TileArbiter.acceptMove}
    return t
end

Now the error is:

16: player and monster cannot step on chest -- TileArbiter:13: attempt to index a function value (local 'entry')

function TileArbiter:moveTo()
    local entry = self:tableMoverAction(self.resident,self.mover)
    local result = entry.moveTo(self)
    return result
end

This is wrong because the tableMoverAction already pulls out the moveTo. I suspect we’re going to wind up wanting the full entry. Let’s switch to that form.

function TileArbiter:tableEntry(resident,mover)
    local rclass = getmetatable(resident)
    local mclass = getmetatable(mover)
    local entry = self:table()[rclass][mclass]
    assert(entry, "missing entry")
    return entry
end

function TileArbiter:moveTo()
    local entry = self:tableEntry(self.resident,self.mover)
    local result = entry.moveTo(self)
    return result
end

Now this might work. It will surely work differently.

17: monster can step on key -- TileArbiter:18: attempt to call a nil value (method 'getTile')

Key doesn’t implement getTile.

function Key:getTile()
    return self.tile
end

Both tests run. Neither monster nor player can step on chests. Monster can step on a key. Now for the player.

We want the player to step on the key, for the key to be given to the player, and for the key to disappear from the tile contents. Let’s do it:

        _:test("player can step on key and receives it", function()
            local runner = GameRunner()
            local room = Room(1,1,20,20, runner)
            local pt = Tile:room(11,10,runner)
            local player = Player(pt,runner)
            runner.player = player
            local kt = Tile:room(10,10, runner)
            local key = Key(kt,runner)
            arb = TileArbiter(key,player)
            local moveTo = arb:moveTo()
            _:expect(moveTo).is(kt)
        end)

This should assert lack of an entry.

18: player can step on key and receives it -- TileArbiter:39: missing entry

This compels the addition of an entry:

function TileArbiter:table()
    local t = {}
    t[Chest] = {}
    t[Key] = {}
    t[Chest][Monster] = {moveTo=TileArbiter.refuseMove}
    t[Chest][Player] = {moveTo=TileArbiter.refuseMove}
    t[Key][Monster] = {moveTo=TileArbiter.acceptMove}
    t[Key][Player] = {moveTo=TileArbiter.acceptMove}
    return t
end

I expect the test to run. It does, so far. We need to extend it.

        _:test("player can step on key and receives it", function()
            local runner = GameRunner()
            local room = Room(1,1,20,20, runner)
            local pt = Tile:room(11,10,runner)
            local player = Player(pt,runner)
            runner.player = player
            local kt = Tile:room(10,10, runner)
            local key = Key(kt,runner)
            _:expect(player.keys).is(0)
            arb = TileArbiter(key,player)
            local moveTo = arb:moveTo()
            _:expect(moveTo).is(kt)
            _:expect(player.keys).is(1)
        end)

I expect the second check for keys to fail saying zero.

18: player can step on key and receives it  -- Actual: 0, Expected: 1

Now we need an action for the Arbiter to impose on the objects. It turns out that Key understands take:

function Key:take()
    self.tile:removeContents(self)
end

So that’s nice. But Player takes the key here:

function Player:startActionWithKey(aKey)
    self.keys = self.keys + 1
    sound(asset.downloaded.A_Hero_s_Quest.Defensive_Cast_1)
    aKey:take()
end

I’m not sure if I like that or not, but I suspect I can make it work. We’ll want an action key in our entries.

Let’s imagine that this is the overall convention:

function TileArbiter:table()
    local t = {}
    t[Chest] = {}
    t[Key] = {}
    t[Chest][Monster] = {moveTo=TileArbiter.refuseMove}
    t[Chest][Player] = {moveTo=TileArbiter.refuseMove}
    t[Key][Monster] = {moveTo=TileArbiter.acceptMove}
    t[Key][Player] = {moveTo=TileArbiter.acceptMove, action=Player.startActionWithKey}
    return t
end

And in our primary method:

function TileArbiter:moveTo()
    local entry = self:tableEntry(self.resident,self.mover)
    local action = entry.action
    if action then self.mover[action](self.mover,self.resident) end
    local result = entry.moveTo(self)
    return result
end

If this works, I’ll explain it. If not, we’ll just pretend this never happened, OK?

Well, I double-called. Should be:

function TileArbiter:moveTo()
    local entry = self:tableEntry(self.resident,self.mover)
    local action = entry.action
    if action then action(self.mover,self.resident) end
    local result = entry.moveTo(self)
    return result
end

Test runs. I’ll explain. In the entry for key:player, we have the action field equal to the Player function startActionWithKey. In our moveTo function, we fetch the entry, check to see if it has an action and if so, we just call it, passing the mover and the resident. The idea will be that all actions are methods on the mover, called with the resident. We’ll see if this idea bears weight over the longer term.

But it does for now. Let’s enhance the test to check tile contents.

        _:test("player can step on key and receives it", function()
            local runner = GameRunner()
            local room = Room(1,1,20,20, runner)
            local pt = Tile:room(11,10,runner)
            local player = Player(pt,runner)
            runner.player = player
            local kt = Tile:room(10,10, runner)
            local key = Key(kt,runner)
            _:expect(player.keys).is(0)
            _:expect(kt.contents).has(key)
            arb = TileArbiter(key,player)
            local moveTo = arb:moveTo()
            _:expect(moveTo).is(kt)
            _:expect(player.keys).is(1)
            _:expect(kt.contents).hasnt(key)
        end)

We’re just checking to be sure the key is in tile kt before we move, and not after. I expect this to work. And it does.

I’m going to commit: TileArbiter in place, tests run, not in use yet.

This is a good spot to stop and sum up: the morning has run a bit long.

Summing Up

The double-dispatch for handling tile entry seems clearly inferior to this table-driven solution, even though the solution is not yet plugged in. Here’s how I know:

All the time I’ve used the double-dispatch scheme, I’ve been confused again and again because of the way the messages bounce back and forth between tiles and entities, to-ers and fro-ers, winners, and losers. And with the table, I’ve felt clear about what I wanted all the time. The only confusion I felt this morning was the irritating thing about the monster running its tweens, which was a separate issue and one that should have been fixed days ago.

There are some design issues that we could debate.

Knowing object type is questionable

With the double-dispatch approach, an object only knows the type of another object to a limited extent, namely that it has been called with that object in a parameter of known type. No one ever actually looks at the true type of anything: behavior is based on the messages sent.

In the TileArbiter solution, at least at present, we’re fetching the actual metatable of the objects in question, and we’re indexing based on that. This is deeper in the bag of tricks, and it is certainly questionable. However, we could easily fix it.

If we were to say that every entity must return an “entity key” which is unique to its class, such as the string “monster” or “player”, we could use those unique values as our indices instead of the metatable. That is in fact such a good idea, what we should do it. I’m glad we had this little talk.

All-knowing “manager” classes are questionable

Another objection to TileArbiter could be raised. In the double-dispatch scheme, there is no all-knowing class that knows everything about everyone. Individual objects work things out for themselves. Now we have this class that knows rather a lot about all the entity classes.

My own current take is that yes, this is a legitimate concern, but the proliferation of methods, and their spreading all over in the system, is a larger concern. So far, I like this approach much better, primarily because it doesn’t confuse me.

TileArbiter does some deep in the bag trickery

Yet another objection is that TileArbiter embeds a table of functions that are called in a somewhat arcane fashion, unlike the regular object:action() notation that we’re used to. This objection is also a valid one. I’d argue that the weirdness is nicely contained, and that a decent Lua programmer should understand passing functions around anyway, while avoiding doing it too often.

Bottom line …

There might be a simpler double-dispatch approach that wouldn’t confuse me so much. There might be a better programmer, who isn’t so readily confused. There might even be yet another even simpler way to do this. But of the ways I know right now, with the understanding I have right now, the TileArbiter is better.

That’s all we ask of ourselves: better.

This is at least the second time, if not third or fourth, that I’ve determined that the objects in this Dungeon weren’t helping me, and tossed the existing solution for a different and better one. I believe that if we determine that soon enough, and change soon enough, we’ll go faster with fewer defects than before. The longer we wait to recognize the problem, of course, the harder the conversion may be, and the lower the benefit. But if the code is actively being worked on, it can pay off even late on.

We’re in early days, and I think this is going to work very nicely. We’ll find out.

See you next time!

D2.zip