Python 62 - Can't be smarter, go slower.
I was off my game yesterday, and want to do better today. I can’t be smarter, but I can go slower. I think that, plus experience, will serve. (I still manage to get in trouble.)
Last night I made a tiny change that I’ll report now. In the afternoon, I changed the game to create a new saucer each time we need to fly one, and observed that that was creating a new surface for the saucer each time through. So I made these changes:
class Saucer:
direction = -1
saucer_surface = None
offset = None
@classmethod
def init_for_new_game(cls):
cls.direction = -1
def __init__(self, position=None, size=2):
self.size = size
Saucer.direction = -Saucer.direction
x = 0 if Saucer.direction > 0 else u.SCREEN_SIZE
self.position = Vector2(x, random.randrange(0, u.SCREEN_SIZE))
self.velocity = Saucer.direction * u.SAUCER_VELOCITY
self.directions = (self.velocity.rotate(45), self.velocity, self.velocity, self.velocity.rotate(-45))
self.radius = 20
if not Saucer.saucer_surface:
raw_dimensions = Vector2(10, 6)
saucer_scale = 4 * self.size
Saucer.offset = raw_dimensions * saucer_scale / 2
saucer_size = raw_dimensions * saucer_scale
Saucer.saucer_surface = SurfaceMaker.saucer_surface(saucer_size)
self.set_firing_timer()
self.set_zig_timer()
def draw(self, screen):
top_left_corner = self.position - Saucer.offset
screen.blit(Saucer.saucer_surface, top_left_corner)
If the class variable saucer_surface is None, we create it. In draw, we use it. This should ensure that we only create it once.
I’m not sure about the syntax for referring to class variables. Hold on a moment while I ask the Internet.
I return unsatisfied, but warned that while you can access a class variable name with self.name, if you assign via self.name = , you’ll create an instance-specific value. So I guess I do have to say Saucer.offset and such in there.
Anyway, that seems to work and should only create the surface once per game, so that’s my report from last night.
I do think that code could be improved. Shall we? Sure.
Looking at init:
def __init__(self, position=None, size=2):
self.size = size
Saucer.direction = -Saucer.direction
x = 0 if Saucer.direction > 0 else u.SCREEN_SIZE
self.position = Vector2(x, random.randrange(0, u.SCREEN_SIZE))
self.velocity = Saucer.direction * u.SAUCER_VELOCITY
self.directions = (self.velocity.rotate(45), self.velocity, self.velocity, self.velocity.rotate(-45))
self.radius = 20
if not Saucer.saucer_surface:
raw_dimensions = Vector2(10, 6)
saucer_scale = 4 * self.size
Saucer.offset = raw_dimensions * saucer_scale / 2
saucer_size = raw_dimensions * saucer_scale
Saucer.saucer_surface = SurfaceMaker.saucer_surface(saucer_size)
self.set_firing_timer()
self.set_zig_timer()
I see that big chunk in the middle for the surface. Let’s extract that, including the if:
def __init__(self, position=None, size=2):
self.size = size
Saucer.direction = -Saucer.direction
x = 0 if Saucer.direction > 0 else u.SCREEN_SIZE
self.position = Vector2(x, random.randrange(0, u.SCREEN_SIZE))
self.velocity = Saucer.direction * u.SAUCER_VELOCITY
self.directions = (self.velocity.rotate(45), self.velocity, self.velocity, self.velocity.rotate(-45))
self.radius = 20
self.create_surface_class_members()
self.set_firing_timer()
self.set_zig_timer()
def create_surface_class_members(self):
if not Saucer.saucer_surface:
raw_dimensions = Vector2(10, 6)
saucer_scale = 4 * self.size
Saucer.offset = raw_dimensions * saucer_scale / 2
saucer_size = raw_dimensions * saucer_scale
Saucer.saucer_surface = SurfaceMaker.saucer_surface(saucer_size)
That’s better, I think. Possibly it should be first or last or something, but that’ll do for now.
Commit: extract method for saucer class member creation.
PyCharm forgets my pinned test tab and auto-run, so I had to set that back up. I’ve thumbs-upped IDEAs on their bug list requesting that.
OK, that’s nice but why are we here?
A Plan
Yesterday, I (I’d say “we” but it was so ragged I don’t want you feeling responsible) modified the game to create a new instance of Saucer every time we need one. The saucer spawning currently looks like this:
def asteroids_tick(self, delta_time):
self.fleets.tick(delta_time)
self.check_saucer_spawn(self.saucers, delta_time)
self.check_ship_spawn(self.ship, self.ships, delta_time)
self.check_saucer_firing(delta_time, self. saucers, self.saucer_missiles, self.ships)
self.check_next_wave(delta_time)
self.control_game(self.ship, delta_time)
self.process_collisions()
self.draw_everything()
if self.game_over: self.draw_game_over()
def check_saucer_spawn(self, saucers, delta_time):
if saucers: return
self.saucer_timer.tick(delta_time, saucers)
def init_saucer_timer(self):
self.saucer_timer = Timer(u.SAUCER_EMERGENCE_TIME, self.bring_in_saucer)
def bring_in_saucer(self, saucers):
saucers.append(Saucer())
What we would like to do is to relieve Game of this responsibility and put it in a more sensible place. I claim that such a place would be in an instance of SaucerFleet, in its tick method.
I think that we still have SaucerFleet from our previous machinations:
class SaucerFleet(Fleet):
def __init__(self, flyers):
super().__init__(flyers)
self.timer = Timer(u.SAUCER_EMERGENCE_TIME, self.bring_in_saucer)
def bring_in_saucer(self, saucer):
saucer.ready()
self.flyers.append(saucer)
Since this doesn’t implement tick it’ll never get called. I think that first I’d like to get bring_in_saucer right, i.e.
def bring_in_saucer(self, saucer):
self.flyers.append(Saucer())
Right. Now, let’s remove the saucer creation code from Game. I’ll show you what I remove:
def bring_in_saucer(self, saucers):
saucers.append(Saucer())
self.init_saucer_timer()
def init_saucer_timer(self):
self.saucer_timer = Timer(u.SAUCER_EMERGENCE_TIME, self.bring_in_saucer)
At this point, I have one test failing, and I’m sure no saucer would ever appear. There is already a timer in SaucerFleet, so let’s do tick. I wind up here:
class SaucerFleet(Fleet):
def __init__(self, flyers):
super().__init__(flyers)
self.timer = Timer(u.SAUCER_EMERGENCE_TIME, self.bring_in_saucer)
def bring_in_saucer(self):
self.flyers.append(Saucer())
def tick(self, delta_time, fleets):
if not self.flyers:
self.timer.tick(delta_time)
super().tick(delta_time, fleets)
Test is still borked but I assume it’s fixable. I want to know if the saucer appears in the game.
Running shows me a reference to init_saucer_timer in insert_quarter. I remove it, but it makes me worry about resetting the timer down in the fleet. I make a note: fleet timers.
Now when I run I find that I’m calling this:
def check_saucer_spawn(self, saucers, delta_time):
if saucers: return
self.saucer_timer.tick(delta_time, saucers)
Again all this seems a bit ragged. I remove the call to that method from asteroids_tick but I leave the method because tests call it. I’ll set it to pass.
The saucer never starts. Am I creating a SaucerFleet? I was not. Fix:
class Fleets:
def __init__(self, asteroids, missiles, saucers, saucer_missiles, ships):
self.fleets = (Fleet(asteroids), Fleet(missiles), SaucerFleet(saucers), Fleet(saucer_missiles), ShipFleet(ships))
Sigh. The controls stopped working. Weirdly, if I say SaucerFleet(saucers), the ship controls break, and if I leave it at Fleet, they work.
I put a print in Ship.tick and when it’s Fleet(saucers), it repeatedly prints, and when it’s SaucerFleet(saucers), it does not print.
Ah. Here’s Fleets.tick:
def tick(self, delta_time):
result = True
for fleet in self.fleets:
result = result and fleet.tick(delta_time, self)
return result
Saucer.tick is returning None and the and early outs. What a crock.
def tick(self, delta_time):
result = True
for fleet in self.fleets:
result = fleet.tick(delta_time, self) and result
return result
That fixes the problem. I don’t even have a decent use for that feature anyway. Let’s make it more bullet-proof.
def tick(self, delta_time):
all_true = True
for fleet in self.fleets:
if not fleet.tick(delta_time, self):
all_true = False
return all_true
And fix the tick to return True:
class SaucerFleet(Fleet):
def tick(self, delta_time, fleets):
if not self.flyers:
self.timer.tick(delta_time)
return super().tick(delta_time, fleets)
I’ve made another Jira note: “tick return True is bad”. In particular, every time I implement tick, I’ll have to remember not to return none. Of course I could put in a throw but no one wants an exception.
It’s a flawed design idea. I need a better one.
For now, let’s fix the tests and ship this. The feature actually works, saucer spawning is done now in SaucerFleet.
def test_spawn_saucer(self):
game = Game(testing=True)
game.game_init()
game.check_saucer_spawn(game.saucers, 0.1)
assert not game.saucers
game.check_saucer_spawn(game.saucers, u.SAUCER_EMERGENCE_TIME)
assert game.saucers
game.saucers.clear()
assert not game.saucers
game.check_saucer_spawn(game.saucers, u.SAUCER_EMERGENCE_TIME)
assert game.saucers
We’d love to have a test like this that actually works. It would be a test on SaucerFleet, wouldn’t it?
This seems like a good test but it fails intermittently.
def test_spawn_saucer(self):
saucers = []
fleet = SaucerFleet(saucers)
fleet.tick(0.1, None)
assert not saucers
fleet.tick(u.SAUCER_EMERGENCE_TIME, None)
assert saucers # first create
saucers.clear()
assert not saucers
fleet.tick(u.SAUCER_EMERGENCE_TIME, None)
assert saucers # second create
This is surely an initialization problem. I need to understand what is going on. First, let me comment:
Wind from Sails
This sort of thing really takes the wind out of my sails. I was moving along fairly well, and the game seems to work, but these intermittent tests tell me that I’ve surely forgotten something.
There is one note that may apply: “Fleet timers”, which was intended to remind me that when we insert a quarter, we should reset the fleet’s timers. I’m not convinced of that and in the test above, we create a new SaucerFleet instance, so its timer is certainly fresh.
The Saucer does have a class variable direction, which will get fiddled as we create saucers. That could be confusing some of my tests.
1011 Hours
I’m buried trying to find what seems to be a timing bug.
I’m put prints throughout and what I’m seeing is that the second time the timer is ticked with 7, the timer says it’s triggering, but the bring_in_saucer method is not called. The return from not calling it prints as None even though the bring_in_saucer returns True if it is called. It is as if the action isn’t called or has changed?
Wow! My prints dump this:
test tick 0.1
fleet tick 0.1 [] 0
fleet ticking 0.1 []
timer tick 0.1 0.1
no trigger
test tick 7
fleet tick 7 [] 0.1
fleet ticking 7 []
timer tick 7 7.1
triggering <bound method SaucerFleet.bring_in_saucer of <fleet.SaucerFleet object at 0x1022f8e90>>
fleet rezzing
complete True
resetting time to zero
timer tick 7 7
triggering <bound method Saucer.zig_zag_action of <saucer.Saucer object at 0x1022f8f90>>
complete None
resetting time to zero
test tick 7 second time
fleet tick 7.1 [] 0
fleet ticking 7.1 []
timer tick 7.1 7.1
triggering <bound method SaucerFleet.bring_in_saucer of <fleet.SaucerFleet object at 0x1022f8e90>>
fleet rezzing
complete True
resetting time to zero
timer tick 7.1 7.1
triggering <bound method Saucer.zig_zag_action of <saucer.Saucer object at 0x1021ba710>>
complete None
resetting time to zero
Note the second and fourth calls are triggering Saucer.ziz_zag_action.
Well, the fleet timer does run the timers of the members of the fleet, so if there is a saucer in the fleet, it will run those timers. So I guess that’s legitimate. I’ll comment out that call.
With the super().tick call commented out, the tests run solidly.
Ah. The saucer moves, and in seven seconds, it moves off screen. When it moves off screen, it deletes itself. For this test to work, I need to set its velocity to zero or something small.
Amazing.
No. Let’s do the super call first and then the fleet call. I decide on this:
def tick(self, delta_time, fleets):
if self.flyers:
super().tick(delta_time, fleets)
else:
self.timer.tick(delta_time)
return True
If there are flyers, we’re good and we tick them. If there aren’t, no point ticking them, so we just do our timer. I think that’s best, but I don’t entirely love it. Is this better?
def tick(self, delta_time, fleets):
super().tick(delta_time, fleets)
if not self.flyers:
self.timer.tick(delta_time)
return True
Yes. We don’t have a right to assume what super().tick does.
We are green and the game runs. Commit: saucer spawning now controlled by SaucerFleet. Whew!
Let’s reflect. There must be a lesson in here somewhere1.
Reflection
The confusion and intermittent tests seem to have been caused, primarily, by the tests using very large values of time to trigger the saucer rezzing event. The result was that, sometimes, depending on the values, because I was calling the super() event last, a saucer that I had created would destroy itself. That broke the test, which was expecting the saucer still to be there.
In the wild, that won’t happen.
I’m not entirely sure why other tests seemed to fail sometimes. Part of me wants to know, but I can’t duplicate the behavior, so I am pretty confident that it’s fixed. I’d prefer to feel more than “pretty confident”, but there clearly was an interaction, now resolved, so the chance that there were two problems, both of which showed up after one change, is fairly low. I’d prefer that the chance was lower than “fairly low”.
But it has stopped happening after fixing what was clearly a problem. I’ll try to get over my slightly lowered confidence. A long period of things going well will fix me right up.
Furthermore …
The test issue notwithstanding, the changes needed to push a timer down from Game into SaucerFleet were tricky. One remaining issue is that we probably need to give the Fleets, Fleet, and maybe even the individual objects the chance to reset for a new game. I’ve made a note for that.
Another issue is that the initialization is still broken out with special code for starting a new game, the insert_quarter code, and I had to change that in order to remove the timer from the Game. I failed to look there and it needed to be looked at. I think it’s called all the time now, but that it duplicates some initialization. It needs sorting. Note for that too.
Jira is filling up:
Even so …
Even so, even so, I am pleased with the effect of moving responsibility down. It makes more sense that the SaucerFleet would keep the saucer fleet up to date, rather than the Game. I think these changes are righteous, but I have not, as yet, found a really smooth way to do it. There are a couple more to do, I think, so maybe I’ll find it yet.
Enough for the morning. I’ll hope to see you next time, and to look a bit better doing the work. Thanks for stopping by!
-
CF: famous joke about two boys, one pessimistic and one optimistic. ↩