Dungeon 197
Today, more refactoring to remove connections between dungeon objects and their tiles. And, as always, some thinking.
Let’s begin with some thinking. We now have a few top-level objects in global variables, including:
- DungeonContents
- Our newly-created collection mapping in-dungeon objects to the tile they presently occupy.
- OperatingMode
- Player vs Developer mode of operation. Controls display of the full map, and intended for additional features to help the makers do the making. (Of the made.)
- GA
- GlobalAnalyzer, a simple tool that tracks use of globals. Not really part of the app, part of making.
- TileLock
- Master flag to lock down tiles from being modified. Possibly belt and suspenders, but inexpensive and a bit of security.
- Bus
- Public name of the EventBus used by objects to publish information and others to subscribe.
- Runner
- Current instance of the GameRunner, central operating object that, well, runs the game.
Fun Fact: I almost started shaving a yak to find who is defining globals r, x, and y in CodeaUnit or my tests, most likely my tests. But that’s not really interesting.
Because of how Codea works, we really need at least one top-level global name to refer to. As I’ve done here, the easiest thing is for that global to contain an instance of some important class, like GameRunner. (Which, you may recall, runs the game.) We could eliminate instance-bearing globals by having important classes implement the method instance(), which would return the current instance. One often does this with the Singleton pattern, for example.
Right now, with only a half-dozen globals, I think we’ll let them be, but there is probably something better available to us here. It just seems like a very small fish to fry just now.
Let’s get back to removing the tile variable from all our in-dungeon objects. We’ll begin with the non-Entity ones, but I think we might do them as well, in the fullness of time.
We have the tile member variable, and the getTile and setTile methods to see about.
We also have one ignored test. Let’s begin there.
_:ignore("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
This very messy test is just checking those ranges to be sure that each cell has an announcer in it with the right messages. We can no longer rely on privateGetFirstContents to deliver the announcer, because tile contents aren’t stored in an array any more. (This has caused us another issue with stacking of multiple objects in a tile, you may recall. I’m prepared to accept that problem.)
We can fix this test, I reckon, by finding the Announcer in the contents.
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 = findAnnouncerIn(t)
msgs = ann.messageTable
_:expect(msgs,"x,y1 "..x..","..y1).is(msgTable)
t = dungeon:privateGetTileXY(x,y2)
ann = findAnnouncerIn(t)
msgs = ann.messageTable
_:expect(msgs,"x,y2 "..x..","..y2).is(msgTable)
end
for y = y1,y2 do
t = dungeon:privateGetTileXY(x1,y)
ann = findAnnouncerIn(t)
msgs = ann.messageTable
_:expect(msgs,"x1,y "..x1..","..y).is(msgTable)
t = dungeon:privateGetTileXY(x2,y)
ann = findAnnouncerIn(t)
msgs = ann.messageTable
_:expect(msgs,"x2,y "..x2..","..y).is(msgTable)
end
end
function findAnnouncerIn(tile)
local cont = tile:getContents()
for i,o in ipairs(cont) do
if o.isa(Announcer) then return o end
end
return nil
end
This will search contents to find an Announcer and return it, returning nil if it isn’t found.
And it’s returning nil. It appears that, whatever else may be working, the Announcers are not going where they should. I double check that by running the Learn level, which is supposed to have an announcer ring all around the second room.
I triple-check by walking south from the startup location in a regular dungeon, expecting to trip over the Announcers that are there. And nothing.
This is somewhat distressing. My sails are luffing. But, we have a test and I think we can trust it.
The test calls announce on the room:
_: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)
The level initialization in a regular level is different:
function GameRunner:placeLever()
local r1 = self.rooms[1]
local tile = r1:centerTile()
local leverTile = tile:getNeighbor(vec2(0,2))
local lever = Lever(leverTile, "Lever 1")
local annTile = tile:getNeighbor(vec2(0,-2))
local ann = Announcer({"This is the mysterious level called", "Soft Wind Over Beijing,",
"where nothing bad will happen to you", "unless you happen to be a princess.", "Welcome!" })
annTile:addDeferredContents(ann)
annTile = tile:getNeighbor(vec2(1,-2))
annTile:addDeferredContents(ann)
annTile = tile:getNeighbor(vec2(-1,-2))
annTile:addDeferredContents(ann)
end
Well, addDeferredContents. What does that do?
function Tile:addDeferredContents(anEntity)
DungeonContents:moveObjectToTile(anEntity, self)
end
Oh my, I think I can guess the issue …
function DungeonContentsCollection:moveObjectToTile(object,tile)
self.contentMap[object] = tile
end
A given object can only be in a single tile. That’s generally exactly what we want, but we coded announcers to be able to be in many cells. This means I should be able to find the Announcer in one of the three cells it’s placed in. And I can. And in the learning level, if I walk around the periphery, I finally trigger the room message.
Fascinating! The new tile-tracking scheme works for everything, so long as a given object can only be in one place at a time. And that is the case, except for my fancy trick with the Announcer.
It would seem to be as simple as making as many instances as we need, except for this:
function Announcer:messages()
if self.sayMessages then
self.sayMessages = false
return self.messageTable
else
return {}
end
end
After an announcer has spoken its message, it sets its sayMessages member to false. When you step on another (but it’s really the same) announcer, its flag is false, so you only see the message once, no matter how many instances you step on.
So if we make multiple copies, they will need to coordinate somehow, all turning their flag off when any one of their siblings speaks.
We need to think. Or at least I do.
Announcer Options
What are some options for what we could do?
- Publish-Subscribe
- We could create multiple instances, and when any given announcer triggers, it publishes a message on the Bus, telling its siblings to turn off their own flags. Since it already publishes its message, that might suffice as the trigger. But what about announcers that have the same message in different areas of the dungeon? We don’t want them to turn off also.
- Proxy
- We could create a single actual Announcer for any given use, and then plunk down proxy announcers at the trigger points. Each of those, when stepped on, would send a message to the actual one, which would work as usual.
- Allow Dups in DungeonContents
- We could reimplement the DungeonContentsCollection to allow for duplicates. We’d need a new method or two, because generally, the model is that an object can only be in one tile, and that’s enforced by the implementation:
object->tile. On the other hand, since objects don’t move, we generally don’t require that enforcement. Unless we decide to put moving Entities in the same collection, and I do have that in mind. - Special Collection for Multiples
- We have the
addDeferredContents, which needs renaming anyway, and which has the property of (effectively) removing the object from any tile it’s already on, and putting it in the new tile. We could have another tile method for adding objects to multiple tiles, and it could use an entirely different collection for such objects.
Of these, I like two, and let’s see if I can explain why.
I don’t like the Publish-Subscribe notion because it feels kind of heavy to me, and we’ll have to give each announcer family a unique identifier. (See Proxy).
I like Proxy. It’s pretty light weight, and automatically provides the “unique identifier”, and then uses it.
I don’t like Allow Dups. It makes the normal mode, being on one and only one tile, a special case instead of the default.
I somewhat like the Special Collection, because it preserves the nice design of the current scheme, and provides a special way to be special.
Of them all, I like Proxy best. We can build one Announcer, then in the loops that lay them down, make a proxy each time. (The test will have to do this, as well as the code that rings the room.) This scheme keeps the oddity where it belongs, close to the Announcer.
Let’s try Proxy.
Proxy
I am so tempted to write a general purpose ProxyDungeonObject that can stand in for any object and forward all messages to and from it. I have no need for such an object, so I will resist the temptation.
We’ll do a ProxyAnnouncer. And we’ll start by changing our test to work, then break it, then make it work again.
I want to make it work, because I changed the checkRing function and I need to be sure that the test actually works.
The test calls Room:announce:
function Room:announce(dungeon, messageTable)
local tile
local ann = Announcer(messageTable)
for x = self.x1, self.x2 do
tile = dungeon:getTile(vec2(x,self.y1))
tile:addDeferredContents(ann)
tile = dungeon:getTile(vec2(x,self.y2))
tile:addDeferredContents(ann)
end
for y = self.y1, self.y2 do
tile = dungeon:getTile(vec2(self.x1,y))
tile:addDeferredContents(ann)
tile = dungeon:getTile(vec2(self.x2,y))
tile:addDeferredContents(ann)
end
end
We should be able to make the test work by creating a new announcer at each add:
function Room:announce(dungeon, messageTable)
local tile
--local ann = Announcer(messageTable)
for x = self.x1, self.x2 do
tile = dungeon:getTile(vec2(x,self.y1))
tile:addDeferredContents(Announcer(messageTable))
tile = dungeon:getTile(vec2(x,self.y2))
tile:addDeferredContents(Announcer(messageTable))
end
for y = self.y1, self.y2 do
tile = dungeon:getTile(vec2(self.x1,y))
tile:addDeferredContents(Announcer(messageTable))
tile = dungeon:getTile(vec2(self.x2,y))
tile:addDeferredContents(Announcer(messageTable))
end
end
I expect, or at least hope, that this test will now run. It doesn’t, because the method isn’t isa, it’s is_a.
Also, it needs to be called with :, not . How many mistakes can you make in five characters?
Test runs. Now let’s change it to use AnnouncerProxy, including expecting them in checkRing.
function findAnnouncerIn(tile)
local cont = tile:getContents()
for i,o in ipairs(cont) do
if o:is_a(AnnouncerProxy) then return o end
end
return nil
end
Test fails nicely. Change it to use the so far non-existent proxy.
function Room:announce(dungeon, messageTable)
local tile
local ann = Announcer(messageTable)
for x = self.x1, self.x2 do
tile = dungeon:getTile(vec2(x,self.y1))
tile:addDeferredContents(AnnouncerProxy(ann))
tile = dungeon:getTile(vec2(x,self.y2))
tile:addDeferredContents(AnnouncerProxy(ann))
end
for y = self.y1, self.y2 do
tile = dungeon:getTile(vec2(self.x1,y))
tile:addDeferredContents(AnnouncerProxy(ann))
tile = dungeon:getTile(vec2(self.x2,y))
tile:addDeferredContents(AnnouncerProxy(ann))
end
end
Time for the class:
AnnouncerProxy = class()
function AnnouncerProxy:init(announcer)
self.announcer = announcer
end
function AnnouncerProxy:draw(ignored, center)
end
function AnnouncerProxy:trigger()
return self.announcer:trigger()
end
function AnnouncerProxy:privateMessageTable()
return self.announcer.messageTable
end
function AnnouncerProxy:repeatMessages()
return self.announcer:repeatMessages()
end
I’m concerned about the repeat. We’ll see how that goes but I think it’ll be trouble. Let’s see how the tests feel. Test feels bad, nothing is going into the tiles:
7: Room:announce places in ring x,y1 3,3 -- Actual: nil, Expected: table: 0x2810728c0
and many more like this ...
The test is flawed, refers to ann.messageTable, which is now a method.
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 = findAnnouncerIn(t)
msgs = ann:privateMessageTable()
_:expect(msgs,"x,y1 "..x..","..y1).is(msgTable)
t = dungeon:privateGetTileXY(x,y2)
ann = findAnnouncerIn(t)
msgs = ann:privateMessageTable()
_:expect(msgs,"x,y2 "..x..","..y2).is(msgTable)
end
for y = y1,y2 do
t = dungeon:privateGetTileXY(x1,y)
ann = findAnnouncerIn(t)
msgs = ann:privateMessageTable()
_:expect(msgs,"x1,y "..x1..","..y).is(msgTable)
t = dungeon:privateGetTileXY(x2,y)
ann = findAnnouncerIn(t)
msgs = ann:privateMessageTable()
_:expect(msgs,"x2,y "..x2..","..y).is(msgTable)
end
end
Test runs. Now to try it live. I expect some trouble.
I didn’t expect this but I knew immediately what it was: I can’t step onto an AnnouncerProxy. Fix TileArbiter:
t[AnnouncerProxy] = {}
t[AnnouncerProxy][Monster] = {moveTo=TileArbiter.acceptMove}
t[AnnouncerProxy][Player] = {moveTo=TileArbiter.acceptMove, action=Player.startActionWithAnnouncer}
This all works. I am slightly surprised that the History button only displays one message. I think I’ll refresh my memory on how that works.
function Player:history()
Announcer:playCache()
end
Super. We don’t re-trigger the guys. I do think there’s another issue but I don’t see how we can ever trigger it. Note this code:
function Room:announce(dungeon, messageTable)
local tile
local ann = Announcer(messageTable)
for x = self.x1, self.x2 do
tile = dungeon:getTile(vec2(x,self.y1))
tile:addDeferredContents(AnnouncerProxy(ann))
tile = dungeon:getTile(vec2(x,self.y2))
tile:addDeferredContents(AnnouncerProxy(ann))
end
for y = self.y1, self.y2 do
tile = dungeon:getTile(vec2(self.x1,y))
tile:addDeferredContents(AnnouncerProxy(ann))
tile = dungeon:getTile(vec2(self.x2,y))
tile:addDeferredContents(AnnouncerProxy(ann))
end
end
This code will put two proxies in each corner. We should make one of the loops not go all the way. Either one will do.
Shall we test this issue? I say yes.
_: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)
local cont = dungeon:privateGetTileXY(3,3):getContents()
_:expect(#cont).is(1)
checkRing(dungeon, 3,3, 7, 12, msgs)
end)
We haven’t placed anything, so these tiles should only have one entry. The corner, as shown, has two. Fix:
function Room:announce(dungeon, messageTable)
local tile
local ann = Announcer(messageTable)
for x = self.x1+1, self.x2-1 do
tile = dungeon:getTile(vec2(x,self.y1))
tile:addDeferredContents(AnnouncerProxy(ann))
tile = dungeon:getTile(vec2(x,self.y2))
tile:addDeferredContents(AnnouncerProxy(ann))
end
for y = self.y1, self.y2 do
tile = dungeon:getTile(vec2(self.x1,y))
tile:addDeferredContents(AnnouncerProxy(ann))
tile = dungeon:getTile(vec2(self.x2,y))
tile:addDeferredContents(AnnouncerProxy(ann))
end
end
Test runs. Extend test to other three corners.
_: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)
local cont = dungeon:privateGetTileXY(3,3):getContents()
_:expect(#cont).is(1)
local cont = dungeon:privateGetTileXY(3,7):getContents()
_:expect(#cont).is(1)
local cont = dungeon:privateGetTileXY(7,3):getContents()
_:expect(#cont).is(1)
local cont = dungeon:privateGetTileXY(7,12):getContents()
_:expect(#cont).is(1)
checkRing(dungeon, 3,3, 7, 12, msgs)
end)
I think we are solid. Commit: fix issues with Announcer. Introduce AnnouncerProxy.
Retro, Reset
Well. That took longer than I expected. The details of the Announcer, which were unique to anything in the Dungeon, had more impact than I though. I had figured that we could just quickly adjust the test and move on.
Which reminds me, that new method in the test isn’t really needed. Let’s address that:
function findAnnouncerIn(tile)
return tile:getContents()[1]
end
No need to search: there can be only one. Commit: simplify announcer test.
After more of a digression from my plan than I had expected, it makes sense to take a little break before diving back in. I’ll see if the cat needs lunch.
Guess what: she did.
OK, what’s the plan again? First let’s rename addDeferredContents to practically anything else. How about addToContents?
Here’s one we really want to change:
function Tile:moveObject(thing)
local from = thing:getTile()
if from then from:removeContents(thing) end
self:addToContents(thing)
thing:setTile(self)
end
We don’t expect ever to have to removeContents: things can only be in one place. So:
function Tile:moveObject(thing)
self:addToContents(thing)
thing:setTile(self)
end
I don’t remove the setTile yet: I’ve not inspected the calls and implementations of it.
Similarly here:
function Tile:moveEntrant(anEntity, newTile)
self:removeContents(anEntity)
newTile:addToContents(anEntity)
end
We can remove the remove. We do still want the base removeContents method, so that we can remove things entirely from the Dungeon as needed, such as when we take them. All seems well after this change. Let’s see about those setTile and getTile calls.
I’ve gone too far too fast. Something is broken. Revert and slow down.
We have an issue with those two move methods:
function Tile:moveObject(thing)
local from = thing:getTile()
if from then from:removeContents(thing) end
self:addDeferredContents(thing)
thing:setTile(self)
end
function Tile:moveEntrant(anEntity, newTile)
self:removeContents(anEntity)
newTile:addDeferredContents(anEntity)
end
In general, I think things don’t need setTile. But the Player does:
function Player:setTile(tile)
self.tile = tile
self.tile:illuminate()
end
I think we’ll find that we access self.tile a lot as well. Let’s look first for those.
function Entity:getTile()
return self.tile
end
We can probably use the DC for this. Let’s try:
function Entity:getTile()
return DungeonContents:getTile(self)
end
Test. We’re moving slowly, the waters here are a bit treacherous. Good so far. Look for self.tile in Player and Monster.
function Entity:distance(aTile)
return aTile:distance(self:getTile())
end
function Entity:manhattanDistanceFromPlayer()
return self.runner:playerManhattanDistance(self:getTile())
end
function Entity:remove()
self:getTile():removeContents(self)
end
And many more replacements in Monster and Player.
Everything seems to be working. I’m no longer storing a value into self.tile in either Monster or player.
However, I do have this:
function Player:setTile(tile)
tile:illuminate()
end
This ensures that the dungeon is correctly illuminated. This could probably be done from somewhere else, obviating the need for the setTile. Now is not the time for that.
Player init still references tile:
function Player:init(tile, runner)
self.runner = runner
self:initAttributes(self:retainedAttributes())
self:initSounds()
self:initVerbs()
tile:moveObject(self)
end
But we never save it in Player or Monster.
I’ve done a bunch of refactoring to remove accesses to the tile members, and feeling the need for a commit before I go much further. The tests and game run. Commit: refactoring to remove refs to self.tile.
Now let’s look at Loot. Standard changes.
I’m having to implement an empty setTile on all these objects. For now I’m allowing this with my eye on not actually saving it. But we do need something better than that and the multiple implementations of getTile.
Commit: Loot does not save tile.
Let’s do Key:
function Key:setTile(tile)
--self.tile = tile
end
function Key:take()
self:getTile():removeContents(self)
end
Works fine. Commit: Key does not save tile.
Next is Chest class.
function Chest:open()
if self:isOpen() then return end
self.opened = true
self.pic = self.openPic
Loot(self:getTile(), "Health", 4,10)
--Health(self.tile, self.runner)
end
And the usual clearing of the setTile. Standard but the tedious character of this makes me think that we need to find a better way. Concrete superclass comes to mind. 😜
And now Spikes.
function Spikes:draw()
pushMatrix()
pushStyle()
spriteMode(CENTER)
local c = self:getTile():graphicCenter()
sprite(self:sprite(),c.x,c.y,64,64)
popStyle()
popMatrix()
end
function Spikes:getTile()
return DungeonContents:getTile(self)
end
function Spikes:toggleUpDown()
self.state = ({up="down", down="up"})[self.state]
if self.stayDown then self.state = "down" end
if self.state == "up" then
local player = self:getTile():getPlayerIfPresent()
if player then self:actionWith(player) end
end
self.delay(2, self.toggleUpDown, self)
end
This breaks a test:
1: Spikes wrong tile -- Actual: nil, Expected: table: 0x2811537c0
I could just revert but I want to see what’s up. I’m glad I looked:
local tile = FakeTile
local spikes = Spikes(tile, fakeTweenDelay)
_:expect(spikes:getTile(),"wrong tile").is(tile)
The FakeTile hasn’t been dealt with. We need to change its moveObject method.
This is somewhat irritating, but often when we use fakes, we have to edit them to match the real objects. So we shouldn’t be deeply surprised.
In this case, I decide just to remove that expectation: I know that if I create a Spikes, it sets its tile properly.
Tests run, commit: Spikes do not save tile.
Now Decor, same drill.
In testing, I notice something odd: If there’s a Mimic, at least I think it’s a Mimic, positioned where the inventory is, it overwrites the inventory. Make a reminder sticky (I haven’t gone to cards yet.)
Now Lever. Same again. Oops, forgot to commit Decor. Commit: Decor and Lever do not save tile.
There’s also a test object called FakeThing.
This is ludicrous. As far as I can see, this tests no part of the real system. Was I just using this as a spike to understand how to move things?
-- TestTileMover
function testTileMover()
CodeaUnit.detailed = false
_:describe("Moving Things Between Tiles", function()
_:test("both exist", function()
local from = FakeTile2()
local to = FakeTile2()
local thing = FakeThing(from)
to:moveObject(thing)
_:expect(thing:getTile()).is(to)
_:expect(from.contents).is(nil)
_:expect(to.contents).is(thing)
end)
_:test("no from", function()
local from = nil
local to = FakeTile2()
local thing = FakeThing(from)
to:moveObject(thing)
_:expect(thing:getTile()).is(to)
_:expect(to.contents).is(thing)
end)
_:test("removal tile", function()
local from = FakeTile2()
local to = RemovalTile()
local thing = FakeThing(from)
to:moveObject(thing)
_:expect(thing:getTile()).is(to)
_:expect(from.contents).is(nil)
_:expect(to.contents).is(thing)
end)
end)
end
FakeTile2 = class()
function FakeTile2:init()
end
function FakeTile2:addDeferredContents(thing)
self.contents = thing
end
function FakeTile2:moveObject(thing)
local from = thing:getTile()
if from then from:removeContents(thing) end
self:addDeferredContents(thing)
thing:setTile(self)
end
function FakeTile2:removeContents(thing)
self.contents = nil
end
FakeThing = class()
function FakeThing:init(tile)
self.tile = tile
end
function FakeThing:getTile()
return self.tile
end
function FakeThing:setTile(tile)
self.tile = tile
end
I’m removing this tab altogether. Commit: Remove useless testTileMover.
This Is Taking a While
Yes, it is. But each change has been quite small and rarely surprising. We could have done this work over a period of days, weeks, or months. We did have an issue with Announcer, and I think it shows up a deficiency in the tests, but I’m sure we have it working as intended. It’d be better to have better tests.
Now I have all the implementations of setTile doing nothing, except for Player:
function Player:setTile(tile)
tile:illuminate()
end
I’d like to remove them all. To do that we’ll need to deal with the senders of setTile. There are only these:
function Tile:moveObject(thing)
local from = thing:getTile()
if from then from:removeContents(thing) end
self:addDeferredContents(thing)
thing:setTile(self)
end
But wait! Didn’t I rename that method to addContentsTo? What happened? I guess that went away in my revert. Let's do the rename first. Commit: rename addDeferredContents to addContentsTo`.
Ahem. OK now senders of setTile:
function Tile:moveObject(thing)
local from = thing:getTile()
if from then from:removeContents(thing) end
self:addContentsTo(thing)
thing:setTile(self)
end
This shouldn’t need those first two lines. Remove, test.
Commit: remove unneeded code from Tile:moveObject.
As I was saying, senders of setTile:
function Tile:moveObject(thing)
self:addContentsTo(thing)
thing:setTile(self)
end
I notice this, right below that:
function Tile:moveEntrant(anEntity, newTile)
self:removeContents(anEntity)
newTile:addContentsTo(anEntity)
end
The remove is surely not needed. Test, commit: remove unneeded code from Tile:moveEntrant.
Now as I was saying, senders of setTile:
function Tile:moveObject(thing)
self:addContentsTo(thing)
thing:setTile(self)
end
function FakeTile:moveObject(object)
self:addContentsTo(object)
object:setTile(self)
end
Those are the only two. Can I remove the one from FakeTile without breaking a test?
Yes. Consider it done.
Now senders of moveObject, who are the only ones who can trigger the setTile.
function GameRunner:movePlayerToRoom1()
local tile = self.rooms[1]:centerTile()
tile:moveObject(self.player)
end
function WayDown:init(tile)
tile:moveObject(self)
end
function Monster:init(tile, runner, mtEntry)
if not MT then self:initMonsterTable() end
tile:moveObject(self)
self.runner = runner
function Monster:basicMoveRandomly(ignoredDungeon)
local moves = {vec2(-1,0), vec2(0,1), vec2(0,-1), vec2(1,0)}
local move = moves[math.random(1,4)]
local tile = self:getTile():legalNeighbor(self,move)
tile:moveObject(self)
end
function Monster:basicRandomMove(tiles)
local tile = self:getTile():validateMoveTo(self,tiles[math.random(1,#tiles)])
tile:moveObject(self)
end
function PathMonsterStrategy:execute(dungeon)
local newTile = self.map:nextAfter(self.monster:getTile())
if newTile then
newTile:moveObject(self.monster)
else
self.monster:pathComplete()
end
end
function Player:init(tile, runner)
self.runner = runner
self:initAttributes(self:retainedAttributes())
self:initSounds()
self:initVerbs()
tile:moveObject(self)
end
function Player:moveBy(aStep)
self.runner:resetVisibility()
local tile = self:getTile():legalNeighbor(self,aStep)
tile:moveObject(self)
end
Well, there are lots, but all in objects who don’t save tiles. That moveBy is the problematic one, because it defers to player to illuminate the tile. We fix that:
function Player:moveBy(aStep)
self.runner:resetVisibility()
local tile = self:getTile():legalNeighbor(self,aStep)
tile:moveObject(self)
tile:illuminate()
end
I believe that now we can remove the call to setTile from moveObject.
function Tile:moveObject(thing)
self:addContentsTo(thing)
-- thing:setTile(self)
end
This nearly works. We need to illuminate at the very beginning.
function GameRunner:movePlayerToRoom1()
local tile = self.rooms[1]:centerTile()
tile:moveObject(self.player)
tile:illuminate()
end
This is not ideal. Moving the player should auto-illuminate: we shouldn’t have to remember to do it. I’ve made a note. Now is not the time.
Curiously the change above does not cause us to illuminate at the beginning. As soon as I move the player, illumination works as expected. I wonder why it fails. It’ll have to do with when we turn off all the lights.
Hm. I’m not sure where this happened. Revert.
OK, back on the “remove unneeded code from moveEntrant” commit, all seems well.
This time I do this:
function Player:init(tile, runner)
self.runner = runner
self:initAttributes(self:retainedAttributes())
self:initSounds()
self:initVerbs()
tile:moveObject(self)
tile:illuminate()
end
function Player:moveBy(aStep)
self.runner:resetVisibility()
local tile = self:getTile():legalNeighbor(self,aStep)
tile:moveObject(self)
tile:illuminate()
end
All is good. Commit: move illumination to player init and moveBy.
I believe that I can now remove all the implementations of setTile.
At first blush I feel like this should require extensive testing. But I’m certain there are no senders of setTile, so run the tests and ship it. Commit: remove all implementors of setTile.
Now I’m going after getTile. Find senders. Here we have an irritating issue that will soon go away, which is that GameRunner has a method named getTile that refers to getting a tile from the dungeon, not from an object. When we’re done here, it will be the only method, so it’ll be OK.
Dungeon has that method as well, which is also OK.
There’s also this:
function Dungeon:playerTile()
return self:getPlayer():getTile()
end
We can replace that:
function Dungeon:playerTile()
return DungeonContents:getTile(self:getPlayer())
end
Here’s a case that may not be atypical:
function WayDown:draw()
pushMatrix()
pushStyle()
spriteMode(CENTER)
local c = self:getTile():graphicCenter()
sprite(Sprites:sprite(asset.steps_down),c.x,c.y,64,64)
popStyle()
popMatrix()
end
function WayDown:getTile()
return DungeonContents:getTile(self)
end
When objects go to draw themselves, they want to know what tile they are on, to get their drawing coordinates. Sometimes we pass in their center. Sometimes we do not.
Should we leave the getTile messages in there? They do communicate what’s going on. They also represent duplication, which we’d like to get rid of. But the real “physical” connection between object and tile is now removed, so we’re probably happy with what is left.
Yes. I’m going to let this be. We know that getTile implementations all go through the DungeonContentsCollection, so we should be at a sensible endpoint.
Commit that previous: Dungeon:playerTile goes direct to DungeonContents.
Despite it being clear that that works, it breaks a test. We are not amused.
2: Dungeon helps monster moves -- Dungeon:319: attempt to index a nil value (global 'DungeonContents')
I add DungeonContents to the test:
_:test("Dungeon helps monster moves", function()
local tiles, x1,x2
local dungeon = Runner:getDungeon()
local tiles = dungeon.tiles
for row = 1,#tiles do
for col = 1,#tiles[row] do
tiles[row][col]:testSetRoom()
end
end
dungeon:setTestPlayerAtPosition(vec2(30,30))
local monsterTile = dungeon:getTile(vec2(34,34))
_:expect(dungeon:manhattanToPlayer(monsterTile)).is(8)
tiles = dungeon:availableTilesCloserToPlayer(monsterTile)
x1,x2 = dungeon:getTile(vec2(33,34)), dungeon:getTile(vec2(34,33))
_:expect(tiles).has(x1)
_:expect(tiles).has(x2)
tiles = dungeon:availableTilesFurtherFromPlayer(monsterTile)
x1,x2 = dungeon:getTile(vec2(35,34)), dungeon:getTile(vec2(34,35))
_:expect(tiles).has(x1)
_:expect(tiles).has(x2)
tiles = dungeon:availableTilesSameDistanceFromPlayer(monsterTile)
x1,x2 = dungeon:getTile(vec2(33,35)), dungeon:getTile(vec2(35,33))
_:expect(tiles).has(x1)
_:expect(tiles).has(x2)
end)
But that just changes the error:
2: Dungeon helps monster moves -- Tile:389: attempt to index a nil value (local 'aTile')
The finger of suspicion points at that setTestPlayerAtPosition method. That’s this crock:
function Dungeon:setTestPlayerAtPosition(aVector)
self.runner.player = DungeonTestPlayer(self, aVector)
end
function DungeonTestPlayer:init(dungeon, aVector)
self.dungeon = dungeon
self.pos = aVector
end
function DungeonTestPlayer:getTile()
print("DungeonTestPlayer:getTile")
return self.dungeon:getTile(self.pos)
end
Meh. I think we need to add ourselves to the contents.
function DungeonTestPlayer:init(dungeon, aVector)
self.dungeon = dungeon
self.pos = aVector
local tile = dungeon:getTile(self.pos)
tile:addContentsTo(self)
end
This runs. It also shows me how dumb this method name is. It should be just addContents if that’s available. Seems to be. Rename. Commit: rename addContentsTo to addContents.
I think this is a wrap. Time to sum up.
Summary
Our mission as of a day or so ago was to remove the direct connection between dungeon objects, both entities and inanimate objects, and the tile they reside in. We did this by producing a collection that contains a single mapping, from any object to its current tile.
We then went through a series of mostly simple refactoring steps, unwinding the messy connections that had been bugging us. A couple of the steps were tricky, notably the Announcer, which had previously been allowed to be in several tiles at once. We solved that readily with a new object, AnnouncerProxy, where we place a proxy everywhere we want to refer to the same announcer.
This was a fairly involved refactoring, but refactoring it was: we have the same system behavior that we had when we started. In the course of this refactoring, we implemented two new classes, and changed or removed a number of methods and a couple of member variables, one in each of 5000+ tiles and another in each of the dungeon objects.
We took too big a bite a few times, and had to revert. Well, I took too big a bite. You had no part in it.
And now we have reduced the technical debt we set out to reduce. This is a good thing.
In addition, in playing around, I’ve found what I think is a long-standing defect: If you go to the learning level without turning off the Spikes, you get a run-time error:
Spikes:118: attempt to index a nil value
stack traceback:
Spikes:118: in field 'callback'
...in pairs(tweens) do
c = c + 1
end
return c
end
function Spikes:toggleUpDown()
self.state = ({up="down", down="up"})[self.state]
if self.stayDown then self.state = "down" end
if self.state == "up" then
local player = self:getTile():getPlayerIfPresent()
if player then self:actionWith(player) end
end
self.delay(2, self.toggleUpDown, self)
end
Line 118 is the local player = line. And this is not a long-standing defect. It can happen that getTile will return nil, because the Spikes are no longer in the current dungeon. So we’ll check for that.
I try this:
function Spikes:toggleUpDown()
local tile = self:getTile()
if tile == nil then return end -- we're being called back from a previous dungeon.
self.state = ({up="down", down="up"})[self.state]
if self.stayDown then self.state = "down" end
if self.state == "up" then
local player = tile:getPlayerIfPresent()
if player then self:actionWith(player) end
end
self.delay(2, self.toggleUpDown, self)
end
That fixes the crash when going to learning level, but other tests break:
1: Spikes -- Actual: 4, Expected: 1
1: Spikes -- Actual: 7, Expected: 1
1: Spikes down sprite -- Actual: Asset Key: trap_up.png (path: "/private/var/mobile/Containers/Data/Application/8237698C-306E-444D-BA10-95B7089EF5DA/Documents/D2.codea/trap_up.png"), Expected: Asset Key: trap_down.png (path: "/private/var/mobile/Containers/Data/Application/8237698C-306E-444D-BA10-95B7089EF5DA/Documents/D2.codea/trap_down.png")
Ah. Our FakeTile is confusing the test. Change it to explicitly set down, and it works:
_:test("Spikes", function()
local downSprite = asset.trap_down
local upSprite = asset.trap_up
local tile = FakeTile
local spikes = Spikes(tile, fakeTweenDelay)
_:expect(FakeTile.added, "not added").is(spikes)
spikes:down() -- <--
local lo = spikes:damageLo()
local hi = spikes:damageHi()
_:expect(lo).is(1)
_:expect(hi).is(1)\
...
Good to go: Fix defect in Spikes that caused Learning Level to crash looking for absent spikes.
So. How do I feel about this work?
- It was tedious
- There were lots of nitty-gritty changes. This is not surprising: when the design isn’t as good as we need, with common behavior (handling of the object’s tile) spread all over, we can expect lots of changes like that.
- There were bad surprises
- There were, but almost all of them we caught by the tests. We did encounter the Spikes issue, with the hanging delay trying to toggle a Spikes that had been collected. About all I can say about the surprises is that when we go to reduce technical debt, there will be some, and we can expect them to occur where our tests are weak.
-
That does suggest not trying to reduce technical debt with weak tests. It also suggest not coding at all with weak tests. Both of these are just about equally obvious.
- There was another way
- It would probably have been possible to resolve the tile pointer, or at least the duplication in the code, by providing a concrete superclass for both Entity and all the various dungeon objects. That would have consolidated all the
setTileandgetTileimplementations up into the superclass, save only the one in Player, which we could have resolved just like we did, by moving the call toilluminateto a couple of different locations. (Creating a tiny bit of duplication, by the way.) -
I’m not sure whether the superclass solution would have been easier, but I strongly suspect that it would have. The downside would have been retaining the direct connection between objects and their tiles, but arguably since there is a logical connection, the direct would be OK. I feel better about the indirect connection, though.
Was it worth it?
Well, as an example of what happens, surely yes. This is about as good as it gets with this big a reduction, although it seems larger than it should because of doing it all in a couple of days.
As something to do with a real product, I’d want to be more careful making the decision than I was here. I didn’t have much doubt that it could be done, and I knew I couldn’t get more than a day away from working, no matter what happened. In a real situation, we’d want to give the idea more consideration, and look a bit harder for the potentially troublesome bits.
I would argue, however, that the issue with Announcer was an indication of a tricky implementation, the use of a single object in multiple locations, which is at least questionable if not actually weird.
And the Spikes issue reflects a long-standing issue with the use of tweens to create changing behavior in the dungeon. A better design might be suitable, perhaps using the Bus to publish and subscribe, or a general collection of people using timers, or some way to clear all the tweens. None of these is straightforward, and they’d risk creating a memory leak. For example, if we had all the Spikes in some master collection so as to send them messages about when to go up and down, we’d have to be sure to clear them out when their level is gone.
Overall, I’m happy to be where we are, and it has just been a couple of days of work, though much longer than my usual two-hour days. This one has been more like eight hours, I’d guess, counting the million words in the two articles.
In any case, we’re here now, and I’m shipping it.
See you next time!