P-235 - Fool Me Twice
Python Asteroids+Invaders on GitHub
There’s a second “timing” defect in Invaders. This one is harder to forgive. Mea culpa. Plus: At least one other defect. And I allowed myself some fun writing this one, too. Same as it ever was.
The Context
The game’s design, which I call “decentralized”, centers1 around a sequence of “events”, three of which are critical to this morning’s observations: begin_interactions, interact, and end_interactions.
- Begin Interactions
- We are about to begin (!) the cycle of interacting each live object with all the others.
- Interact
- Every live object will be given an opportunity to interact with every other object. In such an interaction, the object can check for collisions with the other, make a note of its existence, even note that it never sees the other. Generally speaking, an object concerns itself with itself. It might ask the other for information but generally will not tell it to do anything.
- End Interactions
- Here, the object is informed that interactions are over. Generally, objects do not use this state, but it’s available in case it’s needed. For example, an object might at this point observe that it has not seen any ships and take some action based on there being no ships.
There are other events in the system. The complete sequence is: update, begin_interactions, interact, end_interactions, tick, and draw. These came into being over time, as the design evolved. It seems likely to me that we need fewer than we have, but these seem to make semantic sense.
The First Timing Defect
I don’t recall any timing issues in the Asteroids game, which also uses this framework, though I could just be forgetting. But in the Invaders game the first timing defect was this:
Unlike Asteroids, the Invaders game uses bit masks to help with collision detection. We check first to see if the rectangles containing two possibly-colliding objects intersect, and then if they do, we check to see if the objects have any visible pixels colliding. This provides better collision detection, in particular because the invaders are different sizes and shapes.
Clearly, the mask for an object needs to match the visible shape of the object. Invaders, for example, each have two shapes and therefore have two masks. The visible bits and masks swing into and out of action at the same time, so that collisions always look right.
The game’s shields have a new behavior that our games have not had before: they aren’t just destroyed when hit by a shot, they are damaged. Chunks of them are blown away by the shots. For that to work, the shields all have to have their own masks, and the masks need to be updated whenever a chunk is blasted away from the shield. That’s what we’ve been working on over the past few articles.
The timing defect was this: When the shield detected a shot hitting it, it calculated the damage and edited its visible shape and its mask. Fine, worked perfectly, with an important exception. No matter how I tried to edit the mask, the shot causing the damage did not disappear as it should, but instead tore on down through the shield.
The defect was due to the fact that the interactions are done pairwise via two calls, A versus B and then B versus A. If during A’s calculations about the collision, A modifies itself so that it looks like it wasn’t colliding after all, then when B versus A is called (immediately thereafter), B will not detect a collision and will ignore the call. That is generally bad.
The shield modified the mask to erase the pixels where the shot hit it (and even more pixels to leave a nice mark). So the shot, when it got its turn, checked the masks for intersection and did not see a collision, so it continued on, making a new hole in the shield, carried on, making a new hole in the shield, carried on …
I call this a timing defect, because the change to the mask was made at the wrong time: too soon. The defect was resolved by caching the newly-computed mask and installing it as the official collision mask in end_interactions rather than right during the collision.
I found that defect quite difficult to figure out. I was assuming that I was damaging the mask incorrectly, and learned a lot of ways to damage a mask, some correct and some not. It finally dawned on me, I do not know how, that the code was changing the mask before the other object had a chance to look at it.
Caching the mask until interactions were complete fixed the defect.
The Second Timing Defect
Yesterday I noticed that sometimes when player shots and invader shots collided, an explosion would appear. Other times it seemed that the explosion would not appear, and I’m not sure if it is sometimes or always, the player shot continued on up the screen. (Had I noticed the continuation more consciously, it might have reminded me of the invader shot tearing down through the shield.)
The defect is this: when an invader shot detects that it has collided with a player shot, it removes itself from the mix, as destroyed. It is not supposed to produce a visible effect, it just goes away. The effect comes from the player shot. But there’s a problem …
If the invader shot had just removed itself from the mix, it would have been no problem. The game cycle does not apply insertions and removals until after interactions are over, and in any case the player shot is guaranteed to see the invader shot if the invader shot saw the player shot. But there’s a problem.
The game can only have one instance of each shot type on the screen at a time. The original game worked that way, so that we needed to emulate that behavior. And there’s a problem …
The invader shot, like pretty much all the objects, has a position member, its position on the screen. The position is used to draw it, but also used in interactions. Objects detect collisions when their positions are close enough together. That’s the rectangle-mask logic mentioned above. But you see, there’s a problem …
In my “wisdom”, to indicate that an invader shot is busy, I check its position. In particular, when it becomes available, I set its position to a magic value, ShotController.available, which happens to be (-1, -1), an impossible position. And there, you see, is the problem:
After the invader shot, seeing that it is destroyed, sets itself available, its position has changed—because we used position as a flag(!?!)—and the player shot will not see a collision.
Shame On Me
OK, easily fixed, we can do the wait until end_interactions thing or something. But this defect should never have been allowed to happen at all!
What in the name of all that’s sensible was I thinking that led me to save a boolean member variable “available” by setting a magic value into a position vector? (???) (!?!?) There are three, count them, three invader shots. I “saved” three booleans by mashing an impossible value into position.
And, of course, while I was at it, I broke an unwritten rule that everyone would have expected, that the position of an object would tell us where it was and would contain a valid place to be all the time.
If I had used a separate boolean, this defect would not have happened. And, it seems to me, I really ought to know better.
Now, in my defense,
-
a) I don’t care: I make mistakes all the time and some of them are pretty dumb. I just shake my head, try to learn, and move on, and
-
b) I have decades of experience with computers with small memories, and earned all kinds of clever ways to reuse bits rather than allocate new ones, and sometimes my reflexes get the better of me, and
-
c) It’s more fun for me and my readers when I make a mistake and get to face my lack of perfection, and
-
d) There’s usually a lesson for all of us to learn, which makes it all worth while.
But since a) I don’t care, I need no defense and choose not to offer one. Except wait I already did.
Point is, this defect was due to a mistaken notion of saving a member variable by stashing a magic value in it. Reviewing the article where I did this, I seem to pull the (-1, -1) position out2 almost without comment. In that article, I did note that I was coding beyond the limits of my current test at the time. Never do that, that’s my motto. Anyway, there it is, we know the defect, we see that the “real” fix is probably to have a separate flag. Probably to have a message to send from the controller to the shot to inquire as to its availability.
Let’s try to fix it right, with a new member and a message.
The Code
We have these bits of code:
class ShotController(InvadersFlyer):
max_firing_time = 0x30
available = Vector2(-1, -1)
def fire_specific_shot(self, shot_index, fleets):
shot = self.shots[shot_index]
if shot.position == self.available:
pos = self.select_shot_position(shot, shot_index)
if pos:
shot.position = pos
fleets.append(shot)
self.shot_index = (self.shot_index + 1) % 3
class InvaderShot(InvadersFlyer):
def die(self, fleets):
from shotcontroller import ShotController
self.position = ShotController.available
fleets.remove(self)
We want to change the interaction here to be less invasive, more object-oriented, and, most importantly, to work. Instead of asking the shot for its position and comparing it to our magic value available, we should ask the shot if it is available:
def fire_specific_shot(self, shot_index, fleets):
shot = self.shots[shot_index]
if shot.available:
pos = self.select_shot_position(shot, shot_index)
if pos:
shot.position = pos
fleets.append(shot)
self.shot_index = (self.shot_index + 1) % 3
I am OK with available being a property on the shot rather than a call to is_available. YMMV. Some tests break, not even gonna look. We implement that property:
@property
def available(self):
from shotcontroller import ShotController
return self.position == ShotController.available
The weird import is needed to avoid an import recursion. I think these changes will get rid of that. Tests are green again.
Could commit. Not gonna.
Add a member and use it:
class InvaderShot(InvadersFlyer):
def __init__(self, position, maps):
self.maps = maps
self.masks = [pygame.mask.from_surface(bitmap) for bitmap in self.maps]
self._map = maps[0]
self.map_index = 0
self._rect = self._map.get_rect()
self.rect.center = position
self.count = 0
self.moves = 0
self._available = True
@property
def available(self):
return self._available
Tests break until:
class InvaderShot(InvadersFlyer):
def die(self, fleets):
self._available = True
fleets.remove(self)
class ShotController(InvadersFlyer):
def fire_specific_shot(self, shot_index, fleets):
shot = self.shots[shot_index]
if shot.available:
shot.available = False
pos = self.select_shot_position(shot, shot_index)
if pos:
shot.position = pos
fleets.append(shot)
self.shot_index = (self.shot_index + 1) % 3
I’m becoming less enamored of the use of available as a property. In addition, I think I do not like it so much. The look of that assignment bugs me. It’s just not as meaningful as it might be.
I had to fix one test that was checking the position for -1. I think I should see explosions on shot vs shot now. And I do. Let’s commit this, it’s definitely better. Commit: invader shots use separate flag for available, fixes timing defect.
But I’ve noticed another defect. It is possible to get the game into a state where it fires no more shots. With some experimentation I find that if I kill any entire column of invaders, after each shot fires one last time, no more shots ever come from the invaders.
Let’s review ShotController.
class ShotController(InvadersFlyer):
def fire_specific_shot(self, shot_index, fleets):
shot = self.shots[shot_index]
if shot.available:
shot.available = False
pos = self.select_shot_position(shot, shot_index)
if pos:
shot.position = pos
fleets.append(shot)
self.shot_index = (self.shot_index + 1) % 3
We need look no further. When we find a shot available, we set it unavailable. Then we decide where to drop it from. And if there is no pos we do not drop it … and we do not set it back to available. We need a test but we need a fix even more:
def fire_specific_shot(self, shot_index, fleets):
shot = self.shots[shot_index]
if shot.available:
pos = self.select_shot_position(shot, shot_index)
if pos:
shot.available = False
shot.position = pos
fleets.append(shot)
self.shot_index = (self.shot_index + 1) % 3
Commit: fix defect where empty invader column stopped all shots from dropping.
Reflection
OK, we probably need some tests but I have to say that I would not have thought to test whether shots would drop even if a full column of invaders was absent. That’s just not on my radar. The eminent tester Michael Bolton3 might have thought of that, or perhaps Elizabeth Hendrickson4 or Lisa Crispin]5, or perhaps any of a myriad of testers that I know and love, but not on my radar for sure.
Let’s think about the Shot::ShotController relationship a bit. The controller has three shot instances. It has to have some representation of that three-ness6, because we are only allowed one of each kind of shot on screen at the same time. Arguably reusing the shots is improper, on the grounds that while a shot rightly has a position and behavior falling down the screen, giving it another whole state (available or not) is too much of a burden to put on the poor little thing.
Instead, we might argue, the ShotController should have its own registry of shots and when a new shot is needed, it should create one. ShotController could use the interaction sequence to detect each of its current missiles and when they disappear, could mark them available again.
Even as thing stand, ShotController has some pretty serious feature envy going on. Look at that method above. Let’s imagine that it’s OK to ask about availability. When we do fire the thing, we see two assignments to properties of the shot, available and position.
Now we could argue that yes, a shot controller would in fact program the position of a shot due to be fired, and would in fact throw a switch to turn it on7. But really, we should sent the shot a message, not jam things into it. Let’s do that.
def fire_specific_shot(self, shot_index, fleets):
shot = self.shots[shot_index]
if shot.available:
pos = self.select_shot_position(shot, shot_index)
if pos:
shot.fire_from(pos, fleets)
self.shot_index = (self.shot_index + 1) % 3
Here we tell it what to do rather than mash its members and throw it into the mix. Arguably better, although throwing other objects into the mix is generally OK: it’s how things happen with this design. Anyway, in the the shot:
class InvaderShot(InvadersFlyer):
def fire_from(self, position, fleets):
self._available = False
self.position = position
fleets.append(self)
Now we have the shot managing its own state. We remove the setter on the available flag. Commit: remove a bit of feature envy from ShotController.
We were reflecting. As so often happens, reflection turns to learning, learning turns to action, the lips acquire stains, the stains become a warning … but I digress8.
Reflection Continues
I think it would be “better” if the ShotController created a new shot on every firing. We trust our garbage collector and the fact that our processor is at least a thousand times faster than the one on the original game, and we can afford to program in a more modern style. Then we could remove the available flag from the InvaderShot and that class would be simpler. ShotController would probably be no more complicated than it is now, and would certainly be less coupled to the InvaderShot.
That is a bit of a design change, and more than we want to undertake at this time. Let’s add it to our stories list or make a sticky for it or something. Let’s look at our list:
-
Get one shield on the screen; -
Get all four on the screen; -
Shots ignore shields; -
InvaderShots die upon hitting shields; -
PlayerShots die upon hitting shields; -
InvaderShots do simple damage; -
InvaderShots do fancy damage; -
PlayerShots do simple damage; -
PlayerShots do fancy damage; -
Should there be explosions when shot hits shot? -
Sometimes shot vs shot makes a red shot explosion, sometimes not. Whazzup? - Display invader shot explosion at shield?
- If explosion doesn’t look great, flash it?
- Shouldn’t we have at least SOME tests for all this?
We have so much done that we should start displaying the list in not-done / done order. Maybe next time. Or, we might drop all the shields stories and the spin offs about explosions entirely.
I do like keeping the story list in the articles. It might let me clear the stickies off my keyboard tray. On the other hand, I could put them on cards in a lunch box. That’s the Extreme Programming way to do it.
The household has started to wake up, so I think we’ll summarize and move on to more important matters like breakfast.
Summary
Two “timing” defects have arisen, showing what may be weaknesses in the current decentralized design.
- Some state changes to Flyers can result in subsequent interactions getting bad information;
- Arguably there should be no state changes other than removing or adding to Fleets;
- Arguably there should be no state changes at all, though I wonder how I’d manage motion and animation without state changes.
I do think that a better design principle would have been to do all the funny stuff in end_interactions, but that ship has sailed. We could work to realign things that way but it’s always a bit more complicated that way, and almost never necessary. The only real case in recent memory is the mask caching needed in Shield.
It seems clear that ShotController and InvaderShot are too tightly coupled, and we should address that. I think we’ll find some improvements and perhaps even a deeper learning or two. That remains to be seen.
I think that this code needs special consideration:
def fire_specific_shot(self, shot_index, fleets):
shot = self.shots[shot_index]
if shot.available:
pos = self.select_shot_position(shot, shot_index)
if pos:
shot.fire_from(pos, fleets)
self.shot_index = (self.shot_index + 1) % 3
def select_shot_position(self, shot, shot_index):
if shot_index == 2:
col = self.target_column(self.player_x, self.fleet_x)
else:
col = self.next_column_for(shot_index)
invader = self.invader_fleet.invader_group.bottom_of_column(col)
if invader:
return invader.position
else:
return None
There are a lot of ifs going on in there and the return of a position or None is particularly questionable. I think we could push the actual firing down into the select method. I’m not absolutely certain about this but I think that what happens is that if we happen to choose an empty column to fire from, we skip this attempt, certain that one tick from now we’ll get another column. So it might take a few ticks before we find one but it will happen. That’s far from obvious and I think we can do better.
On the other hand, this is kind of a RORA situation, Runs Once, Run Away. We got it working and thereafter left it alone. It’s better to craft a thing so that we can see that it works, see how it works, even see why it’s done the way it’s done. We usually have to go over an object more than once to get there.
When we build a table, we expect to sand with finer and finer grit. We expect to put multiple coats of paint on things. A really great lawn gets cut in two different directions. We flip our steaks to brown them on both sides. We wipe the crumbs off the table and then wipe again with a damp cloth. We tighten the bolts and then torque them. We lather, rinse, repeat.
Almost everything we do requires repetition and refinement. We do many things over and over, each time making things a bit better. Code is no different. We do well to revise it a few times until it’s what it needs to be. And good programming is difficult. It’s a truly bad idea to imagine that we’re going to get a program, or even a small function, or even a variable name, right the first time.
And, yes, I know. There is pressure to go faster. We’ll succumb to that pressure: the alternative is being shouted at or worse. But still … we do well to improve the code multiple times. Maybe we don’t go hunt it down to improve it, but when we’re working there anyway … we do well to improve the code multiple times.
I find it useful and fulfilling to think when I program. Not just your focus on the problem thinking, no, but a broader look at the overall object and its friends before its too late, watching for the tell-tale signs of corruption. Helps you cultivate horse sense. And a cool head and a keen eye.
It keeps pumping my attention up, keeps me sharp if not genius-level, and it’s fun. It would be even more fun with a pair, a mob or a zoom meeting. But even all by my lonesome, it’s fun and useful. Highly advised, if I gave advice, which I do not.
It has been a good morning, with defect correction, learning, and a rollicking good iced chai. I’ll see you next time!