Dungeon 196

Two starting ideas: Quest, and removing duplication in the dungeon objects. These are not unrelated. And: as so often happens, we go in a surprising direction.

Often, people who like to object to things object to this style of development, which we could call “XP” or “Agile”, or, if we wanted to be fair, “the way Ron writes about”. One of the objections concerns the way we seem to dive right in to developing the product, with the intention to have a working (sub) version in just a few days, and to keep it working thereafter. It is argued that we should have a clear specification of the program, and a clear design, and only then should we even consider programming.

The same objection carries right along. Where is the design in this Agile stuff?

Well, the answer is that we always try to do just barely enough design for the work at had, and we keep the program in good enough shape, so that as we understand the design better, we can readily move the code toward that better design. It should be clear that if we can do that, then we can deliver visible progress, and visible value, sooner than if people have to wait until all the design is done and then all the code is done.

It is, however, a big “if”. What my articles over the past years are here to show is that it can be done. Again and again, I’ve undertaken some application–in this case, a roguelike dungeon game–and built it up with running code from almost day one. That it can be done is no proof that it should be done, but to me, the evidence is clear. Given my experience, I’d work no other way.

But we do do design. We’re about to do some now.

Quests

I just have a glimmer of an idea about quests, gleaned from something Bryan Beecham said about games in Unity. He said that there is a quest “thing”, perhaps it’s an object or class or record, I don’t know, and you can associate that thing with NPCs and objects in the dungeon, and even put dialog in the quest, so that the Jam Lady can respond to you differently based on how many Peaches you have in inventory.

I’m not going to design a Quest object per se, but I am going to think about how it might work, and then I’m going to build one. When it doesn’t work out, I’ll fix it or build another. Soon, with any luck or any skill (I’ll accept either) I’ll have an understanding of how the Quest should be built.

Mind you, I could research Unity and other game-building systems to get an understanding, and perhaps a design, for a Quest. Sometimes I do that. Sometimes I don’t. I have two reasons for that.

First, I like to solve certain kinds of problems, so I often like to address them as a beginner, without a solution handed to me. It makes what I do more fun.

Second, sometimes we just can’t find an existing solution that fits our needs. So we want to “simulate” here, what we do when there’s no canned solution out there.

Third–I lied about the “two”–canned solutions generally don’t quite fit our specialized needs, and they are often larger than we need and angled at 37 degrees to the spot we have for them.

But the main reason is the first one: I want to work on this with as few preconceptions as possible. You shouldn’t try this at home: you should do lots of research.

Or not. I’m not the boss of you, and I have absolutely no insight into what you should do. You get to figure that out for yourself.

And then …

Dungeon Objects

We have all those different kinds of dungeon objects. In addition to the Entities, the Player and the Monsters, we have Decor, Loot, Chest, and Key. Oh, and the WayDown. Oh, and Spikes. And Lever. Wow. As I reported last time, these all have these method names in common:

• init
• draw
• getTile
• query
• setTile

And they have differing but similar ways of operating in the world, including giveItem, take, and open. And probably more.

Every one of these implements getTile the same way:

function WayDown:getTile()
    return self.tile
end

Similarly for setTile:

function Decor:setTile(tile)
    self.tile = tile
end

The init and draw will be similar but not identical.

function WayDown:draw()
    pushMatrix()
    pushStyle()
    spriteMode(CENTER)
    local c = self.tile:graphicCenter()
    sprite(Sprites:sprite(asset.steps_down),c.x,c.y,64,64)
    popStyle()
    popMatrix()
end

function Loot:draw(tiny, center)
    pushStyle()
    spriteMode(CENTER)
    sprite(Sprites:sprite(self.icon),center.x,center.y+10)
    popStyle()
end

function Chest:draw(tiny, center)
    if tiny then return end
    pushMatrix()
    pushStyle()
    spriteMode(CENTER)
    sprite(Sprites:sprite(self.pic),center.x,center.y, 96,127)
    popStyle()
    popMatrix()
end

I’m sure there are other similarities that are mostly accidental, and that could be consolidated and normalized.

What we have here is an opportunity to do that thing I wrote about up top: to improve the design based on what we now know. We have duplication and near-duplication of functionality, and that means there is an opportunity to reduce it, which will reduce the size of the program, and the complexity of understanding it. It will even reduce our chances of making mistakes when we forget to put some required method like setTile into some new dungeon item.

A different language might have an “interface” concept, where we could declare that a Decor is an IDungeonItem, and therefore must implement these methods right here, and the compiler would complain until we have done it. In languages like Lua, that’s not generally how we do it.

It would be possible: we can certainly write code that checks a class to see what methods it does or does not implement. But it’s not generally done.

But even if we had that, we’d still have seven different classes all implementing the same thing the same way. We want to simplify this code, not just have a computer figure out why it might not be right.

A larger question …

We might well ask: why do these objects need to know their tile at all? We do have this “standard” where everything in the dungeon knows what tile it is on. The Entities, who can move, probably do need to know (although I’d bet there’s another way even for them). But why do the other objects even care?

We saw the WayDown asking its tile for the center, so that it could draw the stairway. But we also saw Loot and Chest being told the center around which to draw, which means that they don’t need to use their tile for that.

This is design …

When I came in here, I was thinking that I’d talk about the fact that we could choose between giving dungeon objects a concrete superclass that dealt with the tile and other common elements, or we could build a small object that did that and then compose dungeon objects, somehow, out of that plus the customized bits.

But now I’m thinking … what if these guys don’t need a tile at all? That would be better.

So let’s explore:

Do You Need That Tile?

I’ll start randomly on WayDown. It’s easy to see that WayDown doesn’t use its tile:

function WayDown:init(tile)
    tile:moveObject(self)
end

function WayDown:actionWith(aPlayer)
    if aPlayer:provideWayDownKey() then
        Bus:publish("createNewLevel")
    else
        Bus:publish("addTextToCrawl", self, {text="You must have a key to go further!"})
    end
end

function WayDown:draw()
    pushMatrix()
    pushStyle()
    spriteMode(CENTER)
    local c = self.tile:graphicCenter()
    sprite(Sprites:sprite(asset.steps_down),c.x,c.y,64,64)
    popStyle()
    popMatrix()
end

function WayDown:getTile()
    return self.tile
end

function WayDown:query()
    return "I am the Way Down! Proceed with due caution!"
end

function WayDown:setTile(tile)
    self.tile = tile
end

So the next question is, when does getTile get called, and can we ensure that it’s never called on this–and ultimately on any–dungeon object.

I’m not committed to this idea. If the tile is critical, so be it. But maybe it isn’t. Here’s a possible valid usage:

function Tile:moveObject(thing)
    local from = thing:getTile()
    if from then from:removeContents(thing) end
    self:addDeferredContents(thing)
    thing:setTile(self)
end

Here, the Tile has been told to move the incoming thing to itself, and it uses getTile to decide where that thing is now, and to adjust the contents. And it tells the thing that it has a new tile now. We know from the get and set in our dungeon objects that they don’t really care.

Drill deeper: who calls moveObject, and why? Most decor doesn’t ever move.

In cruising around looking at that question, I find this:

function Decor:init(tile, item, kind)
    self.kind = kind or Decor:randomKind()
    self.sprite = DecorSprites[self.kind]
    if not self.sprite then
        self.kind = "Skeleton2"
        self.sprite = DecorSprites[self.kind]
    end
    self.item = item
    self.tile = nil -- tile needed for TileArbiter and move interaction
    tile:moveObject(self)
    self.scaleX = ScaleX[math.random(1,2)]
    local dt = {self.doNothing, self.doNothing, self.castLethargy, self.castWeakness}
    self.danger = dt[math.random(1,#dt)]
end

Yes, TileArbiter does need to know the tiles things are on, but I think it has them. Let’s do a quick review:

We definitely create the TA without knowing the tiles involved:

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

So any use it makes of tiles, it’ll have to fetch from the resident and mover. We need to review a bit more deeply.

Too Deep?

When we’re looking to do something wild like this idea, getting rid of tile knowledge in dungeon objects, we need to know when to give up, having discovered that the change is too tricky. We are close to that point, especially since the TileArbiter concept is central to object interaction. But it’s also relatively isolated and surely used only once. Let’s see how it’s used:

function Tile:attemptedEntranceBy(enteringEntity, oldRoom)
    local ta
    local acceptedTile
    local accepted = false
    local residents = 0
    for k,residentEntity in pairs(self.contents) do
        residents = residents + 1
        ta = TileArbiter(residentEntity,enteringEntity)
        acceptedTile = ta:moveTo(self)
        accepted = accepted or acceptedTile==self
    end
    if residents == 0 or accepted then
        return self
    else
        return oldRoom
    end
end

Ha. We have two inputs, an entering entity and whatever oldRoom may be. Surely not a room. Likely a tile.

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

Right. It’s a tile. Let’s fix that right now.

function Tile:attemptedEntranceBy(enteringEntity, oldTile)
    local ta
    local acceptedTile
    local accepted = false
    local residents = 0
    for k,residentEntity in pairs(self.contents) do
        residents = residents + 1
        ta = TileArbiter(residentEntity,enteringEntity)
        acceptedTile = ta:moveTo(self)
        accepted = accepted or acceptedTile==self
    end
    if residents == 0 or accepted then
        return self
    else
        return oldTile
    end
end

Commit that.

Now it’s clear that this guy knows all the tiles. He is one of them and he’s been given the other. Therefore let’s give TileArbiter the two tiles it should care about, and cause it not to ask for tiles.

function Tile:attemptedEntranceBy(enteringEntity, oldTile)
    local ta
    local acceptedTile
    local accepted = false
    local residents = 0
    for k,residentEntity in pairs(self.contents) do
        residents = residents + 1
        ta = TileArbiter(residentEntity,self, enteringEntity, oldTile)
        acceptedTile = ta:moveTo(self)
        accepted = accepted or acceptedTile==self
    end
    if residents == 0 or accepted then
        return self
    else
        return oldTile
    end
end

This tells me that the name still isn’t good, rename again:

function Tile:attemptedEntranceBy(enteringEntity, enteringTile)
    local ta
    local acceptedTile
    local accepted = false
    local residents = 0
    for k,residentEntity in pairs(self.contents) do
        residents = residents + 1
        ta = TileArbiter(residentEntity,self, enteringEntity, enteringTile)
        acceptedTile = ta:moveTo(self)
        accepted = accepted or acceptedTile==self
    end
    if residents == 0 or accepted then
        return self
    else
        return enteringTile
    end
end

OK, now fix TA:

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

So far so good. Now change this:

function TileArbiter:acceptMove(defaultTile)
    return self.resident:getTile(defaultTile)
end

What even is that parameter? Does any implementation of getTile return or reference that?

Ah, yes. Announcer:

function Announcer:getTile(defaultTile)
    return defaultTile
end

That’s because Announcer already doesn’t know its tile. So we can safely do this:

function TileArbiter:acceptMove()
    return self.residentTile
end

This should work as it stands: we haven’t removed anyone’s tiles yet. So we can test now and build some confidence … or fix whatever problem we’ve already created.

We have tests failing out our ears. They don’t use the right creation protocol.

I really like this new feature where I only dump errors to the console:

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

Here’s the failing test:

        _:test("TileArbiter: player and monster cannot step on chest", function()
            local runner = Runner
            local room = Room(1,1,21,21, 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 = TileArbiter(chest, player)
            local moveTo = arb:moveTo()
            _:expect(moveTo).is(pt)
            local mt = Tile:room(9,10,runner)
            local monster = Monster(mt,runner)
            arb = TileArbiter(chest,monster)
            moveTo = arb:moveTo()
            _:expect(moveTo).is(mt)
        end)

We need to pass in our tiles, which are pt and ct. Clever names.

            local arb = TileArbiter(chest,ct, player,pt)
            arb = TileArbiter(chest,ct, monster,mt)

That test should run. It does. There are more of course.

Similar changes on a half-dozen more, and the tests all run. Game works. Commit: beginning to use tile parameters in TileArbiter rather than reference room contents for them.

Moving right along in TileArbiter, looking for places where we get tile:

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

That becomes, of course:

function TileArbiter:refuseMove()
    return self.moverTile
end

I think that is all. Test. Tests run. Game works. Commit: TileArbiter no longer requires entities or dungeon objects to know their tile.

But what about setting. We do set the tile on the moving entity.

This is a bit obscure:

function TileArbiter:moveTo(defaultTile)
    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, defaultTile)
    return result
end

That last operational line looks odd. You’d expect to see entry:moveTo(self.defaultTile), but there’s a dot there, not a colon. That code fetches the moveTo entry from the TileArbiter tables and executes it:

    t[Announcer] = {}
    t[Announcer][Monster] = {moveTo=TileArbiter.acceptMove}
    t[Announcer][Player] = {moveTo=TileArbiter.acceptMove, action=Player.startActionWithAnnouncer}

That’s hard to understand. I wonder if renaming the moveTo in those tables to something like moveToFunction would help.

I don’t think I’ll do that right now, I’m on a mission. Do we have all the uses of tiles in dungeon objects out now?

Can we ever be sure?

This is the “big question” when doing something like this. It’s tempting to leave the tile member in because if someone is relying on it, we’ll have a horrible problem and, let’s be honest here, we don’t trust our tests enough to be certain.

But we do have the ability to double-check all the references to the getTile and setTile and to self.tile and so on. So we’ll be careful, do the job incrementally, test often, and if we do encounter an escaping defect, we’ll treat it as an opportunity to improve the tests.

I am so tempted just to remove the tile member variable and accessors and see what happens.

One object at a time. If it goes wrong, we’ll revert.

function Loot:init(tile, kind, min, max)
    self.tile = nil
    self.kind = kind
    self.icon = self:getIcon(self.kind)
    self.min = min
    self.max = max
    self.message = "I am a valuable "..self.kind
    if tile then tile:moveObject(self) end
end

We see here that we init tile to nil then call moveObject on our input tile, which will add us to the contents of that tile … and call us back to set our tile. We don’t like that last bit.

Here’s moveObject:

function Tile:moveObject(thing)
    local from = thing:getTile()
    if from then from:removeContents(thing) end
    self:addDeferredContents(thing)
    thing:setTile(self)
end

We could fix that, but …

function GameRunner:movePlayerToRoom1()
    local tile = self.rooms[1]:centerTile()
    tile:moveObject(self.player)
end

We also have this related method:

function Tile:moveEntrant(anEntity, newTile)
    self:removeContents(anEntity)
    newTile:addDeferredContents(anEntity)
end

This is a bit intricate to untangle, and frankly I think the move to room 1 stuff is a hack and should be done more directly. Let’s do that:

function GameRunner:movePlayerToRoom1()
    local tile = self.rooms[1]:centerTile()
    tile:moveEntity(self.player)
end

function Tile:moveEntity(entity)
    local from = entity:getTile()
    if from then from:removeContents(entity) end
    self:addDeferredContents(entity)
    entity:setTile(self)
end

function Tile:moveObject(thing)
    self:addDeferredContents(thing)
end

This is what I want to have work. I expect trouble, so I test. Tests fail right and left:

8: distance from avatar -- Tile:387: attempt to index a nil value (local 'aTile')

I suspect the code is good and the tests setup isn’t. Here’s that test:

        _:test("distance from avatar", function()
            local x,y = 100,100
            local player = Player:privateFromXY(104,104, Runner)
            Runner.player = player
            local tile = Runner:getTile(vec2(x,y))
            local d = Runner:playerManhattanDistance(tile) -- manhattan
            _:expect(d).is(8)
        end)

Hm. I was tempted to chase the manhattan distance but that private creation method could be trouble.

function Player:privateFromXY(tileX,tileY,runner)
    local tile = runner:getTile(vec2(tileX,tileY))
    return Player(tile,runner)
end

function Player:init(tile, runner)
    self.runner = runner
    self:initAttributes(self:retainedAttributes())
    self:initSounds()
    self:initVerbs()
    tile:moveObject(self)
end

That should be doing moveEntity, and we should init current tile to nil. Which it probably is anyway, but let’s be careful.

The formerly broken tests run but some new ones fail:

19: TileArbiter: player can step on key and receives it -- Key:34: attempt to index a nil value (field 'tile')
3: removal tile  -- Actual: table: 0x283e67980, Expected: table: 0x283e679c0

First one is suggesting that the key doesn’t know its tile any more, and that is as intended. Let’s see that test.

        _:test("TileArbiter: player can step on key and receives it", function()
            local runner = Runner
            local room = Room(1,1,21,21, 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:keyCount()).is(0)
            _:expect(kt.contents).has(key)
            local arb = TileArbiter(key,kt, player,pt)
            local moveTo = arb:moveTo()
            _:expect(moveTo).is(kt)
            _:expect(player:keyCount()).is(1)
            _:expect(kt.contents).hasnt(key)
        end)

What’s Key:34? Ah!

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

This is probably a more general problem than just this spot. When something is picked up, it needs to know the tile from which to remove itself. This is surely sent from TileArbiter …

Well, sort of:

    t[Key][Player] = {moveTo=TileArbiter.acceptMove, action=Player.startActionWithKey}


function Player:startActionWithKey(aKey)
    self.characterSheet:addKey(aKey)
    sound(asset.downloaded.A_Hero_s_Quest.Defensive_Cast_1)
    aKey:take()
end

I think we’ll find similar issues with Loot and so on:

function Player:startActionWithLoot(aLoot)
    aLoot:actionWith(self)
end

~~~lua
function Loot:actionWith(aPlayer)
    self.tile:removeContents(self)
    if self.kind == "Pathfinder" then
        self:addPathfinderToInventory(aPlayer)
    elseif self.kind == "Antidote" then
        self:addAntidoteToInventory(aPlayer)
    else
        aPlayer:addPoints(self.kind, math.random(self.min, self.max))
    end
end

OK. We can see from this that we have a problem, which is that things like Loot and Keys are in the contents of a particular tile, and when the player interacts with them, they typically need to remove themselves from that tile’s contents.

And we can’t “just” ask the player for her tile, because some objects don’t work that way.

I think we’ll revert. My stack is too full, and I need a stable platform from which to suss this out.

Assessing the Situation

It seemed like it would be a pretty straight line to removing the knowledge of its tile from the dungeon entity. For a while, it was, until suddenly it wasn’t.

The issue seems to revolve around tile contents. Our model currently is that the tile has a collection of the things it contains. That’s useful because when we try to enter or interrogate a tile, we need to deal with those specific contents. And we have the contents knowing the tile, with a direct pointer to the tile they are “in”. So we have a two-way pointer situation, tile (contents) pointing to object and object pointing back to tile.

Now it is certainly true that the TileArbiter, which deals with these interactions, knows both tiles. That means that we can, at least in principle, provide the tile information needed by passing it around.

But I’m thinking of another way. Suppose there was a collection of all the dungeon objects, mapping object to tile. When we want to move an object from “wherever it is” to a new tile, we can have that collection do it. When we want to remove an object from the dungeon, we “just” remove it from that collection. (And from the collection that the tile has …

Unless we changed the Tile to use the master collection to determine its contents! We could do that the hard way, by searching the collection for all the objects pointing to our tile. If the hard way was too slow, we could provide an auxiliary structure in the collection to go the other way.

But this is a Big Deal!

Yes. This would perhaps be a substantial change. It certainly seems like it offhand. On the other hand, there are really only a very few questions we have to deal with, which is to give an object the tile it is on, and to move the object to a new tile, including possibly no tile at all.

How big would this collection be? How many objects are there in the dungeon? Reading createLevel tells me there are fewer than 70 non-entity objects in the Dungeon. There are, in addition, one Player and a half-dozen or ten monsters.

So the collection is small, and it would be accessed by the id of the object anyway.

Technical Debt?

Ward Cunningham gave us the notion of “Technical Debt”, the true meaning of which is, roughly, “the difference between the design we have, and the design we now realize we want”. It’s not crufty code: it’s a function of improved understanding as time goes on.

When we remove technical debt, Ward tells us, the system tends to contract. It becomes smaller and simpler, as the not-so-good old ways are folded into the better new way.

We may have exactly that situation here.

Currently we have tiles that know their contents via a collection, and objects that know their tile via pointer. Both moving and static objects do that. The collection for a tile’s contents is unique to that tile: every one of our over 5000 tiles has one.

We also have an odd thing that happens in tile contents, because we are sometimes looping over that collection when something needs to be added to it. So we have the slightly tricky DeferredTable that buffers changes until things are safe. If we used this master table for that purpose, the table being iterated would likely be created on the fly, and have no need for protection. So we could remove this special collection type that we use in just one place.

I would predict that the new tile contents collection thingie will be not much larger than the DeferredCollection is now, and that it will be fast enough even if we compute a given tile’s contents dynamically. (Which, I think, we must do if object addition is to work. We’ll see.)

Let’s Spike This!

What I really mean is, let’s try this, and if it works, keep it, and if it doesn’t, trash it. I am hopeful, and hold the intention, that I’ll use the code we’re about to create in production. With a true “Spike”, we plan to delete the experimental code and do it over, better.

We’ll decide that when the time comes.

We’ll TDD this thing, so let’s think about its protocol: we want to build something that’s pretty close to what we’ll actually use.

It seems to me that we can have a method on everyone in the dungeon telling them moveTo(aTile). The contents collection object will deal automatically with removing them from the other place, if they have one.

What about removal? Well, moveTo(nil) might just do that, or we can have a removeFromUniverse() function if we prefer. There’s some danger there.

How will moveTo(aTile) work? Easy to decide:

function Someone:moveTo(aTile)
    aTile:receive(self)

It’ll be a matter between the tile and its own conscience how this is all done: the new collection can be private to the tile class.

We now have similar methods on tile, notably addDeferredContents.

Solidifying Our Thinking

Enough speculation. Let’s get concrete.

I think I’ll call this thing DungeonContents for now, and I’m gonna TDD it.

        _:test("DungonContents remembers object", function()
            local dc = DungeonContents()
            local object = {1,2,3}
            local tile = 34
            dc:moveObjectToTile(object,tile)
            _:expect(dc:getTile(object)).is(34)
        end)

The “DC”, as I’m now calling it, doesn’t care what it is storing or what its tile is. It does require that the object be unique, that is, if it isn’t, you may not get what you want.

This should be “easy” to make work.

1: DungonContents remembers object -- DungeonContents:20: attempt to call a nil value (method 'moveObjectToTile')

I’m not going to do little tiny steps TDD here: I’m going to do reasonable sized steps, and scale down if I get in over my head. So I’ll do the creation and the method now:

DungeonContents = class()

function DungeonContents:init()
    self.contentMap = {}
end

function DungeonContents:getTile(object)
    return self.contentMap[object]
end

function DungeonContents:moveObjectToTile(object,tile)
    self.contentMap[object] = tile
end

I kind of hope this’ll work. And it does. Now what? This is obviously complete. Let’s do this now

        _:test("DungeonContents get tile contents", function()
            local dc = DungeonContents()
            local object = {1,2,3}
            local tile = 34
            dc:moveObjectToTile(object,tile)
            local obj2 = { 2,3, 4}
            dc:moveObjectToTile(obj2, 45)
            dc:moveObjectToTile({3,4,5}, 34)
            local cont34 = dc:getTileContents(34)
            _:expect(cont34, "object").has(object)
            _:expect(cont34,"3,4,5").has({3,4,5})
            _:expect(cont34,"no obj2").hasnt(obj2)
        end)

You’ll note that as I coded this test, I wrote more and more compact code. That’s because I wanted to get a feeling for how those methods might be used in the wild.

Let’s make that work:

function DungeonContents:getTileContents(desiredTile)
    local result = {}
    for object,tile in pairs(self.contentMap) do
        if tile == desiredTile then
            table.insert(result,object)
        end
    end
    return result
end

Curiously, this does not work:

2: DungeonContents get tile contents 3,4,5 -- Actual: table: 0x283b58ac0, Expected: table: 0x283b58e80

Ah, my bad. Tables are unique. I have to cache it.

        _:test("DungeonContents get tile contents", function()
            local dc = DungeonContents()
            local object = {1,2,3}
            local tile = 34
            dc:moveObjectToTile(object,tile)
            local obj2 = { 2,3, 4}
            dc:moveObjectToTile(obj2, 45)
            local obj3 = {3,4,5}
            dc:moveObjectToTile(obj3, 34)
            local cont34 = dc:getTileContents(34)
            _:expect(cont34, "object").has(object)
            _:expect(cont34,"3,4,5").has(obj3)
            _:expect(cont34,"no obj2").hasnt(obj2)
        end)

Tests run. I think this thing is essentially complete.

Commit: unused DungeonContents object and tests.

Shall we go for it?

It’s 1245. I’ve been beavering away since around before 0900, so it is past my usual closing time. But I’d like to at least see how this thing might be used.

This next bit is a spike. I commit to throwing it away unless I change my mind.

I expect most use of this collection to go through Tile, though I confess I’m not dead certain it’ll wind up there. But certainly for now, Tiles have that addDeferredContents method that we know must be used everywhere.

So Tile has to be able to find the DC. Who owns it, and who creates it?

It could live in Runner, or it could be global. I think I want it global, at least for now. So we’ll init it in Main, probably, and in GameRunner when we create new levels. And we’ll probably want to do something about saving and restoring in the tests. (We might want to remove the duplication from that code.)

So:

I immediately hate the class name. I want to call the global DungeonContents. Rename the class DungeonContentsCollection. My first attempt at that uses Codea’s replace. It crashes Codea.

Second attempt, pass it to Sublime. Done.

Now then:

function GameRunner:dcPrepare()
    TileLock = false
    DungeonContents = DungeonContentsCollection()
    self:createNewDungeon()
    local dungeon = self.dungeon
    self.tiles = dungeon:createTiles(self.tileCountX, self.tileCountY)
    dungeon:clearLevel()
    Announcer:clearCache()
end

That’s the common method called by all dungeon creation methods. So it should be just right.

Let’s do Tile:

function Tile:initDetails()
    self.contents = DeferredTable()
    self.seen = false
    self:clearVisible()
    self.sprite = nil
    self.door = false
end

This wants not to init contents.

function Tile:initDetails()
    self.seen = false
    self:clearVisible()
    self.sprite = nil
    self.door = false
end

Now all references to self.contents need to call back to the DC.

function Tile:addDeferredContents(anEntity)
    self.contents:put(anEntity)
end

That can become:

function Tile:addDeferredContents(anEntity)
    DungeonContents:moveObjectToTile(anEntity, self)
end

And then …

function Tile:getContents()
    self.contents:update()
    return self.contents
end

That turns into:

I ran into this method and wrote it thus:

function Tile:removeContents(anEntity)
    self:getContents():remove(anEntity)
end

We need that method now:

function DungeonContentsCollection:remove(object)
    self.contentMap[object] = nil
end

I’d best test that.

        _:test("DC can remove", function()
            local dc = DungeonContentsCollection()
            local object = {1,2,3}
            local tile = 34
            dc:moveObjectToTile(object,tile)
            local obj2 = { 2,3, 4}
            dc:moveObjectToTile(obj2, 45)
            local obj3 = {3,4,5}
            dc:moveObjectToTile(obj3, 34)
            local cont34 = dc:getTileContents(34)
            _:expect(cont34, "object").has(object)
            _:expect(cont34,"3,4,5").has(obj3)
            _:expect(cont34,"no obj2").hasnt(obj2)
            dc:remove(object)
            cont34 = dc:getTileContents(34)
            _:expect(cont34).hasnt(object)
        end)

That will work but of course right now I have masses of tests broken. Getting close to calling this a spike and backing out.

A lot of the tests failures are because I don’t have a DungeonContents for the tests to use. Fix that in their before.

Ticking through the tests, I find various typos, and lots of people saying tile.contents who should be saying tile:getContents(). I’ve just found nearly the last problem:

function Tile:contains(thing)
    return self:getCcontents():contains(thing)
end

I think the conventional spelling of “contents” would be preferable here:

function Tile:contains(thing)
    return self:getContents():contains(thing)
end

However, there’s another issue here, which is that tables don’t understand contains.

I’ll write that out longhand, but I think maybe I should extent my TableOperations tab for things like this. This should do the job:

function Tile:contains(thing)
    for i,t in ipairs(self:getContents()) do
        if t == thing then return true end
    end
    return false
end

I have one test failing and the game works. I do find one little problem: when I step onto a Chest, it opens as intended, but the Health item that is always inside does not draw on top of the Chest. This used to work because the Chest was first in the tile’s collection, and now, since DungeonContents is hashed, the order of the tile’s contents collection is undefined.

If you step onto the tile, you’ll get the health: it just doesn’t show up on the screen, being covered up by the Chest.

Everything else has worked so far.

Even the WayDown works. This is too good to revert. I’m going to commit this to head and call it a morning, now that it is 1340. We do have a known defect with the Health not showing up. I have a rough plan for the fix, which will be to give contents objects a z level value and sort them on z before drawing.

For now, my user can deal with the defect. We’ll see if they even find it.

We can remove the DeferredTable, I think. Yes, there are no references to it. I’m going to ignore the broken test for now. It’s the same kind of thing, we don’t know where the object is in the table.

        _:test("Room:announce places in ring", function()
            local r = Room(3,3,5,10, Runner)
            local msgs = {"Message"}
            local dungeon = Runner:getDungeon()
            r:announce(dungeon, msgs)
            checkRing(dungeon, 3,3, 7, 12, msgs)
        end)

function checkRing(dungeon, x1,y1, x2,y2, msgTable)
    local ann
    local msgs
    local t
    for x = x1,x2 do
        t = dungeon:privateGetTileXY(x,y1)
        ann = t:privateGetFirstContents()
        msgs = ann.messageTable
        _:expect(msgs,"x,y1 "..x..","..y1).is(msgTable)
        t = dungeon:privateGetTileXY(x,y2)
        ann = t:privateGetFirstContents()
        msgs = ann.messageTable
        _:expect(msgs,"x,y2 "..x..","..y2).is(msgTable)
    end
    for y = y1,y2 do
        t = dungeon:privateGetTileXY(x1,y)
        ann = t:privateGetFirstContents()
        msgs = ann.messageTable
        _:expect(msgs,"x1,y "..x1..","..y).is(msgTable)
        t = dungeon:privateGetTileXY(x2,y)
        ann = t:privateGetFirstContents()
        msgs = ann.messageTable
        _:expect(msgs,"x2,y "..x2..","..y).is(msgTable)
    end
end

That’s weird. I can see how I might fix this but for this morning, I’ll ignore. I’m due some fodder.

Commit: System uses new DungeonContents. One test ignored. Chest contents need to draw on top of chest unconditionally.

Let’s sum up.

Summary

Well, nothing on Quest, but that’s OK, we never really tried. And we found a really interesting way of removing the duplication of dungeon objects having a tile reference: we made the reference unnecessary, by creating a single collection mapping all objects to their tile.

We have not yet removed all the setTile and getTile calls, and we may find that the methods will stay but use the DungeonContents collection rather than a member variable. And we’ve already removed 5000 DeferredTable instances from the system.

Sweet.

The lessons? I’m taking two home. First, no surprise, think about design often, especially when something is getting in the way, like the Tiles knowing contents and the contents knowing tiles.

Second, don’t necessarily trust your first or second ideas to be the best. We got a ways into removing tile by just not having it and found that there were operations that really needed to know the tile.

Third–as so often happens, I lied about the “two”–when you bump into a wall, step back and look around. There may be a door nearby, or a place where you can put a door. In our case, we could put a collection of all room contents items in one place, and let everyone use it.

A good morning, now that it is 1400 hours, and we’ll work more on this tomorrow. Note, however, that aside from the little health glitch, which we do need to fix, we could pause in our refactoring right now and do other things. That’s the way aha aha we like it.

See you next time!

D2.zip