Hello, loves!

The DungeonViewMaker seems to me not to be quite the thing. Let’s do better. Tout le monde déteste l’IA.

We have no cable today. That means no TV, no Internet, and no music! The latter bothers me most, because I listen to Breakfast with the Beatles during morning programming. There is probably native music on my computer somewhere but if I can’t cast to the HomePod, it sounds like a tin-can telephone. I’ll proceed without the moral support.

I feel that there is something not quite right about the KeyPress, KeyedSpriteList, DungeonViewMaker, DungeonView, BuildStepper setup. We’ll look at the code. My initial sense, based on whatever I may have been thinking yesterday, is that part of the issue is in DungeonViewMaker:

class DungeonViewMaker:
    def __init__(self, dungeon):
        self.setup_assets()
        self.dungeon = dungeon
        self.pub_sub = dungeon.pub_sub
        self.keyed_sprites = KeyedSpriteList(arcade.SpriteList())
        self.setup()

    @property
    def view(self):
        return DungeonView(self.dungeon, self.pub_sub, self.keyed_sprites)

    def setup(self):
        self.create_room_sprites()
        self.create_content_lists()

    def update(self, view):
        view.keyed_sprites = self.keyed_sprites

There’s more code in there, the two create lists, but the bottom line here is that the DungeonViewMaker’s actual work is to create and populate the KeyedSpriteList. Then, almost by the way, it has a method to create a DungeonView based on what it has received, plus the KSL that it has populated.

Making the view seems like almost a side gig for the DVM, doesn’t it?

Let’s see how we use it. In ‘main’, and the very end:

main.py
def main():
    ...
    view = DungeonViewMaker(dungeon).view
    view.run(stepper)

In BuildStepper:

class BuildStepper:
    def _do_one_step(self, view):
        self.table[self.index](self.layout, self.dungeon)
        if view:
            maker = DungeonViewMaker(self.dungeon)
            maker.update(view)
        self.index += 1

Here we’re just using it to get the KSL, which we then stuff into the view we’re actually using.

And there are three tests using it to get a view.

Let’s change the rules. We’ll turn the DVM into a KeyedSpriteListMaker. We’ll remove the view method, and whoever uses that will fail. We’ll fix up ‘main’ and BuildStepper, etc.

Change this test:

    def test_room_view_illuminate(self):
        layout = DungeonLayout(20,20)
        dungeon = Dungeon(layout)
        cells = [Cell(x,y) for x in range(9,15) for y in range(9,15)]
        room = Room(cells, layout)
        layout.add_room(room)
        layout.finish_map()
        dungeon.just_set_player_position(Cell(12,12))
        dungeon_view = KeyedSpriteListMaker(dungeon).view
        dungeon_view.illuminate_around_dot()
        lit_cell = Cell(13, 13)
        lit = dungeon_view.keyed_sprites.sprite_at(lit_cell)
        assert lit.visible == True
        unlit_cell = Cell(9, 9)
        unlit = dungeon_view.keyed_sprites.sprite_at(unlit_cell)
        assert unlit.visible == params.sprite_visible

To this:

    ...
        dungeon.just_set_player_position(Cell(12,12))
        ksl = KeyedSpriteListMaker(dungeon).keyed_sprites
        dungeon_view = DungeonView(dungeon, dungeon.pub_sub, ksl)
        dungeon_view.illuminate_around_dot()

Passes. The other test similarly. Passes. BuildStepper should work as-is, since it never asks for the view:

class BuildStepper:
    def _do_one_step(self, view):
        self.table[self.index](self.layout, self.dungeon)
        if view:
            maker = KeyedSpriteListMaker(self.dungeon)
            maker.update(view)
        self.index += 1

Main needs a small change:

main.py
    ksl = KeyedSpriteListMaker(dungeon).keyed_sprites
    view = DungeonView(dungeon, dungeon.pub_sub, ksl)
    view.run(stepper)

Should work, run it. Works. Commit: Replace DungeonViewMaker with KeyedSpriteListMaker and explicit view creation.

I noticed along the way that we pass a PubSub to the DungeonView and that it’s always the one in the Dungeon. And it had better be. Let’s change the View to fetch the PubSub from the Dungeon.

class DungeonView(arcade.View):
    def __init__(self, dungeon, pub_sub, keyed_sprites):
        if arcade.window_commands._window:
            super().__init__()
            self.cameras = Cameras(self, dungeon.max_x, dungeon.max_y, zoom=params.zoom_factor)
        self.dungeon = dungeon
        self.pub_sub = dungeon.pub_sub
        self.keys =  None
        self.keyed_sprites = keyed_sprites
        self.subscribe(self.dungeon, self.pub_sub)

We could commit. First, let’s remove the parameter. Change Signature (Function+Command+6). Done, green. commit: remove pub_sub parameter from DungeonView creation.

Reflection

This is good, things are a bit simpler and more sensible now. But I’m not entirely comfortable. What if the DungeonView knew to use a KeyedSpriteListMaker to build its KSL if it didn’t have one?

Then we’d only have to pass the Dungeon in. We’re on a fresh commit. Let’s try it and see if we like it. The idea will be that we’ll unconditionally use the KSLMaker in the view, and remove the ksl parameter.

class DungeonView(arcade.View):
    def __init__(self, dungeon):
        if arcade.window_commands._window:
            super().__init__()
            self.cameras = Cameras(self, dungeon.max_x, dungeon.max_y, zoom=params.zoom_factor)
        self.dungeon = dungeon
        self.pub_sub = dungeon.pub_sub
        self.keys =  None
        self.keyed_sprites = KeyedSpriteListMaker(dungeon).keyed_sprites
        self.subscribe(self.dungeon, self.pub_sub)

That works. Commit: DungeonView uses KeyedSpriteListMaker to make its sprites.

Go through and remove all the now-redundant calls to the KeyedSpriteListMaker. Green, game runs. Commit: tidying.

Reflection

I think we’re done for this session. We have reduced the responsibility of the KeyedSpriteListMaker to just one: making a KeyedSpriteList, retaining its ability to update a view.

Wait! We shouldn’t be fetching the KSL in DungeonView. We should ask it to update us:

class DungeonView(arcade.View):
    def __init__(self, dungeon):
        if arcade.window_commands._window:
            super().__init__()
            self.cameras = Cameras(self, dungeon.max_x, dungeon.max_y, zoom=params.zoom_factor)
        self.dungeon = dungeon
        self.pub_sub = dungeon.pub_sub
        self.keys =  None
        self.keyed_sprites = None # updated below
        KeyedSpriteListMaker(dungeon).update(self)
        self.subscribe(self.dungeon, self.pub_sub)

Why? Because we use that facility in the BuildStepper and we want to make all the uses of the class consistent. Let’s check to be sure no one is accessing the internal member.

I’m not entirely fond of that, because of the need to create the instance variable above, but we’ll stick with it.

I got carried away and made some changes to KeyedSpriteListMaker, and I want to make more. Here it is as of now:

class KeyedSpriteListMaker:
    def __init__(self, dungeon):
        self.setup_assets()
        self._keyed_sprites = KeyedSpriteList(arcade.SpriteList())
        self.setup(dungeon)

    def setup(self, dungeon):
        self.create_room_sprites(dungeon)
        self.create_content_lists(dungeon)

    def update(self, view):
        view.keyed_sprites = self._keyed_sprites

    def create_room_sprites(self, dungeon):
        for room in dungeon.rooms:
            view = RoomView(room)
            for cell, sprite in view.generate_sprites(dungeon.layout):
                self._keyed_sprites.add(cell, sprite)

    def create_content_lists(self, dungeon):
        for cell, content in dungeon.layout.contents.items():
            for item in content:
                self._keyed_sprites.make_visible_content_sprite(item, cell)

We do need to have the _keyed_sprites member, for update. But would the class be “better” like this:

class KeyedSpriteListMaker:
    def __init__(self, dungeon):
        self.setup_assets()
        self._keyed_sprites = self.setup(dungeon)

    def setup(self, dungeon):
        keyed_sprites = KeyedSpriteList(arcade.SpriteList())
        self.create_room_sprites(dungeon, keyed_sprites)
        self.create_content_lists(dungeon, keyed_sprites)
        return keyed_sprites

    def update(self, view):
        view.keyed_sprites = self._keyed_sprites

    def create_room_sprites(self, dungeon, keyed_sprites):
        for room in dungeon.rooms:
            view = RoomView(room)
            for cell, sprite in view.generate_sprites(dungeon.layout):
                keyed_sprites.add(cell, sprite)

    def create_content_lists(self, dungeon, keyed_sprites):
        for cell, content in dungeon.layout.contents.items():
            for item in content:
                keyed_sprites.make_visible_content_sprite(item, cell)

Recursive Reflection

Is that better, because we create the SpriteList as a temp and pass it to the methods that fill it in, and only then set the member variable?

I honestly don’t know. I have some embedded wisdom that suggests passing a parameter is better than continually updating a member variable, and I don’t know why I even have that notion. It is marginally more efficient to pass the variable rather than fetch it with self. repeatedly, but no one cares about speed in this method.

Commit that.

Reflection.pop()

We have simplified DungeonView down to one creation parameter rather than three. We have replaced the DungeonViewMaker, which had split responsibilities, with KeyedSpriteListMaker, which has only one, which is to update a view with a KeyedSpriteList.

Which suggests something. What if the KSLM was a KeyedSpriteListUpdater and was passed a view in the constructor and updated immediately? Would that be too weird? I suspect that it might. We’ll belay that for now thought and maybe try it later.

Bottom line, external users of the changed classes have less to do, while internally things are mostly improved or, if you hate parameters for some reason, just a tiny bit more complex.

Summary

A bit better by my lights. And the internet appears to be back. See you next time!