P-247 - A Little Bit More

Python Asteroids+Invaders on GitHub

In my quest to spend way too much time on ImageMasher, I have another idea. I should also mention why this exercise is a good one: changing code is our job.

Why is it worthwhile to do all this polishing of the code to erase two bitmaps, leading to ImageMasher class, and to at least two articles improving it?

In terms of its direct effect on the game, or the game’s code quality, this tiny spot clearly makes very little difference. But there are some advantages to this exercise—and I do think it is an exercise.

For the product:

The original code may not have been quite correct, and now we have solid tests showing that the current code is correct.

The original code added quite a few methods to the Shield class and now everything about image mashing is isolated.

What goes on and how it works is somewhat more clear in the existing code, which might be of value if changes are needed.

This impact is still small, although there is a very good chance that we’ll need to work on Shield in the future, so that improving it by making it more simple has a good chance of paying off. I might argue that the original extraction was worth the effort, but refining the ImageMasher twice after that? It’s hard to say that that improves the product enough to justify the expenditure of …

At a guess, counting writing the articles, I will spend between four and six hours on the refactoring improvements. Not that big an expenditure. Two or three articles out of nearly 250. One percent of the total product effort?

That’s almost funny.

But for the programmer:

Day in and day out, what you see in my work may look a lot like what goes on in your work. I find myself feeling pressure to get something done, and I do it. Maybe I write some tests. Sometimes I don’t. I get the thing to work, and I move on. In many, perhaps most programing shops, that’s how it goes. They give us another story, we feel pressure to get it done quickly, we do it, we move on.

What is different here is that I have the freedom to revisit the code I’ve written. I can and do show you what it’s , warts and all, but then I can and do show you how it could be improved. In many, perhaps most shops, developers get very few chances to go back, to reflect on their work, and to improve it.

Our productivity as developers comes down, in large part, to our speed in changing code. Sometimes we are changing by adding, but for most developers, we are more commonly changing code that we—or apparently some demon from hell—wrote at some time in the past. And our code changing productivity depends on two main factors: the quality of the code we’re changing, and our own skill at changing code.

This tiny exercise in testing and refactoring helps to hone my ability to change code.

To me, that’s why we would do well to take some small bit of code and improve it: it hones our ability to change code. So as we reflect on the changes from untested code hanging like a toad on the side of Shield, to well-tested code in a separate class, to code improved yesterday, to code improved however we improve it today, let’s keep in mind not just that the code is better, but that we ourselves are better for the practice.

That’s my story and I’m sticking to it. Let’s get to work.

ImageMasher

Let’s review what ImageMasher does:

1. Given a target and a shot,
2. Damage the target’s image surface and mask,
3. By erasing the image of the shot,
4. And by erasing the image of an explosion associated with the shot.

This morning, I “notice” that the ImageMasher does two erasures in the target mask, one for the shot and one for the shot explosion. And I “notice” that it does that in two slightly different ways:

Damage from the shot:

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 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)

Here we get an overlap mask and erase it from new_mask. The overlap mask will consist of the pixels that the shot mask and target mask have in common. If they do not have certain pixels in common, those pixels must already be erased, or not in the target at all.

We’ll consider the details of how that’s done in a bit. Let’s see how the explosion is handled:

    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

Both of these come down to erase lines that look quite similar but significantly different:

# shot
        self.new_mask.erase(shot_overlap, (0, 0))

# explosion
        self.new_mask.erase(explosion_mask, explosion_offset)

These differ only by the second parameter in the call to erase, the offset. That’s because we asked for the “overlap mask” in the case of the shot, and we didn’t for the explosion. But in both cases we do compute the same offset: it’s just that for the shot it’s provided to the overlap_mask call, and for the explosion, it’s provided to the erase call.

There is another difference that I’d like to reflect upon. The code uses the fact that the target’s position and shot’s position are in proper relation to one another, with the shot’s position somewhere inside the area covered by the shield at its position. This is important because the overlap_mask and erase calls both want the offset of the shot/explosion parameter from the target mask they are going to consider.

The offset we need is the difference between the topleft coordinates of the two masks in question. Why? Just because that’s how the functions we’re given work. Pygame thinks mostly in terms of topleft, and I think mostly in terms of center.

For the shot, the answer is easy. The masks match the objects, aligned on target and shot centers, so we just need the difference between the target and shot rectangles’ toplefts. For the explosion, we just have the mask, and it does not come with the convenient values in it as did the shot. So we have to produce a rectangle that is properly centered where the shot hit:

    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

That second method sets the rectangle’s center to the shot’s position. Now that rectangle is ready to have its topleft differenced from that of the target.

Confusing, isn’t it? I think it would be less confusing if we did these things in the same way. And, I’m not sure of this, but I suspect that a little helper object might be useful.

To improve this code, I propose to take a number of small steps. I do not know what they are: I expect to decide on each step after completing the one before.

A mild concern

I do have one bit of a dilemma: I am not certain that what I do this morning will make things better. I am sure I won’t break things, but I am not sure we’ll end up in a better place. So I am reluctant to commit my changes. But the right thing is small steps, committing when things are good, and that’s what we’ll do. We have a code manager, we can always get this thing back to where it was.

We’ll begin by making the two cases more similar. My plan is to skip the creation of the overlap mask and just erase the shot image directly. As previously mentioned, this will have the same effect.

One way of putting it is that we intend to change this:

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

To be more like this:

    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)

The change is this:

    def apply_shot(self):
        shot_mask = self.shot.mask
        shot_offset = self.shot_offset()
        self.new_mask.erase(shot_mask, shot_offset)

The tests are green, and I know they are solid. Therefore, I will check the game display anyway. It is perfect, just as it was before.

Commit: refactor to make apply_shot and apply_explosion more similar.

We have created some duplication, but it’s only approximate. What about the shot_offset and explosion_offset methods? How are they similar or different?

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

    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 damage_offset_from_target(self, damage_rectangle):
        return self.offset(damage_rectangle.topleft, self.target.rect.topleft)

I want to make the two apply methods the same. Let’s begin by renaming the explosion_rectangle method, and probably its variables as well. It really has nothing to do with explosions per se. It accepts a mask, and returns a rectangle, matching that mask’s rectangle, but with its center moved to the shot position.

Let me see if I can come up with some better names.

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

    def mask_rectangle_in_shot_position(self, mask):
        rectangle_moved_to_shot_position = mask.get_rect()
        rectangle_moved_to_shot_position.center = self.shot.position
        return rectangle_moved_to_shot_position

Commit: rename method and members. Hm. I should have said “and variables”. No harm done.

Let’s rename the explosion_offset method as well. It’s a mask offset, has no explosion notion. I am not entirely happy with these names but here we are now:

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

    def mask_offset_from_target(self, mask):
        explosion_rectangle = self.mask_rectangle_in_shot_position(mask)
        return self.damage_offset_from_target(explosion_rectangle)

    def mask_rectangle_in_shot_position(self, mask):
        rectangle_moved_to_shot_position = mask.get_rect()
        rectangle_moved_to_shot_position.center = self.shot.position
        return rectangle_moved_to_shot_position

Now with the names generalized a bit, I intend to call the newly generalized mask_offset_... method for the shot:

    def apply_shot(self):
        shot_mask = self.shot.mask
        shot_offset = self.mask_offset_from_target(shot_mask)
        self.new_mask.erase(shot_mask, shot_offset)

We are green. Again, in a fit of insecurity, I check the game, even though I really do trust the tests. Totally good, so good that there’s no point showing you another picture.

Now we have an unused method, and another that is only used by a test:

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

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

I remove the test reference, confident that my more modern bitmap-checking tests are stronger, and remove those two methods.

Commit: make apply_shot and apply_explosion more alike, remove unused methods. I could have done two commits there. I am not the master of small steps that Hill is. But I’m getting there.

Now I’ll reorder things a bit to better show flow and duplication:

    def apply_shot(self):
        shot_mask = self.shot.mask
        shot_offset = self.mask_offset_from_target(shot_mask)
        self.new_mask.erase(shot_mask, shot_offset)

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

    def mask_offset_from_target(self, mask):
        explosion_rectangle = self.mask_rectangle_in_shot_position(mask)
        return self.damage_offset_from_target(explosion_rectangle)

    def mask_rectangle_in_shot_position(self, mask):
        rectangle_moved_to_shot_position = mask.get_rect()
        rectangle_moved_to_shot_position.center = self.shot.position
        return rectangle_moved_to_shot_position

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

Commit: reorder methods.

Now we have the rather obvious duplication in apply_shot and apply_explosion. Extract method, selecting the last two lines, I think.

    def apply_shot(self):
        shot_mask = self.shot.mask
        self.apply_mask(shot_mask)

    def apply_mask(self, shot_mask):
        shot_offset = self.mask_offset_from_target(shot_mask)
        self.new_mask.erase(shot_mask, shot_offset)

Commit, just to prove that I can: extract apply_mask method.

Inline:

    def apply_shot(self):
        self.apply_mask(self.shot.mask)

Use new method in apply_explosion:

    def apply_shot(self):
        self.apply_mask(self.shot.mask)

    def apply_explosion(self):
        self.apply_mask(self.shot.explosion_mask)

    def apply_mask(self, shot_mask):
        shot_offset = self.mask_offset_from_target(shot_mask)
        self.new_mask.erase(shot_mask, shot_offset)

I’m not entirely sure why PyCharm didn’t notice that it could do that, but I do like to take part in things, so it’s OK.

Commit: use apply_mask in both apply_shot and apply_explosion.

Shall we review the whole thing? Perhaps in a moment. Those names apply_mask and apply_explosion. Those aren’t just great names are they, given what we’re about?

I think I’ll call them erase or something. I rename three methods:

    def erase_shot(self):
        self.erase_mask(self.shot.mask)

    def erase_explosion(self):
        self.erase_mask(self.shot.explosion_mask)

    def erase_mask(self, shot_mask):
        shot_offset = self.mask_offset_from_target(shot_mask)
        self.new_mask.erase(shot_mask, shot_offset)

Commit: rename apply to erase.

Let’s view the whole ImageMasher and reflect. Then I’ll give you a moment to rest.

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.erase_shot()
        self.erase_explosion()

    def erase_shot(self):
        self.erase_mask(self.shot.mask)

    def erase_explosion(self):
        self.erase_mask(self.shot.explosion_mask)

    def erase_mask(self, shot_mask):
        shot_offset = self.mask_offset_from_target(shot_mask)
        self.new_mask.erase(shot_mask, shot_offset)

    def mask_offset_from_target(self, mask):
        explosion_rectangle = self.mask_rectangle_in_shot_position(mask)
        return self.damage_offset_from_target(explosion_rectangle)

    def mask_rectangle_in_shot_position(self, mask):
        rectangle_moved_to_shot_position = mask.get_rect()
        rectangle_moved_to_shot_position.center = self.shot.position
        return rectangle_moved_to_shot_position

    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):
        area_to_blit = self.new_mask.get_rect()
        damaged_surface = self.new_mask.to_surface()
        surface.blit(damaged_surface, area_to_blit)

Reflection

The “trick”

We’ve carved this down pretty far. The “trick” today was to make the two methods apply_shot and apply_explosion look the same and then remove the resulting duplication. This did require a small insight, namely that we could just erase the shot mask rather than the overlap mask. Once we saw that, the rest was just moving code around.

Invaluable IDE

I should emphasize that I might never go this far without the help of a strong IDE like PyCharm. I certainly know how those Extract refactorings work and can do them by hand, but it is a bit tedious and error-prone, so I would tend to lean away from doing them. I prefer this result: I prefer highly-factored code. But when it’s tedious and error-prone, I will often stop short of my preference. A strong IDE with refactoring is precious

Are we there yet?

We could be there, but there are things to think about.

The ImageMasher works mostly with masks, rectangles and positions, but its member variables are a target and a shot, instances of some pretty high-level objects. We might do well to collect up the specifics right in __init__ and proceed from there.

There is more than one level of abstraction inside here. At the top, target and shot come in, and mask and surface come out. That’s odd all in itself, come to think of it.

Inside, we process masks, rectangles, and positions, and those are three levels of abstraction right there: masks have rectangles and rectangles have positions. So this one object, ImageMasher, threads from Flyers at the top, down through masks, rectangles, positions, and back up not quite all the way, to mask and surface.

Those are hints that there might be profit from improved objects, or better support from existing objects.

We could be there, but there’s reason to suspect that we could benefit from further factoring. That gives me an idea. We’ll break here and continue in the next article. Take a nice break, but come back if it pleases you.

Take a Break

I wrote this article and the next one all in one go. I’m going to break it in half here so that you have a chance to take a break. I hope you’ll come back to the next one.

See you soon!