Hello, loves!

Knowing the fix, let’s install it properly. Then maybe an idea.

Yesterday, during the two hours of thrashing that I didn’t document in detail, and even during the morning, where we figured out what the defect was, I put in some interesting little bits of code. But the files are kind of torn up. I think I’ll do a quick survey of the diff between yesterday’s last good commit and today’s code, and make some notes. Six src files and three tst files are changed at this moment. Quite likely we’ll commit the tests and roll the rest back. But we’ll see.

DungeonView
I pasted in a simple rescale method that can draw the map at a different scale. Easily done, the sprite lists understand rescale. Useful making app capability, should be done.

Patched out the initial announcement to save time in manual testing.

Put a place in handling the B key to put a breakpoint and then cause one whenever I wanted. Useful idea, do something sensible.

Used V key to rescale downward. Come up with something better.

DungeonLayout
Saved connection status in ensure_connected, to report dropping out without connecting. Redundant now?

The fix, make the zip in _make_path_rooms include a wrap-around. I wonder if we would prefer to consider all pairs of suites, instead of in numerical order. Result would be more inter-room paths. Dungeon is already hella complicated tho.

main.py
Hacked in a loop to run until a disconnected dungeon was created. No longer needed. We do need to better encapsulate dungeon building strategies and code. Or would need to, if this were a product.

I had changed things to place Dot far from the center. Needs some sensible behavior if this is to be very product-like. We’ll be thinking about that soon.

Hacked screen size calculation not to depend on cell-size, allowing for more of a smaller-scale dungeon to show up. Good idea, needs sensible implementation. Relates to display of things like status, inventory, controls, …

params.py
Changing cell_size judiciously can rescale the whole display. Do something sensible? Interesting problem, I suspect.
Room
Fiddled with __repr__ to get useful debug display. Should do this more often,, more judiciously.
Suite
Fiddled with __repr__ ditto.
TestCell
Wrote trivial test to ensure that I understand the zip wrap. Remove.
TestContentView
Marked a test to skip, to speed things up. Leave it skipped and think about it.
TestDungeonLayout
A test fails with the fix in, because it finds an additional path in the wrap around. Just make sure it keeps working.

The critical new test that demonstrates the defect and runs when the defect is fixed. Keep it.

I think we’ll commit all the test files and drop the rest. We’ll be left with a broken test or two. Putting the fix back should get us back to green.

Here Goes

Done. Two tests fail. They are the two we expect. The fix is to change this:

class DungeonLayout:
    def _make_path_rooms(self):
        suites = self.define_suites()
        for s1, s2 in zip(suites, suites[1:]):
            cells = s1.find_path_cells(s2)
            if cells:
                room = Room(cells, 'path')
                self.add_room(room)

To this:

class DungeonLayout:
    def _make_path_rooms(self):
        suites = self.define_suites()
        for s1, s2 in zip(suites, suites[1:] + suites[:1]):
            cells = s1.find_path_cells(s2)
            if cells:
                room = Room(cells, 'path')
                self.add_room(room)

Green. Commit: official fix for disconnected dungeon defect.

I note that check for having found a path: if we don’t get any cells we don’t add to the rooms. But we should never add an empty Room. No good could come of it. Does add_room check for that?

Let’s write a test.

    def test_cannot_add_empty_rooms(self):
        layout = DungeonLayout(20, 20)
        empty = Room([], 'nada')
        layout.add_room(empty)
        assert len(layout.rooms) == 0

Test fails. Let’s protect ourselves and then discuss it. Here’s the code in question:

class DungeonLayout:
    def add_room(self, room):
        room.subtract_all(self.rooms)
        for cell in room:
            self.room_map[cell] = room
        self.rooms.append(room)

We’d like to check the length of the room’s cells. The Room acts as an iterable over cells already. But will len work?

    def add_room(self, room):
        if len(room) == 0: return
        room.subtract_all(self.rooms)
        for cell in room:
            self.room_map[cell] = room
        self.rooms.append(room)

Wishful Thinking: 60 tests break. Fix:

class Room:
    def __len__(self):
        return len(self._cells)

Green. Commit: DungeonLayout will not add empty rooms. You can’t go in there anyway.

Before I even start to answer the questions below, I realize that our new room could wind up empty after the subtract_all. Change the implementation:

    def add_room(self, room):
        room.subtract_all(self.rooms)
        if len(room) == 0: return
        for cell in room:
            self.room_map[cell] = room
        self.rooms.append(room)

Commit. Now then:

Reflection

I see at least two questions we should ask ourselves:

  1. Ignoring an empty room in add_room is obviously ignoring an error. Shouldn’t we do something better?
  2. Is making Room act as an iterable over cells seems a bit cute. Is it too fancy?

As for ignoring an error, yes it is. And yes, we should probably do something better, for small values of “should”. I’ll add it to the list of things we might do. However, you can’t ever get into an empty room, and if there’s code somewhere that relies on there being cells in a room, that code had better never be presented an empty room. It might make sense to log errors like this but I’d never raise an exception or anything like that.

As for making Room an iterable over the room’s cells, a Room is basically a named container for cells, with convenient methods for extracting cells from one room that are already assigned to another. It’s pretty standard Python to make a container by providing implementations for __iter__, __len__, and so on.

However, I note that there is a property cells on Room, which returns the cells. It’s used primarily in tests, but there are these uses in the src tree:

class Dungeon:
    def populate(self):
        # consider removing this or providing some kind of data-driven thing
        dot_room = random.choice(self.rooms)
        dot_cell = random.choice(dot_room.cells)
        self.set_player_position_with_interaction(dot_cell)

class DungeonLayout:
    def find_suite(self, room) -> Suite:
        suite_rooms: set[Room] = set()
        start = room.cells[0]
        for cell, _ in Flooder(layout=self, origin=start).in_any_room().flood():
            suite_rooms.add(self.get_room(cell))
        return Suite(self, suite_rooms)

I think that both of these are asking Room to have a method any_cell.

class RoomView:
    def create_floor(self, shape_list):
        cells = set(self.room.cells)
        for cell in cells:
            self.choose_flooring(cell, shape_list)

Why is that even doing all that? Is it trying to be sure that the room doesn’t contain duplicate cells? NunyaBizness.

    def create_floor(self, shape_list):
        cells = set(self.room.cells)
        for cell in cells:
            self.choose_flooring(cell, shape_list)

class Suite:
    @property
    def cells(self):
        for room in self.room_set:
            for cell in room.cells:
                yield cell

That can just be for cell in room. Make it so.

Commit those few changes: don’t go referencing Room.cells.

Now for any_cell, I’ll do some Wishful Thinking:

 class Dungeon:
    def populate(self):
        # consider removing this or providing some kind of data-driven thing
        dot_room = random.choice(self.rooms)
        dot_cell = dot_room.any_cell()
        self.set_player_position_with_interaction(dot_cell)

Tests break, of course.

class Room:
    def any_cell(self):
        return random.choice(list(self._cells))

Green. Commit. Then:

class DungeonLayout:
    def find_suite(self, room) -> Suite:
        suite_rooms: set[Room] = set()
        start = room.any_cell()
        for cell, _ in Flooder(layout=self, origin=start).in_any_room().flood():
            suite_rooms.add(self.get_room(cell))
        return Suite(self, suite_rooms)

Green. Commit.

The method above gets some complaints from PyCharm, which knows that get_room can, in principle return None. A quick change to return an empty room breaks things. I’ve made a note to look into it further, see how we can improve things.

I take a few minutes to tick through all the rest of the references to Room.cells, in tests. I’ve changed them all, and removed the cells method and also the forget method, which had no users.

Reflection

So, this morning we saved the new tests, flushed all the code in the source tree, and reinstalled our less-than-one-line fix to the Dreaded Disconnected Dungeon Defect.

We thought about whether the changes made were legit and decided that they were. Then, while we were in there, we observed code oddities and chased them down, improving a few things, and simplifying both the Room class and its users. There are now about three places where the tests refer to the member variable _cells on Room, which is OK, because they need some details. I think there is one place in the src tree, in Suite:

    def subtract(self, room):
        self._cells = self._cells - room._cells

Method might want a better name, or maybe we should do something more clever, but for now, I’m going to allow it to get the advantage of the set operation. Maybe there is some clever double-dispatch operation to be done. Of course if we kept Room.cells we could just use it but this is the only production-side use for it. Not perfect, but about 99%.

We could have stopped with the fix. But we like to improve the code while we’re in there, and so that’s what we did.

Maybe next time we can get back to the problem I planed to start thinking about yesterday morning: allocation of things in the dungeon. In particular, we’re dealing with a randomly laid-out dungeon. Suppose we want to place a door between two rooms, and to open the door, Dot has to have the Red Key. We want to put the Red Key somewhere in the dungeon on the same side of the door as Dot is. Not necessarily close to Dot nor to the door but one some cell Dot can walk to without going through the locked door.

But wait, don’t answer yet. Suppose there is another door, controlled by the Gold Key. The question we’re really trying to answer is something like “Is there a path from where Dot starts such that she can obtain everything she needs before she needs it, even if there’s only one successful path?”

I think that’s going to lead us down into the Flooder and PathFinder, which are pretty much two heads of the same snake at present. I’m just hoping we don’t have to add an additional head. Ideally, we’d find a way to combine our various schemes that do roughly the same thing.

And that’s not even the real problem, just support for it. I have no idea how to answer the question above. I suspect we’ll settle for something simpler, at least at the beginning, and probably forever.

Although I have no thought that anyone would actually play this game, I’d like to get it far enough along so that we would all agree that it covers enough capability and complexity to make it an actual game, not just a façade of one. That’s not critical to me: all code is grist for my mill, but I’d like to reach a point where we could look back and see that we had enough under control so that if we wanted to keep working, we’d have a game.

It would still be a pretty simple last-century kind of game, but we could see it as a completed thing.

A man has to have a dream, I guess.

See you next time!