Asteroids 39
We move saucer and ship in the direction of our new scheme, and get rid of the cloning of the collection. Also took a nice walk.
We’ll begin today by getting rid of those “clone” calls in findCollisions:
function Universe:findCollisions()
-- we clone the collection to allow live editing
for i,a in pairs(clone(self.objects)) do
self:checkMissileCollisions(a)
if self.ship then self:checkShipCollision(a) end
end
if self.ship then
for i,m in pairs(clone(self.objects)) do
self:checkMissileHitShip(m, self.ship)
end
end
end
The concern here was that we encountered an issue with an earlier case of editing a loop while iterating over it. I managed to find a line in Lua law that said you can change the value of a key in a table while running pairs, but you cannot add items. Behavior when adding is “undefined”. In the case in hand, the program crashed.
The rule explicitly says that you can in fact remove a key, which you do by assigning it a value of nil.
The upshot of this is that our deleteObject code is OK, and our addObject calls are not. My plan is to buffer the adds until we’re outside the loop.
Here’s the relevant code as we begin:
function Universe:addObject(object)
self.objects[object] = object
end
function Universe:deleteObject(object)
self.objects[object] = nil
end
function Universe:addSaucer(saucer)
self.objects[saucer] = saucer
self.saucer = saucer
end
function Universe:deleteSaucer(saucer)
self.objects[saucer] = nil
self.saucer = nil
self.saucerTime = self.currentTime
end
function Universe:addShip(ship)
self.objects[ship] = ship
self.ship = ship
end
function Universe:deleteShip(ship)
local f = function()
Ship()
end
Explosion(ship)
self.objects[ship] = nil
self.ship = nil
tween(6, self, {}, tween.easing.linear, f)
end
We’re still using special adds and deletes for Ship and Saucer. We’ll forward those through the generics first:
function Universe:addSaucer(saucer)
self:addObject(saucer)
self.saucer = saucer
end
function Universe:deleteSaucer(saucer)
self:deleteObject(saucer)
self.saucer = nil
self.saucerTime = self.currentTime
end
function Universe:addShip(ship)
self:addObject(ship)
self.ship = ship
end
function Universe:deleteShip(ship)
local f = function()
Ship()
end
Explosion(ship)
self:deleteObject(ship)
self.ship = nil
tween(6, self, {}, tween.easing.linear, f)
end
Test and commit: “ship and saucer use generic add/delete”.
Now we need to put the adds in a buffer, and apply the buffer outside the find loop. We might as well have all the adds use the buffer, so we’ll apply it at the top of draw, with init and application as shown here:
function Universe:draw(currentTime)
self:applyAdditions()
self:adjustTimeValues(currentTime)
...
end
function Universe:applyAdditions()
for k,v in pairs(self.addedObjects) do
self.objects[k] = v
end
self.addedObjects = {}
end
function Universe:addObject(object)
self.addedObjects[object] = object
end
function Universe:init()
self.processorRatio = 1.0
self.score = 0
self.rotationStep = math.rad(1.5) -- degrees
self.missileVelocity = vec2(MissileSpeed,0)
self.frame64 = 0
self.saucerInterval = 2
self.timeBetweenWaves = 2
self.timeOfNextWave= 0
self:defineSounds()
self.objects = {}
self.addedObjects = {}
self.button = {}
self.attractMode = true
end
I expect this to just work. We’ve seen that happen before. Once, maybe. Trepidation aside, it does work. Now to remove the clone calls and the clone function.
function Universe:findCollisions()
for i,a in pairs(self.objects) do
self:checkMissileCollisions(a)
if self.ship then self:checkShipCollision(a) end
end
if self.ship then
for i,m in pairs(self.objects) do
self:checkMissileHitShip(m, self.ship)
end
end
end
Program runs, but a number of tests fail because they’re counting objects. Let’s take a look.
It’s all the tests where we were checking to be sure things were added to the objects collection. Choices are to call applyAdditions in those or remove them. They’re not adding much value but I think it’s better to do the adds and leave them in.
The adds are easily pasted in, the tests and game run. Commit: “added objects buffered, clone removed”.
We’re green. We could release, and it’s not even 0830 yet.
Next, Collision
Our purpose, I vaguely recall, has been to clean up the “technical debt” resulting from our growing understanding of how things might work, compared to how they do work. This is as it should be.
The basic concern was that the drawing of the objects, and the colliding of the objects, was complex and rather tricky. Finding collisions, in particular, was very ad-hoc and specific, find this kind find that kind. That logic was so intertwingled that a day or so ago we discovered an operation that was being done many times when it only needed to be done once.
The design idea, despite all the thinking I’ve done to try to make it harder than it is, is fairly simple:
- Put all the objects into a single collection;
- Loop over all combinations of two objects, and say to one of them,
one:collide(theOther). The two of them would then decide what to do.
Collision cases include:
- Objects that can’t collide at all, like Explosion;
- Objects out of range of each other;
- Objects that destroy anything they collide with;
- Objects that destroy some things they collide with (Asteroids).
My basic design idea is around how each object will implement collide:
- Just return, ignoring all collision possibilities
- Send
theOther:collideWithXXX, where XXX is the type of destructor we are, Missile, Asteroid, etc.
Objects will implement collideWithXXX according to whether they are, or are not, vulnerable to that kind of collision.
The above does not include whose responsibility it is to detect whether objects are in range. It should probably be done as soon as possible, therefore in collide. We could do it in the Universe loop as well, for added efficiency, but to do so we require Universe to know more about the objects in it than we might like. We’ll see how that goes.
The table of what destroys what is interesting. As written now, no object destroys objects of its own type. We can imagine that we might like to allow missiles to shoot down missiles. (We have another issue, which is that the Saucer is a demon at destroying asteroids, so you can just kind of sit around and let him do the work. We will probably change that, which would impact the missile:missile conflict.)
Other than objects not destroying their own kind, we also find that all destruction is mutual. Both objects in conflict are always destroyed. (A destroyed asteroid may split into fragments.)
That means that the table of destruction is symmetric around its major axis. If A destroys B (and itself), then B destroys A (and itself).
It’s possible that we could implement this destruction table directly and use it. For now, I’m treating it as a design document, and plan to implement destruction directly using collide and collideWith messages.
One more thing …
What is it, Columbo?
Well, sir, it seems to me that you had written some tests about this collision stuff. What about those?
Ah, yes. We have two tests:
_:test("missile hits saucer", function()
U = FakeUniverse()
local m = Missile()
local s = Saucer()
m:collide(s)
_:expect(U:destroyedCount()).is(2)
end)
_:test("saucer hits missile", function()
U = FakeUniverse()
local m = Missile()
local s = Saucer()
s:collide(m)
_:expect(U:destroyedCount()).is(2)
end)
The Missile and Saucer have implementations of collide and collideWith:
function Missile:collide(anObject)
anObject:collideWithMissile(self)
end
function Missile:collideWithSaucer(saucer)
U:destroy(self)
U:destroy(saucer)
end
function Saucer:collide(anObject)
anObject:collideWithSaucer(self)
end
function Saucer:collideWithMissile(missile)
U:destroy(self)
U:destroy(missile)
end
The FakeUniverse test double implements destroy but the real universe does not. (It does, however, implement deleteObject, which is not unrelated.)
We have a dangling issue that this raises. Both the ship and the saucer have distinguished member variables in the universe, U.ship and U.saucer. These are used in colliding, which we can ignore in our new scheme, but they are also used in detemining whether to spawn a new ship or saucer, and in other random spots.
We probably need rapid access to the ship and saucer for those non-collision purposes, but I’d like to deal with that later if possible …
Unless … what if Ship and Saucer provided quick access to the single instance that might exist, or returned nil if it didn’t exist? Then that information would at least be encapsulated in the individual classes.
Let’s try that now. Removing those cases will smooth our new collision logic.
New rule: all access to “the saucer” is done by saying Saucer:instance(), which will return “the saucer” or nil if there is none. To accomplish that, we start with this:
Saucer = class()
local Instance;
function Saucer:init()
function die()
self:die()
end
Instance = self
U:addSaucer(self)
self.pos = 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
tween(7, self, {}, tween.easing.linear, die)
end
function Saucer:instance()
return Instance;
end
function Saucer:die()
U:deleteSaucer(self)
Instance = nil
end
I surely should have TDD’d that. Bad Ron. I’ll let it go for now, but we simply must talk about this laxity.
Now to use the instance. We have these references to U.saucer:
function Universe:checkSaucer()
if self.attractMode or self.saucer then return end
if self.currentTime - self.saucerTime > self.saucerInterval then
self.saucerTime = currentTime
Saucer()
end
end
function Universe:addSaucer(saucer)
self:addObject(saucer)
self.saucer = saucer
end
function Universe:deleteSaucer(saucer)
self:deleteObject(saucer)
self.saucer = nil
self.saucerTime = self.currentTime
end
If I fix that top one, I can remove the latter two and have the saucer call generic add and delete:
function Universe:checkSaucer()
if self.attractMode or Saucer:instance() then return end
if self.currentTime - self.saucerTime > self.saucerInterval then
self.saucerTime = currentTime
Saucer()
end
end
function Saucer:init()
function die()
self:die()
end
Instance = self
U:addObject(self)
self.pos = 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
tween(7, self, {}, tween.easing.linear, die)
end
function Saucer:die()
U:deleteObject(self)
Instance = nil
end
There’s also a similar construct in the tests that I’ll ignore for now, as it runs on the FakeUniverse.
This is supposed to work now … wrong again:
Universe:93: attempt to perform arithmetic on a nil value (field 'saucerTime')
stack traceback:
Universe:93: in method 'checkSaucer'
Universe:64: in method 'draw'
Main:12: in function 'draw'
That happens when the first saucer disappears after its time on screen. The problem … U.saucerTime is nil. We see that deleteSaucer set that time, and now no one does. I’m a bit confused about who nils it.
However, this “fixes” it:
function Saucer:die()
U:deleteObject(self)
Instance = nil
U.saucerTime = U.currentTime
end
I don’t understand, and I feel that I should. Yes, that fixed it, but none of the accesses to saucerTime nil it, and it is initialized in startGame. Let’s check all references.
Ah:
function Universe:checkSaucer()
if self.attractMode or Saucer:instance() then return end
if self.currentTime - self.saucerTime > self.saucerInterval then
self.saucerTime = currentTime
Saucer()
end
end
Note the bare reference to currentTime there. It’s nil. Should be self.currentTime. I surely broke that when I factored out checkSaucer and the problem was covered by the saucer delete. It took two mistakes to make that fail. Very rare, that.
Fixed that. Great, now the little beggar goes away and always comes back. Commit: “saucer Instance()”.
Again we could ship this. We are doing a “major refactoring” in tiny steps, with releasable product always in hand.
There have been some issues. Two or three defects have not been detected. That is, I have not detected two or three defects. We will discuss this.
Ship Instance
Now we want to do the same thing with the ship, implement instance() to return “the ship”, and remove the specialized ship adding and deleting that was required.
function Ship:init()
self.pos = vec2(WIDTH, HEIGHT)/2
self.radians = 0
self.step = vec2(0,0)
Instance = self
U:addObject(self)
end
Unlike the saucer, the ship does not have a “die” method. I’ve determined a while back, and I hope I mentioned it, that a convention we want is for all objects that can die to have a die method, which is guaranteed to be called when they die.
In that method, they can do any cleanup that they need, and they should then ask the universe to delete them.
First, I’ll implement that method on the ship and then see who’d better call it:
function Ship:die()
Instance = nil
U:deleteObject(self)
end
OK, who’s calling deleteShip?
function Universe:checkMissileHitShip(missile, ship)
if not missile:is_a(Missile) then return end
if not ship then return end
if missile.pos:dist(ship.pos) < ship:killDist() then
self:deleteShip(ship)
missile:die()
end
end
function Universe:checkShipCollision(asteroid)
if not asteroid:is_a(Asteroid) then return end
if self.ship.pos:dist(asteroid.pos) < asteroid:killDist() then
asteroid:score()
asteroid:split()
self:deleteShip(self.ship)
end
end
If we change those this way:
function Universe:checkMissileHitShip(missile, ship)
if not missile:is_a(Missile) then return end
if not ship then return end
if missile.pos:dist(ship.pos) < ship:killDist() then
ship:die() -- <-----
missile:die()
end
end
function Universe:checkShipCollision(asteroid)
if not asteroid:is_a(Asteroid) then return end
if self.ship.pos:dist(asteroid.pos) < asteroid:killDist() then
asteroid:score()
asteroid:split()
Ship:instance():die() -- <---
end
end
We should be good. Now other accesses to the universe’s ship pointer. Most of them are easy, like this:
function Universe:findCollisions()
for i,a in pairs(self.objects) do
self:checkMissileCollisions(a)
if Ship:instance() then self:checkShipCollision(a) end -- <---
end
if self.ship then
for i,m in pairs(self.objects) do
self:checkMissileHitShip(m, Ship:instance()) -- <---
end
end
end
function Universe:findCollisions()
for i,a in pairs(self.objects) do
self:checkMissileCollisions(a)
if Ship:instance() then self:checkShipCollision(a) end -- <---
end
if Ship:instance() then
for i,m in pairs(self.objects) do
self:checkMissileHitShip(m, Ship:instance())
end
end
end
function Universe:checkShipCollision(asteroid)
if not asteroid:is_a(Asteroid) then return end
if Ship:instance().pos:dist(asteroid.pos) < asteroid:killDist() then
asteroid:score()
asteroid:split()
Ship:instance():die()
end
end
But there is one problem:
function Universe:deleteShip(ship)
local f = function()
Ship()
end
Explosion(ship)
self:deleteObject(ship)
self.ship = nil
tween(6, self, {}, tween.easing.linear, f)
end
This is the thing we do that makes a new ship appear, 6 seconds after the old one dies. We have to move this logic to Ship, which is arguably where it belongs. Only arguably: a case can be made for Universe control over this. But for now, it’ll be the same:
function Ship:die()
local f = function()
Ship()
end
Explosion(self)
U:deleteObject(self)
Instance = nil
tween(6, self, {}, tween.easing.linear, f)
end
I expect this to work as soon as I double-check that I didn’t type capital I Instance anywhere I shouldn’t.
And we’re good to go. Commit: “saucer instance”.
We’ve removed all the specialized adds and deletes and the special save points for ship and saucer. We’ve reduced the number of lines of code substantially.
We’re ready to dive into the new collision scheme itself. The work done so far has had value on its own, but we see more benefits that we’ll harvest soon.
For now, let’s sum up.
Summing Up
Overall, this “new collision” effort has gone smoothly, with a large number of small refactoring steps each leading in the general direction of objects having more autonomy, and more generality in the Universe.
What generality is … and isn’t.
Let me comment on that generality.
There seem to be two ways of adding generality. One of them adds code, and one removes code. Removing code is better.
When we added a new object to the universe, say missiles, we extended Universe to deal with it, by adding specialized code to handle all the special aspects of missiles. Yes, we looked for commonalities, I suppose, and exploited them when we could.
When we were done, we might have said “We generalized the Universe class to handle Missiles”. I suppose from the outside, the universe was more “general” in that it handled more kinds of things.
But inside it was more specialized, handling different things differently.
In the work of the past day or three we’ve done something different. We have removed specialized code, leaving only truly generalized code that handles objects more and more anonymously, without regard to their special aspects.
We’re moving toward a point where the Universe only knows what it really has to about what’s in it, and that’s coming down to very little. We’re nearly at a point of “draw everything, move everything, collide everything, rinse, repeat”.
That’s what good generalization looks like: handling more cases with less code.
Think about that.
But your tests???
OK, so that aspect has been quite good. But what about those defects and tests? There the story still isn’t as I’d like it to be.
I’m used to having such a nice suite of tests that I can press the button and when they run, I’m sure I’m good to go. The tests are a bit stronger now, and I do rely on them a little bit, but I just don’t have that same sense of confidence that I like to have.
What about tests that we coulda shoulda woulda written today? We mostly just removed the ship and saucer instances off to their classes, and removed their special handling in Universe. Should we have written tests for that?
Yes and no. Yes, because our tests aren’t very good. No, because if the tests were good, the changes we made should have made little or no difference to the tests. If they ran, our changes were good, if not, our changes weren’t good.
So it was “natural” not to write any more tests, because if the world were as I’m more used to, there would already be “enough”.
If. Big IF. I’ve had trouble seeing how tests might work in this game, and the result is that when I make changes, instead of pressing a test button and seeing a bunch of tests that tell me the game works as intended, I run the tests if I remember, and then run the game to see what “really” happens.
And that can take a lot of time. It seems that if I want to test whether the saucer can kill the ship, I have to wait for several minutes until the random so-and-so finally hits me. And when I want to test something else, the spawny git hits me right away.
Manual testing is like that. It takes longer and longer the more you use it. And when something does go wrong, you’ve seen the kind of messages Codea gives me. Nil object at some line, thanks a lot, how did it get that way? When a microtest fails, you know exactly what went wrong and you go and fix it then and there.
So. What I’d like to be learning and showing about testing, is how nice it is when you have them. And, since I really don’t know how to do decent testing of a game like this, I had hoped to learn.
So far, that objective has not been met.
That said, we have a nicely improving code base, supporting an interesting and fun game, and the code base is getting better, not worse.
Not everyone can make that claim, but we can.
Thanks for your help. See you next time!