Python 126 - PRs, and Gunner

I’ve received a PR or two from Jani, and then on to more work on Gunner,

Let me say up front that I haven’t had the luxury of working with a team that used pull requests, so my knowledge of Git and GitHub is as thin as I can survive. So when Rickard Lindberg sent me a pull request a while back, I thanked him and did not pull it. Today, I plan to learn enough to actually do the thing.

In PyCharm, it goes like this:

The menu Git > GitHub > View Pull Requests shows me the outstanding PRs:

Two PRs in a nice window

Double-clicking Jani’s PR, I get more detail, including two commits, on two files. Jani wanted to get rid of the magic number 11 in the ship’s missile firing.

open PR showing files etc

PyCharm will show me the diffs between his code and the current code.

In Missile, Jani has moved the radius member to a class variable. I thought it was already that way. Good idea in any case.

pr showing missile changes

And in Ship, Jani has used the missile radius in the firing code.

class Ship(Flyer):
    def missile_start(self):
        start_distance = self.radius + Missile.radius + 1
        offset = Vector2(start_distance, 0).rotate(-self._angle)
        return self.position + offset

Very bold move there, positioning the missile only one pixel outside the combined radii. It will work, however.

I find a command “Merge Pull Request”. That seems to make no change to my local. What I really wanted to do was to install it into my local and push it later. The GitHub project says it is merged there. OK, makes some sense.

See what I mean about never having done this?

I guess what I need to do now is to pull to my local. Ah. Git > Pull.

That was easy-ish.

Success! I’ve merged a pull request, and I think I’ve even done it almost correctly. The instructions were not as helpful as they might have been, because key icons that the help describes did not seem to be present in my viewer. Anyway … I do seem to have the same code as Jani submitted in both local and remote.

Everything seems to be OK. Let’s get back to Gunner.

Gunner

To figure out where we left off, I’ll review my Jira sticky notes and the tests.

The Jira notes are two:

Test missile tally
Change early tests to use create_missile with proper params

The first reminds me that I forgot to send the missile tally in on the fire method, and the second is suggesting that the earlier tests, which call fire, will probably start failing as the class gets closer to done, because create_missile lets us send in the random values, where fire and fire_missile do not. We’ll review the current code in a moment.

Tests right now include:

    def test_exists(self):
    def test_no_fire_on_short_time(self):
    def test_fire_on_time(self):
    def test_random_missile(self):

I just renamed those middle two to make it more clear what they do. Commit: rename tests.

Let’s look at Gunner in its entirety.

class Gunner:
    def __init__(self):
        self._timer = Timer(u.SAUCER_MISSILE_DELAY)
        self._radius = 20

    def fire(self, delta_time, saucer_position, saucer_velocity, ship_position, fleets):
        self._timer.tick(delta_time, self.fire_missile, saucer_position, saucer_velocity, ship_position, fleets)

    def fire_missile(self, saucer_position, saucer_velocity, ship_position, fleets):
        should_target = random.random()
        random_angle = random.random()
        self.create_missile(should_target, random_angle, saucer_position, saucer_velocity, ship_position, fleets)

    def create_missile(self, should_target, random_angle, saucer_position, saucer_velocity, ship_position, fleets):
        if should_target <= u.SAUCER_TARGETING_FRACTION:
            self.create_targeted_missile(saucer_position, ship_position, fleets)
        else:
            self.create_random_missile(random_angle, saucer_position, saucer_velocity, fleets)

    def create_random_missile(self, random_angle, saucer_position, saucer_velocity, fleets):
        missile = self.missile_at_angle(saucer_position, random_angle*360.0, saucer_velocity)
        fleets.add_flyer(missile)

    def missile_at_angle(self, position, desired_angle, velocity_adjustment):
        missile_velocity = Vector2(u.MISSILE_SPEED, 0).rotate(desired_angle) + velocity_adjustment
        offset = Vector2(2 * self._radius, 0).rotate(desired_angle)
        return Missile.from_saucer(position + offset, missile_velocity)

    def create_targeted_missile(self, saucer_position, ship_position, fleets):
        missile = Missile.from_saucer(Vector2(-5, -5), Vector2(0, 0))
        fleets.add_flyer(missile)

I think that flows pretty nicely. Let’s drive out the missile tally. The rules are that we can only have two live missiles at a time. So if we try to fire three, there should only be two in the final list of saucer missiles.

We can put the check in fire_missile which will only be called if the timer has ticked down.

I will need a new parameter in fire_missile. My test for this is:

    def test_can_only_fire_limited_number(self):
        no_target = 0.5
        angle = 0.0
        fleets = Fleets()
        fi = FI(fleets)
        sauce_position = Vector2(500, 500)
        velocity = Vector2(0, 0)
        ship_position = Vector2(0, 0)
        count = u.SAUCER_MISSILE_LIMIT
        Gunner().fire_missile(count, sauce_position, velocity, ship_position, fleets)
        assert not fi.saucer_missiles

I think this is right. Of course now I need a new signature for fire_missile. Change Signature:

    def fire_missile(self, missile_tally, saucer_position, saucer_velocity, ship_position, fleets):
        if missile_tally >= u.SAUCER_MISSILE_LIMIT:
            return
        should_target = random.random()
        random_angle = random.random()
        self.create_missile(should_target, random_angle, saucer_position, saucer_velocity, ship_position, fleets)

I put in the check while I was there. But we don’t have the tally coming in. Two tests are failing.

Let’s fix up fire while we’re here. Another Change Signature, and a change inside.

    def fire(self, delta_time, missile_tally, saucer_position, saucer_velocity, ship_position, fleets):
        self._timer.tick(delta_time, self.fire_missile, missile_tally, saucer_position, saucer_velocity, ship_position, fleets)

One test failing. PyCharm guessed wrong on the change signature. Should have done the Change Signature before putting in my new call.

    def test_can_only_fire_limited_number(self):
        no_target = 0.5
        angle = 0.0
        fleets = Fleets()
        fi = FI(fleets)
        saucer_position = Vector2(500, 500)
        velocity = Vector2(0, 0)
        ship_position = Vector2(0, 0)
        count = u.SAUCER_MISSILE_LIMIT
        Gunner().fire_missile(count, saucer_position, velocity, ship_position, fleets)
        assert not fi.saucer_missiles

The test is green. Commit: Gunner checks missile tally. Commit goes and pushes. This is a relief, tells me that I didn’t mess up the pull too badly.

We could write a special test to be sure that we do fire when the tally is less than the limit but I’m comfortable with the existing tests.

It comes down to targeting. Our Gunner code will call this method to get a targeted missile:

    def create_targeted_missile(self, saucer_position, ship_position, fleets):
        missile = Missile.from_saucer(Vector2(-5, -5), Vector2(0, 0))
        fleets.add_flyer(missile)

The method is currently stubbed to create a useless missile, to keep everything running.

Regular missiles are fired like this:

    def create_random_missile(self, random_angle, saucer_position, saucer_velocity, fleets):
        missile = self.missile_at_angle(saucer_position, random_angle*360.0, saucer_velocity)
        fleets.add_flyer(missile)

    def missile_at_angle(self, position, desired_angle, velocity_adjustment):
        missile_velocity = Vector2(u.MISSILE_SPEED, 0).rotate(desired_angle) + velocity_adjustment
        offset = Vector2(2 * self._radius, 0).rotate(desired_angle)
        return Missile.from_saucer(position + offset, missile_velocity)

This is borrowed from the existing code. The trick is that regular missiles are fired such that the saucer’s velocity is added to the missile’s intrinsic velocity, so that they travel faster when fired ahead, slower behind, as one would expect. However, when we fire a targeted missile, we pass in a zero saucer velocity, so that the missile travels in the direction we want. Minor hack so that we don’t have to compute a strange angle.

So for the targeted missile we need the angle from the saucer to the ship. We have both positions.

Uh Oh: I think of an issue. What if there is no ship? Well, I think the rule is supposed to be that if there is no ship, fire will be called with ship_position equal to None. We have not implemented that feature. Where could it go? It should go into create_missile. We need a new test for this:

    def create_missile(self, should_target, random_angle, saucer_position, saucer_velocity, ship_position, fleets):
        if should_target <= u.SAUCER_TARGETING_FRACTION:
            self.create_targeted_missile(saucer_position, ship_position, fleets)
        else:
            self.create_random_missile(random_angle, saucer_position, saucer_velocity, fleets)

I create an empty test, test_no_ship_overrides_targeting, but I think we need to check inside the targeted missile code anyway, just as a belt and suspenders. So let’s set up a true targeted one and see what we have to do.

    def test_targeted(self):
        should_target = 0.1
        angle = 45
        fleets = Fleets()
        fi = FI(fleets)
        saucer_position = Vector2(500, 500)
        saucer_velocity = Vector2(100, 200)
        ship_position = Vector2(500, 550)
        Gunner().create_missile(should_target, angle, saucer_position, saucer_velocity, ship_position, fleets)
        missile = fi.saucer_missiles[0]
        assert missile.velocity_testing_only.x == 0
        assert missile.velocity_testing_only.y == u.MISSILE_SPEED

I think that should do it. Test fails with a zero in y, which is what my fake missile does. Speaking of Fake, let’s fake this a bit better.

    def create_targeted_missile(self, saucer_position, ship_position, fleets):
        missile = Missile.from_saucer(Vector2(-5, -5), Vector2(0, 666))
        fleets.add_flyer(missile)

You know what? I could just test this method. Why mess around with all that other stuff. Revise my test.

    def test_targeted(self):
        fleets = Fleets()
        fi = FI(fleets)
        saucer_position = Vector2(500, 500)
        ship_position = Vector2(500, 550)
        Gunner().create_targeted_missile(saucer_position, ship_position, fleets)
        missile = fi.saucer_missiles[0]
        assert missile.velocity_testing_only.x == 0
        assert missile.velocity_testing_only.y == u.MISSILE_SPEED

Better. Fails, of course, but I hope to see my 666.

Expected :166.66666666666666
Actual   :666.0

Perfect. Now, finally, the hard part.

Let’s review how it’s done now, in the Saucer code we’re aiming to replace:

    def suitable_missile(self, should_target, random_angle):
        if self.cannot_target_ship(should_target):
            return self.missile_at_angle(random_angle * 360.0, self._velocity)
        else:
            targeting_angle = self.angle_to(self._ship)
            velocity_adjustment = Vector2(0, 0)
            return self.missile_at_angle(targeting_angle, velocity_adjustment)

    def angle_to(self, ship):
        aiming_point = nearest_point(self.position, ship.position, u.SCREEN_SIZE)
        return Vector2(0, 0).angle_to(aiming_point - self.position)

def nearest_point(shooter, target, wrap_size):
    nearest_x = nearest(shooter.x, target.x, wrap_size)
    nearest_y = nearest(shooter.y, target.y, wrap_size)
    return Vector2(nearest_x, nearest_y)


def nearest(shooter, target, wrap_size):
    direct_distance = abs(target - shooter)
    target_wrap_left = target - wrap_size
    wrap_left_distance = abs(target_wrap_left - shooter)
    target_wrap_right = target + wrap_size
    wrap_right_distance = abs(target_wrap_right - shooter)
    if wrap_left_distance < direct_distance:
        return target_wrap_left
    elif wrap_right_distance < direct_distance:
        return target_wrap_right
    else:
        return target

I code this, guided by the existing code in Saucer:

    def create_targeted_missile(self, saucer_position, ship_position, fleets):
        aiming_point = nearest_point(saucer_position, ship_position, u.SCREEN_SIZE)
        angle = Vector2(0, 0).angle_to(aiming_point - saucer_position)
        missile = self.missile_at_angle(saucer_position, angle, Vector2(0, 0))
        fleets.add_flyer(missile)

And I copied the nearest functions over. We’ll want to clean that up before we’re done-done.

My test runs. Let’s do one more.

    def test_targeted_harder(self):
        fleets = Fleets()
        fi = FI(fleets)
        saucer_position = Vector2(500, 500)
        ship_position = Vector2(550, 550)
        Gunner().create_targeted_missile(saucer_position, ship_position, fleets)
        missile = fi.saucer_missiles[0]
        assert missile.velocity_testing_only.x == pytest.approx(missile.velocity_testing_only.y)
        assert missile.velocity_testing_only.y == pytest.approx(u.MISSILE_SPEED*0.707, 0.1)

I learned the pytest.approx feature just now. Test runs. Target is at 45 degrees, so velocity x and y are equal to full speed time the square root of two over two, as everyone knows.

Commit: Gunner now honors missile count and can target correctly.

I’ve been at it for two hours, so it’s time for a break. Next time, we’ll see about cleaning up this code and installing it in Saucer. It should be trivial, and I’m tempted to do it now, but I think a break would be wise.

Summary

The process of building the Gunner with TDD has gone quite smoothly. We even conveniently did other work while the Gunner was under construction, entirely safely because it’s not plugged in to anything else, but it is in the official repo.

Ah, there is a problem, though. We’re not checking ship_position anywhere. If anyone ever sends in a None, we’re doomed.

Let’s write a test and leave it red in my local.

    def test_handle_ship_position_none(self):
        do_target = 0.1
        angle = 0.0
        fleets = Fleets()
        fi = FI(fleets)
        saucer_position = Vector2(500, 500)
        velocity = Vector2(0, 0)
        ship_position = Vector2(0, 0)
        count = u.SAUCER_MISSILE_LIMIT
        Gunner().create_missile(do_target, 0.0, saucer_position, velocity, None, fleets)
        assert fi.saucer_missiles

That fails as expected:

    def nearest_point(shooter, target, wrap_size):
>       nearest_x = nearest(shooter.x, target.x, wrap_size)
E       AttributeError: 'NoneType' object has no attribute 'x'

The fix is so easy, let’s do it.

    def create_missile(self, should_target, random_angle, saucer_position, saucer_velocity, ship_position, fleets):
        if ship_position and should_target <= u.SAUCER_TARGETING_FRACTION:
            self.create_targeted_missile(saucer_position, ship_position, fleets)
        else:
            self.create_random_missile(random_angle, saucer_position, saucer_velocity, fleets)

Commit: Gunner does not target if no ship position provided.

That often happens to me. I think something is just too hard but after writing the test to be sure I don’t forget, it’s easy to fix and since there’s a test in place, my change is verified on the spot.

I do think we should double-check in targeted, and I’ve made a sticky note for that one.

Now can I rest? Thanks! See you next time for the thrilling installation of Gunner.