Asteroids 58
I feel like reviewing some code. What I learn? Kill your darlings.
Our original objective, something about replicating the old Asteroids program, is pretty well met. There may be some unintentional differences (and there are a few intentional ones) but it would take an expert to spot many of them.
This is a good time to review the code, see what looks good and what could use improvement – and maybe even improve some of it if we see opportunities.
There will be a lot of code in this article. I think I’ll mostly show sensible snippets of what I observe, rather than the complete contents of all the tabs. You can, of course, see all the code in the zipped versions that accompany the articles.
I’ll start with Main.
Nothing much to see here, it looks pretty good to me. There is this:
function draw()
U:draw(ElapsedTime)
if U.attractMode then
pushStyle()
fontSize(50)
fill(255,255,255, 128)
text("TOUCH SCREEN TO START", WIDTH/2, HEIGHT/4)
text(Console, WIDTH/2, HEIGHT - 200)
popStyle()
end
end
You could make a case for extracting the attract mode bits. And someday I’d like the test console to fade out, leaving just the touch screen to start bit. But nothing worth refactoring here.
The next tab is Destructible, the base class for objects that can be destroyed. It looks good to me. I’m sure this won’t continue indefinitely or this will be a very short and particularly boring article.
Ship
Skipping the tests for now, the next candidate is Ship.
Here things start getting a bit more messy.
function Ship:drawAt(pos,radians)
local sx = 10
local sy = 6
pushStyle()
pushMatrix()
translate(pos.x, pos.y)
rotate(math.deg(radians))
strokeWidth(1)
stroke(255)
scale(self.scale or 2)
line(-3,-2, -3,2)
line(-3,2, -5,4)
line(-5,4, 7,0)
line(7,0, -5,-4)
line(-5,-4,-3,-2)
accel = (accel+1)%3
if U.button.go and accel == 0 then
strokeWidth(1.5)
line(-3,-2, -7,0)
line(-7,0, -3,2)
end
popMatrix()
popStyle()
end
Particularly interesting are sx and sy up there. What’s that about? I don’t see any references to them. And what about accel, what’s that? We have to look around a little for that one:
local accel = 0
function Ship:draw()
self:drawAt(self.pos, self.radians)
end
The purpose of this is to display the little flame bit that shows up during acceleration, only one out of three times. It needs to be external to the draw because it has to persist over multiple draw cycles. It probably should be a member variable of ship, rather than whatever it’s trying to be here.
That’s three things that need some changing, I’m going to do a little refactoring here.
Wow. Changed accel to a member variable (I’ll show the code below) and this happened:
Ship:39: attempt to perform arithmetic on a nil value (field 'accel')
stack traceback:
Ship:39: in method 'drawAt'
Score:21: in method 'draw'
Universe:105: in method 'drawEverything'
Universe:65: in method 'draw'
Main:28: in function 'draw'
Here’s the code:
function Ship:init(pos)
self.pos = pos or vec2(WIDTH, HEIGHT)/2
self.radians = 0
self.step = vec2(0,0)
self.scale = 2
self.accel = 0
Instance = self
U:addObject(self)
end
function Ship:draw()
self:drawAt(self.pos, self.radians)
end
function Ship:drawAt(pos,radians)
pushStyle()
pushMatrix()
translate(pos.x, pos.y)
rotate(math.deg(radians))
strokeWidth(1)
stroke(255)
scale(self.scale or 2)
line(-3,-2, -3,2)
line(-3,2, -5,4)
line(-5,4, 7,0)
line(7,0, -5,-4)
line(-5,-4,-3,-2)
self.accel = (self.accel+1)%3 -- <--- line 39
if U.button.go and self.accel == 0 then
strokeWidth(1.5)
line(-3,-2, -7,0)
line(-7,0, -3,2)
end
popMatrix()
popStyle()
end
I don’t see how that can possibly go wrong. it has to mean that the second reference, the one inside the parens, found a nil accel. We could lazy init it there but I want to know what happened. Surely there’s no way to make a ship without going through init?
This took me longer to work out than it should have much longer. The problem is in Score:
function Score:draw()
local s= "000000"..tostring(self.totalScore)
s = string.sub(s,-5)
pushStyle()
fontSize(100)
text(s, 200, HEIGHT-60)
for i = 1,self.shipCount do
Ship:drawAt(vec2(330-i*20, HEIGHT-120), math.pi/2)
end
if self.gameIsOver then
text("GAME OVER", WIDTH/2, HEIGHT/2)
end
popStyle()
end
Score uses the ship’s drawAt function, but it isn’t drawing a ship instance. Until now, that didn’t matter, because there were no references to member variables inside drawAt. Now there are.
We knew that was odd and we thought we could get away with it. We did, for a while. The fix is for Score to have a private copy of the ship to draw, or live with this rule.
Fixing this is beyond our scope, but we definitely should fix this or it may bite us again. For now, make a card, revert to fresh, and carry on.
Well. That took the wind right out of my sails. Let’s continue our look at Ship though.
function Ship:enterHyperspace()
local appear = function()
if self:safeToAppear() then
U:addObject(self)
self.scale = 10
tween(1, self, {scale=2})
else
self:signalUnsafe()
tween.delay(3, self.hyperReturn)
end
end
U.objects[self] = nil
local w = math.random(WIDTH-200)
local h = math.random(HEIGHT-300)
self.pos = vec2(w,h) + vec2(100,200)
self.hyperReturn = appear
tween.delay(3,appear)
end
This is a bit intricate, mostly because of the need to have that nested function. We need that because it has to be a closure, bound to the current value of self. But there’s more.
U.objects[self] = nil
isn’t right. That should be
U:deleteObject(self)
The next few lines are setting our position inset by a bit. We’ll be centered in width, but we’ll be above center in height by 100 pixels. Or 50. 50, I guess. Assuming I even understand that code.
What do we want? We want a safe emergence zone, which is inset on the sides by 100, inset from the top by 100 and inset from the bottom by 200. Couldn’t we just say something like
randomPointIn(100,200, WIDTH-100, HEIGHT-100)
I’m scared to refactor. That acceleration thing scared me. I can feel it.
Let’s do it:
function Ship:randomPointIn(x1, y1, x2, y2)
local widthRange = x2 - x1
local heightRange = y2 - y1
local width = math.random(widthRange)
local height = math.random(heightRange)
return vec2(x1,y1) + vec2(width,height)
end
With:
function Ship:enterHyperspace()
local appear = function()
if self:safeToAppear() then
U:addObject(self)
self.scale = 10
tween(1, self, {scale=2})
else
self:signalUnsafe()
tween.delay(3, self.hyperReturn)
end
end
U:deleteObject(self)
self.pos = self:randomPointIn(100,200, WIDTH-100, HEIGHT-100)
self.hyperReturn = appear
tween.delay(3,appear)
end
Slightly better. Commit: “refactor hyperspace entry”.
What else?
function Ship:actualShipMove()
if U.button.go then
U:playStereo(U.sounds.thrust, self)
local accel = vec2(0.015,0):rotate(self.radians)*U.processorRatio
self.step = self.step + accel
self.step = maximize(self.step, 6)
else
self.step = self.step*(0.995^U.processorRatio)
end
self:finallyMove()
end
First, I generally prefer the smaller of two branches of an if to be first. Second, arguably, the entire contents of the larger branch could be pulled out into something about handling acceleration. We might then notice that we have two things going on in there, one being setting acceleration and the other being turning on the sound. Arguably those do not belong together. We could conceivably put the sound into its own if or into the button.
Not worth doing now, but I’ll make a note of it.
You know what, though? Making this list is just blessing inferior code and sweeping it under the carpet for later. What we’re seeing here is the essential problem with pure code review. We will surely find things to complain about, but nothing really gets better.
We should undertake this review differently. We should review the code with the intention of fixing things that pop out as needing fixing, and flat ignoring things that don’t rise to the level of really being “worth it” to improve.
What about things that clearly need improvement, but are too big to undertake right now? We might make a list of those. The weird ship drawing rises to that level, I think, but the code here, we should fix it or ignore it.
I vote ignore.
Time for a break. Then another tab.
Score
I popped out to pick up a Wendy’s spicy chicken sandwich for lunch, and thus fortified, I propose to fix the problem between Score and Ship. My cunning plan is to create an unregistered ship for Score to use. We could do that in Score but it is best to create special objects in the class they are, not some other class.
I should say, “my cunning plan was”. No sooner did I start this simple set of changes than did I notice that sometimes, when the saucer goes away, its sound does not. I’ve dug around a lot, set asserts to see what is going on. Certainly it has to do with that intricate sound-repeating code:
function Saucer:startSound()
local play = function()
U:playStereo(self.sound, self)
self.sounder = tween.delay(self.soundDelay, self.playFunction)
end
self.playFunction = play
self.soundDelay = 0.2
self.sound = self:selectSound()
U:playStereo(self.sound, self)
self.sounder = tween.delay(self.soundDelay, self.playFunction)
end
function Saucer:stopSound()
tween.stop(self.sounder)
end
I’ve checked to be sure there isn’t a value in sounder before I set a new value into it. There really can’t be a live one inside here: the only way we get inside here is when the sounder delay has run out, and we’ve called play.
There’s a long delay between a saucer timing out or hitting something, and the next saucer rezzing. So the sounder should have been killed here:
function Saucer:dieQuietly()
self:stopSound()
U:deleteObject(self)
Instance = nil
U.saucerTime = U.currentTime
end
How could this happen? If there is a tween running on soundDelay, then it would trigger into play, and it would repeat. But it would set the new tween into sounder. That could hide one that was in there legitimately.
This is way hard to think about. It argues against using tween for anything this complicated.
I’ve smashed my head against this long enough. I’m going to reimplement the sound without tweens.
This is most distressing, but there’s a lesson to learn. I’ll try to learn it once this works.
I’ll keep sound and soundDelay, but use them in a timer. I shouldn’t need stopSound at all: when the saucer exists, it will tweet and when it doesn’t, it won’t, because it isn’t there.
function Saucer:init(optionalPos)
function die()
if self == Instance then
self:dieQuietly()
end
end
Instance = self
U:addObject(self)
self.size = Score:instance():shouldDrawSmallSaucer() and 0.5 or 1
self.shotSpeed = 3
self.firstShot = true
self.pos = optionalPos or vec2(0, math.random(HEIGHT))
self.step = vec2(2,0)
self.fireTime = U.currentTime + 1 -- one second from now
if math.random(2) == 1 then self.step = -self.step end
self:setRandomTurnTime()
self:startSound()
tween.delay(7, die)
end
function Saucer:startSound()
self.soundDelay = 0.2
self.sound = self:selectSound()
self:playSound()
end
function Saucer:playSound()
U:playStereo(self.sound, self)
self.nextSoundTime = U.currentTime + self.soundDelay
end
This works just fine and is way less intricate. Commit: remove saucer sound tween.
Summing Up
I’ve left the commentary here until the next morning (July 1), partly because this problem really put the screws to me, and partly to get some distance from it before commenting.
I’ve spent literally hours trying to figure out how this tween defect fails, and come up with nothing. There are only two possibilities that I can see.
First, it is conceivable that tween.stop sometimes fails and the tween triggers again anyway. That would create a second tween on top of the one that was running, and once that happened, the function that restarts the tween would happily start a new one for every one that ever stopped.
The other possibility that I can see is that somehow the saucer gets destroyed without closing out the tween and then, again, there is a tween running freely.
I put asserts and prints all around the code, and never caught it doing anything bad. But I have learned never to believe there’s a bug in the compiler or operating system just because I can’t find the bug in my program. There could be: it’s just not the way to bet.
And if there were a defect in Codea, it should be possible to create a program that causes it to happen reliably, and I can’t see how to do that.
The bottom line, however, is that that self-perpetuating tween was too complex to be allowed to live. Oh, I was proud of it. Making it work involved a deep understanding of some pretty intricate parts of Codea.
Kill your darlings
“Kill your darlings” is a famous bit of advice in writing. The “darling” is a piece of your writing that you are very attached to, so attached that you can’t assess it objectively. When the scene or sentence keeps giving you trouble making it fit or working around it, but you love it so much that you can’t bear to get rid of it … kill your darlings.
The idea applies to our code as well, and perhaps more powerfully. It is very likely that when another programmer, or even this programmer next year, ran across that code, they’d have difficulty seeing how it worked, difficulty being certain that it even did work. They’d certainly have difficulty seeing that while it seems to work, in fact, sometimes, it doesn’t.
“Kill your darlings” says to get rid of that code. That’s what we did just now, and the new code is far simpler, far easier to understand, and it’s much more obvious that it has to work.
The old version had a separate “task” running, the tween, that had to be killed with certainty when the saucer died, for any of the reasons the saucer can die. The new version clearly only runs if the saucer is alive, and certainly runs when it is.
Look at them:
Old Way
function Saucer:startSound()
local play = function()
U:playStereo(self.sound, self)
self.sounder = tween.delay(self.soundDelay, self.playFunction)
end
self.playFunction = play
self.soundDelay = 0.2
self.sound = self:selectSound()
U:playStereo(self.sound, self)
self.sounder = tween.delay(self.soundDelay, self.playFunction)
end
function Saucer:stopSound()
tween.stop(self.sounder)
end
New Way
function Saucer:startSound()
self.soundDelay = 0.2
self.sound = self:selectSound()
self:playSound()
end
function Saucer:playSound()
U:playStereo(self.sound, self)
self.nextSoundTime = U.currentTime + self.soundDelay
end
function Saucer:move()
if self.turnTime < ElapsedTime then
self:randomTurn()
end
U:moveObject(self)
if U.currentTime >= self.fireTime then
U:playStereo(U.sounds.saucerFire, self)
self.fireTime = U.currentTime + 0.5
self:fireMissile()
end
if U.currentTime > self.nextSoundTime then
self:playSound()
end
end
Even though it’s spread over three methods, the new way is easy to understand:
- We start the sound by setting up an interval at which to play the sound and paying the sound once.
- We play the sound once and set to the next interval.
- When the saucer moves, if it’s time, it plays the sound.
It’s pretty clear: no saucer, no sound. Saucer, sound.
I do think that this approach to timed events could be cleaner. There is a duplication of the pattern “current time bigger than time to do the thing”, and that suggests that there may be a way to reduce that duplication making timing easier, more consistent and more clear. Right now I don’t see that way, but this way is small, consistent, and it’s pretty easy to convince ourselves that it works. We could even add in another timed event without feeling too scared.
“Kill your darlings” would have been good advice for me here. I even thought about it at one point. If you’d been here, you’d have suggested that that trick was just too cool.
I still think it should work. If I knew why it didn’t work, I’d be tempted to use it, even though I know better.
There is another copy of this pattern in the system, in the hyperspace emergence code. So far, I haven’t caught that one failing.
Not exactly a great recommendation.
Kill your darlings. We’ll see about killing that one real soon now.