Hello, loves!

Just Noticed … a simplification. At least one more instance variable should disappear. I wonder, though …

Last night while waiting for something to happen in the Big Game I thought to glance at the DungeonView init:

class DungeonView(arcade.View):
    def __init__(self, dungeon, testing=False):
        if not testing:
            super().__init__()
        self.dungeon = dungeon
        self.pub_sub = dungeon.pub_sub
        self.subscribe(dungeon, self.pub_sub)
        self.setup_assets()
        self.key_lock = None
        self.keyed_floor_sprites = KeyedSpriteList(arcade.SpriteList())
        self.content_views: dict[Content, ContentView] = dict()
        self.content_views_by_cell: dict[Cell, list[ContentView]] = defaultdict(list)
        self.illuminated_cells: set[Cell] = set()
        self.content_sprite_list = None
        self.dungeon_camera = None
        self.dungeon_camera_bounds = None
        self.scroller = None
        self.scroller_camera = None

There’s a lot to see there, and we’ll come back to that below. The thing that I noticed was

        self.illuminated_cells: set[Cell] = set()

It came to me that a cell is illuminated if its visible property is true, and that we have access to the sprite via our new KeyedSpriteList. Both objects are hashed, so the speed of access to either will be about the same. So why couldn’t we use the keyed_floor_sprites to answer the question of whether a cell is illuminated? I have no doubt that we could, and in this case it seems pretty sure that we “should”.

We’ll talk below about my wondering why this wasn’t always obvious. For now, let’s see how that thing is accessed.

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

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

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

Three are also two tests accessing the illuminated_cells variable. If think that the illuminate method must be called from outside? Find senders. None. Amusing. Remove it and commit.

If we change the test in make_view_and_sprite, we should have no more need for the illuminated_cells in DungeonView and then we’ll fix up the tests.

There might be something nicer to do but for now we’ll keep it simple.

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

Green, commit. Now I’ll remove the setting of the member, see the tests break, fix them.

    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)
        layout.ensure_connected()
        layout.make_passages()
        layout.make_borders()
        dungeon.just_set_player_position(Cell(12,12))
        dungeon_view.create_rooms()
        dungeon_view.illuminate_around_dot()
        lit_cell = Cell(13, 13)
        lit = dungeon_view.keyed_floor_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 = dungeon_view.keyed_floor_sprites[unlit_cell]
        assert unlit.visible == False

Ah, perfect. The references are both in this test and they are there to ensure that we do in fact fill in the illuminated_cells properly. Those two lines go. Commit. Remove the instance variable. Commit.

Here we are, this much article and twenty minutes and we have removed an instance variable and five or ten lines of code. Just a bit better.

I Wonder Though …

I think that at the time the illuminated_cells set was put in, we surely already had the old structure that mapped cells to floor sprites. So, in principle, we could have done this, or essentially this, right then, referencing the private property of the sprite. Why didn’t I see the possibility? Or, if I saw it, why didn’t I do it, instead creating that new set?

I have the luxury of having recorded most of the history of this program, so I see that what happened was that I wanted to answer is_lit, and it just came to me that a cell is lit if it is in the set of illuminated cells, and I just created that set. As far as I can tell from the article, I didn’t really look in enough detail to see that we could get the information a bit differently. Partly, I suspect, I was on a roll. But I surely had to look at the init to put the new instance variable in, so why didn’t I see that then?

I think the reason is pretty clear:

class DungeonView(arcade.View):
    def __init__(self, dungeon, testing=False):
        if not testing:
            super().__init__()
        self.dungeon = dungeon
        self.pub_sub = dungeon.pub_sub
        self.subscribe(dungeon, self.pub_sub)
        self.setup_assets()
        self.key_lock = None
        self.keyed_floor_sprites = KeyedSpriteList(arcade.SpriteList())
        self.content_views: dict[Content, ContentView] = dict()
        self.content_views_by_cell: dict[Cell, list[ContentView]] = defaultdict(list)
        self.content_sprite_list = None
        self.dungeon_camera = None
        self.dungeon_camera_bounds = None
        self.scroller = None
        self.scroller_camera = None

There is way too much going on here, and it’s not very well organized. There’s too much going on and the names of things are all so similar that they are confusing and no particular one comes to mind as you read content_this and content_that, and, in those days, cell_this and cell_that.

Now in fact, there is a natural reason why this class is the way it is, and, pace Weinberg, it isn’t just because it got that way. It got that way because it is the central nexus for viewing and controlling the dungeon. Classes like this arise, and begin to accumulate things that seem to belong there, or near there, and there isn’t a better place for them, and, especially when we’re just shaping things, we don’t really know quite how a separate object might help us.

So we get a mess. And we have a mess. The DungeonView class is just shy of 300 lines, 291 right now, although it does have ContentView bound into it, just to make things more confusing.

So, what to do? We improve it, of course. We’ll even do a bit more this morning.

First, move ContentView to its own file. That took 9 keystrokes and about nine seconds. DungeonView is down to a slightly less portly 266 lines.

Looking at that init, we can see a few different things going on:

  1. Keeping our pointer to the dungeon. Seems appropriate to a DungeonView.
  2. Setting up to publish and receive publications.
  3. Setting up assets (defining the resource handle our textures need).
  4. Defining the keyboard lock toggle.
  5. Managing floor sprites.
  6. Managing content, content view, and sprites.
  7. Managing cameras and the scroller.

That’s a lot to ask one object to be remembering.

What is it doing? DungeonView has over 30 methods: compute_margin, create_content_lists, create_rooms, draw_adventurer, draw_content_objects, draw_contents, draw_flood, draw_passages, illuminate_around_dot, illuminate_cell, make_initial_announcement, make_view_and_sprite, margin_rectangle, max_x, max_y, move_player, on_draw, on_key_press, on_key_release, on_update, run, scroll_dungeon_camera, setup, setup_assets, setup_cameras, setup_dungeon_camera, setup_scroller, subscribe, subscribe_to_announce, subscribe_to_remove_content, subscribe_to_state_number, view_do. Eeek!

That’s just terribly daunting, isn’t it? With a few exceptions, the methods seem chaotic and all over the map. Looking at that, if we were feeling any kind of pressure to actually get things done, I could understand why we’d just find a place to jam in what we need, making things a bit worse. It’s just about impossible to take it in. At least it is for me, and I wrote it!

What do we do? We look for small improvements. And we look for ways to make smaller objects to take on some of the load. In particular, we might look for places where a MethodObject or similar helper could offload some activity.,

I’m wondering about whether grouping some things might be useful. Take those camera bits:

    def __init__...
        self.dungeon_camera = None
        self.dungeon_camera_bounds = None
        self.scroller_camera = None
        self.scroller = None
        ...

    def setup_cameras(self):
        self.setup_dungeon_camera()
        self.scroller_camera = arcade.Camera2D()

    def setup_dungeon_camera(self):
        self.dungeon_camera = arcade.Camera2D()
        zoom = 4
        width_margin = self.compute_margin(self.max_x, zoom)
        height_margin = self.compute_margin(self.max_y, zoom)
        self.dungeon_camera_bounds = (
            self.margin_rectangle(width_margin, height_margin))
        self.dungeon_camera.zoom = zoom

    def on_draw(self):
        self.clear()
        self.scroll_dungeon_camera()
        with self.dungeon_camera.activate():
            self.keyed_floor_sprites.draw()
            self.draw_contents()
            self.draw_passages()
            self.draw_flood()
        with self.scroller_camera.activate():
            self.scroller.draw()

Let’s try making a simple object for those four instance variables. It’s not clear that scroller should be in there but I think we’ll try it that way.

I think I’ll TDD that a bit, just to get a sense of it.

class TestCameras:
    def test_exists(self):
        assert False

So far so good. A brief time elapses.

I basically copied all the stuff I needed, but I can’t really test it without setting up a window, which I refuse to do in a test. I think I can just make it work. Yes, it went easily. The TDD helped, not because the test was helpful but because typing the thing into that test window helped.

Here’s the new class, basically a MethodObject refactoring:

class Cameras:
    def __init__(self, view, max_x, max_y, zoom):
        self.max_x = max_x
        self.max_y = max_y
        self.view = view
        self.scroller_cam = arcade.Camera2D()
        self.scroller = Scroller(lines=4, base=(512,800))
        self.dungeon_cam = arcade.Camera2D()
        self.dungeon_cam.zoom = zoom
        width_margin = self.compute_margin(self.max_x, zoom)
        height_margin = self.compute_margin(self.max_y, zoom)
        self.dungeon_camera_bounds = (
            self.margin_rectangle(width_margin, height_margin))

    def compute_margin(self, cell_count, zoom):
        cells_visible = cell_count // zoom
        desired_margin = cells_visible // 2
        pixel_margin = desired_margin * cell_size
        return pixel_margin

    def margin_rectangle(self, width_margin, height_margin):
        return arcade.LRBT(
            width_margin,
            self.view.width - width_margin,
            height_margin,
            self.view. height - height_margin)

It contains the dungeon_cam, scroller_cam, and scroller. I think it’s a mistake to include those other members. We’ll check that shortly.

Here are the lines changed in DungeonView:

class DungeonView(arcade.View):
    def __init__(self, dungeon, testing=False):
        if not testing:
            super().__init__()
        self.dungeon = dungeon
        ...
        self.cameras = None

    def setup_cameras(self):
        zoom = 4
        self.cameras = Cameras(self, self.max_x, self.max_y, zoom)

    def on_draw(self):
        self.clear()
        self.scroll_dungeon_camera()
        with self.cameras.dungeon_cam.activate():
            self.keyed_floor_sprites.draw()
            self.draw_contents()
            self.draw_passages()
            self.draw_flood()
        with self.cameras.scroller_cam.activate():
            self.cameras.scroller.draw()

    def on_update(self, delta_time: float):
        def receive_announcement(msg):
            self.cameras.scroller.append(msg)

        self.dungeon.announce_via(receive_announcement)
        self.cameras.scroller.update()
        self.pub_sub.publish('on_update', 'view', delta_time=delta_time)

    def scroll_dungeon_camera(self):
        cx, cy = self.dungeon.player_cell.xy
        self.cameras.dungeon_cam.position = (cx * cell_size, cy * cell_size)
        # Constrain the camera's position to the camera bounds.
        self.cameras.dungeon_cam.view_data.position = (
            arcade.camera.grips.constrain_xy(
            self.cameras.dungeon_cam.view_data, self.cameras.dungeon_camera_bounds
        ))

In every case we just forward to the cameras to do what we need. This works, though it is perforce only testable in the game. I truly do not see how to make things like this directly testable in any useful way.

I think we can commit this: creating Cameras helper object.

Now we can simplify Cameras: it has too many members. I sort of created them by rote. The max_x and max_y are easy, as they are only used in the init:

class Cameras:
    def __init__(self, view, max_x, max_y, zoom):
        self.view = view
        self.scroller_cam = arcade.Camera2D()
        self.scroller = Scroller(lines=4, base=(512,800))
        self.dungeon_cam = arcade.Camera2D()
        self.dungeon_cam.zoom = zoom
        width_margin = self.compute_margin(max_x, zoom)
        height_margin = self.compute_margin(max_y, zoom)
        self.dungeon_camera_bounds = (
            self.margin_rectangle(width_margin, height_margin))

The view is used here:

    def margin_rectangle(self, width_margin, height_margin):
        return arcade.LRBT(
            width_margin,
            self.view.width - width_margin,
            height_margin,
            self.view. height - height_margin)

We can pass those value in. Change Signature:

class Cameras:
    def __init__(self, view, max_x, max_y, zoom):
        self.scroller_cam = arcade.Camera2D()
        self.scroller = Scroller(lines=4, base=(512,800))
        self.dungeon_cam = arcade.Camera2D()
        self.dungeon_cam.zoom = zoom
        width_margin = self.compute_margin(max_x, zoom)
        height_margin = self.compute_margin(max_y, zoom)
        self.dungeon_camera_bounds = (
            self.margin_rectangle(view.width, view.height, width_margin, height_margin))

    def compute_margin(self, cell_count, zoom):
        cells_visible = cell_count // zoom
        desired_margin = cells_visible // 2
        pixel_margin = desired_margin * cell_size
        return pixel_margin

    def margin_rectangle(self, view_width, view_height, width_margin, height_margin):
        return arcade.LRBT(
            width_margin,
            view_width - width_margin,
            height_margin,
            view_height - height_margin)

So that’s nice. Commit.

I’m quite sure that we can push more capability into Cameras, but this is a good place to stop. I’m just two hours in.

Reflection

DungeonView is down to 237 lines, from an initial 291, and we have removed the cameras, bounds and scroller from instance variables and replaced them with one somewhat intelligent object, Cameras, that creates and contains those others.

I’m not sure why dungeon_camera_bounds was a member variable. If we need it, we’ll find out. I’m sure that if we do window sizing we’ll need to do something with it, but that should go inside Cameras.

Looking at the DungeonView code using Cameras, I suspect that we can move more code inside it. We’ll look at that later, in another session.

The creation of Cameras was straightforward. We could probably read about how to do it in Martin Fowler’s excellent Refactoring book. The lack of tests for this kind of thing troubles me, but we can’t always get what we want. I suppose I could have faked arcade and written some tests. Maybe that would have been better. Maybe we should still do it, just to start getting some view tests in place. All that is for another day.

Summary

Once again, we see that a little thinking and a little coding can reduce the complexity of our design, improving it substantially, with very short investments of time and effort. We don’t need 40 days and 40 nights to refactor Dungeon. We need 40 sessions, spread over time. Maybe 80. Maybe 120. Doesn’t matter: every time we do it, we make things a bit better, make our work a bit easier.

Makes me wish programming was still a profession. It’s too much fun to lose.

See you next time!