Hello, loves!

OK, let’s do the thing we love: find something to improve, and improve it. Shall we start where we left off? Result: Not as joyful as some days.

We pared down the RoomView by removing its references to layout, dungeon, and dungeon view, passing them as parameters when they were needed. Pretty much what the received wisdom would suggest and I’ve done it many times and don’t recall ever regretting it. There is a place for encapsulating a bunch of objects that need to be processed together, often using the pattern called Method Object that replaces a method with a small object bound to the info needed to do one thing, but as a rule, long-standing objects that connect to lots of other objects just make things hard to change and hard to test.

One could wonder why I do it so often.

Let’s start by looking at RoomView again and thinking about what we have wrought.

class RoomView:
    def __init__(self, room):
        self.room = room
        self.sprites = []
        self.cell_sprites = dict()
        self.texture_finder = TextureFinder()
    def create_floor(self, layout, shape_list):
        for cell in self.room:
            self.choose_flooring(layout, cell, shape_list)

    def choose_flooring(self, layout, cell, shape_list):
        texture = self.choose_flooring_texture(layout, cell)
        sprite = params.make_adjusted_sprite(cell, texture)
        self.cell_sprites[cell] = sprite
        self.sprites.append(sprite)
        shape_list.append(sprite)

    def illuminate(self, dungeon_view, center_cell, distance):
        for cell, sprite in self.cell_sprites.items():
            if cell.manhattan_distance(center_cell) < distance:
                dungeon_view.illuminate(cell)
                sprite.visible = True

    def choose_flooring_texture(self, layout, cell):
        borders: BorderList = layout.get_borders(cell)
        border_type = borders.border_string()
        name = self.texture_finder.full_name(border_type)
        return arcade.load_texture(name)

As I mentioned yesterday, it’s pretty clear that there are two things going on in this object, creating the sprites for the room’s flooring, and illuminating the sprites upon demand.

Arguably there are two phases to the sprite creation, choosing the texture to be used, and creating the actual sprite.

The two choose methods should be private, and together. Do that.

    def create_floor(self, layout, shape_list):
        for cell in self.room:
            self._choose_flooring(layout, cell, shape_list)

    def _choose_flooring(self, layout, cell, shape_list):
        texture = self._choose_flooring_texture(layout, cell)
        sprite = params.make_adjusted_sprite(cell, texture)
        self.cell_sprites[cell] = sprite
        self.sprites.append(sprite)
        shape_list.append(sprite)

    def _choose_flooring_texture(self, layout, cell):
        borders: BorderList = layout.get_borders(cell)
        border_type = borders.border_string()
        name = self.texture_finder.full_name(border_type)
        return arcade.load_texture(name)

    def illuminate(self, dungeon_view, center_cell, distance):
        for cell, sprite in self.cell_sprites.items():
            if cell.manhattan_distance(center_cell) < distance:
                dungeon_view.illuminate(cell)
                sprite.visible = True

Commit.

The question in my mind is what this object really is, since it’s not really a view, and whether it should be separated into two objects, one about choosing flooring and one about illuminating the floor.

It is not a dungeon-level object, because it thinks about textures, which are view-level, not dungeon level.

Illumination, though. The DungeonView knows, for all cells, which are illuminated. Currently the dungeon does not know whether it is illuminated or not, and we have no story that tells us that it needs to know, although it is easy to think of why we’ll need it. We don’t do things because we’re going to need them, we do them when we need them.

As written, illuminate marks sprites and informs the DungeonView that a cell is illuminated. DungeonView just does this:

class DungeonView:
    def illuminate(self, cell):
        self.illuminated_cells.add(cell)

Hm, how does the RoomView get told to illuminate, one wonders.

class DungeonView:
    def illuminate_around_dot(self):
        dot = self.dungeon.player_cell
        if not dot: return # crock to allow a test to run
        room = dot.room
        radius = 1000 if self.dungeon.dot_has('a brilliant torch') else 4
        self.room_views[room].illuminate(self, dot, radius)
        for content_view in self.content_views_by_room[room]:
            content_view.illuminate(dot, radius)

While we’re at it:

class ContentView:
    def illuminate(self, dot, range):
        if dot.manhattan_distance(self.cell) < range:
            self.sprite.visible = True

Now as things stand, the DungeonView has all the sprites for the floor, all in a big sprite list. And the RoomView maintains a dictionary from cell to sprite, one such dictionary for each room.

I just noticed that RoomView maintains the sprite_list member but never uses it. Remove it. Two more lines gone from RoomView. Commit.

Design Thinking

I’m not ready to try this but just thinking …

The DungeonView has access to the Room instances. A Room is, among other responsibilities, a set of cells. If DungeonView maintained the mapping from cell to sprite then illumination of a room could be done in DungeonView and illuminate removed from the RoomView, removing that responsibility and leaving it as no more than a texture finder for flooring.

I think that will be better. Let’s do it.

I want to sketch some code that is like the code I’ll want for illuminate_around_dot as shown above. Maybe something like this:

    def illuminate_around_dotX(self):
        dot = self.dungeon.player_cell
        if not dot: return # crock to allow a test to run
        room = dot.room
        radius = 1000 if self.dungeon.dot_has('a brilliant torch') else 4
        for cell in room:
            if cell.manhattan_distance(dot.room) <= radius:
                self.illuminate_cell(cell)

    def illuminate_cell(self, cell):
        sprite = self.cell_sprites[cell]
        sprite.visible = True
        self.illuminated_cells.add(cell)
        for content_view in self.cell_content_view[cell]:
            content_view.illuminate()

For this to work, DungeonView needs two new dictionaries, cell_sprites, which needs to be a mapping from all cells to their sprites, and cell_content_view, a mapping from cell to a list of contents (or an empty list if absent).

To get the cell_sprites dictionary, we have to deal with the flooring logic in RoomView, which is presently passed a sprite list and adds to it. Where do we do that?

class DungeonView:
    def create_rooms(self, shape_list):
        for room in self.dungeon.rooms:
            view = RoomView(room)
            self.room_views[room] = view
            view.create_floor(self.dungeon.layout, shape_list)

I think that what we want here is for the view to act as a generator, producing a cell and a sprite for each cell in the room.

Can we build that internally to RoomView and use existing tests to be sure we’re right?

Seems like a small step. Let’s try it.

class RoomView:
    def create_floor(self, layout, shape_list):
        for cell in self.room:
            self._choose_flooring(layout, cell, shape_list)

    def _choose_flooring(self, layout, cell, shape_list):
        texture = self._choose_flooring_texture(layout, cell)
        sprite = params.make_adjusted_sprite(cell, texture)
        self.cell_sprites[cell] = sprite
        shape_list.append(sprite)

I refactor to this:

class RoomView:
    def create_floor(self, layout, shape_list):
        for cell, sprite in self.generate_sprites(layout):
            self.cell_sprites[cell] = sprite
            shape_list.append(sprite)

    def generate_sprites(self, layout):
        for cell in self.room:
            sprite = self.make_sprite(layout, cell)
            yield cell, sprite

    def _choose_flooring(self, layout, cell, shape_list):
        sprite = self.make_sprite(layout, cell)
        self.cell_sprites[cell] = sprite
        shape_list.append(sprite)

    def make_sprite(self, layout, cell):
        texture = self._choose_flooring_texture(layout, cell)
        sprite = params.make_adjusted_sprite(cell, texture)
        return sprite

This works. I note that no tests broke while I was typing that in, which tells me we probably need more testing around this. I did test in play and it works.

Now there is a generator that DungeonView can call. If we use that instead of create_floor, the RoomView will no longer have its local list of sprites. Let’s move that one line up:

    def generate_sprites(self, layout):
        for cell in self.room:
            sprite = self.make_sprite(layout, cell)
            self.cell_sprites[cell] = sprite
            yield cell, sprite

Now let’s see if we can use the generate to create our cell-to-sprite dictionary …

After more hackery than I am comfortable with, I have things working. We’ll need to consider this a spike and it needs doing over. I will not roll it back yet. I need to learn from it a bit more but here’s what I’ve got.

class RoomView:
    def create_floor(self, layout, shape_list):
        for cell, sprite in self.generate_sprites(layout):
            self.cell_sprites[cell] = sprite
            shape_list.append(sprite)

    def generate_sprites(self, layout):
        for cell in self.room:
            sprite = self.make_sprite(layout, cell)
            self.cell_sprites[cell] = sprite
            yield cell, sprite

    def make_sprite(self, layout, cell):
        texture = self._choose_flooring_texture(layout, cell)
        sprite = params.make_adjusted_sprite(cell, texture)
        return sprite

I extracted the new method make_sprite and use it in the new generate_sprites method, which keeps create_floor working, although now that the rest is working I think it’s no longer called or needed.

In DungeonView there’s more:

class DungeonView:
    def __init__(self, dungeon, testing=False):
        ...
        self.content_views_by_cell: dict[Cell, list[ContentView]] = defaultdict(list)
        self.cell_sprites: dict[Cell, Sprite] = dict()
        ...

    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_views_by_cell[cell].append(view)
        self.content_sprite_list.append(view.sprite)
        if cell in self.illuminated_cells:
            view.sprite.visible = True

    def create_rooms(self, shape_list):
        for room in self.dungeon.rooms:
            view = RoomView(room)
            self.room_views[room] = view
            for cell, sprite in view.generate_sprites(self.dungeon.layout):
                self.cell_sprites[cell] = sprite
                self.room_sprite_list.append(sprite)

    def illuminate_around_dot(self):
        dot = self.dungeon.player_cell
        if not dot: return # crock to allow a test to run
        room = dot.room
        radius = 1000 if self.dungeon.dot_has('a brilliant torch') else 4
        for cell in room:
            if cell.manhattan_distance(dot) <= radius:
                self.illuminate_cell(cell)

    def illuminate_cell(self, cell):
        sprite = self.cell_sprites[cell]
        sprite.visible = True
        self.illuminated_cells.add(cell)
        for content_view in self.content_views_by_cell[cell]:
            content_view.just_illuminate()

Reflection

The bad news is that having just written about the joy I find in programming, what I’ve just done was not joyful. It was confusing, irritating, and working without a net of tests. That’s why I’m not going to commit it now, and while next time around I’ll probably just revert and do it over.

The good news is that the basic idea of generating the sprites in RoomView and handing them over to DungeonView does work. I think that the illuminate method in RoomView is no longer used.

The observations include:

  • It’s just possible that despite its flaws, this is a better structure for things.
  • create_rooms doesn’t create rooms and never did. Needs better name, if only create_sprites or something.
  • There are way too many lists and dictionaries in DungeonView. Some of them may already be unused. Others should be obviated.
  • There are no tests for any of this, and never have been.

The hopeful observation is that as I sit with it for a moment, it’s not really that bad. It might make sense not to throw it away, but instead to write the tests that we should have had right along, and then refactor this code to make it decent.

We’ll see. I’m late to my appointment with an iced Chai. See you next time!