Dungeon 151
We’ll start with the garbage collection issue and move on from there. We should be able to remove all Floater’s references to GameRunner–if nothing else goes wrong.
I think there’s no question that the jerkiness of the Floater was due to an accumulation of memory that couldn’t be garbage collected. The fundamental issue was that the EventBus was created early on in running the program, to make it available to the tests. Then the tests started up aaround 30 GameRunner instances, all of which had a Floater, all of which subscribed to the Bus, and all the Floaters have a reference to the GameRunner.
Since EventBus is a global, this meant that there were, by the time the actual game started, 32 completely populated instances of GameRunner, each including 5440 instances of Tile and whatever all else GameRunner holds on to.
My current uncommitted fix seems to solve the problem. What I’ve done is this:
function setup()
Bus = --- <---
if CodeaUnit then
runCodeaUnitTests()
CodeaTestsVisible = true
end
local seed = math.random(1234567)
print("seed=",seed)
math.randomseed(seed)
showKeyboard()
TileLock = false
Bus = EventBus() -- <---
Runner = GameRunner()
Runner:createLevel(12)
TileLock = true
if false then
sprite(xxx)
end
end
The second occurrence releases the first one, so that the cruft can all go to the bin.
I also modified the tests:
_:describe("Tiled Game", function()
_:before(function()
_TileLock = TileLock
TileLock = false
_tweenDelay = tween.delay
tween.delay = fakeTweenDelay
_Runner = Runner
Bus = EventBus()
Runner = GameRunner()
Runner:createTiles()
end)
_:after(function()
TileLock = _TileLock
Runner = _Runner
Bus = EventBus()
tween.delay = _tweenDelay
end)
This is pretty much belt plus suspenders, and should be more than sufficient. However, now I’m going to remove the first creation from Main, and see what breaks, and then put Bus creation into those tests, so that we don’t even create this dangerous global early on. I expect to need to fix a number of tests.
The test testDungeon breaks.
_:before(function()
_bus = Bus
Bus = EventBus()
_runner = Runner
local gm = GameRunner()
Runner = gm
gm:createLevel(12)
dungeon = gm:getDungeon()
_TileLock = TileLock
TileLock = false
end)
_:after(function()
Bus = _bus
TileLock = _TileLock
Runner = _runner
end)
This is more polite: saving the old value (probably nil) and restoring it. I’ll go back and do the other test that way as well. The other was a quick fix.
The test for manhattan distance fails. Same fix. Combat Operations test fails. Same fix.
And, amusingly, the Floater test fails. Same fix.
Tests are green. Commit: Tests needing EventBus create their own _before, restore any previous one _after.
I’m not done yet, but first I have to feed the cat.
OK. Cat pulled back from the brink of death by starvation. And I made myself a nice iced chai as well.
I want now to do two, or one-and-a-half things to EventBus. I want to give it a clear method, to empty it completely, and I want to turn its events table into a “weak” table, that is, a Lua table that does not prevent its contents from being garbage collected.
This is another belt-suspenders-nailing pants to hips kind of move, but it seems like a good thing to do, and since I’ve never done it before, I can get the merit badge.
We have this:
function EventBus:init()
self.events = {}
end
We refactor to this:
function EventBus:init()
self:clear()
end
function EventBus:clear()
self.events = {}
end
Simple Extract Method refactoring. Now to do the weak table trick, we look it up on the Internet1. Here’s the page.
We modify clear:
function EventBus:clear()
local meta = { __mode="kv" }
self.events = {}
setmetatable(events, meta)
end
So that’s done. I remember at one point that I modified the unsubscribeAll, and did so incorrectly. Let’s check it:
function EventBus:unsubscribeAll(object)
for event,subs in pairs(self.events) do
subs[object] = nil
end
end
Nope, we’re good there. We go through all the events in the table and nil out the entry for the object who’s trying to unsubscribe. Let’s change the names a bit, for improved clarity:
I change another method as well:
function EventBus:subscriptions(event)
local subscriptions = self.events[event]
if not subscriptions then
subscriptions = {}
self.events[event] = subscriptions
end
return subscriptions
end
function EventBus:unsubscribeAll(subscriber)
for event,subscriptions in pairs(self.events) do
subscriptions[subscriber] = nil
end
end
I think that’s more clear. Commit: EventBus uses weak table for event subscriptions. Rename some variables.
OK. Now we were working with Floater and its relationship with GameRunner. Floater now has one reference to its member variable runner:
function Floater:draw()
local n = self:numberToDisplay()
if n == 0 then return end
pushStyle()
fill(255)
fontSize(20)
local y = self:yOffset()
local pos = self.runner:playerGraphicCenter()
pos = pos + vec2(0,self.yOff)
for i = 1,n do
text(self.buffer[i], pos.x, pos.y)
pos = pos - vec2(0,self.lineSize)
end
self:increment()
popStyle()
end
Since GameRunner calls draw, let’s have it pass in the player center:
function Floater:draw(playerCenter)
local n = self:numberToDisplay()
if n == 0 then return end
pushStyle()
fill(255)
fontSize(20)
local y = self:yOffset()
local pos = playerCenter + vec2(0,self.yOff)
for i = 1,n do
text(self.buffer[i], pos.x, pos.y)
pos = pos - vec2(0,self.lineSize)
end
self:increment()
popStyle()
end
And in GameRunner, convert:
function GameRunner:drawMessages()
pushMatrix()
self:scaleForLocalMap()
self.cofloater:draw()
popMatrix()
end
To:
function GameRunner:drawMessages()
pushMatrix()
self:scaleForLocalMap()
self.cofloater:draw(self:playerGraphicCenter())
popMatrix()
end
Arrgh, major mistake. I didn’t run the tests after the change to make a metatable, and it was wrong.
function EventBus:clear()
local meta = { __mode="kv" }
self.events = {}
setmetatable(self.events, meta)
end
I left off the self on events inside setmetatable. Fix that. Commit: EventBus set metatable was setting a nil table. (I just committed the EventBus, not the other open changes. Yay Working Copy.)
Now where were we? Oh, right, testing the change to pass in the princess location to Floater:draw.
We have a flaw. The Decor messages aren’t coming out. Either we haven’t subscribed, or we haven’t published. Something may have gotten lost in all the excitement.
Floater does subscribe:
function Floater:init(runner, yOffsetStart, lineSize, lineCount)
self.runner = runner
self.provider = Provider("")
self.yOffsetStart = yOffsetStart
self.lineSize = lineSize
self.lineCount = lineCount
Bus:subscribe(self,self.addTextToCrawl, "addTextToCrawl")
Bus:subscribe(self,self.addItemsFromBus, "addItemsToCrawl")
end
Does Decor publish?
function Decor:damage(aPlayer)
local co = CombatRound(self,aPlayer)
co:appendText("Ow! Something in there is sharp!")
local damage = math.random(1,5)
co:applyDamage(damage, "_speed")
co:publish()
end
What about CombatRound?
function CombatRound:publish()
Bus:publish("addItemsToCrawl", self, self.commandList)
end
This does not please us. We are very sad at this moment. We’ve lost a change. A quick check tells me that combat doesn’t work, nor do spikes.
I’m going to try some back versions.
Some fiddling tells me that the setmetatable has done it. I think I see why, but first, let’s remove it and see if we can properly commit.
function EventBus:clear()
self.events = {}
end
Test. It’s all good. Commit: remove fancy weak table BS from EventBus.
OK, What Happened Here?
Yes, well. I got carried away, got all excited about garbage, and did something that couldn’t possibly work.
My real concern was that garbage collection one. The EventBus, a global, was filling up with instances of events,
One thing after another around here
I chanced to look at my atomic clock, and it had stopped. Since the National Radio Clock in Colorado syncs from the clock in my bathroom, I had to address that problem immediately. I hope none of your dates were corrupted and apologize for any inconvenience.
Anyway, what happened?
Well, I made the events table weak. My concern was that the Floater holds on to a GameRunner and so that meant that back when the tests were creating lots of Floaters, and the Floaters were subscribing, the EventBus filled up with entire copies of the game. So I made the EventBus table weak.
That was wrong, because that means it can’t hold on to anything, including its own subscriptions. Just simply, not OK.
So that’s right out. Serves me right for digging that deep in the bag of tricks. The good news is, it was broken for just a few minutes.
However, we have just removed the last reference to the runner variable in Floater, which means that we can remove it, which means that the concern is solved closer to the root. And we’ve modified the tests to use their own floater anyway.
(We could also provide a NullEventBus to test with. Right now that’s not in my headlights.)
Let’s check Floater and remove its reference to runner, and change its creation method.
This:
function Floater:init(runner, yOffsetStart, lineSize, lineCount)
self.runner = runner
self.provider = Provider("")
self.yOffsetStart = yOffsetStart
self.lineSize = lineSize
self.lineCount = lineCount
Bus:subscribe(self,self.addTextToCrawl, "addTextToCrawl")
Bus:subscribe(self,self.addItemsFromBus, "addItemsToCrawl")
end
Becomes:
function Floater:init(yOffsetStart, lineSize, lineCount)
self.provider = Provider("")
self.yOffsetStart = yOffsetStart
self.lineSize = lineSize
self.lineCount = lineCount
Bus:subscribe(self,self.addTextToCrawl, "addTextToCrawl")
Bus:subscribe(self,self.addItemsFromBus, "addItemsToCrawl")
end
And we have to change its creators:
function GameRunner:init()
self.tileSize = 64
self.tileCountX = 85 -- if these change, zoomed-out scale
self.tileCountY = 64 -- may also need to be changed.
self.cofloater = Floater(self, 50,25,4)
self.musicPlayer = MonsterPlayer(self)
self:initializeSprites()
self.dungeonLevel = 0
self.requestNewLevel = false
self.playerRoom = 1
Bus:subscribe(self, self.createNewLevel, "createNewLevel")
end
Which becomes:
function GameRunner:init()
self.tileSize = 64
self.tileCountX = 85 -- if these change, zoomed-out scale
self.tileCountY = 64 -- may also need to be changed.
self.cofloater = Floater(50,25,4)
self.musicPlayer = MonsterPlayer(self)
self:initializeSprites()
self.dungeonLevel = 0
self.requestNewLevel = false
self.playerRoom = 1
Bus:subscribe(self, self.createNewLevel, "createNewLevel")
end
There were also two tests that needed the same treatment.
Tests run, game seems to work (q.v.), commit: Floater has no reference back to GameRunner (yay!)
Reflection
This is a good thing. Our mission with EventBus was to reduce explicit connections between objects in the game. One of our biggest such concerns is that everyone seems to have a reference to the GameRunner, which results in difficult testing, and in GameRunner having a very broad interface as it mediates between objects and passes messages around. EventBus can manage a lot of the messaging with its publish/subscribe style.
However, there were stumbles along the way, and a lot of what I’ve done has required “manual”, in-game testing rather than automated tests. I’ve been moving rapidly, almost in a panic style, certainly in a manic style, and even if I could have written more tests, I wasn’t doing it.
This happens. It’s not good, but it does happen. When it does, I try to slow down, reflect, and renew my desire to do better. Sometimes I’ll even write a test, because doing so generally teaches me something. Right now, though, I’m not feeling it.
This is probably not good. Sorry, but despite rumors to the contrary, I’m only human, and I’m here to show you how this human actually programs, not how some idealized version of me, 50 years younger, programs2.
What Next, Then?
Well, there’s a sticky here saying “Let Decor deal Lethargy or Weakness”, which means that I want dangerous decors to sometimes impact player strength, and sometimes impact speed. If they say something fancy, that would be nice as well.
Let’s see how that works now.
function Decor:init(tile, item, kind)
self.sprite = kind and DecorSprites[kind] or Decor:randomSprite()
self.item = item
tile:moveObject(self)
self.scaleX = ScaleX[math.random(1,2)]
self.dangerous = math.random() > 0.5
end
function Decor:actionWith(aPlayer)
if self.dangerous then
self:damage(aPlayer)
end
if self.item then self.item:addToInventory() end
end
function Decor:damage(aPlayer)
local co = CombatRound(self,aPlayer)
co:appendText("Ow! Something in there is sharp!")
local damage = math.random(1,5)
co:applyDamage(damage, "_speed")
co:publish()
end
Let’s suppose there are three things that can happen, with these probabilities:
| chance | action |
|---|---|
| 0.5 | nothing |
| 0.25 | lethargy |
| 0.25 | weakness |
Let me recast that against four equally probable integers:
| integer | action |
|---|---|
| 1 | nothing |
| 2 | nothing |
| 3 | lethargy |
| 4 | weakness |
What I propose to do is to change that roll for dangerous to roll three actions, doNothing, castLethargy, and castWeakness. I’m calling them “cast” right now, thinking magic, but I’m not sure what I want the Floater messages to say.
In a spirit of trying to do better, let’s see if I can test this a bit. We do have some rudimentary Decor tests, so maybe I can do some more.
I start to write the test …
_:test("Decor damage", function()
local tile = FakeTile()
local decor = Decor(tile,item)
_:expect
end)
And now I wonder what I’d like to do. The main damage method creates a CombatRound and tells it to do things. Let’s refactor that freehand, from this:
function Decor:actionWith(aPlayer)
if self.dangerous then
self:damage(aPlayer)
end
if self.item then self.item:addToInventory() end
end
function Decor:damage(aPlayer)
local co = CombatRound(self,aPlayer)
co:appendText("Ow! Something in there is sharp!")
local damage = math.random(1,5)
co:applyDamage(damage, "_speed")
co:publish()
end
To this, passing in the CombatRound from action:
function Decor:actionWith(aPlayer)
if self.dangerous then
self:damage(CombatRound(self,aPlayer))
end
if self.item then self.item:addToInventory() end
end
function Decor:damage(round)
round:appendText("Ow! Something in there is sharp!")
local damage = math.random(1,5)
round:applyDamage(damage, "_speed")
round:publish()
end
This should be OK, but let’s test it in my new test. I’ll need a FakeCombatRound.
_:test("Decor damage", function()
local tile = FakeTile()
local decor = Decor(tile,item)
local round = FakeCombatRound()
decor:damage(round)
end)
This will break, telling me what methods to add to my FakeCombatRound, and hinting what I could check. I plan to elaborate this a lot.
3: Decor damage -- Decor:44: attempt to call a nil value (method 'appendText')
Right.
function FakeCombatRound:appendText(text)
appendedText = text
end
That’s a local that I just created in this tab. Now to check it:
_:test("Decor damage", function()
local tile = FakeTile()
local decor = Decor(tile,item)
local round = FakeCombatRound()
decor:damage(round)
local match = appendedText:match("Ow!")
_:expect(match).isnt(nil)
end)
We first have to add two more methods to FakeCombatRound, applyDamage, and publish: I leave them empty for now, but plan to add the ability to check as we go forward.
The test runs, we’re getting the “Ow!” message.
Now what I want, what I really really want, is to have a method into which I pass the displayed message, the damage desired, and the attribute to apply damage to. Presently we have this:
function Decor:actionWith(aPlayer)
if self.dangerous then
self:damage(CombatRound(self,aPlayer))
end
if self.item then self.item:addToInventory() end
end
function Decor:damage(round)
round:appendText("Ow! Something in there is sharp!")
local damage = math.random(1,5)
round:applyDamage(damage, "_speed")
round:publish()
end
Let’s parameterize damage further and change our call to it.
function Decor:actionWith(aPlayer)
if self.dangerous then
local round = CombatRound(self,aPlayer)
local msg = "Ow! Something in there is sharp!"
local damage = math.random(1,5)
self:damage(round, message, damage "_speed")
end
if self.item then self.item:addToInventory() end
end
function Decor:damage(round, message, damage, attribute)
round:appendText(message)
round:applyDamage(damage, attribute)
round:publish()
end
This ought to work and pass tests. And wow do you see any Feature Envy in that damage method?
Hm, got this error from the tests.
3: Decor damage -- Decor:133: attempt to index a nil value (upvalue 'appendedText')
Oh, well, I didn’t pass in all the stuff in my test. Fix that”
Tests run but stepping on a Decor I get this:
Decor:41: attempt to call a number value (local 'damage')
stack traceback:
Decor:41: in method 'actionWith'
Player:237: in local 'action'
TileArbiter:27: in method 'moveTo'
Tile:110: in method 'attemptedEntranceBy'
Tile:390: in function <Tile:388>
(...tail calls...)
Player:192: in method 'moveBy'
Player:140: in method 'executeKey'
Player:186: in method 'keyPress'
GameRunner:322: in method 'keyPress'
Main:39: in function 'keyboard'
function Decor:actionWith(aPlayer)
if self.dangerous then
local round = CombatRound(self,aPlayer)
local msg = "Ow! Something in there is sharp!"
local damage = math.random(1,5)
self:damage(round, message, damage "_speed")
end
if self.item then self.item:addToInventory() end
end
We can’t call a local var damage and then call damage. I was thinking that Lua could disambiguate that but it can’t. That self:damage just means damage(self, ...) so it refers to the local. Rename to dam.
Curiously, the missing comma between dam and “_speed” caused that error. Why?
Remember that in Lua you can pass a table to a method by saying meth{foo=bar}? It looks to me as if you can pass a string that way also. Anyway, putting in the comma fixes it.
Now we can do some more tests.
I propose to replace the member variable dangerous with a method name, which will be doNothing, castLethargy, or castWeakness, then implement those methods, and we’ll be done. Driven by tests, of course3.
function Decor:init(tile, item, kind)
self.sprite = kind and DecorSprites[kind] or Decor:randomSprite()
self.item = item
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
Now to TDD out those methods. (I’m not going to test the random number.)
_:test("Decor damage", function()
local tile = FakeTile()
local decor = Decor(tile,item)
local round = FakeCombatRound()
decor:damage(round, "Ow! Dammit!", 5, "_health")
local match = appendedText:match("Ow!")
_:expect(match).isnt(nil)
appendedText = ""
decor:doNothing()
_:expect(appendedText).is("")
end)
This is pretty dull, but it gets me started.
function Decor:doNothing()
end
The game works, because we’re not using the new danger variable, and Decor is checking the now nil dangerous variable. And the test runs.
Expand the test:
_:test("Decor damage", function()
local tile = FakeTile()
local decor = Decor(tile,item)
local round = FakeCombatRound()
decor:damage(round, "Ow! Dammit!", 5, "_health")
local match = appendedText:match("Ow!")
_:expect(match).isnt(nil)
appendedText = ""
decor:doNothing()
_:expect(appendedText).is("")
decor:castLethargy()
_:expect(appendedText).is("Fumes! A feeling of lethargy comes over you!")
end)
In writing it, I realize I need to have a round provided:
function Decor:castLethargy(round)
local msg = "Fumes! A feeling of lethargy comes over you!"
local dam = math.random(1,5)
self:damage(round,message,dam,"_speed")
end
We’ll refactor when this is all done.
_:test("Decor damage", function()
local tile = FakeTile()
local decor = Decor(tile,item)
local round = FakeCombatRound()
decor:damage(round, "Ow! Dammit!", 5, "_health")
local match = appendedText:match("Ow!")
_:expect(match).isnt(nil)
appendedText = ""
decor:doNothing()
_:expect(appendedText).is("")
decor:castLethargy(round)
_:expect(appendedText).is("Fumes! A feeling of lethargy comes over you!")
end)
I expect this to run green. As so often happens, my expectations are not met:
3: Decor damage -- Actual: nil, Expected: Fumes! A feeling of lethargy comes over you!
Didn’t get the message. Helps if you spell it right:
function Decor:castLethargy(round)
local msg = "Fumes! A feeling of lethargy comes over you!"
local dam = math.random(1,5)
self:damage(round,message,dam,"_speed")
end
Fix that:
function Decor:castLethargy(round)
local msg = "Fumes! A feeling of lethargy comes over you!"
local dam = math.random(1,5)
self:damage(round,msg,dam,"_speed")
end
Test is green. Do the weakness:
decor:castWeakness(round)
_:expect(appendedTest).is("A poison needle! Suddenly you feel weakened!")
function Decor:castWeakness(round)
local msg = "A poison needle! Suddenly you feel weakened!"
local dam = math.random(1,5)
self:damage(round,msg,dam,"_speed")
end
Should be green. But no. Again with the nil:
3: Decor damage -- Actual: nil, Expected: A poison needle! Suddenly you feel weakened!
Well that’s curious. Oh. Look at the test lines. I check for appendedTest not Text.
Remind me in the next reflection to comment on this. You may be thinking that I’m being careless, and you’re not quite wrong but you’re not quite right.
Tests are green. I can plug in the new code. But I will need to make sure that I get a real CombatRound into my damage method, and right now, my casts are expecting it to be provided. We can do this:
function Decor:actionWith(aPlayer)
if self.dangerous then
local round = CombatRound(self,aPlayer)
local msg = "Ow! Something in there is sharp!"
local dam = math.random(1,5)
self:damage(round, message, dam, "_speed")
end
if self.item then self.item:addToInventory() end
end
I do this:
function Decor:actionWith(aPlayer)
local round = CombatRound(self,aPlayer)
self.danger(self,round)
if self.item then self.item:addToInventory() end
end
This will create a CombatRound even if there’s a doNothing object. I’m not sweating the small stuff at this point.
Run and crash into some decor.
Looks perfect. Commit: Decor now deals either Lethargy or Weakness.
Let’s reflect, sum up, and get outa here.
Reflection
Where’s the fire, bud?
If you were paying attention–and I know you were–you probably noticed some fairly silly mistakes, like typing test when I meant text. You might have had the sense that I was just flying along, not taking a lot of care, letting the chips fall where they may.
And to a degree, that’s true. But it’s OK, or mostly OK. Why? Because I was working in an area where I had some solid tests and was making them more solid, so that the odds were in my favor that if I did make mistakes, the tests would catch them.
Those of you who love strict compile-time typing might have been thinking that in your precious language StrictFoo, those errors wouldn’t even have compiled. And true, they wouldn’t. Those of you who are thinking Deep Lua might have wondered whether protecting my code against creating random new globals, like appendedTest would be a good addition. And true, it might.
But the point of having good tests is that, by and large, we’re protected by the tests. If we have strict typing, or magic global preventers, that’s all to the good, but our tests are the main line of defense.
According to my theory of things, the presence or absence of tests is the main determinant of whether I ship defects. When I have tests, shipped defects go way down. And when I don’t, they go up.
This is article 151, and so far I’ve shipped, what, less than half a dozen defects. Call it one out of 30 releases. Not great. I’ve seen better. And yes, this program is small, and if there were five of me working on it, like there are five of you on your product, we’d have five times as many defects. Unless we were pairing. In which case we wouldn’t.
So, the tests are our main line of defense, not our compiler or other more complex tools. Those are valuable tools, and certainly if we have them, we should apply them. But even if we have them, our own tests will tell us more quickly and more accurately what’s wrong.
Taking out the trash
That garbage collection issue was nearly unique. Certainly I’ve had the experience of consuming all the memory before, but usually that happens continuously until you run out of memory entirely. The situation here was that we created a lot of should-be garbage and then just held on to it, slowing things down but not really running out of memory. And, possibly, the jerky crawl would have smoothed out after a while. The game doesn’t create much garbage in use.
On the other hand … would another level have created more? I don’t know.
The garbage bug was a bit deep. Took me a while to spot it, but once I twigged to it, it was easy to patch, and then easy to fix correctly.
Interesting, though. Sometimes you get an interesting defect, and this certainly was that.
Still, after a weak day yesterday, today was pretty strong. Except for my weak table. That was too weak.
See you tomorrow! Or Sunday. Or Monday.
Or right now.
I just noticed that weakness is affecting speed, not strength. Copy-paste bug:
function Decor:castWeakness(round)
local msg = "A poison needle! Suddenly you feel weakened!"
local dam = math.random(1,5)
self:damage(round,msg,dam,"_strength")
end
That’s the fix. Here’s the test:
_:test("Decor damage", function()
local tile = FakeTile()
local decor = Decor(tile,item)
local round = FakeCombatRound()
decor:damage(round, "Ow! Dammit!", 5, "_health")
local match = appendedText:match("Ow!")
_:expect(match).isnt(nil)
appendedText = ""
decor:doNothing()
_:expect(appendedText).is("")
decor:castLethargy(round)
_:expect(appendedText).is("Fumes! A feeling of lethargy comes over you!")
_:expect(damageAttribute).is("_speed")
decor:castWeakness(round)
_:expect(appendedText).is("A poison needle! Suddenly you feel weakened!")
_:expect(damageAttribute).is("_strength")
end)
Commit: Decor incorrectly damaged speed in all cases.