Python Asteroids on GitHub

It’s 0430 hours, commonly called oh dark thirty. I have a desperate need to clear my mind and get back to sleep. I try some things and settle on none. I learn some things but no net code change.

Warning
This is long and I reach no final conclusion. If you skip out, I won’t be offended. What you can find here is a lot of thinking about code and trying things. But there’s a bit of why are you working at this hour effect in here. I wasn’t at my best, knew that going in.

In the ShotOptimizer object, which aims saucer missiles at the ship, there is this code:

    def lead_the_target(self, best_target_position, safe_distance, shooter_position):
        target_position = best_target_position
        for _ in range(3):
            target_position = self.improved_aiming_point(
                target_position,
                self.ship.velocity,
                best_target_position,
                shooter_position,
                u.MISSILE_SPEED,
                safe_distance)
        return target_position

    @staticmethod
    def improved_aiming_point(
            initial_aiming_point,
            target_velocity,
            target_starting_position,
            gunner_position,
            missile_speed,
            missile_offset):
        missile_travel_distance = gunner_position.distance_to(initial_aiming_point) - missile_offset
        missile_travel_time = missile_travel_distance / missile_speed
        target_motion = missile_travel_time * target_velocity
        better_aiming_point = target_starting_position + target_motion
        return better_aiming_point

I happen to know what this does. Given the best_target_position, which is either the ship’s actual position or a position over the border that is a better shot, this code calls improved_aiming_point three times, so as to lead the target (aim ahead of it so as to allow for missile flight time).

I think I can even explain improved_aiming_point. Basically, it works like this:

  1. Figure out how long it will take a missile to get to the aiming point;
  2. Figure out where the ship will be at that time;
  3. Return that point as a better aiming point.

It gets the time to the current aiming point by dividing the distance to that point by the missile’s speed. The distance to the aiming point is its true distance, minus the missile offset, which is the distance away from the saucer where the missile starts. If it started right where the saucer is, it would destroy the saucer.

It gets where the ship will be at that time by adding how far the missile will travel in that time to the ship’s position. The distance the ship will travel is, of course, its velocity times the time.

We repeat that process three times because each step improves the aiming point a bit, and three has been shown to be good enough, by experimentation.

Why is this fairly simple scheme not obvious in the code? Names might be part of it. I’ve used generic terms here, gunner and target, because I had originally worked on the code on my iPad, where typing is pretty much not the thing. Let’s rename some things here:

    @staticmethod
    def improved_aiming_point(
            initial_aiming_point,
            ship_velocity,
            ship_position,
            saucer_position,
            missile_speed,
            missile_offset):
        missile_travel_distance = saucer_position.distance_to(initial_aiming_point) - missile_offset
        missile_travel_time = missile_travel_distance / missile_speed
        ship_motion = missile_travel_time * ship_velocity
        better_aiming_point = ship_position + ship_motion
        return better_aiming_point

I was tempted to call better_aiming_point something like ship_position_when_missile_gets_there but it’s so long. How about this:

    @staticmethod
    def improved_aiming_point(
            initial_aiming_point,
            ship_velocity,
            ship_position,
            saucer_position,
            missile_speed,
            missile_offset):
        missile_travel_distance = saucer_position.distance_to(initial_aiming_point) - missile_offset
        missile_travel_time = missile_travel_distance / missile_speed
        ship_motion = missile_travel_time * ship_velocity
        anticipated_ship_position = ship_position + ship_motion
        return anticipated_ship_position

Maybe better. Let’s try some extractions.

    @staticmethod
    def improved_aiming_point(
            initial_aiming_point,
            ship_velocity,
            ship_position,
            saucer_position,
            missile_speed,
            missile_offset):
        missile_travel_time = ShotOptimizer.time_to_target(initial_aiming_point, missile_offset, missile_speed, saucer_position)
        anticipated_ship_position = ShotOptimizer.ship_moves_to(missile_travel_time, ship_position, ship_velocity)
        return anticipated_ship_position

    @staticmethod
    def ship_moves_to(missile_travel_time, ship_position, ship_velocity):
        ship_motion = missile_travel_time * ship_velocity
        anticipated_ship_position = ship_position + ship_motion
        return anticipated_ship_position

    @staticmethod
    def time_to_target(initial_aiming_point, missile_offset, missile_speed, saucer_position):
        missile_travel_distance = saucer_position.distance_to(initial_aiming_point) - missile_offset
        missile_travel_time = missile_travel_distance / missile_speed
        return missile_travel_time

I like the idea but not the code.

  1. The fact that the improve_aiming_point is marked static has forced the word ShotOptimizer into the code, making it harder to read.
  2. The argument lists are long and make it difficult to see what matters.

I can’t readily change back from @staticmethod because some tests know the method is static.

Bag that. I’ll reverse this out, make the method non-static, and see how it looks in better form. We’ll commit the renames above and then plan to roll back if we need to later.

Undo done, commit: tidying names in improved_aiming_point.

Now make the method non-static and extract again. I plan to roll this back: I just want to see it.

    def improved_aiming_point(
            self,
            initial_aiming_point,
            ship_velocity,
            ship_position,
            saucer_position,
            missile_speed,
            missile_offset):
        missile_travel_time = self.time_to_target(initial_aiming_point, missile_offset, missile_speed, saucer_position)
        anticipated_ship_position = self.ship_moves_to(missile_travel_time, ship_position, ship_velocity)
        return anticipated_ship_position

    def time_to_target(self, initial_aiming_point, missile_offset, missile_speed, saucer_position):
        missile_travel_distance = saucer_position.distance_to(initial_aiming_point) - missile_offset
        missile_travel_time = missile_travel_distance / missile_speed
        return missile_travel_time

    def ship_moves_to(self, missile_travel_time, ship_position, ship_velocity):
        ship_motion = missile_travel_time * ship_velocity
        anticipated_ship_position = ship_position + ship_motion
        return anticipated_ship_position

Is all this even worthwhile, you’re asking yourself. Here and now, possibly not, but working out how to make something like this understandable surely is. And if we ever want to improve targeting, we’re going to wish we could understand this.

One interesting fact comes to mind. Over the course of our three iterations, most of the parameters to improved_aiming_point do not vary:

    def lead_the_target(self, best_target_position, safe_distance, shooter_position):
        target_position = best_target_position
        for _ in range(3):
            target_position = self.improved_aiming_point(
                target_position,
                self.ship.velocity,
                best_target_position,
                shooter_position,
                u.MISSILE_SPEED,
                safe_distance)
        return target_position

Perhaps we need another small object. If we were to create, um, an AimAdjuster, it could save those variables and refer to them with self. That would reduce the calling sequences.

But I have another idea. Roll back. Look at this:

    def lead_the_target(self, best_target_position, safe_distance, shooter_position):
        target_position = best_target_position
        for _ in range(3):
            target_position = self.improved_aiming_point(
                target_position,
                self.ship.velocity,
                best_target_position,
                shooter_position,
                u.MISSILE_SPEED,
                safe_distance)
        return target_position

    @staticmethod
    def improved_aiming_point(
            initial_aiming_point,
            ship_velocity,
            ship_position,
            saucer_position,
            missile_speed,
            missile_offset):
        missile_travel_distance = saucer_position.distance_to(initial_aiming_point) - missile_offset
        missile_travel_time = missile_travel_distance / missile_speed
        ship_motion = missile_travel_time * ship_velocity
        anticipated_ship_position = ship_position + ship_motion
        return anticipated_ship_position

What if we were to inline improved_aiming_point? I have to remove the decorator but then:

    def lead_the_target(self, best_target_position, safe_distance, shooter_position):
        target_position = best_target_position
        for _ in range(3):
            missile_travel_distance = shooter_position.distance_to(target_position) - safe_distance
            missile_travel_time = missile_travel_distance / u.MISSILE_SPEED
            ship_motion = missile_travel_time * self.ship.velocity
            anticipated_ship_position = best_target_position + ship_motion
            target_position = anticipated_ship_position
        return target_position

Now let’s extract!

    def lead_the_target(self, ship_position, saucer_position, missile_offset):
        target_position = ship_position
        for _ in range(3):
            missile_travel_time = self.time_to_target(saucer_position, target_position, missile_offset)
            anticipated_ship_position = self.ship_moves_to(ship_position, missile_travel_time)
            target_position = anticipated_ship_position
        return target_position

    def ship_moves_to(self, best_target_position, missile_travel_time):
        ship_motion = missile_travel_time * self.ship.velocity
        anticipated_ship_position = best_target_position + ship_motion
        return anticipated_ship_position

    def time_to_target(self, shooter_position, target_position, safe_distance):
        missile_travel_distance = shooter_position.distance_to(target_position) - safe_distance
        missile_travel_time = missile_travel_distance / u.MISSILE_SPEED
        return missile_travel_time

I did a little renaming in there as well.

I almost like this. Roll back and try again. Again, we start here:

    def lead_the_target(self, best_target_position, safe_distance, shooter_position):
        target_position = best_target_position
        for _ in range(3):
            target_position = self.improved_aiming_point(
                target_position,
                self.ship.velocity,
                best_target_position,
                shooter_position,
                u.MISSILE_SPEED,
                safe_distance)
        return target_position

    @staticmethod
    def improved_aiming_point(
            initial_aiming_point,
            ship_velocity,
            ship_position,
            saucer_position,
            missile_speed,
            missile_offset):
        missile_travel_distance = saucer_position.distance_to(initial_aiming_point) - missile_offset
        missile_travel_time = missile_travel_distance / missile_speed
        ship_motion = missile_travel_time * ship_velocity
        anticipated_ship_position = ship_position + ship_motion
        return anticipated_ship_position

I do understand why we might want to refer to shooter and target, for generality. We might want to use this object for some other shooting problem someday. We’ll see what we get. Try these renamings:

    def lead_the_target(self, initial_target_position, shooter_position, missile_offset):
        aiming_point = initial_target_position
        for _ in range(3):
            aiming_point = self.improved_aiming_point(
                aiming_point,
                self.ship.velocity,
                initial_target_position,
                shooter_position,
                u.MISSILE_SPEED,
                missile_offset)
        return aiming_point

OK, now inline.

    def lead_the_target(self, initial_target_position, shooter_position, missile_offset):
        aiming_point = initial_target_position
        for _ in range(3):
            missile_travel_distance = shooter_position.distance_to(aiming_point) - missile_offset
            missile_travel_time = missile_travel_distance / u.MISSILE_SPEED
            ship_motion = missile_travel_time * self.ship.velocity
            anticipated_ship_position = initial_target_position + ship_motion
            aiming_point = anticipated_ship_position
        return aiming_point

Inline once:

    def lead_the_target(self, initial_target_position, shooter_position, missile_offset):
        aiming_point = initial_target_position
        for _ in range(3):
            missile_travel_time = self.time_to_aiming_point(aiming_point, shooter_position, missile_offset)
            ship_motion = missile_travel_time * self.ship.velocity
            anticipated_ship_position = initial_target_position + ship_motion
            aiming_point = anticipated_ship_position
        return aiming_point

    def time_to_aiming_point(self, aiming_point, shooter_position, missile_offset):
        missile_travel_distance = shooter_position.distance_to(aiming_point) - missile_offset
        missile_travel_time = missile_travel_distance / u.MISSILE_SPEED
        return missile_travel_time

Inline again:

    def lead_the_target(self, initial_target_position, shooter_position, missile_offset):
        aiming_point = initial_target_position
        for _ in range(3):
            missile_travel_time = self.time_to_aiming_point(aiming_point, shooter_position, missile_offset)
            anticipated_ship_position = self.anticipated_ship_position(initial_target_position, missile_travel_time)
            aiming_point = anticipated_ship_position
        return aiming_point

    def anticipated_ship_position(self, initial_target_position, missile_travel_time):
        ship_motion = missile_travel_time * self.ship.velocity
        anticipated_ship_position = initial_target_position + ship_motion
        return anticipated_ship_position

Inline variable:

    def lead_the_target(self, initial_target_position, shooter_position, missile_offset):
        aiming_point = initial_target_position
        for _ in range(3):
            missile_travel_time = self.time_to_aiming_point(aiming_point, shooter_position, missile_offset)
            aiming_point = self.anticipated_ship_position(initial_target_position, missile_travel_time)
        return aiming_point

    def time_to_aiming_point(self, aiming_point, shooter_position, missile_offset):
        missile_travel_distance = shooter_position.distance_to(aiming_point) - missile_offset
        missile_travel_time = missile_travel_distance / u.MISSILE_SPEED
        return missile_travel_time

    def anticipated_ship_position(self, initial_target_position, missile_travel_time):
        ship_motion = missile_travel_time * self.ship.velocity
        anticipated_ship_position = initial_target_position + ship_motion
        return anticipated_ship_position

Is this better? I think it is. I made most, if not all these changes with machine refactorings, but I have lost the connection between the tests and this code, because the tests still call the unchanged static method.

I am still not satisfied. I don’t like the reference to self.ship.velocity down there in anticipated_ship_position.

I’m going to roll back again and call it a morning, after some reflection.

Reflection

I do prefer the inlined version with the extractions, except for the reference to ship.velocity down at the bottom. We had a parameter there and I wish we had kept it.

Looking at this:

    def lead_the_target(self, initial_target_position, shooter_position, missile_offset):
        aiming_point = initial_target_position
        for _ in range(3):
            missile_travel_time = self.time_to_aiming_point(aiming_point, shooter_position, missile_offset)
            aiming_point = self.anticipated_ship_position(initial_target_position, missile_travel_time)
        return aiming_point

That code updates aiming point based on aiming point. It’s an iterative solution, of course. But to me the code doesn’t really express the iterative character as well as it might.

What would be better? Well, what if it could look like this:

for _iteration in range(3):
	aiming_point += self.adjustment_to_aim( etc etc)
return aiming_point

That would be helpful. I’m sure we could get there, though I’m not entirely sure we’d like the code inside. The adjustment is basically the difference between our new aiming point and the one we came in with, so we could certainly get the code into that shape.

Note
This is actually a good idea, I think, even at the end of the next article. Why? It expresses the iterative updating idea. But it does lead to a really weird defect.

Oh, I think I see an approach. Let me write it down here lest I forget. No, let me try it.

    def lead_the_target(self, best_target_position, safe_distance, shooter_position):
        target_position = best_target_position
        for _ in range(3):
            target_position += self.aim_adjustment(
                target_position,
                self.ship.velocity,
                best_target_position,
                shooter_position,
                u.MISSILE_SPEED,
                safe_distance)
        return target_position

    @staticmethod
    def aim_adjustment(
            initial_aiming_point,
            ship_velocity,
            ship_position,
            saucer_position,
            missile_speed,
            missile_offset):
        missile_travel_distance = saucer_position.distance_to(initial_aiming_point) - missile_offset
        missile_travel_time = missile_travel_distance / missile_speed
        ship_motion = missile_travel_time * ship_velocity
        anticipated_ship_position = ship_position + ship_motion
        return anticipated_ship_position - initial_aiming_point

All I did there was subtract the initial aiming point from the new one. Then I renamed the method and put a += up in lead_the_target. Tests broke because they’re not expecting the adjustment, they’re expecting the actual.

I’m glad I worked on the tests. The += does not work. Look at this test, which checks the += vs simple +:

    def test_iterated_aiming_point(self):
        ship_position = Vector2(100, 100)
        ship_velocity = Vector2(10, 10)
        saucer_position = Vector2(0, 0)
        missile_speed = 100
        new_target = ship_position
        test = new_target
        for _ in range(3):
            test += ShotOptimizer.aim_adjustment(test, ship_velocity, ship_position, saucer_position, missile_speed, 0)
            new_target = new_target + ShotOptimizer.aim_adjustment(new_target, ship_velocity, ship_position, saucer_position, missile_speed, 0)
            assert test == new_target

That test fails.

One important thing will be to get the tests wired up so that they support the effort. Machine refactoring is solid but it wouldn’t take much of a mistake in any hand edits to break things. I need those tests. Probably. Depending what they really do.

For now, It’s 0640, I’ve been at this for two hours with some interruptions to deal with local issues. I wasn’t very clever when I started and I am less clever now. No harm no foul. We’ll try again later.

See you then …