Hello, loves!

The split between making the view and running it is working. Now we get to make it a bit more right. Result: quite pleased so far.

Some issues that I’m aware of even before opening the code include; four tests that didn’t survive the DungeonView splitting off the new RealDungeonView class; the class names need “improvement”; and I think we need to pay particular attention to where the cameras get created.

Let’s do the naming first. I propose DungeonViewMaker creating DungeonView. All in favor? OK, let’s do it. Four quick dialogs and PyCharm does the work. Classes and files renamed. Commit.

The cameras are created in DungeonViewMaker:

class DungeonViewMaker(arcade.View):
    def __init__(self, dungeon):
        self.cameras = None
        if arcade.window_commands._window:
            super().__init__()
            self.cameras = Cameras(self, dungeon.max_x, dungeon.max_y, zoom=4)
        self.setup_assets()
        self.dungeon = dungeon
        self.pub_sub = dungeon.pub_sub
        self.keyed_sprites = KeyedSpriteList(arcade.SpriteList())
        self.content_sprites_by_cell: dict[Cell, list[Sprite]] = defaultdict(list)
        self.setup()
        real_view = DungeonView(self.dungeon, self.pub_sub, self.cameras,
                                self.keyed_sprites,
                                self.content_sprites_by_cell)
        real_view.run()

As soon as we look at this we see a few things to improve:

The Maker is inheriting from view. The reason for that is so that it can make the cameras. I’m not sure that’s a good enough reason.

The Maker just up and creates and runs the view. That means that to test it, we have to accept that it’s going to drag along the creation and execution of the view. That’s probably part of why the tests aren’t happy.

Let’s see what goes on in Cameras. It’s entirely possible that the View should be creating them, not the maker.

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))

I think those references to view.width and view.height are actually inherited from arcade.View and I’m not sure where they come from. Let’s see what happens if we cause the maker not to inherit from View. Sure enough, we get a message telling us that the maker doesn’t understand width or height.

Let’s move cameras creation down to the actual view. We do it the same way, inside the check for window, though I am hopeful that we will be able to avoid needing that. Depends on how much testing the View needs.

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

I think it makes more sense to create the cameras inside the view. And, like the subscriptions, if and when we change the cameras, we’d prefer not to need to change both view and maker to do it. Commit this: cameras created in view.

Enough Warmup

Those were easy and simple enough. Now, before we look at the tests, let’s change the maker so that it does all its getting ready during init, but creates and returns the actual view only on demand. We go from this:

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.content_sprites_by_cell: dict[Cell, list[Sprite]] = defaultdict(list)
        self.setup()
        real_view = DungeonView(self.dungeon, self.pub_sub, self.keyed_sprites, self.content_sprites_by_cell)
        real_view.run()

To this:

    def __init__(self, dungeon):
        self.setup_assets()
        self.dungeon = dungeon
        self.pub_sub = dungeon.pub_sub
        self.keyed_sprites = KeyedSpriteList(arcade.SpriteList())
        self.content_sprites_by_cell: dict[Cell, list[Sprite]] = defaultdict(list)
        self.setup()

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

Now we need to use it in main. We change the last line to

    DungeonViewMaker(dungeon).view.run()

All good. Commit: DungeonView only created upon calling maker.view.

About Those Tests

Oh, right. Let’s have a look at those. There were four that I marked .skip('needs revision').

I pick one, remove the skip. It runs. I find another. It appears there were five. The others are recalcitrant and don’t just work. Have to do some programming, make some decisions.

This one:

    def test_announcements(self):
        count=0
        def count_them(msg):
            nonlocal count
            count += 1
        layout = DungeonLayout(10, 10)
        room_cell = Cell(5, 5)
        room = Room([room_cell])
        layout.add_room(room)
        dungeon = Dungeon(layout)
        DungeonViewMaker(dungeon)
        factory = ContentFactory()
        item_1 = factory.receivable(name="treasure", resource='none', scale=0.5)
        item_2 = factory.receivable(name="more treasure", resource='none', scale=0.5)
        room_cell.add_content(item_1)
        room_cell.add_content(item_2)
        assert len(dungeon.announcements) == 0
        dungeon.set_player_position_with_interaction(room_cell)
        count = 0
        dungeon.announce_via(count_them)
        assert count == 2
        assert len(dungeon.announcements) == 0

Let’s see what it is whining about. It’s blowing up on the maker creation. Let’s just remove that. Test runs. Let’s commit these: unskipping tests. Sublime Text thinks unskipping is not a word. Agree to disagree.

    def test_room_view_illuminate(self):
        layout = DungeonLayout(20,20)
        dungeon = Dungeon(layout)
        dungeon_view = DungeonViewMaker(dungeon)
        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_room_sprites()
        dungeon_view.illuminate_around_dot()
        lit_cell = Cell(13, 13)
        lit = dungeon_view.keyed_sprites[lit_cell]
        assert lit.visible == True
        unlit_cell = Cell(9, 9)
        unlit = dungeon_view.keyed_sprites[unlit_cell]
        assert unlit.visible == False

This one needs a bit of thinking. What are we trying to figure out here? We’re actually trying to be sure that the sprites get illuminated. An actual view test, I think. Let’s get the view from the maker, see if that fixes it up.

Needs a little rearrangement, and some things are already done in the maker, so this runs:

    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 = DungeonViewMaker(dungeon).view
        dungeon_view.illuminate_around_dot()
        lit_cell = Cell(13, 13)
        lit = dungeon_view.keyed_sprites[lit_cell]
        assert lit.visible == True
        unlit_cell = Cell(9, 9)
        unlit = dungeon_view.keyed_sprites[unlit_cell]
        assert unlit.visible == False

class TestHandlers:
    @pytest.mark.skip('needs revision')
    def test_keydown(self):
        fake = FakeDungeon()
        view = DungeonViewMaker(fake)
        
        event = arcade.key.UP
        view.on_key_press(event, 0)
        assert fake.received == Direction.NORTH
        view.on_key_release(event, 0)

        event = arcade.key.DOWN
        view.on_key_press(event, 0)
        assert fake.received == Direction.SOUTH
        view.on_key_release(event, 0)

        event = arcade.key.LEFT
        view.on_key_press(event, 0)
        assert fake.received == Direction.WEST
        view.on_key_release(event, 0)

        event = arcade.key.RIGHT
        view.on_key_press(event, 0)
        assert fake.received == Direction.EAST
        view.on_key_release(event, 0)

First fetch the actual view. Doesn’t quite fix it. Small mods to the fake dungeon fix the test, add an actual layout, and return an empty room list.

class FakeDungeon:
    def __init__(self):
        self.received = 'nothing'
        self.layout = DungeonLayout()
        self.pub_sub = PubSub()

    def move_player(self, direction):
        self.received = direction

    @property
    def rooms(self):
        return []

    @property
    def player_cell(self):
        return None

    def subscribe(self, *args, **kwarg):
        pass

Test passes. I suspect we would do better to test against the KeyPress object but that’s more work than I want to do right now.

Commit: all tests skipped for new view are updated and green.

Observation

I notice, because I opened it for some reason that KeyPress sends messages to the view:

    def on_key_press(self, symbol: int, modifiers: int) -> bool | None:
        if self.key_lock is not None:
            return
        self.key_lock = (symbol, modifiers)
        if symbol == arcade.key.RIGHT:
            self.view.move_player(Direction.EAST)
        elif symbol == arcade.key.LEFT:
            self.view.move_player(Direction.WEST)
        ...

class DungeonView...
    def move_player(self, direction: Direction):
        self.dungeon.move_player(direction)
        self.illuminate_around_dot()

I suspect that it may make more sense to send messages from the keys to the dungeon. Who owns the keyboard, the dungeon or the view? Who does the keyboard report to, the dungeon or the view? I think that for now, I don’t care.

Relatedly, at least in my mind, is the question of illumination. Is that a view property or a dungeon property. I think that in the fullness of time, it should be a dungeon property, because different things happen in the dark than in the light. So that may want to change. But for now, no.

Reflection

We now have all the tests that ran before we did the maker-view split running again. The DungeonView (the new one) includes only run-time things. Well … I believe that’s true. Careful enough inspection might turn something up.

There is some duplication. This method appears in both the maker and the view:

    def make_view_and_sprite(self, cell, item):
        sprite = ContentSpriteMaker(item.resources, item.scale).sprite
        sprite.position = cell.center_position(cell_size)
        self.content_sprites_by_cell[cell].append(sprite)
        self.keyed_sprites.add(item, sprite)
        if self.keyed_sprites[cell].visible:
            sprite.visible = True

The reason is that the maker creates all the sprites that are pre-allocated in the dungeon, but some can come into existence dynamically, and the view needs to make those sprites, and that means that those two collections need to be updated dynamically. I don’t see much to be done about that. We could make some kind of utility method perhaps, but at this moment it’s not bothering me enough to sort it. As soon as the rules change for those collections, though …

Maybe there’s an object that combines access for those two collections? The keyed_sprites one is already a smart collection. Some kind of ConvenientlyAddressableSpriteContainer, perhaps. Interesting …

All that is for another day.

Summary

We have renamed the classes to sensible names and made all the tests run. We’ve made a couple of small adjustments to where things live. And, most interesting, we have refactored the very messy original DungeonView, from around 250 lines at its heaviest, down to 100 right now, with the extra weight moving to other simple classes, including DungeonViewMaker at 62 lines.

So once again we see that we can take truly horrid code and over a series of very small changes, improve it substantially. My guess is that it’s always possible, and given my personal experience with rewriting big products, my guess is that it’s almost always preferable to refactor over rewriting.

I’m pleased with what we have … and sure we’ll find something to improve next time!

See you then!