Can't Sleep

Hello, loves!

Let’s see if I can tire myself out with code. I enjoy crafting code and ain’t no oligarch-feeding resource-consuming job-stealing computer program gonna take that away from me.

It’s 0030, I can’t sleep, so I’m going to code. We’ll begin with reviewing the responsibilities and connections of the various objects and layers that make up our design. I’ve been finding it hard to think about and work with and want to understand why. I suspect responsibilities in the wrong place, connections that aren’t right, the usual suspects.

Time Passes …

I annotated some dictionaries to make it more clear to me what’s in them. Then I moved about nine or ten methods out of DungeonLayout, the ones answering questions about cells, such as layout.is_available(cell) and the like. I provided similar methods and properties on Cell, such as cell.is_available. Those were all straightforward, although to finish the cleanup, I had to edit all the old versions to use the new versions. In a large system, that part could have been undertaken over time, with the layout ones deprecated. With everything in one repo and PyCharm at our fingertips, it was easy enough to do each one completely.

The result was to simplify DungeonLayout substantially, while the corresponding capabilities make more sense in Cell, and are usually a bit simpler as well. I’ll spare you the details. I believe that no test ever failed during this cycle. Certainly there was no significant trouble.

I got to bed at 0200 and slept uneventfully until 0830 or something like that.

Motivation

Of course I would not claim that I really understand why I do things, but certainly I have been thinking for some time that the DungeonLayout / Dungeon / DungeonView family are too complex and that there are aspects in at least some of those classes that should be pulled over to another of the family, or perhaps into helper classes. So simplifying is on my mind.

This Morning …

I plan to continue looking for ways to simplify and normalize the design. It’s high time, and there’s no one to say that I can’t.

One concern that I have has to do with Content, because a content item has a very substantial multiple personality. It has a presence in the Layout, in the sense that it is “in” a Cell. It has a presence in the Dungeon, in that Dot interacts with it when she moves, and the content item can even prohibit her from moving. It has a presence in the Views, in that its graphical representation resides in the sprite lists. And there is interaction among these roles.

Take Spikes, for example. They have three operating states, cycling, stopped up, and stopped down. They have two graphical states, up and down. When fully implemented, they will inconvenience Dot when up or running, and may even prohibit her from entering. They are controlled by another content item, which, when Dot bumps it, changes its own visible character, and publishes its state, which the spikes hear and use to change their own state.

Having told that story, I’m motivated to track through some of the code that is involved. Here’s the creation method:

class ContentFactory:
    def spikes(self, *, name):
        resource1 = '/Users/ron/Desktop/DungeonTiles/png/objects/trap/1.png'
        resource2 = '/Users/ron/Desktop/DungeonTiles/png/objects/trap/2.png'
        resources = [resource1, resource2]
        scale = 0.75
        cases = {
            0: (True, 0),
            1: (False, 1),
            2: (False, 0),
            3: (True, 0),
        }
        info = SimpleNamespace(cycling=True, cases=cases, time=0)

        def cycle(self, pub_sub, delta_time):
            if not self.info.cycling: return
            self.info.time += delta_time
            if self.info.time >= 1:
                self.info.time = 0
                self.state = (self.state+1)%len(self.resources)
                pub_sub.publish('state_number', self.name, content=self, state=self.state)
        cycle_sub = Subscription(event='on_update', caller_id='view', callback=cycle)

        def control(self, *, pub_sub, state):
            try:
                self.info.cycling, self.state = self.info.cases[state]
                pub_sub.publish('state_number', self.name, content=self, state=self.state)
            except KeyError:
                return
        control_sub = Subscription(event='control', caller_id=name, callback=control)

        return Content(name=name, resources=resources, scale=scale,
                       info=info, subs=[cycle_sub, control_sub],
                       )

NOte that it has two event callbacks, cycle and control, responding to ‘on_update’ and ‘control’, respectively. Both of those events publishes state_number. That event is fielded in DungeonView:

class DungeonView:
    def subscribe_to_state_number(self, dungeon):
        def callback(*, pub_sub, content, state):
            self.view_do(
                content,
                lambda view: view.set_state(state))
        dungeon.subscribe('state_number', '', callback)

That callback is given just the Content instance and its state. It finds the ContentView instance corresponding to that Content, and sends it set_state:

class DungeonView:
    def view_do(self, content_item, action):
        try:
            view = self.content_views[content_item]
            action(view)
        except KeyError:
            return

class ContentView:
    def set_state(self, state_number):
        self.sprite.set_texture(state_number)

When the ContentViews are set up, each one builds a sprite containing all the textures (images) that the sprite can take on:

class ContentView:
    error_texture = ':resources:images/items/star.png'
    def __init__(self, cell, content_item, resources, scale=0.5):
        self.cell = cell
        self.item = content_item
        self.sprite = Sprite()
        for resource in resources:
            try:
                texture = arcade.load_texture(resource)
            except (FileNotFoundError, AttributeError):
                texture = arcade.load_texture(self.error_texture)
            self.sprite.append_texture(texture)
        self.sprite.set_texture(0)
        self.sprite.visible = False
        self.sprite.scale = scale_texture(self.sprite.textures[-1], scale)

The ContentItems provide the Views with a mapping from the item to the item’s cell and its sprite. The DungeonView maintains a dictionary from Content item to ContentView, from Room to RoomView (q.v), and from Room to a list of ContentView. That last structure is used to illuminate content when we illuminate a section of the Dungeon.

So We See …

It gets kind of gnarly. I’m going to just look at random methods that seem odd. Here’s one now:

class DungeonView:
    def is_lit(self, cell):
        return cell in self.illuminated_cells

Who’s calling that?

class DungeonView:
    def make_view_and_sprite(self, cell, item):
        resources = item.resources
        scale = item.scale
        view = ContentView(cell, item, resources, scale)
        view.sprite.position = cell.center_position(cell_size)
        self.content_views[item] = view
        self.content_views_by_room[cell.room].append(view)
        self.content_sprite_list.append(view.sprite)
        if self.is_lit(cell):
            view.sprite.visible = True

OK, this is there to make dynamically-added content visible on arrival. Any outside callers?

class RoomView:
    def is_lit(self, cell):
        return self.dungeon_view.is_lit(cell)

Right but who’s asking about that?

    def test_room_view_illuminate(self):
        layout = DungeonLayout(20,20)
        dungeon = Dungeon(layout)
        dungeon_view = DungeonView(dungeon, True)
        cells = [Cell(x,y) for x in range(9,15) for y in range(9,15)]
        room = Room(cells, layout)
        layout.add_room(room)
        room_view = RoomView(dungeon_view, room)
        room_view.cell_sprites = {cell: FakeSprite() for cell in cells}
        room_view.illuminate_around(Cell(12, 12), 3)
        lit_cell = Cell(13, 13)
        lit = room_view.cell_sprites[lit_cell]
        assert room_view.is_lit(lit_cell)
        assert lit.visible == True
        unlit_cell = Cell(9, 9)
        assert not room_view.is_lit(unlit_cell)
        unlit = room_view.cell_sprites[unlit_cell]
        assert unlit.visible == False

Let’s change that test to ask the DungeonView, not the room. Then we can remove that method from Room.

        ...
        lit = room_view.cell_sprites[lit_cell]
        assert dungeon_view.is_lit(lit_cell)
        assert lit.visible == True
        unlit_cell = Cell(9, 9)
        assert not dungeon_view.is_lit(unlit_cell)
        unlit = room_view.cell_sprites[unlit_cell]
        assert unlit.visible == False

Remove the method. Green. Commit. Now inline those two:

        ...
        lit = room_view.cell_sprites[lit_cell]
        assert lit_cell in dungeon_view.illuminated_cells
        assert lit.visible == True
        unlit_cell = Cell(9, 9)
        assert not unlit_cell in dungeon_view.illuminated_cells
        unlit = room_view.cell_sprites[unlit_cell]
        assert unlit.visible == False

Now there is only one use of DungeonView is_lit:

    def make_view_and_sprite(self, cell, item):
        resources = item.resources
        scale = item.scale
        view = ContentView(cell, item, resources, scale)
        view.sprite.position = cell.center_position(cell_size)
        self.content_views[item] = view
        self.content_views_by_room[cell.room].append(view)
        self.content_sprite_list.append(view.sprite)
        if self.is_lit(cell):
            view.sprite.visible = True

Inline that and remove the method.

        if cell in self.illuminated_cells:
            view.sprite.visible = True

Commit.

All that for one tiny method?

Well, yes. And it was two tiny methods and included coupling between two classes. I do like tiny methods like is_lit that cover slightly obscure things like cell in self.illuminated_cells. You’ve seen me create them many times. Here, where the class has too many methods and the use is all internal to the class, I prefer the slightly less clear construct with the method gone. I could be wrong, of course.

Moving Right Along

I’m just spotting things that look like they’d like to be improved. I note that Dungeon class has forwarders for the entire pub_sub protocol:

class Dungeon:
    def publish(self, event, caller_id, *args, **kwargs):
        self.pub_sub.publish(event, caller_id, **kwargs)
    def subscribe_all(self, subscriptions):
        self.pub_sub.subscribe_all(subscriptions)
    def subscribe(self, event, caller_id, callback):
        self.pub_sub.subscribe(event, caller_id, callback)
    def subscribe_once(self, event, caller_id, callback):
        self.pub_sub.subscribe_once(event, caller_id, callback)

It’d be nice not to need those. Who is using them? The commonly-used one is publish. A raft of calls are made to Interactor, which implements just publish for use by content that needs to publish stuff.

class Interactor:
    def __init__(self, dungeon, cell):
        self.dungeon = dungeon
        self.cell = cell

    def publish(self, event, caller_id, *args, **kwargs):
        self.dungeon.publish(event, caller_id, *args, **kwargs)

Let’s give the Interactor a PubSub. (It could fetch it from Dungeon, but let’s not make that assumption here.)

class Interactor:
    def __init__(self, dungeon, pub_sub, cell):
        self.dungeon = dungeon
        self.pub_sub = pub_sub
        self.cell = cell

    def publish(self, event, caller_id, *args, **kwargs):
        self.pub_sub.publish(event, caller_id, *args, **kwargs)

PyCharm fills in the single call to the constructor. Now there are fewer calls to publish in Dungeon.

Time Passes …

Various tedious tiny changes allow me to remove the PubSub protocol from Dungeon. That simplifies the coupling: things that used to go me -> Dungeon -> PubSub now all have direct access to the PubSub instance.

Again, no tests broke substantially along the way. Just calls that needed to be changed to refer to the available pub_sub.

Dungeon is down to just these methods:

_announce
_interactions_allow_move
_redo_interactions_in_current_cell
announce_via
contents_at
dot_has
find_cell_containing
find_path_to
inventory
just_set_player_position
maker_flood
max_x
max_y
move_player
place_content_at
populate
receive_content_from_cell
remove_content_from_cell
rooms
run
set_player_position_with_interaction
show_path_to

That’s fewer than before by quite a few, but looking at the list, it doesn’t tell much of a story, does it? I can see a few themes, which may suggest new objects:

• dot_has and inventory sounds like a Player class wanting to be born. Maybe with set_player..., just_set..., and move_player as well.

• Why are we using max_x and max_y at all? main uses them to set screen size, we could do that some other way. They seem to be used in DungeonView scrolling. And DungeonView implements them as well, forwarding to Dungeon. We can do better, I’m sure.

• populate is a placeholder. Currently only positions Dot somewhere. main overrides the setting anyway, mostly for debugging purposes. There will probably be a separate object for allocating things into the Dungeon. Some day.

Enough. Two hours of refactoring and typing. Enough.

Summary

If I had to evaluate the overall code quality here, I’d use a term which was used against a team of mine some years back: a lot of it is mediocre. Some is pretty nice, and the individual concepts aren’t too gnarly, but it just doesn’t present itself as clear and clean.

I blame the media. No, I blame the GOP. No, it’s Chet’s fault.

These are my principles.
If you don’t like them, I have others.
– Groucho Marx

No, it’s just what happens to code as we evolve it. Quite likely many of the issues could have been spotted earlier and dealt with then. But we see what we see, we made the decisions we make, and when they no longer please us, we make others.

Over ninety minutes at oh-dark-thirty and two hours this morning, I’ve made the code much better, and not with any deep thinking, mostly just pushing a bit of code up or down, occasionally sideways, in the class structure. The Dungeon class is much smaller and more cohesive. DungeonView is improved a bit. Cell is much more powerful, but has remained pretty simple, mostly just properties, attributes, and neighbor generation. There was some tedium rewiring tests but no actual logic changes.

I think the message is that with small changes, even late in the game, we can make things better.

And, of course, my main message is that I enjoy crafting code and ain’t no oligarch-feeding resource-consuming job-stealing computer program gonna take that away from me.