P-240 - Bye, Darling

Python Asteroids+Invaders on GitHub

Some reflection, and an exchange with Rickard, makes me think that my lovely little Tasks object isn’t as great as I initially thought it was. Let’s think, code, think, and learn.¹

Let me begin with my own observations, then we’ll let Rickard drive a nail or two into the coffin of my Tasks object.

The Tasks object itself is simple, and I think, elegant:

class Tasks:
    def __init__(self):
        self.tasks = []
        
    def remind_me(self, func):
        self.tasks.append(func)

    def finish(self):
        for func in self.tasks:
            func()
        self.clear()

    def clear(self):
        self.tasks.clear()

It simply keeps a list of functions, and when you call finish, it performs all the functions on the list. Perfect little object for putting off until later something that you don’t want to do right now.

The motivation for Tasks was the fact, and it is a fact, that some Flyer objects really do need to remind themselves to do something later—or not to do it. The best recent example is the Shield, which needs to keep its mask constant during an interaction cycle, only updating it when the cycle is over. The reason for that is that other interacting objects use the mask to decide whether they are colliding with the Shield, so if it changes the mask during interactions, those other objects are confused.

Aside

As I write that, I see something that I had not really seen before. Collision detection in Invaders uses both the rectangle of an object and its actual bitmap (mask) to decide whether there is a collision. The Collider is given two objects and it pulls out their rect and mask and does the necessary checks.

That’s not really good, is it? It’s generally not the best idea for one object to pull out internal state of another, and then do things to it. I wrote Collider so that all the objects wouldn’t be burdened with duplicate code checking rectangles and masks, and it serves that need, but it’s not the best solution.

What is the best solution? I do not know, but it probably involves one object sending another its bitmap and mask and asking that object if it collides with those. Even that’s pretty far down in the weeds. Maybe there should be some kind of “CollisionInfo” object …

Anyway, the point at the moment is that Shield needs to update its mask if it has collided with anything, because unlike any object before, the shield changes shape when it is hit. But the shield cannot change the mask used to detect collisions, because if it does, future checks will not work correctly. Before we dealt with this issue, shots would tear clear through the shield, rather than just blasting out a fragment.

I originally fixed the shield problem by making a copy of the current mask, updating it during the interactions, and plugging it in as the “real” mask after interactions were over. The Tasks object came out of my finding that implementation hard to understand and was an attempt at providing a standard way for Flyer objects to defer operations until after interactions were over.

We have a few uses of the feature. Some of them, I think, are just not sensible. PlayerShot is an example:

class PlayerShot(InvadersFlyer):
    def __init__(self, position=u.CENTER):
        offset = Vector2(2, -8*4)
        ...
        self._tasks = Tasks()

    def begin_interactions(self, fleets):
        self._tasks.clear()

    def hit_invader(self, fleets):
        self.remind_me_to_die(fleets)

    def end_interactions(self, fleets):
        self._tasks.finish()

    def interact_with_invadershot(self, shot, fleets):
        if self.colliding(shot):
            self.remind_me_to_explode_and_die(fleets)

    def interact_with_shield(self, shield, fleets):
        if self.colliding(shield):
            self.remind_me_to_die(fleets)

    def interact_with_topbumper(self, top_bumper, fleets):
        if top_bumper.intersecting(self.position):
            self.remind_me_to_explode_and_die(fleets)

    def remind_me_to_explode_and_die(self, fleets):
        self.remind_me_to_explode(fleets)
        self.remind_me_to_die(fleets)

    def remind_me_to_explode(self, fleets):
        self._tasks.remind_me(lambda: self.actually_explode(fleets))

    def remind_me_to_die(self, fleets):
        self._tasks.remind_me(lambda: self.actually_die(fleets))

    def actually_explode(self, fleets):
        fleets.append(ShotExplosion(self.position))

    def actually_die(self, fleets):
        fleets.remove(self)

That’s a lot, but I would (and did) argue that tiny methods like that are fine things. But here’s the real rub:

There is no need to defer those last two actions. It is perfectly OK to add and remove objects during interactions, and in fact it’s the official way to do almost everything.

Let’s undo use of Tasks here, and see the difference, as part of our assessment of whether Tasks is bearing sufficient weight given how hard it is to think about … at least on the reading side. I don’t find it too difficult to write code that uses Tasks, but later you have to decode something like the above to figure out how it works.

Let’s just do the things that need doing everywhere in PlayerShot.

    def begin_interactions(self, fleets):
        pass

    def hit_invader(self, fleets):
        fleets.remove(self)

    def end_interactions(self, fleets):
        pass

    def interact_with_invadershot(self, shot, fleets):
        if self.colliding(shot):
            fleets.append(ShotExplosion(self.position))
            fleets.remove(self)

    def interact_with_shield(self, shield, fleets):
        if self.colliding(shield):
            fleets.remove(self)

    def interact_with_topbumper(self, top_bumper, fleets):
        if top_bumper.intersecting(self.position):
            fleets.append(ShotExplosion(self.position))
            fleets.remove(self)

All the tasks stuff is just gone. I can remove begin and end if I wish, because they are defaulted to pass in InvadersFlyer (pace Hill).

There are similar changes to make in InvaderShot, and they are even more sensible. We have

class InvaderShot(InvadersFlyer):
    def begin_interactions(self, fleets):
        self._tasks.clear()

    def end_interactions(self, fleets):
        self._tasks.finish()

    def die_on_collision(self, flyer, fleets):
        if self.colliding(flyer):
            self._tasks.remind_me(lambda: self.die(fleets))

    def die(self, fleets):
        self._available = True
        fleets.remove(self)

The rest of the interaction code all just calls die_on_collision, so it is the only use of remind me … and it can do this instead:

    def die_on_collision(self, flyer, fleets):
        if self.colliding(flyer):
            self.die(fleets)

I retain die because it encapsulates the two things we need to do, set the available flag and remove the shot.

These two classes are better off without Tasks. Commit: Remove darling Tasks from PlayerShot and InvaderShot.

The showcase for Tasks is Shield, and it’s nearly good there but probably more complex than it needs to be. Before we look at that, let’s see what Rickard and I shared yesterday:

Rickard: @RonJeffries Reading half the article, I find the reminder stuff a little hard to understand. When is it important to have a reminder, and when not? And handling reminders correctly seems error prone.

Then I thought… What about immutability?

What if the fleet/flyer are immutable? On each iteration a new list is created. Each flyer returns what it should be replaced with. Returning empty list means delete myself.

Not sure how it would play out. But wanted to share.

Ron:
@rickardlindberg interesting idea. Might be good for next design. You still have the two-phase issue, though, with something like shield, that has to save up mask updates.

There’s no doubt that the reminders are more than some objects need, but I think having a standard way to do two-phase might pay off.

I could be wrong. 😀

Ron, later:
@rickardlindberg
I think reminders give a standard way of recording facts during interactions and taking action afterward. The biggest need is like in shield, which needs to make extensive changes due to each interaction, but need to be immutable in the eyes of the other interactor.

It may be too much, but has the advantage of standardizing how we do the two-phase wait till end_interactions buffering.

But the immutability thought gives me a bit of an idea …

Thanks!

There’s more in the threads on Mastodon if you’re interested. Check them out. One point that Rickard brings up this morning is that with the Tasks thing, if there is an error in your call, you find out about it later, which might be a problem. Yes, and tests will help with that. However, I think that it is an inherent aspect of this design that some actions are deferred and we won’t know if that code works until the deferred actions are unwound. That does strongly suggest that deferred actions should be very simple.

Anyway, the poster child for Tasks is Shield, so let’s review it and see if we can do better without Tasks.

class Shield(InvadersFlyer):
    def begin_interactions(self, fleets):
        self._tasks.clear()

    def end_interactions(self, fleets):
        self._tasks.finish()

We have the usual boilerplate initializing the tasks and executing them. In operation:

    def interact_with_invadershot(self, shot, fleets):
        self.process_shot_collision(shot, self._explosion_mask)

    def interact_with_playershot(self, shot, fleets):
        self.process_shot_collision(shot, self._player_explosion_mask)

    def process_shot_collision(self, shot, explosion_mask):
        collider = Collider(self, shot)
        if collider.colliding():
            overlap_mask: Mask = collider.overlap_mask()
            self.update_mask_and_visible_pixels(collider, explosion_mask, overlap_mask, shot)

    def update_mask_and_visible_pixels(self, collider, explosion_mask, overlap_mask, shot):
        self._tasks.remind_me(lambda: self.erase_shot_and_explosion_from_mask(shot, collider.offset(), overlap_mask, explosion_mask))
        self._tasks.remind_me(lambda: self.erase_visible_pixels(overlap_mask, self._mask))

    # These operations are deferred and executed on `finish`, in `end_interactions`

    def erase_shot_and_explosion_from_mask(self, shot, collider_offset, shot_overlap_mask, explosion_mask):
        self._mask.erase(shot_overlap_mask, (0, 0))
        self.erase_explosion_from_mask(collider_offset, explosion_mask, shot)

    def erase_explosion_from_mask(self, collider_offset, explosion_mask, shot):
        expl_rect = explosion_mask.get_rect()
        offset_x = (shot.rect.w - expl_rect.w) // 2
        adjust_image_to_center = collider_offset + Vector2(offset_x, 0)
        self._mask.erase(explosion_mask, adjust_image_to_center)

    def erase_visible_pixels(self, shot_mask, shield_mask):
        rect = shot_mask.get_rect()
        surf = shield_mask.to_surface()
        self._map.blit(surf, rect)

(That comment about deferred isn’t in the code, I put it there in the article for our reading benefit. Perhaps it should be in the actual code.)

I want to explain to myself what’s going on in the deferred operations. It is more than a bit tricky, and I freely grant that I do not have all the answers at my fingertips. I was thinking yesterday that some explanatory tests might be helpful. Anyway …

Invaders objects have two well-known properties, a rectangle rect and a mask. The rectangle contains the image, but the mask says which pixels are present and which are not. The shield has those angled bits, and the shots have strange shapes, and the invaders are smaller than their rectangles. So we need both the rect and the mask to detect collisions.

When the shield is hit by a shot, either a player shot or an invader shot, we want to erase some pixels from the shield. We erase two things: the pixels of the shot that overlap the shield, and an “explosion”, which is a shape that the explosion makes, essentially the shape of the fragment blown out of the shield.

The mask needs to match the picture, and in fact we change the picture by computing the new mask, with removed bits, and then we mask the shield’s visible surface, as we’ll see below. Point is, we have to update both the mask and the shield surface.

The explosion shape is different for player shots and invader shots, and the Shield object knows those shapes.

Pygame provides an “overlap_mask”, a mask object with bits set only where the two objects (shield and shot) have bits in common. These are exactly the bits we want to erase due to the shot. The overlap mask is returns set up so that we can just say

        self._mask.erase(shot_overlap_mask, (0, 0))

To erase those pixels from our mask. That’s the easy and obvious bit. The explosion is more tricky and the code reflects that:

    def erase_explosion_from_mask(self, collider_offset, explosion_mask, shot):
        expl_rect = explosion_mask.get_rect()
        offset_x = (shot.rect.w - expl_rect.w) // 2
        adjust_image_to_center = collider_offset + Vector2(offset_x, 0)
        self._mask.erase(explosion_mask, adjust_image_to_center)

The explosion is a small image and has a small rectangle with its top left at (0, 0). We want it centered where the shot it, so we offset its x coordinate so that it will line up better with the shot. I don’t want to explain why that calculation does it, but it does. I have a picture here somewhere.

But then, we need to position the bits to the place in the shield where the shot hit, which is the collider_offset, provided by the Collider we used to detect the hit. We add the two adjustments to get the offset vector that tells us where to erase pixels in the mask.

If that looks ad hoc, well, it is. I have no excuse. Is there some amazing abstraction that would make this all clear, or unnecessary? Perhaps, but at this moment I do not know it. I do know that this works. It’s kind of “write-only” code. I figured out most of it, possibly mashed it with a small hammer a couple of times, and it does the job.

So we erase the explosion mask from the shield mask. Then, finally:

    def erase_visible_pixels(self, shot_mask, shield_mask):
        rect = shot_mask.get_rect()
        surf = shield_mask.to_surface()
        self._map.blit(surf, rect)

We convert the new shield mask to a surface, and blit it to the actual picture of the shield, named _map for reasons I cannot guess. The name shot_mask is not helpful. I’m not sure what would be.

Apologia Pro Vita Sua²

I have no real defense against the argument that this code is unclear. The pygame documentation is not clear to me, and I have not worked through enough examples to be able to explain it. Nor have I worked out an overlapping collision abstraction of my own that would cover pygame’s masks and surfaces with something that would at least make sense on the outside.

I would argue that it is still early days for Shield and that it was the first case that required us to really dig deeply into the world of masks and surfaces, and so it is what it is. But what it is is unclear, and it’s not helping us understand how this all works.

Here in Shield, the use of Tasks almost helps, in that it isolates the weird code from the intention of the weird code. The intention is update_mask_and_visible_pixels, which is done, later, in two steps, erase_shot_and_explosion_from_mask and erase_visible_pixels. Back away slowly, try not to show fear.

Not much of an Apologia really. But it’s the best we’ve got, for now. I’m left now with two questions. Let’s reflect.

Reflection

I now have two questions:

• Does Tasks bear its own weight?
• Can we find a better way to do Shields without Tasks?
• Can we encapsulate this mask updating code to our advantage?³

Let’s tick through those, um, two.

• Does Tasks bear its own weight?

I strongly suspect that Tasks does not bear its own weight. I think it’s quite elegant in what it does and how it does it, but it does tend to make the code hard to understand in that there are methods in it that happen at a very different time from others. Now part of that is essential to our current design, in that there are things that happen during interactions that can only be dealt with after interactions are over. But the Tasks capability does seem to make it a bit more obscure what’s going on.

Furthermore, we only have one valid use of it now, since we’ve just seen that the other two applications really didn’t improve the code. So I think we could argue that Tasks is hanging by a thread at the moment, despite its important role in Shield.

• Can we find a better way to do Shields without Tasks?
• Can we encapsulate this mask updating code to our advantage?

These two questions are a bit intertwingled. Let’s fant’sy. Suppose there was an object, an ImageMasher, that got created with a mask and a surface. And suppose you could send it a message mash, passing in a bitmap and an offset, and the object would erase the bits from the mask and surface.

Then we could, in begin_interactions, create an ImageMasher. When we perceived a collision, we could ask the masher to mash the overlap pixels and the explosion pixels. When all the excitement is over, we could ask the masher to give us the current mask and surface and replace the old with the new.

I think that might be so close to obvious that using the Tasks object would no longer make sense, though we certainly could do something like this:

    def begin_interactions(self, fleets):
    	self._tasks.clear()
    	self._masher = ImageMasher(self._map, self._mask)
    	self._remind_me(lambda: self.replace_picture())

    def end_interactions(self, fleets):
        self._tasks.finish()

    def replace_picture():
    	self._map = self._masher.map
    	self._mask = self._masher.mask

But I don’t see why that’s better than this possibility:

    def begin_interactions(self, fleets):
    	self._tasks.clear()
    	self._masher = ImageMasher(self._map, self._mask)

    def end_interactions(self, fleets):
        self.replace_picture()

    def replace_picture():
    	self._map = self._masher.map
    	self._mask = self._masher.mask

Or even this if you’d prefer it:

    def begin_interactions(self, fleets):
    	self._tasks.clear()
    	self._masher = ImageMasher(self._map, self._mask)

    def end_interactions(self, fleets):
    	self._map = self._masher.map
    	self._mask = self._masher.mask

I really like what Tasks does, deferring work until later. It reminds me of powerful things like finalize and Future and Promise. But those are also easy to use and not so easy to understand. I think we’ll find that we can in fact figure out an ImageMasher that can do the job for us more clearly. I think we’ll remove this darling.

We won’t kill it: we are merciful here. We will thank it for its service and set it free. If it comes back, we’ll welcome it. If it finds somewhere else to be of service, that’s fine too. And if it just flies around as a free idea, that, too, is as it should be.

Summary

Have I just wasted a couple of days with the Tasks object? I don’t think so. First of all, I’ve learned a very nice technique which may pay off at some future time. Second, I’ve confirmed that for most objects, a simpler approach is surely better. Third, thinking about how Tasks divides up the actions in Shield has led to an idea for an object that can help Shield with its job and might well allow us to remove Tasks from its last use in the system.

Sometimes I can see that something is a bad idea before I do it. Sometimes I have to do it to see that it’s a not the best idea. Sometimes I see that right away. Sometimes I see it later. I continue to move in the general direction of better. Sometimes that involves steps that look a lot like moving backward. So be it. Sometimes we go down a dead end road. It’s OK. Sometimes at the end of the dead end road we see a better path to where we’re trying to go, or even see a better place to go than we originally had in mind.

We’re in this for the long game. We’re in this to change code to make it do what we want and what our customers want, and to make it do those things better. We don’t imagine that we can do things right the first time, and in fact, because we work to make changing easy, we don’t pay a high price for change, which lets us take small steps and learn from them.

We learn faster, and learning faster, we go faster.⁴

We’ll see what happens, but I’d bet that Shield is going to be improved and that that improvement will not include Tasks. That will be marvelous. Improved is improved. Better is better. Thanks, Tasks and Rickard.

See you next time!