A common but rookie mistake needs fixing. Using the same RoomMaker to make multiple rooms engenders chaos. Today, as often happens, I don’t do what I thought I might.

After I wrote yesterday’s summary, it occurred to me that there might be an issue, and I changed main to use a single RoomMaker to make all its rooms:

main.py
    maker = RoomMaker(space)
    for _ in range(number_of_rooms):
        size = random.randint(90, 90)
        origin = space.random_available_cell()
        room = maker.cave(size, origin)
        dungeon.add_room(room)

The results were … interesting.

map with huge rooms and tiny ones

We have huge blob rooms and a few that are just fragments out in space. I particularly enjoy that long path to a single cell. Perhaps it’s a bathroom. The issue, of course, is that the RoomMaker has instance variables that it uses to build up the room, and those are not cleared at the beginning of the call to cave. It suffices to clear them, perhaps like this:

class RoomMaker:
    def __init__(self, space: CellSpace):
        self.space = space
        self.cells:list[Cell] = []
        self.growth_candidates:list[Cell] = []

    def cave(self, number_of_cells: int, origin: Cell, name =''):
        self.cells = list()
        self.growth_candidates = list()
        self._build_cave(number_of_cells, origin)
        print(f'RoomMaker has {len(self.cells)} cells')
        return Room(self.cells, origin, name)

However. A developer—Ron, for example—could forget to clear those members in one of the future alternative ways of building a room, and that would be bad. We need a better idea. I have a few:

  1. Disallow reuse of a RoomMaker. This seems undesirable from a user viewpoint, and might get in the way of future ideas, perhaps where the RoomMaker keeps some kind of cross-room information and bases decisions on that information.

  2. Don’t use member variables for the accumulators, instead passing them down to the methods that use them. That seems credible to me, but the convenience of members would be missed. In favor of this idea is that these members change rapidly during the room making and then change seldom at the beginning or end of a making. They are temporally weird. This suggests an issue with the design.

  3. Have RoomMaker create separate classes to build the various room types: CaveRoomMaker, RoundRoomMaker, and so on. It would create them, run them, toss them away. I hadn’t thought of that when I started this list, but the solution to the timing issue above is often to create additional classes, one for each time cycle.

  4. Create a method that is used to create the Room, passing the cells, and then clear the RoomMaker’s lists. Everyone uses that method after creating their room cells. This seems likely to work: developers—Ron, for example—are less likely to forget to use that method than they are to forget to clear, although I think Ron could do it.

Of these, I think that #3 is probably the “right” answer. Create an instance of some “inner” class for each room build, use it, and throw it away. And #4, I believe, is the right thing to do right now … unless we want to try the inner class idea right now while the situation is as small as it will ever be.

Yes. We’ll do the inner class idea: it should be almost trivial to do it that way. Let’s write a failing test that checks for clearing and that should work when we do it right. I’ll remove my code that clears the lists, my quick fix.

    def test_room_maker_does_not_reuse_cells(self):
        random.seed(304)
        space = CellSpace(64, 56)
        maker = RoomMaker(space)
        room_10_start = space.at(10,10)
        room_10 = maker.cave( 10, room_10_start)
        assert len(room_10.cells) == 10
        room_50_start = space.at(50,50)
        room_50 = maker.cave( 10, room_50_start)
        assert len(room_50.cells) == 10
        assert len(room_10.cells) == 10

With my hack removed, this fails as expected:, on room_50

Expected :10
Actual   :20

All the cells wind up in Room_50. I put the second check on room_10 in, just to be sure they don’t get stolen somehow, once this works.

OK, the plan is to create an inner class inside RoomMaker. We could make the classes that it calls public but I think we’ll keep RoomMaker as the single class we have to think about when creating dungeons. Besides, I don’t think I’ve ever created an inner class in Python before.

class RoomMaker:
    def __init__(self, space: CellSpace):
        self.space = space

    def cave(self, number_of_cells: int, origin: Cell, name =''):
        cave_maker = CaveRoomMaker(self.space)
        room = cave_maker.cave(number_of_cells, origin, name)
        return room

Test went green. Could commit … not quite yet …

So far I just renamed the old RoomMaker to CaveRoomMaker and used it above. We can inline some of that. Let’s see if we like it:

    def cave(self, number_of_cells: int, origin: Cell, name =''):
        cave_maker = CaveRoomMaker(self.space)
        return cave_maker.cave(number_of_cells, origin, name)

Definitely OK. How about the other inline?

    def cave(self, number_of_cells: int, origin: Cell, name =''):
        return CaveRoomMaker(self.space)
            .cave(number_of_cells, origin, name)

I think we’ll allow it. It fits on one line in PyCharm.

Right now the class isn’t “inner”, I left it at the same level as it was. In addition, I was building it in the Room file. Let’s give it its own, moving both classes. Done. Let’s commit, we’re doing good work here: RoomMaker uses CaveRoomMaker, one-time use.

I think to make the inner class I just have to indent. But I am losing interest in doing that. Not sure why. Maybe I feel it’s premature?

I did notice that there is a _build_diamond method in CaveRoomMaker. Let’s give it its own class, see how we like this new scheme.

class RoomMaker:
    def __init__(self, space: CellSpace):
        self.space = space

    def diamond(self, number_of_cells: int, origin: Cell, name =''):
        return DiamondRoomMaker(self.space).diamond(number_of_cells, origin, name)

class DiamondRoomMaker:
    def __init__(self, space: CellSpace):
        self.space = space

    def diamond(self, number_of_cells: int, origin: Cell, name='diamond'):
        cells = self._build_diamond(number_of_cells, origin)
        return Room(cells, origin, name)

    def _build_diamond(self, number_of_cells, origin):
        cells:list[Cell] = []
        count = 0
        for cell in origin.generate(lambda c: c.is_available):
            cells.append(cell)
            cell.room = self
            count += 1
            if count == number_of_cells:
                break
        return cells

I wrote a trivial test for that, just checking length. Commit: added DiamondRoomMaker.

Then I modified main to draw 14 rooms, half of them cave, half diamond, with the diamond ones either of size 61 or 25, which is four ply or six ply. The result looks good and is interesting:

map with some diamond shaped and some irregular rooms

A careful look at some of the would-be diamond rooms shows an interesting behavior, unsurprising once you think of it: if a would-be diamond is obstructed by the edge or by another room, it will continue to add cells around itself, skipping obstructed ones. The result will be somewhat diamond-like, but a bit misshapen. I kind of like it.

Let’s sum up.

Summary

Yesterday’s mistake led to a reasonable-seeming approach to room building: a main class RoomBuilder that understands the various room type names, and that dispatches each type off to a small class specialized to build that type of room. And we now have two available types, the original cave type, and the diamond type that was supposed to be round but isn’t.

While I like this solution, there are things not to like about it. I have a feeling that the makers are going to get repetitive. Nothing to do about that until it happens, but I have an uneasy feeling that it may happen. And it may turn out that not all the makers will take the same arguments, but if they do, there is repetition there that might be an indication of something needing improvement.

Finally, I am not entirely comfortable with creating the little makers with space and then calling them with the other parameters. Perhaps we should create them with all their parms and then just call make().

However these concerns may eventuate, I think things are in good shape for now, and we can move on to making more interesting room shapes, and better paths. I still want to have paths only between “suites”, rooms that are adjacent, and maybe a few extra paths just for interest sake.

And maybe that one incredibly long path to a single cell. I’m sure the players would love that.

See you next time!