Do Not Like
Python Asteroids+Invaders on GitHub
I think that I don’t quite like how the Invaders deal with the edge of known space. Let’s explore. Result: I am pleasantly surprised.
This series is not going as I anticipated. Life is not going as I anticipated, either, so perhaps this series is a symptom of something larger. Be that as it may, today I want to look at how the invaders and the bumpers interact.
The overall design of this system revolves around a single notion: any two objects in the universe will be given a chance to interact on each game cycle. They can use that interaction for something simple, such as detecting that they have collided, or they can do something a bit more sophisticated, such as count things during interactions and take action based on the count after interactions are over.
That notion, once you get it in your head, is rather easy to work with. Overall, I rather like it. But the invaders make things more difficult.
The invaders are arranged in rows and columns, and when they move, they move in a sort of rippling fashion, because in each game cycle, just one invader moves at a time. In view of this, the game has a sort of sub-fleet, the InvaderFleet, which handles the cycling, and, using a helper object the InvaderGroup, moves the invaders one at a time. So far, not so bad.
When the rack of invaders gets far enough to one side of the screen, they reverse direction and march back across the other way, ad infinitum. (They move a bit downward on each reversal. Scary!) Because the invaders are in columns and rows, or perhaps rows and columns, in a given full cycle of their motion, more than one invader will typically hit the boundary. But we do not want to reverse until the whole rack has moved. And that is where it starts to get tricky.
There are two Bumper objects that help with this, one on the left, and the other, perhaps not surprisingly, on the right. When the InvaderFleet gets a call to its interact_with_bumper, it sends interact_with_bumper to the InvaderGroup, which does this thing, part of which I do not like:
class InvaderGroup:
def interact_with_bumper(self, bumper, _fleet):
if self._next_invader < len(self.invaders):
return
colliding = [invader.is_entering(bumper, self.current_direction) for invader in self.invaders]
self.should_reverse |= any(colliding)
This method just returns unless we have moved the last invader. (index == len) If we have moved the last invader, this method creates a list comprehension of True/False values, one for each existing invader, saying whether that invader is entering the bumper in question.
Here’s part of the rub: the concern here was that invaders might move onto the bumper while moving toward it, which should signal to reverse direction, but that they might be found still hitting the bumper while cycling back out.
As I look at this code now, that reversal concern cannot happen: we only check after all invaders have moved, so however many moved in last time will have moved out next time.
There is another issue, however, which is that if we are moving right we should only concern ourselves with the right hand bumper and if moving left, the left hand one.
- Note
- This is not going as I thought it would. As I mused this morning, as I am wont to do, I was concerned about the fact that we aren’t checking the interaction of bumper and invader with a Sprite, instead using this
is_enteringmethod. I was thinking that I’d refactor Bumper to use a Sprite. -
But now, I think this code can be made simpler, and that we should therefore make it simpler and then see what else it needs.
The simplifications I [vaguely] see include:
- Check the bumper here in the Group and don’t process it if it isn’t the one we’re approaching;
- Simplify the
is_enteringto ignore the direction, since given that we’re checking only at the end of an invader move cycle, the edge ones will all be either in, in which case reverse, or out, in which case, we won’t reverse. - In fact, since we will have reversed, if we process only the bumper we’re approaching, we won’t even check the one we’re near.
Let’s do those. I have concerns over tests, but we’ll let them break and see what they tell us. If any break at all.
def interact_with_bumper(self, bumper, _fleet):
if self._next_invader < len(self.invaders):
return
if self.current_direction != bumper.incoming_direction:
return
colliding = [invader.is_entering(bumper, self.current_direction) for invader in self.invaders]
self.should_reverse |= any(colliding)
There’s thing one. This code could use improvement. We’ll look at that. We can commit this, so let’s do: Only check the bumper we’re approaching.
Now let’s rename is_entering to colliding. (We should rename all our colliding method to is_colliding. Maybe later.) Ewww, we get a warning. Leave the name for now. colliding is defined in Spritely, and in fact we’d like it to work. We’ll try to get there. For now, we’ll explore is_entering:
class Invader(Spritely):
def is_entering(self, bumper, current_direction):
return bumper.am_i_entering(self._sprite, current_direction)
class Bumper(InvadersFlyer):
def am_i_entering(self, rect, direction):
return self.intersecting(rect) and direction == self.incoming_direction
We should have been checking direction here first: it would have been faster. Not that we care, our computer is much faster than the old Invaders game. Heck, the processor in my watch is much faster. Anyway:
class Bumper(InvadersFlyer):
def am_i_entering(self, rect):
return self.intersecting(rect)
class Invader(Spritely):
def is_entering(self, bumper):
return bumper.am_i_entering(self._sprite)
class InvaderGroup:
def interact_with_bumper(self, bumper, _fleet):
if self._next_invader < len(self.invaders):
return
if self.current_direction != bumper.incoming_direction:
return
colliding = [invader.is_entering(bumper) for invader in self.invaders]
self.should_reverse |= any(colliding)
We have simplified the signatures right up and down. Commit: simplify invader-bumper checking.
- Confession
- In testing, I found a fatal crash-causing defect that had reached prod. There was a reference left to
rectin Invader and it caused the game to crash when the Player fired a shot. -
I am not sure when I broke that, sometime yesterday, but it’s a serious mistake when a crash gets to prod. I can only hope that someone played the game at lunch time and stopped the deployment. But seriously, This was a major blow to my pride. Fortunately, my pride, high or low though it may be, has a large Armor Class, 18 with a +4 against stupid programming attacks, so my pride, such as it is, remains about where it was.
-
Seriously though, not good. The bug was in the calculation of the stereo position for the shot sound, so it was a bit obscure but a simple run of the game would have found it.
The core issue is that rect was returning None, which is not a rectangle. I had changed Spritely to return None from rect as part of trying to get rid of it. Let’s just not do that.
class Spritely:
@property
def rect(self):
return self.sprite.rectangle
Harmless and safer. No biscuit for Ron today. Commit: return sensible rectangle from rect.
Now. Where we we? Oh, right, that weird way that we check for hitting the bumper. Whether we call it colliding or not, that’s what it’s trying to do. I was really thinking that I’d like to use a Sprite here, but to do so would require a very long thin surface to mask against, because Sprite collision checks bits in the mask. We could do that, but it seems artificial.
Perhaps the whole darn thing is artificial?
The universal constants file includes these two:
BUMPER_LEFT = 64
BUMPER_RIGHT = 960
Would it be all that terrible if the InvaderGroup just checked all the invaders to see if any were past those values?
Currently, the bumper does the checking like this:
def am_i_entering(self, rect):
return self.intersecting(rect)
def intersecting(self, sprite: Sprite):
return self.beyond_on_right(sprite) if self.incoming_direction > 0 else self.beyond_on_left(sprite)
def beyond_on_left(self, sprite):
return sprite.topleft[0] <= self.x
def beyond_on_right(self, sprite):
return sprite.topright[0] >= self.x
Note that we check the nearest edge at the moment. We may find it necessary to tweak some numbers here, but I think it might be worth it. It would certainly simplify Bumper. We could quite likely get rid of it, or make it inactive.
Let’s first change the Bumper to use the centerx of the invader rather than her corners.
def beyond_on_left(self, sprite):
return sprite.centerx <= self.x
def beyond_on_right(self, sprite):
return sprite.centerx >= self.x
A couple of tests fail. That’s good, what are they?
def test_bumper_intersecting_left(self):
bumper = Bumper(64, -1)
rect = Rect(64, 512, 64, 32)
assert bumper.intersecting(rect)
rect = Rect(65, 512, 64, 32)
assert not bumper.intersecting(rect)
def test_bumper_intersecting_right(self):
bumper = Bumper(960, +1)
rect = Rect(960-64, 512, 64, 32)
assert bumper.intersecting(rect)
rect = Rect(959-64, 512, 64, 32)
assert not bumper.intersecting(rect)
THey’re not really testing anything we are going to care about. But we won’t have a test for the new scheme unless we write one now.
Let me sketch what I propose to do, and then figure out how to test-drive it.
We want to change this:
class InvaderGroup:
def interact_with_bumper(self, bumper, _fleet):
if self._next_invader < len(self.invaders):
return
if self.current_direction != bumper.incoming_direction:
return
colliding = [invader.is_entering(bumper) for invader in self.invaders]
self.should_reverse |= any(colliding)
Well, maybe not: we should still ask the invader if she is past the edge. So perhaps we want to change this method, including its name:
class Invader(Spritely):
def is_entering(self, bumper):
return bumper.am_i_entering(self._sprite)
I think we want to ask each invader if it is out of bounds and if it is, we’ll reverse.
So the group will say something like this:
def interact_with_bumper(self, bumper, _fleet):
if self._next_invader < len(self.invaders):
return
if self.current_direction != bumper.incoming_direction:
return
colliding = [invader.is_out_of_bounds(left, right) for invader in self.invaders]
self.should_reverse |= any(colliding)
Hah, so that’s easy to TDD.
def test_invader_bounds(self):
maker = BitmapMaker.instance()
sprite = Sprite(maker.invaders)
invader = Invader(1, 1, sprite)
invader.position = Vector2(100, 500)
assert not invader.is_out_of_bounds(90, 1000)
The numbers don’t matter, since we’ll be providing them. Now I think I’ll just use the centerx and adjust things if I don’t like the picture.
def is_out_of_bounds(self, low, high):
return self.sprite.centerx < low or self.sprite.centerx > high
Expand the test to match the code, I got a bit carried away there.
def test_invader_bounds(self):
maker = BitmapMaker.instance()
sprite = Sprite(maker.invaders)
invader = Invader(1, 1, sprite)
invader.position = Vector2(100, 500)
assert not invader.is_out_of_bounds(90, 1000)
assert invader.is_out_of_bounds(110, 1000)
assert invader.is_out_of_bounds(80, 90)
So that’s just fine. Commit: invader can check in bounds.
The invaders are reversing only after their center gets to the line. I did change them to check centerx, so that is expected, but I probably shouldn’t have committed. Anyway we want to check against narrower bounds than those in u. Let’s use our new method:
def interact_with_bumper(self, bumper, _fleet):
if self._next_invader < len(self.invaders):
return
if self.current_direction != bumper.incoming_direction:
return
colliding = [invader.is_out_of_bounds(u.BUMPER_LEFT + 32, u.BUMPER_RIGHT - 32) for invader in self.invaders]
self.should_reverse |= any(colliding)
32 is half an invader width. Constant needs improvement. Also, we no longer need the bumpers at all, I think.
Commit this, as it is working as needed: use is_out_of_bounds for invader reversal.
Move the 32 over to u:
def interact_with_bumper(self, bumper, _fleet):
if self._next_invader < len(self.invaders):
return
left = u.BUMPER_LEFT + u.INVADER_HALF_WIDTH
right = u.BUMPER_RIGHT - u.INVADER_HALF_WIDTH
colliding = [invader.is_out_of_bounds(left, right) for invader in self.invaders]
self.should_reverse |= any(colliding)
We make handy variables for communication and making the line shorter. We no longer check bumper direction. And in fact we really don’t need to be calling this on bumpers.
We can commit this: simplify interact_with_bumper.
Now when should we really check this? We should just call it at the end of a move cycle.
Here’s the invader motion code:
class InvaderFleet(InvadersFlyer):
def update(self, delta_time, _fleets):
result = self.invader_group.update_next(self.origin, self.direction)
self.process_result(result)
def process_result(self, result):
if result == CycleStatus.CONTINUE:
pass
elif result == CycleStatus.NEW_CYCLE:
self.step_origin()
elif result == CycleStatus.REVERSE:
self.reverse_travel()
class InvaderGroup:
def update_next(self, origin, current_direction):
self.handle_direction_change(current_direction)
return self.perform_update_step(origin)
def handle_direction_change(self, current_direction):
if self.current_direction != current_direction:
self.current_direction = current_direction
def perform_update_step(self, origin):
if self._next_invader < len(self.invaders):
self.move_one_invader(origin)
return CycleStatus.CONTINUE
else:
return self.end_cycle()
def move_one_invader(self, origin):
invader = self.next_invader()
invader.move_relative_to(origin)
self._next_invader += 1
def end_cycle(self):
self._next_invader = 0
return CycleStatus.REVERSE if self.should_reverse else CycleStatus.NEW_CYCLE
I think we can just check at the end of the cycle and ignore all the bumper stuff. Let’s try that. I consider this a bit of a spike but I expect it to work and to be good enough. But I am prepared to be wrong and to need to revert.
Extract Method:
class InvaderGroup:
def interact_with_bumper(self, bumper, _fleet):
if self._next_invader < len(self.invaders):
return
self.need_to_reverse()
def need_to_reverse(self):
left = u.BUMPER_LEFT + u.INVADER_HALF_WIDTH
right = u.BUMPER_RIGHT - u.INVADER_HALF_WIDTH
colliding = [invader.is_out_of_bounds(left, right) for invader in self.invaders]
self.should_reverse |= any(colliding)
Use it:
def end_cycle(self):
self._next_invader = 0
return CycleStatus.REVERSE if self.need_to_reverse() else CycleStatus.NEW_CYCLE
A test fails, probably checking the reversal flag. Also the game does not work: the invaders just march right off screen.
That’s because I didn’t return the flag:
def need_to_reverse(self):
left = u.BUMPER_LEFT + u.INVADER_HALF_WIDTH
right = u.BUMPER_RIGHT - u.INVADER_HALF_WIDTH
colliding = [invader.is_out_of_bounds(left, right) for invader in self.invaders]
self.should_reverse |= any(colliding)
return self.should_reverse
This needs a revert and do over. Bite the belt and do it.
Let’s extract a different method. First extract variable:
def interact_with_bumper(self, bumper, _fleet):
if self._next_invader < len(self.invaders):
return
left = u.BUMPER_LEFT + u.INVADER_HALF_WIDTH
right = u.BUMPER_RIGHT - u.INVADER_HALF_WIDTH
colliding = [invader.is_out_of_bounds(left, right) for invader in self.invaders]
any_are_colliding = any(colliding)
self.should_reverse |= any_are_colliding
And now:
def interact_with_bumper(self, bumper, _fleet):
if self._next_invader < len(self.invaders):
return
any_are_colliding = self.any_out_of_bounds()
self.should_reverse |= any_are_colliding
def any_out_of_bounds(self):
left = u.BUMPER_LEFT + u.INVADER_HALF_WIDTH
right = u.BUMPER_RIGHT - u.INVADER_HALF_WIDTH
colliding = [invader.is_out_of_bounds(left, right) for invader in self.invaders]
any_are_colliding = any(colliding)
return any_are_colliding
And inline:
def any_out_of_bounds(self):
left = u.BUMPER_LEFT + u.INVADER_HALF_WIDTH
right = u.BUMPER_RIGHT - u.INVADER_HALF_WIDTH
colliding = [invader.is_out_of_bounds(left, right) for invader in self.invaders]
return any(colliding)
And use in end_cycle:
def end_cycle(self):
self._next_invader = 0
return CycleStatus.REVERSE if self.any_out_of_bounds() else CycleStatus.NEW_CYCLE
This last change breaks three tests. Not very surprising, but what are they?
def test_no_reversal(self):
group = InvaderGroup()
bumper = Bumper(u.BUMPER_RIGHT, +1)
group.interact_with_bumper(bumper, None)
result = group.end_cycle()
assert result == CycleStatus.NEW_CYCLE
def test_reversal_on_entry(self):
group = InvaderGroup()
bumper = Bumper(u.BUMPER_RIGHT, +1)
invader = group.invaders[0]
_pos_x, pos_y = invader.position
invader.position = (u.BUMPER_RIGHT, pos_y)
group.testing_set_to_end()
group.begin_interactions(None)
group.interact_with_bumper(bumper, None)
assert group.should_reverse
result = group.end_cycle()
assert result == CycleStatus.REVERSE
# continuing the story ...
origin = (0, 0)
result = CycleStatus.CONTINUE
group.begin_interactions(None)
while result == CycleStatus.CONTINUE:
invader.position = (u.BUMPER_RIGHT, pos_y)
result = group.update_next(origin, -1)
assert result == CycleStatus.NEW_CYCLE
def test_no_reversal_on_exit(self):
group = InvaderGroup()
group.current_direction = -1
bumper = Bumper(u.BUMPER_RIGHT, +1)
invader = group.invaders[0]
_pos_x, pos_y = invader.position
invader.position = (u.BUMPER_RIGHT, pos_y)
group.interact_with_bumper(bumper, None)
result = group.end_cycle()
assert result == CycleStatus.NEW_CYCLE
This will be a [REDACTED] to fix. I’m going to take the position that the bounds thing is checked and that this doesn’t need a test. That is probably wrong, but for now, I’m going to push on. I comment out those tests. Make fun of me if I don’t at least somewhat replace them.
We don’t want the bumper check to do anything now.
def interact_with_bumper(self, bumper, _fleet):
pass
See if the game works. In fact it does. We’re on a good path but let’s not commit until we’ve cleaned this up and fixed up some testing.
We no longer need the should_reverse flag. Remove all references, one in init and one in begin_interactions, which can now be removed.
We no longer need interact_with_bumper remove that and the calls. I could remove the send of the message from Bumper, but in fact I think we’ll remove Bumper itself in a moment.
I notice this odd code:
def update_next(self, origin, current_direction):
self.handle_direction_change(current_direction)
return self.perform_update_step(origin)
def handle_direction_change(self, current_direction):
if self.current_direction != current_direction:
self.current_direction = current_direction
What is that doing? What do we even do with self.current_direction? I suspect nothing: we used to use it to check bumper incoming. It is set but never referenced. Remove it.
def update_next(self, origin, current_direction):
return self.perform_update_step(origin)
def perform_update_step(self, origin):
if self._next_invader < len(self.invaders):
self.move_one_invader(origin)
return CycleStatus.CONTINUE
else:
return self.end_cycle()
Tests are green but of course we have those boundary ones that are no good, which I commented out but did not remove, as I would like to see if we can better test the reversal code.
update_next no longer cares about current direction. Change signature and track down the variable in the caller.
The variable is actually used in the InvaderFleet, as it moves things, so we let that ride.
I think we no longer need to add the bumpers to the game, if we can live without the green lines. Update coin. Frankly, I prefer them being there: It looks better, since the game window is wider than the game field for Invaders. Leave them in.
I think we are down to doing some tests. I un-comment the three breaking ones. We’ll see if we can refit them.
def test_no_reversal(self):
group = InvaderGroup()
bumper = Bumper(u.BUMPER_RIGHT, +1)
group.interact_with_bumper(bumper, None)
result = group.end_cycle()
assert result == CycleStatus.NEW_CYCLE
We no longer need to test using a bumper. What we’d like to do is end the cycle with invaders in various positions and see that we get NEW_CYCLE or REVERSE as appropriate.
def test_no_reversal(self):
group = InvaderGroup()
group.position_all_invaders(Vector2(u.BUMPER_LEFT + 100, 512))
result = group.end_cycle()
assert result == CycleStatus.NEW_CYCLE
So far so good.
def test_reversal(self):
group = InvaderGroup()
bumper = Bumper(u.BUMPER_RIGHT, +1)
invader = group.invaders[0]
_pos_x, pos_y = invader.position
invader.position = (u.BUMPER_RIGHT, pos_y)
result = group.end_cycle()
assert result == CycleStatus.REVERSE
invader.position = (u.BUMPER_LEFT, pos_y)
assert result == CycleStatus.REVERSE
There! Perfectly good tests, at least given that someone calls end_cycle. That’s rather out of scope for what we’re at, but let’s see what it would take to check it. Ah, we have a test already!
def test_update_next(self):
group = InvaderGroup()
origin = Vector2(100, 100)
for i in range(55):
result = group.update_next(origin)
assert result == CycleStatus.CONTINUE
result = group.update_next(origin)
assert result == CycleStatus.NEW_CYCLE
That checks that we do in fact change from CONTINUE to NEW_CYCLE, which is just what we needed. Life is good, my pride is restored to its previous value.
We can commit. We’ve simplified the InvaderFleet, the InvaderGroup, the Invader, and the Bumper. We could live without the Bumper altogether but they draw a line that we like. We could certainly strip them down, but let’s save that for another day.
Commit: vast improvements to handling of invaders out of bounds.
Summary
I knew I wasn’t happy with the handling of the bumpers, since they were not sprites and had their own collision logic. My intention this morning was to simplify the code a bit and then convert the Bumper to use Sprite. As Professor von Moltke told us in Programming 401, no plan survives first contact with the code. The flow of changes just led from one to the next, without much deep thinking:
- Only process the bumper we’re approaching;
- Simplify signatures to no longer need the direction;
- Find and fix a fatal defect that made it to prod;
- Briefly flagellate offending programmer;
- Switch to using invader center to see how it looked;
- Ignore how it looked. Dubious push of a version;
- Write simple tests for invader out of bounds;
- Implement method;
- Use method.
- Dislike the distance they move;
- In a few steps, create new offset constants and use them;
- Review code and see that we need only check out of bounds at cycle end;
- Do that—took two tries with a revert of the first try;
- Three tests break, replaced below;
- Remove references to unused flag;
- Simplify signature to reflect lack of flag;
- Check to see if we like the look and feel without bumpers;
- Leave bumpers in;
- Fit new tests to the new logic; simpler than the old tests;
Over the course of things, we have simplified tests and provided a new one, removed a handful of methods from Bumper, removed two variables and another few methods from InvaderGroup, removed a couple of methods from InvaderFleet, and simplified Invader a bit.
I like this code better, I think it is easier to understand. It might even appeal more to Bruce if he reads this.
A good morning’s work, and I’m just delighted that the code led me in a different direction than I expected to go.
See you next time!