I have an idea, or some fraction of an idea. Let’s see what we can make of it. (As often happens, I don’t go where I thought I would.)

In the SurfaceMaker, there’s duplication of a kind. We make three different surfaces, in a very similar fashion. Take a look:

class SurfaceMaker:
    def __init__(self):
        pass

    @staticmethod
    def adjust(self, point, center_adjustment, scale_factor):
        return (point + center_adjustment) * scale_factor

    def get_ship_points(self):
        ship_points = [vector2(-3.0, -2.0), vector2(-3.0, 2.0), vector2(-5.0, 4.0),
                       vector2(7.0, 0.0), vector2(-5.0, -4.0), vector2(-3.0, -2.0)]
        return [self.adjust(point, vector2(7, 4), 4) for point in ship_points]

    def get_flare_points(self):
        flare_points = [vector2(-3.0, -2.0), vector2(-7.0, 0.0), vector2(-3.0, 2.0)]
        return [self.adjust(point, vector2(7, 4), 4) for point in flare_points]

    def make_ship_surface(self, ship_points):
        ship_surface = pygame.Surface((60, 36))
        ship_surface.set_colorkey((0, 0, 0))
        pygame.draw.lines(ship_surface, "white", False, ship_points, 3)
        return ship_surface

    def make_accelerating_surface(self, flare_points, ship_points):
        ship_accelerating_surface = self.make_ship_surface(ship_points)
        pygame.draw.lines(ship_accelerating_surface, "white", False, flare_points, 3)
        return ship_accelerating_surface

    def prepare_surfaces(self):
        ship_points = self.get_ship_points()
        flare_points = self.get_flare_points()
        return (self.make_ship_surface(ship_points)), (self.make_accelerating_surface(flare_points, ship_points))

    def prepare_surface(self):
        surface = pygame.Surface((128, 128))
        surface.set_colorkey((0, 0, 0))
        adjusted = [self.adjust(point, vector2(4, 4), 16) for point in raw_rocks[0]]
        pygame.draw.lines(surface, "white", False, adjusted, 3)
        return surface

I just marked adjust as a static method. It surely is. I don’t know what good the marker is but it seemed like the thing to do. But we’re here to think about the duplication in this code.

We have three occurrences of calls to adjust, each in a list comprehension. We’re creating three different surfaces. One of them is for an asteroid, and there will be at least three more of those. (I’m thinking of creating 12, one for each of the three sizes of the four shapes.) Two of them are for the ship, one with and one without the flare.

But there are three of these things. What are they? They each start with a list of points, and they each make a surface from it. The process they go through is called adjust, and it actually does two things to each point: it translates it, and it scales it.

Now that I think about that, it seems odd. You’d think that we should scale first and then shift the points. I think this only works because our points are nicely arranged around the zero point already. Must think about that. But it works and seems to work correctly, so I don’t believe I need to think about it just now. (That may turn out to be a mistake.)

2023-04-29-0737

Well, it’s tomorrow. Our zoom session started, so that’s about all the thinking I got done. I did show the code, so there may be some changes in there. We’ll see them when they go by. I did remove the @staticmethod decorator, I didn’t like the need to then refer to the method by the class name. And we’re refactoring around it anyway.

When we look at the SurfaceMaker above, we should notice that it makes three surfaces, and it does it similarly each time. The phases include:

  1. Allocate a Surface of a specified size. It may not be possible to compute that size from the input data, because of the need for the ship and ship plus flare to have the same center.
  2. Expand the points to a specified scale. This seems OK as a user-specified value. You could imagine some rigmarole of specifying relative sizes, but let’s get real.
  3. Translate the points to accommodate pygame’s top left fetish.
  4. Write the lines specified by the points onto the surface
  5. Return the surface.

It seems ideal to compute all the surfaces we need once, at the beginning. And I am leaning toward computing three different sizes of asteroids, rather than scale them at run time. Scaling a surface does weird things with the picture.

What do we want to have when we’re done? It’s usually good to ask that before we engage in anything too ambitious. Our sole purpose, I think, is to feed the surface into draw functions:

class Ship:
    def draw(self, screen):
        ship_source = self.select_ship_source()
        rotated = pygame.transform.rotate(ship_source.copy(), self.angle)
        half = pygame.Vector2(rotated.get_size()) / 2
        screen.blit(rotated, self.position - half)

    def select_ship_source(self):
        if self.accelerating and random.random() >= 0.66:
            return self.ship_accelerating_surface
        else:
            return self.ship_surface

class Asteroid:
    def draw(self, screen):
        half = vector2(self.surface.get_size()) / 2
        screen.blit(self.surface, self.position - half)

In the case of ship, we select the specific surface based on whether we are accelerating. When asteroids are done, we will probably select the surface from a collection of three, one for each scale. Another possibility is that we’ll destroy split asteroids and create new ones with the right size surface included, but even then we’ll be looking it up based on scale.

Right now, we’re creating all the surfaces when we create the object:

class Ship:
    def __init__(self, position):
        self.position = position
        self.velocity = vector2(0, 0)
        self.angle = 0
        self.acceleration = u.SHIP_ACCELERATION
        self.accelerating = False
        self.ship_surface, self.ship_accelerating_surface = SurfaceMaker().prepare_surfaces()

class Asteroid:
    def __init__(self):
        self.position = vector2(u.SCREEN_SIZE/2, u.SCREEN_SIZE/2)
        self.surface = SurfaceMaker().prepare_surface()
        rotation = random.randint(0,360)
        self.velocity = u.ASTEROID_SPEED.rotate(rotation)

I should reorder the calls in asteroid. Hold on, I’ll do it now. The idea will be to do the things in the same order, for easier understanding:

class Asteroid:
    def __init__(self):
        self.position = vector2(u.SCREEN_SIZE/2, u.SCREEN_SIZE/2)
        angle_of_travel = random.randint(0, 360)
        self.velocity = u.ASTEROID_SPEED.rotate(angle_of_travel)
        self.surface = SurfaceMaker().prepare_surface()

I renamed rotation to angle_of_travel because that’s what it is. Commit: Tidying

This is a change of direction, and rather a fancy idea to boot.

Let’s try something fancy. Let’s ask SurfaceMaker more explicitly for what we want, and let’s ask in draw. And then, let’s make SurfaceMaker cache surfaces so that we don’t compute them on every draw. We’ll begin by leaving the surface fetching in __init__ but move it in a while. I’ll do asteroid first, because it’s simpler, at least for now:

    def __init__(self):
        self.position = vector2(u.SCREEN_SIZE/2, u.SCREEN_SIZE/2)
        angle_of_travel = random.randint(0, 360)
        self.velocity = u.ASTEROID_SPEED.rotate(angle_of_travel)
        self.surface = SurfaceMaker().asteroidSurface(type=0, size=3)

I’m specifying the argument names here mostly to tell us what I’m thinking. Now we need that method. Python warns me that type shadows something. shape is better anyway:

    def asteroidSurface(self, shape, size):
        return self.prepare_surface()

This will of course just work, which is fine. Oh, dammit, camel-case name. Change it.

    def asteroid_surface(self, shape, size):
        return self.prepare_surface()

I’ll learn that, I promise. I just don’t promise how long it’ll take. Now commit: working toward caching asteroid_surface.

Now let’s come up with a data structure to hold on to our asteroid surfaces. They’ll be indexed by rock number and size 1-3. But my plan — and this is way too fancy but I’m going to do it anyway — is that we’ll dynamically create each surface the first time it is asked for. So we need a structure that may or may not have an asteroid with the provided shape and size. An array of array of surface would work. But we really ought to TDD such a thing. Let’s move to a testing mode.

Before I even get started, I think I want a new object to serve as a cache that the surface maker can use freely. And we can guess that our objects will have a kind (ship, asteroid, saucer), possibly a shape, 0-3, and a size 1-3. The ship will be of shape 0 and size 1. We’ll never ask for anything else. Or — looking forward — we might make the size be the desired scale. We’ll see.

I’m going to consider the tests I write here to be sketches, spikes. But I do not plan to throw anything away … unless I decide I’m on a bad path. Which could happen.

I start with this:

class TestSurfaceMaker:
    def test_key(self):
        maker = CachedMaker()
        key = maker.key("ship", 5, 7)
        assert key == "ship-5-7"
        key = maker.key("asteroid")
        assert key == "asteroid-0-1"


class CachedMaker:
    def __init__(self):
        self.cache = dict()

    def key(self, kind, shape=0, size=1):
        return "{0}-{1}-{2}".format(kind, shape, size)

I’ve decided, tentatively, to have an object CachedMaker that we’ll use to store things, and that it’ll use a dictionary with a string key based on the kind, shape, and size. The test above runs. Now let’s see how we might use the thing.

    def test_fetch(self):
        maker = CachedMaker()
        
        def make(kind, shape, size):
            return "made {0}-{1}-{2}".format(kind, shape, size)
        
        made = maker.fetch("ship", 5, 7, make)
        assert made == "made ship-5-7"

Still feeling my way, but the idea is that maker.fetch takes the desired keys and a function to call to create the thing. Let’s see if I can make this work.

    def test_fetch(self):
        maker = CachedMaker()
        make_count = 0

        def make(kind, shape, size):
            nonlocal make_count
            make_count += 1
            return "made {0}-{1}-{2}".format(kind, shape, size)

        made = maker.fetch("ship", 5, 7, make)
        assert made == "made ship-5-7"
        assert make_count == 1
        made = maker.fetch("ship", 5, 7, make)
        assert made == "made ship-5-7"
        assert make_count == 1

Here I provide a make method that just makes a string. The subsequent code expects make_count to increase on the first fetch and not on the second. And:

    def fetch(self, kind, shape, size, action):
        key = self.key(kind, shape, size)
        cached = self.cache.get(key)
        if cached:
            return cached
        new_one = action(kind, shape, size)
        self.cache[key] = new_one
        return new_one

The test is green. CachedMaker works. Commit: CachedMaker.

I need a break. Let’s reflect and decide what to do next, possibly including stopping: I’ve been at it for about two hours, a break is quite logical.

Reflection

I didn’t have the caching idea in mind when I started this article, and it is a bit fancy, but as we can see from the code above, it’s pretty simple, so I don’t think I’ll be drummed out of the corps for it. We’ll see. Right now, all the programmers in the office approve.

I think I was wise to move to TDD to implement the idea, and I think I was also wise to break out the general caching / creation logic from the specific surface-making logic. I should be able to create a caching surface maker by using the CachedMaker object, passing it the right creation functions to call.

I was perhaps unwise to let CachedMaker know how to create the key. More likely it should be provided the key and the function to call and not know how to create the key.

Before moving forward, if I decide that was in fact unwise — and I’m sure it was — I should change CachedMaker via its tests and then move on to the surface making.

And CachedMaker is presently right in the same file as its tests. That’s not altogether bad, given the size of this program. I’ll ask my pals about it, see what they think.

I think it’s time for an extended break. I’ll wrap this, since we’re on a handy commit, and come back for another article later.

See you then!

Added while musing, before resting
I thought of this formulation for the fetch. I think I like it:
    def fetch(self, kind, shape, size, action):
        key = self.key(kind, shape, size)
        try:
            return self.cache[key]
        except KeyError:
            new_one = action(kind, shape, size)
            self.cache[key] = new_one
            return new_one

I suppose we might argue that it’s a bit costly, but it’s quite clear. I’ll consider further and ask my pals.

See you soon!