Space Invaders 61
There’s no reason to think that I’m smarter today than I was yesterday, but I am a bit more experienced.
And I’m certainly motivated not to shave many yaks nor go down many ratholes. We’ll see how that goes.
We have a defect: some code that doesn’t work. We’ll begin by making it work, and then see if we can make it right. The defect is that when the player manages to shoot a saucer, the game crashes.
The message is:
Saucer:82: attempt to index a nil value (global 'Gunner')
stack traceback:
Saucer:82: in method 'score'
Saucer:71: in method 'die'
Saucer:62: in method 'killedBy'
Army:242: in method 'checkForKill'
Army:222: in method 'processMissile'
Missile:26: in method 'draw'
Player:61: in method 'draw'
GameRunner:24: in method 'draw'
Main:36: in function 'draw'
And the code is:
function Saucer:score()
return self.scores[Gunner:shotsFired()%15 + 1]
end
And the defect is, we no longer initialize the global Gunner to the current player, in our zeal to have fewer globals.
A direct fix would be to create the global again. A less direct fix would be to direct the shotsFired question to an object that the saucer knows, where that object knows the gunner. I’m a bit concerned about that approach, because yesterday I wound up with a long dependency loop between objects, which made setting up tests much more difficult.
Speaking of tests, we have comprehensive tests for Saucer:score(). Why didn’t they fail and clue us in?
Here’s why:
_:test("Saucer Score depends on shots fired", function()
local army = Army()
Gunner = Player()
local s = army.saucer
_:expect(s:score()).is(100)
Gunner:unconditionallyFireMissile(true)
_:expect(s:score()).is(50)
Gunner:unconditionallyFireMissile(true)
_:expect(s:score()).is(50)
Gunner:unconditionallyFireMissile(true)
_:expect(s:score()).is(100)
Gunner:unconditionallyFireMissile(true)
_:expect(s:score()).is(150)
Gunner:unconditionallyFireMissile(true)
_:expect(s:score()).is(100)
Gunner:unconditionallyFireMissile(true)
_:expect(s:score()).is(100)
Gunner:unconditionallyFireMissile(true)
_:expect(s:score()).is(50)
Gunner:unconditionallyFireMissile(true)
_:expect(s:score()).is(300)
Gunner:unconditionallyFireMissile(true)
_:expect(s:score()).is(100)
Gunner:unconditionallyFireMissile(true)
_:expect(s:score()).is(100)
Gunner:unconditionallyFireMissile(true)
_:expect(s:score()).is(100)
Gunner:unconditionallyFireMissile(true)
_:expect(s:score()).is(50)
Gunner:unconditionallyFireMissile(true)
_:expect(s:score()).is(150)
Gunner:unconditionallyFireMissile(true)
_:expect(s:score()).is(100)
Gunner:unconditionallyFireMissile(true)
_:expect(s:score()).is(100)
end)
The test sets up the global! No one else does, just the test. If we change that top line to this:
_:test("Saucer Score depends on shots fired", function()
local army = Army()
local Gunner = Player()
local s = army.saucer
...
We get this:
32: Saucer Score depends on shots fired -- Saucer:82: attempt to index a nil value (global 'Gunner')
Somehow, when I removed the global Gunner, either I missed that test, or–even worse–perhaps I thought it was OK to have a global.
Now this test does need a Gunner, because it’s testing the relationship between shots and saucer score. It’s a bit larger than a conventional unit test, but it is that loop of connections that we want to test. Gunner counts shots, saucer “random” score is based on number of shots.
I’m confident that with the gunner set to local in that test, it will correctly test our saucer problem. I’ll surely test actually shooting a saucer when this fix is done, but during the process, I trust the test … now.
There is only one Player (the class of the thing called “gunner” is Player) created during the game. It is reused each time a new gunner is spawned. It is a “singleton”. As such, there’s a standard aspect of the Singleton Pattern that I am minded to use here.
I’m going to create a member variable on the Player class, named instance, and keep the Player instance in that variable. Then, anyone who wants to talk to the current player can say `Player:getInstance()’ and the Player class will give it to him.
This may seem odd, but it’s a pretty standard way of making an instance available when it’s needed. Let’s try it.
function Player:init(pos)
self:setPos(pos)
self.count = 210 -- count down to first player
self.missile = Missile()
self.gunMove = vec2(0,0)
self.explosions = { readImage(asset.playx1), readImage(asset.playx2) }
self.missileCount = 0
self.alive = false
self.drawingStrategy = self.drawNothing
Player.instance = self
end
We can do that, because a class is just a table, and we can put things in it. Now the getter:
function Player:getInstance()
return Player.instance
end
Now our test should still fail … and it does. So we fix:
function Saucer:score()
return self.scores[Player:getInstance():shotsFired()%15 + 1]
end
And the tests run. And I’m certain that shooting an actual saucer will also work. Before I test that: chat run. Commit: saucer tests run.
Back from chai … and in two starts of the game, the saucer never appeared. I’ve seen that behavior before. I recall having no idea how it could fail. Let’s see what the code says:
function Army:dropRandomBomb()
if not self:runSaucer() then
local bombs = {self.rollingBomb, self.plungerBomb, self.squiggleBomb}
local bombType = math.random(3)
self:dropBomb(bombs[bombType])
end
end
function Army:runSaucer()
if self.saucer:isRunning() then return false end
if self:yPosition() > 89 then return false end
if math.random(1,20) < 10 then return false end
self.saucer:go(self.player:shotsFired())
return true
end
Well. If the saucer returned true to isRunning() then we’d never start one. Let’s look at that:
function Saucer:isRunning()
return self.alive or self.exploding > 0
end
I imagine that it’s possible that our tests have left the actual saucer alive. But when we start a game, we do this:
function startGame()
Runner = GameRunner()
SoundPlayer = Sound()
invaderNumber = 1
end
function GameRunner:init()
self.lm = LifeManager(3, self.spawn, self.gameOver)
Shield:createShields()
self.player = Player()
TheArmy = Army(self.player)
self:resetTimeToUpdate()
self.weaponsTime = 0
end
function Army:init(player)
self:initMemberVariables(player)
self:marshalTroops()
self:defineBombs()
end
function Army:initMemberVariables(player)
self.player = player
self.weaponsAreFree = false
self.armySize = 55
self.invaderCount = 0
self.invaderNumber = 1
self.overTheEdge = false
self.motion = vec2(2,0)
self.bombDropCycleLimit = 0x30
self.bombCycle = 0
self.saucer = Saucer(self)
self.score = 0
end
function Saucer:init(army, optionalShotsFired)
self.army = army
self.exploding = 0
self.scores = {100,50,50,100,150,100,100,50,300,100,100,100,50,150,100}
if optionalShotsFired == nil then
self.alive = false
else
self:go(optionalShotsFired)
end
end
So that makes me think it can’t be alive. I’ll check with an assert:
function Army:runSaucer()
if self.saucer:isRunning() then
assert(false, "saucer running")
return false
end
if self:yPosition() > 89 then return false end
if math.random(1,20) < 10 then return false end
self.saucer:go(self.player:shotsFired())
return true
end
I’m a bit afraid that this is a Heisenbug that never appears when you’re looking for it. But we’ll push a bit and see.
This time, of course, the saucer started. Then after it started, the assert fired but the point is the saucer started. So it’s not alive when it shouldn’t be. That makes sense, the code seems pretty clear.
Looking again at the runSaucer:
function Army:runSaucer()
if self.saucer:isRunning() then return false end
if self:yPosition() > 89 then return false end
if math.random(1,20) < 10 then return false end
self.saucer:go(self.player:shotsFired())
return true
end
We are looking at shotsFired, and while we haven’t really changed that, we are looking at the Army’s copy of the player to do it. We need to unwind all the cached player instances, in order to play fair with the Singleton pattern. Let’s look at Saucer:go for more clues.
function Saucer:go(shotsFired)
self.startPos = vec2( 8,185)
self.stopPos = vec2(208,185)
self.step = vec2( 2, 0)
if shotsFired%2==1 then
self.step = -self.step
self.startPos,self.stopPos = self.stopPos,self.startPos
end
self.alive = true
self.pos = self.startPos
self.exploding = 0
SoundPlayer:play("ufoLo")
end
OK. If this got called, even if it somehow messed up the saucer step or start position, it would still play the sound. I don’t hear the sound. So I think we didn’t ever get this far.
Reviewing again:
function Army:runSaucer()
if self.saucer:isRunning() then return false end
if self:yPosition() > 89 then return false end
if math.random(1,20) < 10 then return false end
self.saucer:go(self.player:shotsFired())
return true
end
We are sure we haven’t been getting to go. So either yPosition() didn’t get below 89, or the random number generator never rolled less than 10. The latter seems unlikely, we call runSaucer essentially every cycle.
I do recall something that may be interesting. The failure happens when I’ve killed masses of invaders and bombs are just raining down. Let me see if I can cause the bug. Well, it happened once, but not again. Could there be a problem with yPosition?
function Army:yPosition()
return self.invaders[1].pos.y
end
What if the invader weren’t updated? We do have that continue trick that we do. If this is the bug, then killing the first invader early on should make the saucer never appear.
Ha. I shot just that one guy and the saucer never appears. Let’s review the update code:
function Army:updateOneLiveInvader()
local continue = true
while(continue) do
continue = self:nextInvader():update(self)
end
end
That name is telling, isn’t it?
function Army:nextInvader()
self:handleCycleEnd()
local inv = self.invaders[self.invaderNumber]
self.invaderNumber = self.invaderNumber + 1
return inv
end
How do they update:
function Invader:update(army)
if not self.alive then return true end
army:reportingForDuty()
self.picture = (self.picture+1)%2
self.pos = self.pos + army:invaderMotion()
Shield:checkForShieldDamage(self)
if self:overEdge() then
army:invaderOverEdge(self)
end
return false
end
Uh, yeah, so if the first invader isn’t alive, he won’t be updated, and we won’t see the y position falling. I think we can safely just update all the invaders and still return the alive status. They know not to draw anyway. So:
function Invader:update(army)
self.pos = self.pos + army:invaderMotion() -- required for yPosition
if not self.alive then return true end
army:reportingForDuty()
self.picture = (self.picture+1)%2
Shield:checkForShieldDamage(self)
if self:overEdge() then
army:invaderOverEdge(self)
end
return false
end
Now to test. Sure enough, now the saucer shows up when it should. Now, if I recall, I was going to test whether saucer killing works. And it does.
Commit: fixed saucer kill; fixed saucer did not fly if first alien dead.
Let’s clean up Player references
We’ve gone to the Player:getInstance()', providing a rudimentary but complete Singleton pattern for Player. To use this properly, no one should cache the current Player, they should use the getInstance`, and they should use it dynamically.
In practice, we know that it never changes. Well, hardly ever. But once we make Player class responsible for knowing the current instance, we had better use it. I’ll look for references to .player and fix them. Here’s the first one I found:
function touched(touch)
if Runner and Runner:livesRemaining() ~= 0 then
Runner.player:touched(touch)
else
if touch.state == ENDED then
testsVisible(false)
startGame()
end
end
end
This becomes:
function touched(touch)
if Runner and Runner:livesRemaining() ~= 0 then
Player:getInstance():touched(touch)
else
if touch.state == ENDED then
testsVisible(false)
startGame()
end
end
end
I give that one a quick test. Player is still controllable. For most of the replacements, I plan to show the version after changed here, but not before changed unless it’s interesting.
function GameRunner:init()
self.lm = LifeManager(3, self.spawn, self.gameOver)
Shield:createShields()
Player()
TheArmy = Army(self.player)
self:resetTimeToUpdate()
self.weaponsTime = 0
end
Interesting already. We’re passing our player to Army. We don’t want to do that so let’s fix Army next, including not wanting that player parameter:
function Army:initMemberVariables(
self.weaponsAreFree = false
self.armySize = 55
self.invaderCount = 0
self.invaderNumber = 1
self.overTheEdge = false
self.motion = vec2(2,0)
self.bombDropCycleLimit = 0x30
self.bombCycle = 0
self.saucer = Saucer(self)
self.score = 0
end
function Army:runSaucer()
if self.saucer:isRunning() then return false end
if self:yPosition() > 89 then return false end
if math.random(1,20) < 10 then return false end
self.saucer:go(Player:getInstance():shotsFired())
return true
end
function Army:targetedColumn()
local gunnerX = Player:getInstance().pos.x
local armyX = self:xPosition()
local relativeX = gunnerX - armyX
local result = 1 + relativeX//16
return math.max(math.min(result,11),1)
end
Now we can return to GameRunner:
function GameRunner:init()
self.lm = LifeManager(3, self.spawn, self.gameOver)
Shield:createShields()
Player()
TheArmy = Army()
self:resetTimeToUpdate()
self.weaponsTime = 0
end
We should get rid of TheArmy too, one of these first days.
function GameRunner:draw()
pushMatrix()
pushStyle()
noSmooth()
stroke(255)
fill(255)
self:scaleAndTranslate()
TheArmy:draw()
Player:getInstance():draw()
Shield:drawShields()
self:drawStatus()
popStyle()
popMatrix()
end
function GameRunner:update60ths()
self.time120 = self.time120 + (DeltaTime < 0.016 and 1 or 2)
if self.time120 < 2 then return end
self.time120 = 0
Player:getInstance():update()
TheArmy:update()
if self.weaponsTime > 0 and ElapsedTime >= self.weaponsTime then
TheArmy:weaponsFree()
self.weaponsTime = 0
end
end
function GameRunner:spawn()
Player:getInstance():spawn()
self:setWeaponsDelay()
end
That’s quite a bit of changing. I’ll run the tests, maybe commit, we’ll discuss that in a moment.
One test fails:
9: Bomb hits gunner -- Tests:74: attempt to index a nil value (local 'Gunner')
_:test("Bomb hits gunner", function()
Runner = GameRunner()
local Gunner = Player:getInstance()
Gunner.alive = true
Gunner.pos=vec2(50,50)
...
I thought that would fix it, but no:
9: Bomb hits gunner -- Bomb:69: attempt to index a nil value (field 'player')
I think what we have here is that there is no Player instance in the Player class. I think there should be because GameRunner inits Player. But let’s assert on it:
_:test("Bomb hits gunner", function()
Runner = GameRunner()
_:expect(Player:getInstance()).isnt(nil)
local Gunner = Player:getInstance()
Gunner.alive = true
Gunner.pos=vec2(50,50)
Weird. That test passes, and then we get the same message
Oh. That's in Bomb. We haven't fixed Bomb yet. Duh. We'll fix it:
~~~lua
function Bomb:killsGunner()
if not Player:getInstance().alive then return false end
local hit = rectanglesIntersectAt(self.pos,3,4, Player:getInstance().pos,16,8)
if hit == nil then return false end
Player:getInstance():explode()
return true
end
Test runs. We access the player three times in this method. That does seem a lot like feature envy, doesn’t it? We aren’t going to fix it now. I am going to cache the player instance locally, however. Is that kosher? I suspect not, but I’m sure it’s OK.
function Bomb:killsGunner()
local player = Player:getInstance()
if not player.alive then return false end
local hit = rectanglesIntersectAt(self.pos,3,4, player.pos,16,8)
if hit == nil then return false end
player:explode()
return true
end
And that’s all the references I can find. Tests all rn, game plays properly. Commit: player singleton complete.
About that conversion …
We converted all the classes to use the new Singleton instance, pretty much all in one go. It’s important to note that we didn’t really have to do that in this case.
We certainly did want to get to a point where every reference to the player was using the guaranteed real singleton instance. But in this case we also knew that the player wasn’t bouncing around changing all the time. We went to the getInstance approach to avoid having the global variable Runner. As such, we could have allowed caching of the Player instance and changed the files separately, over a period of weeks.
Often when we do a “major refactoring”, that is, one that impacts many classes and methods, we feel that we have to do all the changes in one go. I once saw a team hold up releasing for 44 days while doing a “single refactoring”.
It’s almost never the case that we have to do a sweeping change like that all at once. One common approach would be to implement the new, well-factored way of doing the thing, but to keep the old way working while people change the referring code over time. Sometimes you can even make the old way call the new way, perhaps defaulting some variables that the old users don’t need, and leave the old way forwarding forever.
In my view, all refactorings can be done incrementally. My view is probably wrong. It’s probably possible to come up with some wild-ass refactoring that can’t be done incrementally. But that’s not the way to bet.
My view is close to true. All refactorings, even all features, can be done incrementally–or so close to it that we should assume there’s an incremental way and find it.
Why? Well, first, if you hold up release for 44 days, someone is going to take a large bit out of your tail. Even if they don’t, you’ll be holding up the work, and probably holding up other features that are waiting for you to get out of the way.
Second, our chance of inserting an error into our code is at least linear in the number of lines we change and probably beyond linear. If I make a mistake per ten lies, then if I write a thousand lines, there will be a hundred mistakes in there. Odds are that my tests won’t spot them all, and I’ll have a day like yesterday, chasing my tail, which already has a bite out of it.
If I write ten lines and test them, or even just read them, I may find the bug before I ship. Even if I don’t spot it right away, as long as it get spotted soon, I’ve only got a few lines to check.
Today’s “Heisenbug” is an interesting example. I had seen that thing where the saucer didn’t run before. I’m sure I mentioned it in the articles at least once. Every time I looked for it, it went away, because the code is obviously correct, and usually when I was trying to make it happen, I didn’t kill the bottom-left invader.
Arguably, the bug was due to an optimization: we saved a vector addition on dead invaders. So that was valuable. It does make perfectly good sense not to update a dead invader–unless someone is using the bottom-left invader to know how far they have travelled. We could have done that some other way. We didn’t.
The right test, kill the first alien and see if yPosition is correct, would have found the defect immediately. But what a silly test to write, it would have seemed. Until it wasn’t.
We should write that test, right now. Why? Because the code to update required a comment:
function Invader:update(army)
self.pos = self.pos + army:invaderMotion() -- required for yPosition
if not self.alive then return true end
army:reportingForDuty()
self.picture = (self.picture+1)%2
Shield:checkForShieldDamage(self)
if self:overEdge() then
army:invaderOverEdge(self)
end
return false
end
That tells us that there’s something weird going on in the code. We do have some tests involving invader movement. Let’s see if there’s something there we could adapt.
Here’s an interesting one:
_:test("bomb drops below bottom-most live invader", function()
local army = Army()
local v, vold
vold = army:bombPosition(1)
army:directKillInvader(1)
army.motion = vec2(16,0)
for inv = 1,55 do
army:updateOneLiveInvader()
end
v = army:bombPosition(1)
_:expect(v.x).is(vold.x + 16)
end)
This test is checking the x position of the invaders after a move, to ensure that bombs drop from invader center. It seems clear that this code isn’t referencing yPosition, or it wouldn’t work. Anyway we can write a related test:
_:test("first invader updates even if not alive", function()
local army = Army()
local yOld = army:yPosition()
army:directKillInvader(1)
army.motion = vec2(16,16)
for inv = 1,55 do
army:updateOneLiveInvader()
end
yNew = army:yPosition()
_:expect(yNew-yOld).is(16)
end)
Closing the barn door. But now if I move that line down where it was:
function Invader:update(army)
if not self.alive then return true end
self.pos = self.pos + army:invaderMotion() -- required for yPosition
army:reportingForDuty()
self.picture = (self.picture+1)%2
Shield:checkForShieldDamage(self)
if self:overEdge() then
army:invaderOverEdge(self)
end
return false
end
We get a failure:
22: first invader updates even if not alive -- Actual: 0.0, Expected: 16
Put it back and we’re good.
I’m reminded that that ignored test can be removed because LifeManager doesn’t have the 4 to get 3 lives issue. I removed it yesterday but reverted out.
Tests all green. Commit: added test for yPosition; removed redundant ignored test.
Let’s sum up.
Sum(up)
Well, today went a lot better than yesterday, didn’t it? We didn’t put in our zombie flag logic yet but we did fix the saucer bug rather nicely if I do say so myself, and we found the Heisenbug and fixed that as well. We wrote a new test or two and cleaned up our ignored one so that the tests show green. The game is playable again.
What’s the difference between today and yesterday? I’m surely not any smarter, but as I said at the beginning, I am more experienced.
My experience yesterday made me a bit more careful. It gave me a chance to think a bit more about how to fix the player access problem, which raised the Singleton pattern a bit in my head. although I had considered it yesterday.
I think it probably makes a difference that yesterday I was trying to build a feature and got sidetracked to chase the saucer thing. That was a diversion and I probably was feeling rushed, so I thought I’d just hammer in a change and get back to whatever I “should” be doing.
But no. If I’m going to change hats from “adding feature” to “fixing problem”, I owe it to myself to pause, take off the one hat, check the new hat, and put it squarely on my head, going into “fix the problem mode”.
The truth is, debugging is harder than implementing. It needs to be done more consciously and more carefully, not less.
And the yaks! How many yaks have to appear before you realize you’ve moved into their playground? How many times does the same damn yak come back for another shave, before you decide to stop, revert, regroup.
I suspect that if I had done that the first time I thought of it yesterday, I’d have completed something yesterday instead of nothing.
But that was yesterday. It’s over, and so is today’s session. I’m happy with this one. But I’m not going to forget yesterday’s for a while.
See you next time!