Python 149 - Abandon Plan!

Python Asteroids on GitHub

This morning I discover that the code doesn’t do what I thought it did, nor what I now want it to do. Inching toward better.

I thought this morning that I would try an idea I had for improving targeted missiles. The issue, mentioned yesterday, is that the missiles start offset from the saucer by twice its radius, which starts them closer to the targeted ship than the quadratic optimizer expects, which results in the missiles getting there a bit early.

My clever idea is, since the missile starts a bit closer to the ship than the calculation expects, slow down the missile just enough to compensate. I believe that should be easy. Or I used to believe that …

I wrote a hundred or so somewhat interesting lines of article, and then discovered that the code is not well-organized to make the change I had in mind. While looking to see how to make room for the idea, I realized that the changes I made yesterday, while they didn’t break anything, didn’t resolve the issue they were intended to resolve.

I had been pushing the quadratic targeting solution “down” in the code, so that it would only be done if in fact we were going to fire the missile. It turns out that I didn’t push it far enough, and furthermore, it’s really obviously wrong.

The Saucer just asks the Gunner to fire on every tick. The Gunner knows that it can only fire every half second, and only if there are less than two saucer missiles on the screen.

class Gunner:
    def fire(self, delta_time, saucer, ship_or_none: Ship | None, fleets):
        self._timer.tick(delta_time, self.fire_missile, saucer, ship_or_none, fleets)

Fire just ticks the timer, and calls fire_missile if it is time for a missile. Recall that that doesn’t mean we can fire: we also have to be sure there is a missile available.

    def fire_missile(self, saucer, ship_or_none, fleets):
        ship_position = self.select_aiming_point(saucer, ship_or_none)
        if saucer.missile_tally < u.SAUCER_MISSILE_LIMIT:
            self.select_missile(random.random(), fleets, saucer, ship_position)

As you see, we do select_aiming_point before we check to see whether we can actually fire. And that’s where the quadratic is.

OK, that’s not as bad as I feared, we can just move the aiming point selection inside.

    def fire_missile(self, saucer, ship_or_none, fleets):
        if saucer.missile_tally < u.SAUCER_MISSILE_LIMIT:
            ship_position = self.select_aiming_point(saucer, ship_or_none)
            self.select_missile(random.random(), fleets, saucer, ship_position)

This will have no discernible impact on the game: the point is still calculated when we need it, just not when we don’t need it.

So now we can commit: Gunner only calculates when time has elapsed and a missile is available to be fired.

So let’s review my cunning plan. Since the missile starts closer to the target than the quadratic expects, it tends to get there too soon and misses more often than it should. It seems to me that if we slow the missile down in proportion to the difference in distances between actual and expected. It should get there just on time.

Unfortunately, the code is not well-organized to make that possible. Furthermore … well, let’s take a look.

Having decided that there is a missile available to fire, we select_aiming_point:

    def select_aiming_point(self, saucer, ship_or_none):
        if ship_or_none:
            return self.choose_aiming_point(saucer, ship_or_none)
        else:
            return self.random_position()

If the ship is not present, we choose a random position to fire at. Not interesting. If the ship is present, we choose_aiming_point:

    def choose_aiming_point(self, saucer, ship):
        delta_position = ship.position - saucer.position
        aim_time = self.time_to_target(delta_position, ship.velocity)
        return ship.position + ship.velocity * aim_time

We’ll skip over the time_to_target. That’s the quadratic code that computes a time such that if we aim at where the ship will be at that time, a missile fired from our center will also arrive there at that same time. Boom!

So this function returns the ideal aiming point, and that’s returned from the select_aiming_point, so that fire_missile calls select_missile with that point:

    def fire_missile(self, saucer, ship_or_none, fleets):
        if saucer.missile_tally < u.SAUCER_MISSILE_LIMIT:
            ship_position = self.select_aiming_point(saucer, ship_or_none)
            self.select_missile(random.random(), fleets, saucer, ship_position)

    def select_missile(self, chance_of_targeting, fleets, saucer, ship_position):
        if saucer.always_target or chance_of_targeting <= u.SAUCER_TARGETING_FRACTION:
            velocity_adjustment = Vector2(0, 0)
        else:
            velocity_adjustment = saucer.velocity
        self.create_targeted_missile(saucer.position, ship_position, velocity_adjustment, fleets)

What the heck is this? Now we look to see if we want to actually target or not? The velocity adjustment is a bit of a hack anyway, in that if we are actually going to shoot at the ship, we want to factor out the “drift” that would come from firing a missile and adding in the saucer’s velocity as should actually happen according to the laws of physics and the Federation. Aimed missiles adjust for that.

But this means that all missiles, when the ship is present, are targeted at the ship! It’s just that some do not get the velocity adjustment.

Now it’s true that the velocity adjustment is enough to make them miss. But this is not what I intended. And the names, the names … select_aiming_point, choose_aiming_point, select_missile. These are not helping me.

Before I try my adjustment trick, I need to abandon that plan and whip this code into shape. Let’s first decide what it should do, in what order.

Try to fire on every cycle;
If the timer has expired, fire a missile if one is available;
(Worry about whether the timer resets properly.)
If we are always targeting, fire a targeted missile;
If the odds are not in our favor, fire a targeted missile;
Otherwise, fire in a random direction (or at a random point).

What I meant about the timer is this. When a timer ticks down, it calls the provided method, in our case fire_missile. If that method returns True or None, the timer is reset. If the method returns False, the timer is not reset. fire_missile is returning None:

    def fire_missile(self, saucer, ship_or_none, fleets):
        if saucer.missile_tally < u.SAUCER_MISSILE_LIMIT:
            ship_position = self.select_aiming_point(saucer, ship_or_none)
            self.select_missile(random.random(), fleets, saucer, ship_position)

If that’s the case, then if there are two missiles in the air, the timer ticks down, we do not fire, but the timer resets anyway. Let’s fix that. But why isn’t there a test for it? Let’s see if we can write one.

If the reset is happening when it shouldn’t, then a immediate attempt to fire after trying to fire but with too many missiles in the air should fire, but will not.

    def test_timer_reset(self):
        fleets = Fleets()
        fi = FI(fleets)
        saucer = Saucer()
        saucer.missile_tally = u.SAUCER_MISSILE_LIMIT
        Gunner().fire(u.SAUCER_MISSILE_DELAY,saucer, None, fleets)
        assert not fi.missiles
        saucer.missile_tally = 0
        Gunner().fire(0.1, saucer, None, fleets)
        assert fi.missiles

Missiles are full. We try to fire and do not emit a missile. Missiles go away. We try quickly to fire again, and we should get a missile. The test currently fails. Perfect.

This method must return True if it fires, False if not:

    def fire_missile(self, saucer, ship_or_none, fleets):
        if saucer.missile_tally < u.SAUCER_MISSILE_LIMIT:
            ship_position = self.select_aiming_point(saucer, ship_or_none)
            self.select_missile(random.random(), fleets, saucer, ship_position)

Easily fixed:

    def fire_missile(self, saucer, ship_or_none, fleets):
        if result := saucer.missile_tally < u.SAUCER_MISSILE_LIMIT:
            ship_position = self.select_aiming_point(saucer, ship_or_none)
            self.select_missile(random.random(), fleets, saucer, ship_position)
        return result

However, the test still fails. I hate when that happens. Grr. Creating a new Gunner for each call won’t work.

    def test_timer_reset(self):
        fleets = Fleets()
        fi = FI(fleets)
        saucer = Saucer()
        saucer.missile_tally = u.SAUCER_MISSILE_LIMIT
        gunner = Gunner()
        gunner.fire(u.SAUCER_MISSILE_DELAY,saucer, None, fleets)
        assert not fi.missiles
        saucer.missile_tally = 0
        gunner.fire(0.1, saucer, None, fleets)
        assert fi.missiles

Still fails with my fix removed. Put fix back in. Test passes. Yay, me!

Commit: test and ensure that Saucer firing timer is properly reset.

Now our mission here is to (try to) change Gunner so that its algorithm is as described:

Try to fire on every cycle;
If the timer has expired, fire a missile if one is available;
If we are always targeting, fire a targeted missile;
If the odds are not in our favor, fire a targeted missile;
Otherwise, fire in a random direction (or at a random point).

We presently have this:

    def fire(self, delta_time, saucer, ship_or_none: Ship | None, fleets):
        self._timer.tick(delta_time, self.fire_missile, saucer, ship_or_none, fleets)

    def fire_missile(self, saucer, ship_or_none, fleets):
        if result := saucer.missile_tally < u.SAUCER_MISSILE_LIMIT:
            ship_position = self.select_aiming_point(saucer, ship_or_none)
            self.select_missile(random.random(), fleets, saucer, ship_position)
        return result

Let’s rename:

    def fire(self, delta_time, saucer, ship_or_none: Ship | None, fleets):
        self._timer.tick(delta_time, self.fire_if_missile_available, saucer, ship_or_none, fleets)

    def fire_if_missile_available(self, saucer, ship_or_none, fleets):
        if result := saucer.missile_tally < u.SAUCER_MISSILE_LIMIT:
            ship_position = self.select_aiming_point(saucer, ship_or_none)
            self.select_missile(random.random(), fleets, saucer, ship_position)
        return result

Extract:

    def fire(self, delta_time, saucer, ship_or_none: Ship | None, fleets):
        self._timer.tick(delta_time, self.fire_if_missile_available, saucer, ship_or_none, fleets)

    def fire_if_missile_available(self, saucer, ship_or_none, fleets):
        if result := saucer.missile_tally < u.SAUCER_MISSILE_LIMIT:
            self.fire_available_missile(fleets, saucer, ship_or_none)
        return result

    def fire_available_missile(self, fleets, saucer, ship_or_none):
        ship_position = self.select_aiming_point(saucer, ship_or_none)
        self.select_missile(random.random(), fleets, saucer, ship_position)

Should we commit these tiny changes? Yes, we should, because they are correct and tiny steps toward better. What if we decide we don’t like this? We’ll take more tiny steps toward better.

Commit: rename, extract.

We’re about ready for item 3:

Try to fire on every cycle;
If the timer has expired, fire a missile if one is available;
If we are always targeting, fire a targeted missile;
If the odds are not in our favor, fire a targeted missile;
Otherwise, fire in a random direction (or at a random point).

But there’s an issue. This is part of why I abandoned my plan and went for these changes.

The select_aiming_point code does this:

    def select_aiming_point(self, saucer, ship_or_none):
        if ship_or_none:
            return self.choose_aiming_point(saucer, ship_or_none)
        else:
            return self.random_position()

    def choose_aiming_point(self, saucer, ship):
        delta_position = ship.position - saucer.position
        aim_time = self.time_to_target(delta_position, ship.velocity)
        return ship.position + ship.velocity * aim_time

A, this doesn’t match our outline: we’re not checking targeting to decide what to do. And B, when we do get a point back from here, we do select_missile. (These similar names are killing me.)

    def select_missile(self, chance_of_targeting, fleets, saucer, ship_position):
        if saucer.always_target or chance_of_targeting <= u.SAUCER_TARGETING_FRACTION:
            velocity_adjustment = Vector2(0, 0)
        else:
            velocity_adjustment = saucer.velocity
        self.create_targeted_missile(saucer.position, ship_position, velocity_adjustment, fleets)

    def create_targeted_missile(self, from_position, to_position, velocity_adjustment, fleets):
        best_aiming_point = self.best_aiming_point(from_position, to_position, u.SCREEN_SIZE)
        angle = self.angle_to_hit(best_aiming_point, from_position)
        missile = self.missile_at_angle(from_position, angle, velocity_adjustment)
        fleets.append(missile)

The first bit is that velocity hack. It sort of randomizes the shot but not as our outline intends. But then, create_targeted_missile calls best_aiming_point!

Remember that? That’s the code that fires across the border if the target is closer that way. Good idea … unless we’re targeting, in which case it’s guaranteed to throw the missile off target.

We should select the best aiming point before we ever call the aiming code.

I’d really like to rewrite this, but I think I’d better stick to small steps.

In this function, we should check for the best aiming point:

    def choose_aiming_point(self, saucer, ship):
        delta_position = ship.position - saucer.position
        aim_time = self.time_to_target(delta_position, ship.velocity)
        return ship.position + ship.velocity * aim_time

Easily done. Extract variable:

    def choose_aiming_point(self, saucer, ship):
        target_position = ship.position
        delta_position = target_position - saucer.position
        aim_time = self.time_to_target(delta_position, ship.velocity)
        return target_position + ship.velocity * aim_time

Commit: extract variable.

Call the optimal point choosing thingie:

    def choose_aiming_point(self, saucer, ship):
        target_position = self.best_aiming_point(saucer.position, ship.position, u.SCREEN_SIZE)
        delta_position = target_position - saucer.position
        aim_time = self.time_to_target(delta_position, ship.velocity)
        return target_position + ship.velocity * aim_time

And remove the other call:

    def create_targeted_missile(self, from_position, to_position, velocity_adjustment, fleets):
        angle = self.angle_to_hit(to_position, from_position)
        missile = self.missile_at_angle(from_position, angle, velocity_adjustment)
        fleets.append(missile)

This is not a legitimate refactoring. The tests are good. Let me try the game just to see if it’s OK. Seems OK. I’ll commit but try to think of better tests. I’m sure this is closer to what we want.

Commit: move target wrap selection ahead of aiming.

Let’s see where we are relative to my numbered plan:

Try to fire on every cycle;
If the timer has expired, fire a missile if one is available;
If we are always targeting, fire a targeted missile;
If the odds are not in our favor, fire a targeted missile;
Otherwise, fire in a random direction (or at a random point).

    def fire_if_missile_available(self, saucer, ship_or_none, fleets):
        if result := saucer.missile_tally < u.SAUCER_MISSILE_LIMIT:
            self.fire_available_missile(fleets, saucer, ship_or_none)
        return result

    def fire_available_missile(self, fleets, saucer, ship_or_none):
        ship_position = self.select_aiming_point(saucer, ship_or_none)
        self.select_missile(random.random(), fleets, saucer, ship_position)

    def select_aiming_point(self, saucer, ship_or_none):
        if ship_or_none:
            return self.choose_aiming_point(saucer, ship_or_none)
        else:
            return self.random_position()

The plan is incomplete as we see from the code. We have to consider whether there is a ship to target.

Try to fire on every cycle;
If the timer has expired, fire a missile if one is available;
Set target
1. if no ship: random
2. if always targeting: targeted
3. if odds not in our favor: targeted
4. else random
Set velocity adjustment
1. if targeted: (0,0)
2. else saucer velocity

I think I’m going to let this code have its duplication and then see how to remove it.

    def select_aiming_point(self, saucer, ship_or_none):
        if ship_or_none:
            return self.choose_aiming_point(saucer, ship_or_none)
        else:
            return self.random_position()

Invert if:

    def select_aiming_point(self, saucer, ship_or_none):
        if not ship_or_none:
            return self.random_position()
        else:
            return self.choose_aiming_point(saucer, ship_or_none)

Now to add in items from 3:

    def select_aiming_point(self, saucer, ship_or_none):
        if not ship_or_none:
            return self.random_position()
        elif saucer.always_target:
            return self.choose_aiming_point(saucer, ship_or_none)
        elif random.random() < u.SAUCER_TARGETING_FRACTION:
            return self.choose_aiming_point(saucer, ship_or_none)
        else:
            return self.random_position()

    def choose_aiming_point(self, saucer, ship):
        target_position = self.best_aiming_point(saucer.position, ship.position, u.SCREEN_SIZE)
        delta_position = target_position - saucer.position
        aim_time = self.time_to_target(delta_position, ship.velocity)
        return target_position + ship.velocity * aim_time

Rename:

    def select_aiming_point(self, saucer, ship_or_none):
        if not ship_or_none:
            return self.random_position()
        elif saucer.always_target:
            return self.optimal_aiming_point(saucer, ship_or_none)
        elif random.random() < u.SAUCER_TARGETING_FRACTION:
            return self.optimal_aiming_point(saucer, ship_or_none)
        else:
            return self.random_position()

    def optimal_aiming_point(self, saucer, ship):
        target_position = self.closest_aiming_point(saucer.position, ship.position, u.SCREEN_SIZE)
        delta_position = target_position - saucer.position
        aim_time = self.time_to_target(delta_position, ship.velocity)
        return target_position + ship.velocity * aim_time

So we are back here with the point we want to hit:

    def fire_available_missile(self, fleets, saucer, ship_or_none):
        ship_position = self.select_aiming_point(saucer, ship_or_none)
        self.select_missile(random.random(), fleets, saucer, ship_position)

    def select_missile(self, chance_of_targeting, fleets, saucer, ship_position):
        if saucer.always_target or chance_of_targeting <= u.SAUCER_TARGETING_FRACTION:
            velocity_adjustment = Vector2(0, 0)
        else:
            velocity_adjustment = saucer.velocity
        self.create_targeted_missile(saucer.position, ship_position, velocity_adjustment, fleets)

But the select_missile won’t work, because we have already rolled the dice. For now, let’s just never adjust velocity:

    def select_missile(self, chance_of_targeting, fleets, saucer, ship_position):
        velocity_adjustment = Vector2(0, 0)
        self.create_targeted_missile(saucer.position, ship_position, velocity_adjustment, fleets)

    def create_targeted_missile(self, from_position, to_position, velocity_adjustment, fleets):
        angle = self.angle_to_hit(to_position, from_position)
        missile = self.missile_at_angle(from_position, angle, velocity_adjustment)
        fleets.append(missile)

A test has broken, because we’re testing that flag. Undo that last change and rethink.

We are passing in chance_of_targeting. Let’s roll the dice higher up and pass them to the select select aiming point code:

    def fire_if_missile_available(self, saucer, ship_or_none, fleets):
        if result := saucer.missile_tally < u.SAUCER_MISSILE_LIMIT:
            chance = random.random()
            self.fire_available_missile(chance, fleets, saucer, ship_or_none)
        return result

    def fire_available_missile(self, chance, fleets, saucer, ship_or_none):
        ship_position = self.select_aiming_point(chance, saucer, ship_or_none)
        self.select_missile(chance, fleets, saucer, ship_position)

    def select_aiming_point(self, chance, saucer, ship_or_none):
        if not ship_or_none:
            return self.random_position()
        elif saucer.always_target:
            return self.optimal_aiming_point(saucer, ship_or_none)
        elif chance < u.SAUCER_TARGETING_FRACTION:
            return self.optimal_aiming_point(saucer, ship_or_none)
        else:
            return self.random_position()

    def select_missile(self, chance_of_targeting, fleets, saucer, ship_position):
        if saucer.always_target or chance_of_targeting <= u.SAUCER_TARGETING_FRACTION:
            velocity_adjustment = Vector2(0, 0)
        else:
            velocity_adjustment = saucer.velocity
        self.create_targeted_missile(saucer.position, ship_position, velocity_adjustment, fleets)

This is working and the tests are green. I think this is at least in part due to the weakness of the tests. There are, however, two tests on select_missile, which is reassuring:

    def test_large_saucer_does_not_target(self):
    def test_small_saucer_does_target(self):

I would like to fix the coupling between the target selection and optimization, and this velocity adjustment thing. One very simple thing would be never to adjust velocity, giving the saucer missiles that always go at the same speed. That will break a test but I think it might be better. We could make a story for putting the capability back and find a better way if we choose the story.

I’ll inline select_missile in Gunner.

    def fire_available_missile(self, chance, fleets, saucer, ship_or_none):
        ship_position = self.select_aiming_point(chance, saucer, ship_or_none)
        if saucer.always_target or chance <= u.SAUCER_TARGETING_FRACTION:
            velocity_adjustment = Vector2(0, 0)
        else:
            velocity_adjustment = saucer.velocity
        self.create_targeted_missile(saucer.position, ship_position, velocity_adjustment, fleets)

And now set the adjustment to zero always:

    def fire_available_missile(self, chance, fleets, saucer, ship_or_none):
        ship_position = self.select_aiming_point(chance, saucer, ship_or_none)
        velocity_adjustment = Vector2(0, 0)
        self.create_targeted_missile(saucer.position, ship_position, velocity_adjustment, fleets)

Now all saucer missiles should go at the standard speed. They do. And the sauce just shot me down with a wrapped shot. Nice.

I’m going to commit this. I am confident that it is working as intended. Let’s do one more thing, though, rename that method that I inlined to show that it is not used.

    def select_missile_UNUSED_METHOD(self, chance_of_targeting, fleets, saucer, ship_position):
        if saucer.always_target or chance_of_targeting <= u.SAUCER_TARGETING_FRACTION:
            velocity_adjustment = Vector2(0, 0)
        else:
            velocity_adjustment = saucer.velocity
        self.create_targeted_missile(saucer.position, ship_position, velocity_adjustment, fleets)

I’m thinking that will remind me to do as the commit is about to suggest:

Commit: Gunner code much improved, Needs better testing. See select_missile_UNUSED_METHOD for example.

I am tired. We’ll sum up.

Summary

First summary item: when tired, stop programming. No good can come of it.

I had what seemed like a good idea for adjusting the aimed missiles’ velocity to give them a better chance of hitting the ship. But the code’s arrangement did not support that idea. Looking at the code, I saw that it was also in need of a bit of refactoring.

I had not completed the task of ensuring that targeting calculations were done only when we were really going to fire. That wasn’t strictly necessary but it was a bit inefficient and a bit confusing to create values and then ignore them.

That showed me that the Gunner code, though its external behavior was OK, wasn’t really doing what we wanted in terms of targeting. I can’t explain what it did, but what it did wasn’t what we wanted.¹

So we planned the steps the code should take, and refactored (and changed logic) so that it now, I’m rather confident, does what was intended. And the tests all run. They could be better, but they’re not bad.

We intentionally inlined a tested function, to pull out the velocity adjustment code: we now always ignore saucer velocity when firing a saucer missile. There are still two tests testing the method that was inlined. They should be recast to test something useful, or should be thrown away.

I made the decision to break a “feature”, which was that unaimed missiles accrued saucer velocity, so that saucer missiles used to travel at different speeds, and now they do not.

It looks fine in the game and if we ever do want it, we’ll put it in in a better place.

It’s not that the bear bit me today: the coding went well and the results are good. It’s just that my plan, to further optimize targeting, was put completely on hold so that we could improve the Gunner.

I am not satisfied with Gunner even now. In addition to the unused method, which should be fixed or removed, there is still oddness around the velocity adjustment, which probably should be removed if we’re not going to have it, and better dealt with if we decide to put it back.

The names, the names still bug me, but they are better. I wonder whether we have more than one object tangled up in here.

In terms of the product, we’re fine. But our purpose here isn’t to ship the product, it is to explore how to make code that we have better, so that we can keep our product alive. We’d have settled for what we have in Gunner ages ago if this were a product. It worked well enough. We could save this new stuff for Asteroids II: Death Saucer!!! or something.

The changes today were all tiny and safe. Once, something broke and I immediately rolled it back out. Otherwise we inched toward better … and that’s how you do that!

See you next time!

It’s not that I can’t make up my mind whether to write “I” or “we”. I write “I” when I’m talking about something questionable or mistaken that I did, and “we” when things are going well and I want you to share in the pleasure. I’m just thinking of you, all the time. Hell of a guy, right? ↩