Python Asteroids+Invaders on GitHub

Let’s take a look at the ImageMasher. We’ll see how far toward perfection (or lily-gilding) we can push it. We go a long way—and at the end I think I see still more that we can do.

The ImageMasher does its job perfectly. From a target mask, it removes the pixels corresponding to a shot that has hit the target at some point, and then removes pixels that correspond to an explosion mask associated with the shot. I am satisfied with its tests, in particular this one:

    def test_plus_square(self, make_target, make_square, make_plus):
        x = make_square
        x.position = (100, 200)
        plus = make_plus
        plus.position = (100, 200)
        plus.explosion_mask = x.mask
        target = make_target
        target.position = (100, 200)
        masher = ImageMasher(target, plus)
        masher.apply_shot()
        masher.apply_explosion()
        mask = masher.get_mask()
        hits = [(2, 2), (3, 2), (4, 2), (5, 2), (6, 2),
                (2, 3), (4, 3), (6, 3),
                (2, 4), (3, 4), (4, 4), (5, 4), (6, 4),
                (2, 5), (4, 5), (6, 5),
                (2, 6), (3, 6), (4, 6), (5, 6), (6, 6)]
        self.check_bits(mask, hits)

When that one runs, we get this:

11111111
11111111
11000001
11010101
11000001
11010101
11000001
11111111
[(2, 2), (3, 2), (4, 2), (5, 2), (6, 2), (2, 3), (4, 3), (6, 3), (2, 4), (3, 4), (4, 4), (5, 4), (6, 4), (2, 5), (4, 5), (6, 5), (2, 6), (3, 6), (4, 6), (5, 6), (6, 6)]

The shot in the picture above is a 3x3 “plus sign” and the explosion is a 5x5 open square. Both are positioned in the 8x8 target as centrally as they can be. Because the center of an 8x8 if (4, 4), the extra pixels are on the top and left.

When I finally wrote this test, it was the first one I had done where the shot and the explosion were not the same size. I just didn’t think to test that until yesterday.

That oversight is hard to explain, because I had already figured out that masking needs to consider each applied mask separately, positioning its rectangle center to the collision point, and then offsetting by the difference between the top left of the target and the other. In fact, I cannot explain it. I just never twigged to the fact that the shot and explosion being the same size was the easy case.

We are not perfect. Things like this happen. It helps to have more than one person involved. It would probably help to have the habit of writing down potential test cases. I myself do not have that habit.

But what’s good is having tests and improving them just as we improve the code. They form a tighter and tighter fence around the program’s behavior, keeping it within the bounds we need.

A curious fact

A curious fact is that when the mask calculation was being done inside Shield, it was difficult to test and I had no automated tests for it, even after I had automated some general tests for masking. The code seemed to be working, and the tests would be tricky, and I just didn’t do it.

But when I decided to build a separate object to do the job, building that object with TDD was straightforward, and generating tests for it was moderately easy.

It became easier still when, first, I extended the test to check and display a little matrix of the result, as we see above. Then, second, I built a testing tool that let me type in one of those pictures, and turn it into a mask for use in the testing. And finally third, I extended the checker to produce a list of the erased pixels it saw, so that I could inspect the result picture and when I deemed it correct, use the output list in the test. (This last trick is sometimes called an Approval Test.)

The little lesson

The little lesson is that it is almost invariably easier to test a small single-purpose object than it is to test the same behavior when it is part of some larger, more powerful, object that serves some larger purpose.

In short, small objects are easier to test. That makes them easier to get right. And being small, they are generally easier to understand, and even if they’re a bit tricky, you can mostly ignore the details and consider only their results.

Reviewing ImageMasher

ImageMasher is used exactly once in the actual game:

class Shield(InvadersFlyer):

    def mash_image(self, shot):
        masher = ImageMasher(self, shot)
        masher.apply_shot()
        masher.apply_explosion()
        self._mask = masher.get_mask()
        rect = self._mask.get_rect()
        surf = self._mask.to_surface()
        self._map.blit(surf, rect)

self._mask is the current Shield mask and self._map is the current Surface containing the actual picture of the shield. The blit is a bit of a hack. The _map is set so that black pixels are transparent, and all the other pixels are white. The surf will default to white where the mask has bits and black otherwise. So it essentially rewrites the whole surface.

I think we could do better if we needed to, but if there is a way to apply a mask to a surface, just erasing parts of it, I couldn’t find it. One possible future feature is that in the original game, using a sheet of colored plastic, the shields and player were colored green. If we need to do this, we’d have to come here.

One question is: why do we have to call two methods, apply_shot and apply_explosion to get the job done? You’d think one would suffice.

I think this question is even more germane: why does the Shield have to blit the surface? Why doesn’t the ImageMasher do it … especially since its name suggests that it works on images. We’ll put that on the list of things to fix. Anywhere, here’s ImageMasher as it stands now:

class ImageMasher:
    def __init__(self, target, shot):
        self.target = target
        self.shot = shot
        self.new_mask = self.target.mask.copy()

    def offset(self, point1, point2):
        return Vector2(point1) - Vector2(point2)

    def apply_explosion(self):
        explosion_mask = self.shot.explosion_mask
        explosion_rect = explosion_mask.get_rect()
        explosion_rect.center = self.shot.position
        explosion_offset = self.offset(explosion_rect.topleft, self.target.rect.topleft)
        self.new_mask.erase(explosion_mask, explosion_offset)

    def apply_shot(self):
        offset = self.shot_offset()
        overlap = self.target.mask.overlap_mask(self.shot.mask, offset)
        self.new_mask.erase(overlap, (0, 0))

    def get_mask(self):
        return self.new_mask

    def shot_offset(self):
        return self.offset(self.shot.rect.topleft, self.target.rect.topleft)

What do we see when we look at this? Or, perhaps more important, what do we not see?

  1. Why is the offset in the apply_shot call to erase (0, 0) and different in the call from apply_explosion?
  2. What is overlap? Should it be named overlap_mask?
  3. What is going on with the code in apply_explosion, with all the mask and rectangle stuff?
  4. Why are we happy calling just offset in apply_explosion but calling shot_offset in apply_shot? Shouldn’t we do it the same way each time?
  5. Should get_mask be renamed to something like get_new_mask? Should there also be get_new_surface?
  6. Doesn’t it seem that we’re not getting much help from our target, shot, and masks, with all the extended references to xxx.rect.topleft? Is there a new object that could help us?

Ideally, all these questions should be answered by the code, or should never arise at all because the code is more clear.

Just for fun.

Just for fun, let’s see how squeaky clean we can make this little object. Then we’ll ask ourselves when, if ever, we should do what we’re about to do.

shot_offset

There are only two references to shot_offset, one above in the ImageMasher, and one in a test. The test goes on to check the actual results and the call to shot_offset was there as a kind of stepping stone. I rather hate to remove it. Instead, let’s extract a corresponding explosion_offset, which will provide a bit more meaning and more symmetry to the code …

An interesting thing happens. PyCharm notices that we can do this:

    def apply_explosion(self):
        explosion_mask = self.shot.explosion_mask
        explosion_rect = explosion_mask.get_rect()
        explosion_rect.center = self.shot.position
        explosion_offset = self.explosion_offset(explosion_rect)
        self.new_mask.erase(explosion_mask, explosion_offset)

    def explosion_offset(self, explosion_rect):
        return self.offset(explosion_rect.topleft, self.target.rect.topleft)

    def shot_offset(self):
        return self.explosion_offset(self.shot.rect)

Refactoring suggests rename

It has rewritten shot_offset to call explosion_offset. Clearly the right name of that method is not explosion_offset. What is it? It is the offset from a damage rectangle to the target!

Let’s rename a bit. Here’s the whole thing, sorted and with some renames:

class ImageMasher:
    def __init__(self, target, shot):
        self.target = target
        self.shot = shot
        self.new_mask = self.target.mask.copy()

    def apply_shot(self):
        offset = self.shot_offset()
        overlap = self.target.mask.overlap_mask(self.shot.mask, offset)
        self.new_mask.erase(overlap, (0, 0))

    def apply_explosion(self):
        explosion_mask = self.shot.explosion_mask
        explosion_rect = explosion_mask.get_rect()
        explosion_rect.center = self.shot.position
        explosion_offset = self.damage_offset(explosion_rect)
        self.new_mask.erase(explosion_mask, explosion_offset)

    def shot_offset(self):
        return self.damage_offset(self.shot.rect)

    def damage_offset(self, damage_rectangle):
        return self.offset(damage_rectangle.topleft, self.target.rect.topleft)

    @staticmethod
    def offset(point1, point2):
        return Vector2(point1) - Vector2(point2)

    def get_new_mask(self):
        return self.new_mask

Multiple steps suggest extract.

Let’s extract from apply_shot. There are two things going on there, getting an overlap mask, and erasing it. One Extract Method, two Inlines later:

    def apply_shot(self):
        shot_overlap = self.shot_overlap_mask()
        self.new_mask.erase(shot_overlap, (0, 0))

    def shot_overlap_mask(self):
        return self.target.mask.overlap_mask(self.shot.mask, self.shot_offset())

Multiple steps here too:

This method has four detailed lines setting up an erase. Can we give ourselves more information about what they are doing?

    def apply_explosion(self):
        explosion_mask = self.shot.explosion_mask
        explosion_rect = explosion_mask.get_rect()
        explosion_rect.center = self.shot.position
        explosion_offset = self.damage_offset(explosion_rect)
        self.new_mask.erase(explosion_mask, explosion_offset)

The middle bit is just getting the offset. Extract Method.

    def apply_explosion(self):
        explosion_mask = self.shot.explosion_mask
        explosion_offset = self.explosion_offset(explosion_mask)
        self.new_mask.erase(explosion_mask, explosion_offset)

    def explosion_offset(self, explosion_mask):
        explosion_rect = explosion_mask.get_rect()
        explosion_rect.center = self.shot.position
        return self.damage_offset(explosion_rect)

This code asks a question:

We see that the name damage_offset asks the question “from what?”. We answer with rename:

    def explosion_offset(self, explosion_mask):
        explosion_rect = explosion_mask.get_rect()
        explosion_rect.center = self.shot.position
        return self.damage_offset_from_target(explosion_rect)

Clearly those first two lines are producing the explosion rectangle. Extract.

    def explosion_offset(self, explosion_mask):
        explosion_rectangle = self.explosion_rectangle(explosion_mask)
        return self.damage_offset_from_target(explosion_rectangle)

    def explosion_rectangle(self, explosion_mask):
        explosion_rect = explosion_mask.get_rect()
        explosion_rect.center = self.shot.position
        return explosion_rect

Not entirely clear

Is the name explosion_rectangle clear enough? Should we call it explosion_rectangle_at_shot_position? properly_positioned_explosion_rectangle? Let’s rename the variable inside, not the method:

    def explosion_rectangle(self, explosion_mask):
        explosion_rect_moved_to_shot_position = explosion_mask.get_rect()
        explosion_rect_moved_to_shot_position.center = self.shot.position
        return explosion_rect_moved_to_shot_position

I might prefer that because it tells me why it’s setting center.

Let’s review the big picture again.

class ImageMasher:
    def __init__(self, target, shot):
        self.target = target
        self.shot = shot
        self.new_mask = self.target.mask.copy()

    def apply_shot(self):
        shot_overlap = self.shot_overlap_mask()
        self.new_mask.erase(shot_overlap, (0, 0))

    def shot_overlap_mask(self):
        return self.target.mask.overlap_mask(self.shot.mask, self.shot_offset())

    def apply_explosion(self):
        explosion_mask = self.shot.explosion_mask
        explosion_offset = self.explosion_offset(explosion_mask)
        self.new_mask.erase(explosion_mask, explosion_offset)

    def explosion_offset(self, explosion_mask):
        explosion_rectangle = self.explosion_rectangle(explosion_mask)
        return self.damage_offset_from_target(explosion_rectangle)

    def explosion_rectangle(self, explosion_mask):
        explosion_rect_moved_to_shot_position = explosion_mask.get_rect()
        explosion_rect_moved_to_shot_position.center = self.shot.position
        return explosion_rect_moved_to_shot_position

    def shot_offset(self):
        return self.damage_offset_from_target(self.shot.rect)

    def damage_offset_from_target(self, damage_rectangle):
        return self.offset(damage_rectangle.topleft, self.target.rect.topleft)

    @staticmethod
    def offset(point1, point2):
        return Vector2(point1) - Vector2(point2)

    def get_new_mask(self):
        return self.new_mask

I don’t know. Green, though. Commit: intensive refactoring.

User needs single call

Let’s provide the single method to do the work, and the method to update the surface. Is there a clever way that I can use the safe PyCharm tools to do that?

I can call the method I want and PyCharm will create an empty one, that’s somewhat good.

    def mash_image(self, shot):
        masher = ImageMasher(self, shot)
        masher.apply_damage()
        self._mask = masher.get_new_mask()
        rect = self._mask.get_rect()
        surf = self._mask.to_surface()
        self._map.blit(surf, rect)

PyCharm creates:

    def apply_damage(self):
        pass

I do my part:

    def apply_damage(self):
        self.apply_shot()
        self.apply_damage()

Good. Commit: new apply_damage method.

NO!
It is not good. It’s rather obviously wrong. But I didn’t see it, and didn’t even test it! Fortunately I find out very quickly.

Now let’s have the ImageMasher update our surface.

Should we pass it in now … or should we have passed it in at the creation? I think now.

Arrgh!

I have broken something. When I run the game and shoot the shield, there is a recursion:

  File "/Users/ron/PycharmProjects/firstGo/ImageMasher.py", line 12, in apply_damage
    self.apply_damage()
  File "/Users/ron/PycharmProjects/firstGo/ImageMasher.py", line 12, in apply_damage
    self.apply_damage()
  File "/Users/ron/PycharmProjects/firstGo/ImageMasher.py", line 12, in apply_damage

Yes there certainly is. I should have run the game, since there are no tests for that new method. Fix:

    def apply_damage(self):
        self.apply_shot()
        self.apply_explosion()

We’re green and good. Commit: fix recursion recursion …

Now the test we should have written is easy, just change any test that calls both methods to call the new one:

    def test_masher_both(self, make_missile, make_target, make_small_explosion):
        shield = make_target
        shield.position = (100, 200)
        shot = make_missile
        shot.position = (100, 200)
        expl = make_small_explosion
        shot.explosion_mask = expl.mask
        masher = ImageMasher(shield, shot)
        masher.apply_damage()  # <=========
        mask = masher.get_new_mask()
        hits = [(3, 3), (4, 3), (5, 3), (3, 4), (4, 4), (5, 4), (3, 5), (4, 5), (5, 5)]
        self.check_bits(mask, hits)

Commit: Test that would detect recursion had I not patched it.

You patched without a test!

Why did I do the patch and then the test? Because the build was committed and broken. YMMV and it would have only taken a moment longer to change the test and find the recursion and then fix it. Probably would have been better. I panicked.

Now back to the blitting. I think it’s better to adjust the surface in place, since we’re not really going to replace it. Code again by intention:

class Shield(InvadersFlyer):
    def mash_image(self, shot):
        masher = ImageMasher(self, shot)
        masher.apply_damage()
        self._mask = masher.get_new_mask()
        masher.damage_surface(self._map)

class ImageMasher:
    def damage_surface(self, surface):
        rect = self.new_mask.get_rect()
        damaged_surface = self.new_mask.to_surface()
        surface.blit(damaged_surface, rect)

Better names?

I think it would be better if the primary method was determine_damage and the surface one was apply_damage_to_surface. Make it so:

    def mash_image(self, shot):
        masher = ImageMasher(self, shot)
        masher.determine_damage()
        self._mask = masher.get_new_mask()
        masher.apply_damage_to_surface(self._map)

One last look at ImageMasher, then let’s sum up for now.

class ImageMasher:
    def __init__(self, target, shot):
        self.target = target
        self.shot = shot
        self.new_mask = self.target.mask.copy()

    def determine_damage(self):
        self.apply_shot()
        self.apply_explosion()

    def apply_shot(self):
        shot_overlap = self.shot_overlap_mask()
        self.new_mask.erase(shot_overlap, (0, 0))

    def shot_overlap_mask(self):
        return self.target.mask.overlap_mask(self.shot.mask, self.shot_offset())

    def apply_explosion(self):
        explosion_mask = self.shot.explosion_mask
        explosion_offset = self.explosion_offset(explosion_mask)
        self.new_mask.erase(explosion_mask, explosion_offset)

    def explosion_offset(self, explosion_mask):
        explosion_rectangle = self.explosion_rectangle(explosion_mask)
        return self.damage_offset_from_target(explosion_rectangle)

    def explosion_rectangle(self, explosion_mask):
        explosion_rect_moved_to_shot_position = explosion_mask.get_rect()
        explosion_rect_moved_to_shot_position.center = self.shot.position
        return explosion_rect_moved_to_shot_position

    def shot_offset(self):
        return self.damage_offset_from_target(self.shot.rect)

    def damage_offset_from_target(self, damage_rectangle):
        return self.offset(damage_rectangle.topleft, self.target.rect.topleft)

    @staticmethod
    def offset(point1, point2):
        return Vector2(point1) - Vector2(point2)

    def get_new_mask(self):
        return self.new_mask

    def apply_damage_to_surface(self, surface):
        rect = self.new_mask.get_rect()
        damaged_surface = self.new_mask.to_surface()
        surface.blit(damaged_surface, rect)

What about one more tiny change, from the meaningless rect to the notion of “area”.

    def apply_damage_to_surface(self, surface):
        area_to_blit = self.new_mask.get_rect()
        damaged_surface = self.new_mask.to_surface()
        surface.blit(damaged_surface, area_to_blit)

I think I prefer that. We might want to change some other names similarly, but I promised we’d sum up.

Commit: Some renaming of methods and temps.

Summary

By my lights, this is better, and it could use a little improvement. The best ways I know to make code understandable is through the use of meaningful names and small meaningful methods. But what is lacking here, I think, is still some overall sense of what it’s doing.

What it’s doing is:

  1. Given a target object and a shot object,
  2. where the target and shot have positions that are colliding,
  3. where the shot object knows an additional “explosion mask”,
  4. produce a new “damaged” mask for the target, by
  5. erasing both the shot image and the explosion image,
  6. returning the new target mask on demand, and
  7. Updating the target surface on demand.

That’s rather a lot, and it just scratches the surface, no pun intended.

We could provide more detail, saying:

  1. It erases the shot from the target by
  2. Getting the shot’s offset from the target
  3. Calling mask.overlap

  4. Erasing the overlapped pixels.

  5. It erases the explosion from the target similarly, by
  6. Getting the explosion’s offset from the target

  7. Erasing the entire mask.

  8. It updates the surface by
  9. Converting the new mask to a surface, and
  10. Blitting it right over the existing surface.

Hmm. As I write out this detail, it occurs to me that we could do without calling overlap and simply erase all the pixels of the shot. The effect will be the same and it might reduce the two cases to the same code. Interesting. May have to try that … later.

Summary Summary

The new ImageMasher is better—by my lights—and looking over the improved version suggests an even simpler possibility that we may explore later.

I suspect that a small object holding a mask and a rectangle / position would be helpful. We might explore that later as well. Two things make me think it would be useful.

  1. The Thing test helper object was useful in encapsulating everything I needed for my tests, which are kind of the opposite side of the mirror from the ImageMasher, so maybe a similar object would help it.

  2. There is extensive and rather obscure manipulation of rectangles going on, and they aren’t even helpful enough to be able to difference their coordinates. I blame pygame for that one, but a little helper object might help us be more clear.

Finally, it’s still hard to understand the point of the Masher. All it really does is take two images, represented by masks, position them according to a collision, and punch holes in a source mask where there are pixels in the two images.

Maybe there should be a HolePuncher object that we use twice …

Shall we see how far we can push this thing? I predict that we shall.

See you next time!