Python 111 - Order?

Python Asteroids on GitHub

This morning I have concerns about the order of events, and what they are. Let’s see if we need more clarity there. It goes well. Important footnotes.

Over the past few days, we’ve been implementing new Flyer subclasses and removing all the special Fleet subclasses. We now have the Fleets object holding a single Fleet instance. I think that Fleets and Fleet should probably coalesce into one container class, and we’ll get to that soon.

As I was reflecting on that, this morning as I began to wake up, I became concerned about the order of events. Let me check to see that I know what the order actually is:

class Game:
    def asteroids_tick(self, delta_time):
        self.fleets.tick(delta_time)
        self.control_game()
        self.process_interactions()
        self.draw_everything()

class Fleets:
    def tick(self, delta_time):
        if self._asteroids and self._ships:
            self.thumper.tick(delta_time)
        else:
            self.thumper.reset()
        self.flyers.tick(delta_time, self)

class Fleet:
    def tick(self, delta_time, fleets):
        for flyer in self:
            flyer.tick(delta_time, self, fleets)

We begin with tick, which, aside from the thumper bit, just ticks all the objects. What do they do when ticked?

• Asteroid moves.
• Explosion removes itself and creates Fragments.
• Fragments, next time around, tick their timer, and move.
• GameOver passes.
• Missile ticks its timer and moves.
• Saucer plays its sound, fires if it can, zig-zags if it’s time, and moves.
• SaucerMaker ticks its timer if the saucer is gone.
• Score passes.
• ScoreKeeper passes.
• Ship reads its controls and moves.
• ShipMaker ticks its ship creation timer if there’s no ship and the game isn’t over.
• WaveMaker ticks its timer if there are no asteroids.

So everyone who can move, moves. A number of them tick their action timer if a condition is met, typically whether there are any instances of some class present.

How do they know? This is where part of my concern lies. They know about instances, not because of anything that has happened in this cycle, because the begin-interact-end cycle has not yet run on this call to asteroids_tick. They are making use of information gleaned in a preceding cycle. That information may be out of date, it seems to me.

Perhaps deciding about ticking timers belongs at the end of the cycle, not the beginning.

Looking further, we find that process_interactions creates an Interactor and asks it to perform_interactions¹, and Interactor sends begin_interactions to Fleets, then it sends interact_with to each element of each pair of objects in Fleets, and then it sends end_interactions to Fleets.

No Flyer implements end_interactions. Five real flyers implement begin_interactions, each one initializing a flag or counter. The ones who implement begin also implement one or more interact_with_xxx methods, where they adjust the flag or counter.

Those are the flags or counters that are referenced in the next call to tick.

Now it seems to me that this sequence isn’t quite right, although it also seems to me that it isn’t wrong. That is, while it might make more sense to tick the timers in end_interactions if we find the flags still set, doing it at the beginning of the next cycle has the same effect, perhaps losing a 60th of a second on the timer.

What should the cycle be?

It seems to me that the cycle should be, at each time through:

1. Move all the objects that want to move;
2. Inform all the objects begin_interactions;
3. Interact all pairs interact_with;
4. Inform all the objects end_interactions;
5. Tick any timers;
6. Draw all the objects.

If an object wants to take an action based on its experience during interactions, it should take that action on end_interactions, not in the next tick. In fact, there’s no point to a tick method in Flyer, but there should be a move method.

We presently have three abstract methods in Flyer: interact_with, draw, and tick. Every Flyer must override those three methods. And there are many “event” methods that Flyer implements as pass, namely begin_interactions, end_interactions, and all the class-specific interact_with_xxx methods.

We are essentially saying that a Flyer must implement interact_with, draw, and tick, and may implement interact_with_xxx for any xxx it cares about.

Design improvement

I don’t see why we should require a draw method. It should be an optional event, like the interact_with_xxx methods. I think that tick should be renamed to move and that it should be optional, and last except for draw. And I think that objects that want to tick their timers should do so during end_interactions, or tick if we keep it. If we remove tick it would have to be in end_interactions. Recall that end_interactions is not used at all at present.

Does it make sense to do this?

Well, no, and yes. No, because the game is working fine, we have very few additional changes to make to “finish” it, and so “uh… everything’s perfectly all right now. We’re fine. We’re all fine here now, thank you. How are you?”²

Yes, because we are trying to understand this decentralized design, because we might, at least in principle, put another set of game objects in there, and the implementors of that game should have a sensible architecture to follow, and because we’re here to move code around, learning how to do that, and more importantly, that we can “always” improve our design without breaking things.

Anyway, you know I’m gonna do it.

The small steps

Some candidate small steps include:

1. Rename tick to move;
2. Implement a new cycle element move, with tick to be preserved until it’s not needed;
3. Make tick and move non-abstract, i.e. not required;
4. Incrementally move tick logic to end_interactions in Flyer subclasses;

I really like the idea of renaming tick to move everywhere in the Flyers. Let’s see what happens if we try a rename at the top. But a number of the Flyers already implement move and call it from their tick operation.

I think this means that we need to do the second idea above, add a move call while preserving tick, but we can’t call it move if it is to be sent to everyone, until everyone is ready.

OK, a tentative plan. First, rename all the existing move methods to be private, i.e. _move. Then we can insert the new move more freely.

That was relatively easy. Commit: Flyers now call private _move, leaving room for new move event.

Now I think I’d like to try moving the tick call to the end of the cycle rather than the beginning, just to see what happens. I think things may break.

class Game:
    def asteroids_tick(self, delta_time):
        self.control_game()
        self.process_interactions()
        self.fleets.tick(delta_time)
        self.draw_everything()

The tests all pass. Must try game though. Game seems fine. Commit: do tick after interactions rather than before.

I think that somehow I failed to actually execute the _move commit. No matter, it’s out there now.

Now let’s add a new move call to things. First, implement a pass move as part of Flyer, so that Flyers can go to the new scheme as they wish.

class Flyer:
    def move(self, delta_time, fleets):
        pass

Now call it. A question in my mind is whether all this sequencing should be in Game, or perhaps down in Fleets. We’ll leave it for now.

class Game:
    def asteroids_tick(self, delta_time):
        self.control_game()
        self.fleets.move(delta_time)
        self.process_interactions()
        self.fleets.tick(delta_time)
        self.draw_everything()

class Fleets:
    def move(self, delta_time):
        self.flyers.move(delta_time, self)

class Fleet:
    def move(self, delta_time, fleets):
        for flyer in self:
            flyer.move(delta_time, fleets)

I doubt that anyone wants the fleets parameter … well, saucer might remove itself if it moves off-screen. We’ll stay alert for that. Anyway the parameter is there if anyone wants it.

I noticed when changing asteroids_tick that it is never called by a test. This impels me to try the game, and to try to think of at least a smoke test for it. We’re trying to get this committed, so I’ll run the game. This is a risk. Game seems fine, which is almost guaranteed at this point, since move does nothing and we’re sure it’s called and defined all the way down. Commit: game cycle is now move, interact, tick, draw.

Now we can, at our convenience, change Flyers implementing _move to rely on the new move event call.

Old:

class Asteroid(Flyer):
    def _move(self, delta_time, _asteroids):
        self._location.move(delta_time)

    def tick(self, delta_time, fleet, _fleets):
        self._move(delta_time, fleet)

The tick method is abstract now, and required. So I can do this:

    def move(self, delta_time, _fleets):
        self._location.move(delta_time)

    def tick(self, delta_time, fleet, _fleets):
        pass

I expect this to work. Also I know there are only a couple of tests calling _move, so just renaming locally, as I did above, may cause tests to break. But not on Asteroid. All is well. Commit: Asteroid uses move, passes on tick.

How about Missile?

class Missile(Flyer):
    def _move(self, delta_time):
        self._location.move(delta_time)

    def tick(self, delta_time, fleet, _fleets):
        self.tick_timer(delta_time, fleet)
        self._move(delta_time)

This invites³ the question: For an object with a finite lifetime, like Missile, when should it tick its timer? Perhaps the convention should be that it is done on end_interactions? Or should we retain tick, perhaps with a new name like end_cycle.

I do not know the answer to the question but I can certainly do this for now:

    def move(self, delta_time, _fleets):
        self._location.move(delta_time)

    def tick(self, delta_time, fleet, _fleets):
        self.tick_timer(delta_time, fleet)

Commit: Missile uses move, ticks timer on tick.

So far so good. I am not pleased that there seem to be no tests exercising this code.

class Fragment(Flyer):
    def _move(self, delta_time):
        position = self.position + self.velocity * delta_time
        position.x = position.x % u.SCREEN_SIZE
        position.y = position.y % u.SCREEN_SIZE
        self.position = position
        self.theta += self.delta_theta*delta_time

    def tick(self, delta_time, fragments, _fleets):
        self.timer.tick(delta_time, fragments)
        self._move(delta_time)

That becomes:

    def move(self, delta_time, _fleets):
        position = self.position + self.velocity * delta_time
        position.x = position.x % u.SCREEN_SIZE
        position.y = position.y % u.SCREEN_SIZE
        self.position = position
        self.theta += self.delta_theta*delta_time

    def tick(self, delta_time, fragments, _fleets):
        self.timer.tick(delta_time, fragments)

And, praise be to pytest, a test breaks. It needs to say:

    def test_fragment_move(self):
        frag = Fragment(position=u.CENTER, angle=0, speed_mul=1, fragments=["ignored"])
        assert frag.velocity == Vector2(u.FRAGMENT_SPEED, 0)
        frag.move(0.1, [])
        assert frag.position == u.CENTER + Vector2(u.FRAGMENT_SPEED * 0.1, 0)

Not too exciting. Commit: Fragment uses move, ticks timer on tick.

class Saucer(Flyer):
    def _move(self, delta_time, saucers):
        off_x, off_y = self._location.move(delta_time)
        if off_x:
            if self in saucers:
                saucers.remove(self)

    def tick(self, delta_time, fleet, fleets):
        player.play("saucer_big", self._location, False)
        self.fire_if_possible(delta_time, fleets)
        self.check_zigzag(delta_time)
        self._move(delta_time, fleet)

This one is actually interesting. All three of fire, check, and _move should be done at the same time. I’d say they should be done in move. And as for the sound, it could be anywhere. The change will be a bit tricky. I plan to put in a new tick that just says pass, and to change this tick to be move. To make that work, I’ll have to change _move a bit, because we won’t have the specific fleet any more.

I remain concerned about the in. I suspect that bad things might happen without it, and they might be intermittent. Did someone put that in for a reason, or from an abundance of caution?

Let’s write the code we want.

    def move(self, delta_time,fleets):
        player.play("saucer_big", self._location, False)
        self.fire_if_possible(delta_time, fleets)
        self.check_zigzag(delta_time)
        self._move(delta_time, fleets)

    def tick(self, delta_time, fleet, fleets):
        pass

    def _move(self, delta_time, fleets):
        off_x, off_y = self._location.move(delta_time)
        if off_x:
            fleets.remove_flyer(self)

Let’s review Fleets.remove_flyer, though:

class Fleets:
    def remove_flyer(self, flyer):
        self.flyers.remove(flyer)

Fleet class saves the day:

class Fleet:
    def remove(self, flyer):
        if flyer in self.flyers:
            self.flyers.remove(flyer)

So remove is safe. This should work find. Test. We seem good. I am confident. Commit: saucer uses move, has pass in tick.

We’re left with Ship:

class Ship(Flyer):
    def _move(self, delta_time, _ships):
        self._location.move(delta_time)

    def tick(self, delta_time, fleet, fleets):
        self.control_motion(delta_time, fleet, fleets)
        self._move(delta_time, fleet)

The move should do what tick does, tick should pass. But there’s an issue, which is that fleet parameter. Move doesn’t have that. Where is it used?

class Ship(Flyer):
        if keys[pygame.K_SPACE]:
            self.enter_hyperspace_if_possible(fleet, fleets)
        else:
            self._can_enter_hyperspace = True

    def enter_hyperspace_if_possible(self, _ships_fleet, fleets):
        if not self._can_enter_hyperspace:
            return
        self._can_enter_hyperspace = False
        roll = random.randrange(0, 63)
        if self.hyperspace_failure(roll):
            self.explode(fleets)
        else:
            self.hyperspace_transfer()

Should have removed that parameter earlier. Do it now. Three tests fail. Two are trivial renames of the parameter. One is this:

    def test_vanish_at_edge(self):
        Saucer.init_for_new_game()
        saucer = Saucer()
        saucers = [saucer]
        assert saucer.position.x == 0
        saucer._move(1, saucers)
        assert saucers
        time = 0
        delta_time = 0.1
        while time < 10:
            time += delta_time
            saucer._move(delta_time=delta_time, fleets=saucers)
        assert not saucers

We don’t pass the saucers and can’t really check them. We need a Fleets and a FleetsInspector.

    def test_vanish_at_edge(self):
        Saucer.init_for_new_game()
        fleets = Fleets()
        fi = FI(fleets)
        saucer = Saucer()
        fleets.add_flyer(saucer)
        assert saucer.position.x == 0
        saucer.move(1, fleets)
        assert fi.saucers
        time = 0
        delta_time = 0.1
        while time < 10:
            time += delta_time
            saucer.move(delta_time=delta_time, fleets=fleets)
        assert not fi.saucers

OK, where were we? Right, making hyperspace not require the fleet. Now change this signature not to include fleet:

    def control_motion(self, delta_time, fleet, fleets):

Now we have this:

    def tick(self, delta_time, fleet, fleets):
        self.control_motion(delta_time, fleets)
        self._move(delta_time, fleet)

    def _move(self, delta_time, _ships):
        self._location.move(delta_time)

And we want this:

    def tick(self, delta_time, fleet, fleets):
        pass

    def move(self, delta_time,fleets):
        self.control_motion(delta_time, fleets)
        self._move(delta_time, fleets)

    def _move(self, delta_time, _fleets):
        self._location.move(delta_time)

We’ll improve that in a moment. Test. We’re good. Commit: ship uses move, passes on tick.

Now let’s inline. PyCharm tells me there is a test calling _move. I just let the refactoring go through. Tests still run. Somewhere there is a test that looks weird.

class Ship(Flyer):
    def tick(self, delta_time, fleet, fleets):
        pass

    def move(self, delta_time,fleets):
        self.control_motion(delta_time, fleets)
        self._location.move(delta_time)

Commit: inline method.

I think that now everyone who moves is implementing move and no one is moving on tick. Inspection tells me that all the Flyers either pass on tick or tick a timer if conditions are right for ticking.

Upon reflection, I think that’s just right. The sequence, fully expanded, is:

Move

Adjust your direction via controls or code, then move yourself.

Begin_Interactions

Record default values of any flags or tallies.

Interact_with

Unconditionally issue interact_with_me where me is your class name.

Interact_with_k

Adjust your flags or tallies based on classes k that you care about.

End_Interactions

Take any desired action. Presently unused.

Tick

Based on flags, tallies, or whim, tick your magic timers⁴.

Draw

Draw yourself if you’re so inclined.

To verify that that’s what happens:

class Game:
    def asteroids_tick(self, delta_time):
        self.control_game()
        self.fleets.move(delta_time)
        self.perform_interactions()
        self.fleets.tick(delta_time)
        self.draw_everything()

    def perform_interactions(self):
        Interactor(self.fleets).perform_interactions()

class Interactor:
    def perform_interactions(self):
        self.fleets.begin_interactions()
        for target, attacker in itertools.combinations(self.fleets.all_objects, 2):
            self.interact_one_pair(target, attacker)
        self.fleets.end_interactions()

    def interact_one_pair(self, target, attacker):
        attacker.interact_with(target, self.fleets)
        target.interact_with(attacker, self.fleets)

I think we’re done. Let’s sum up.

Summary

This whole process, a series of eight commits, implemented a design change to the sequence of events described just above, Move, Interact, Tick, Draw. It was accomplished pretty easily, aided by the idea of first renaming existing move methods to _move so that a new move could be implemented. All but one of those was later removed, but one is still used because that object has a complicated _move.

We should review what’s abstract and what’s defaulted in the Flyer superclass. It doesn’t make much difference. Some would say we should have a more explicit event structure. I do not agree but I see the point.

I doubt that there are many universes where this was truly worth doing for its direct benefit, since externally it doesn’t change behavior at all, and its benefit in making the design more clear is pretty small. But in terms of making the game’s core into a sort of framework, it’s a better framework now, and of course the real point is just to push the clay into a different shape, for the sheer joy of making pretty shapes in clay.

Tune in next time and we’ll do much of the same, but differently.