Dungeon 267

Pecking away to reshape things more to our liking. Small steps in roughly the right direction, that’s the ticket.

Today, a moderately low-energy feeling so far. I’ll try to compensate by taking tiny steps, which works best for me in any case.

The plan for the day is to continue on my quest to move dungeon building over to DungeonBuilder, and away from GameRunner, and, insofar as practical, from the Dungeon class as well. I’d like those two classes to focus on handling the game during play, and DungeonBuilder to deal with creating the various levels.

I do not have a “grand plan” for this. It surely would be possible to draw some diagrams, list out some methods, then work out another diagram and method layout. Years back, that’s what I’d surely do, and if you’re working with a team, a bit of that sort of thing could help get everyone on the same page. But there are issues.

For me, the biggest issue with a grand plan is that things never work out that way. So long as we realize that no plan survives contact with the code¹, the plan can help us be aware of the general pattern of the changes we want to make. But, too often, the grand plan becomes cast in stone, leading to code that isn’t really better than what we had, to frustration, and to work in large chunks. Often, once we see the grand plan, we even give up: “That’s too much, we could never do it”.

What I practice in these articles is almost all opportunistic. Oh, sure, we’re thinking and planning all the time. But, as you’ll have noticed if you read at all carefully, often I’ll type a paragraph saying what I’m going to do, and the next thing, I’ve done something else. I’ve come to believe that to be a better approach. We’re in the code somewhere, we see something that isn’t quite right, we make it better. Over time, everywhere we work has become a bit better.

Be that as it may, it’s what I do. If you want to hold me up as a bad example, that’s fine with me. If what you do works for you, I’m all for it. On the other hand, if you have a lot oc desired code improvements waiting for sometime when you have a free month … maybe smaller steps could help you too.

Let’s get started. Before looking, I think I’m interested in the part of things that creates rooms and hallways, which is in Dungeon class, and Room class, I believe. Here goes:

Code Browsing

As part of warming up, I always browse around in the code, sometimes with purpose, like today, but sometimes just following my nose. Often I can find a truly tiny change to make, to get me started. Today, I’m going to start in Dungeon …

The first thing I notice is a pending commit. Yesterday’s final changes didn’t get saved. I think I was distracted by something around the house. Commit: convert Tile to have dungeon rather than runner.

Second thing, I’m not starting in Dungeon, I’m starting in DungeonBuilder. I misspoke, because I misthought. Anyway … here’s the code that builds a dungeon level for ordinary play:

function DungeonBuilder:buildLevel(roomCount)
    self:dcPrepare()
    self:defineDungeonLayout(roomCount)
    self:placePlayer()
    self:customizeContents()
    self:dcFinalize()
    return self.dungeon
end

Clearly we need to look at the layout code …

function DungeonBuilder:defineDungeonLayout(count)
    self.dungeon:createRandomRooms(count)
    self.dungeon:connectRooms(self:getRooms())
    self.dungeon:convertEdgesToWalls()
end

Aha. There’s yer problem right there. Dungeon is doing its own room creation and connecting, and we want builder to handle it. Let’s have a glance at dungeon:

function Dungeon:createRandomRooms(count)
    local r
    self.rooms = {}
    while count > 0 do
        count = count -1
        local timeout = 100
        local placed = false
        while not placed do
            timeout = timeout - 1
            r = Room:random(self.tileCountX,self.tileCountY, 5,14, self.runner)
            if r:isAllowed(self) then
                placed = true
                table.insert(self.rooms,r)
                r:paint(#self.rooms)
            elseif timeout <= 0 then
                placed = true
            end
        end
    end
    return self.rooms
end

Well. This clearly has little or nothing to do with Dungeon. But we see here that Room:random is receiving a runner. Let’s take a look at Room and see if it would settle for something less.

function Room:init(x,y,w,h, runner, paint, announcement)
    if paint == nil then paint = true end
    self.x1 = x
    self.y1 = y
    self.x2 = x + w - 1
    self.y2 = y + h - 1
    self.runner = runner
    if paint then
        self:paint()
    end
    if announcement then
        self:announce(runner:getDungeon(), announcement)
    end
end

Room stores the runner … then gets the dungeon here, so we’d be fine getting a dungeon so far. Now to look at all the self.runner lines up in this Room.

Ah yes. We find just this:

function Room:getDungeon()
    return self.runner:getDungeon()
end

If this were a month later, I wouldn’t likely remember that yesterday we prepared Room to accept a dungeon instead of a runner. But what I would discover would be clear evidence that Room doesn’t need runner.

Cortical-thalamic Pause …

What I should do is find all the creators of Room and make them pass in a Dungeon. But I have had a probably evil idea that I want to at least think about.

What if we had a new class, say DungeonHolder, that contained a single member variable, a Dungeon, and one method getDungeon()? No, too weird and not helpful …

What if we were to check inside Room’s init, to see whether we were passed a GameRunner or a Dungeon? If the former, grab its Dungeon and carry on. Then we could switch over incrementally, and remove the cleverness at some future time.

That may be too clever to live. But if there are a large number of changes to make, it might be worth it as a trick to let us spread out the work over time.

I’d better look to see how much work there is to do.

There are three or four tests … one call in Dungeon … one call to fromXYWHA … another test for random … the call we’re working on in Dungeon:createRandomRooms … and that’s all.

This time we’ll just convert them all. If I’ve missed any, the tests will surely find them, especially if I put an assert into Room. We’ll start there:

function Room:init(x,y,w,h, dungeon, paint, announcement)
    assert(dungeon:is_a(Dungeon), "Room requires a dungeon instance")
    if paint == nil then paint = true end
    self.x1 = x
    self.y1 = y
    self.x2 = x + w - 1
    self.y2 = y + h - 1
    self.dungeon = dungeon
    if paint then
        self:paint()
    end
    if announcement then
        self:announce(runner:getDungeon(), announcement)
    end
end

function Room:getDungeon()
    return self.dungeon
end

I’ll rename the parameters in the class methods:

function Room:fromXYWHA(xywhaTable, dungeon)
    local x,y,w,h,a = table.unpack(xywhaTable)
    return Room(x,y,w,h, dungeon, true, a)
end

function Room:random(spaceX, spaceY, minSize, maxSize, dungeon)
    local w = math.random(minSize,maxSize)
    local h = math.random(minSize,maxSize)
    local x = math.random(2,spaceX-w - 1) -- leave room for wall
    local y = math.random(2,spaceY-h - 1) -- leave room for wall
    return Room(x,y,w,h,dungeon, false)
end

Now to find and fix all the uses:

        _:test("random rooms", function()
            local x1,y1,x2,y2
            for i = 1,100 do
                local r = Room:random(60,60, 5,16, runner:getDungeon())
                x1,y1,x2,y2 = r:corners()
                _:expect(x1>0 and x1<=60).is(true)
                _:expect(y1>0 and y1<=60).is(true)
                _:expect(x2>0 and x2<=60).is(true)
                _:expect(y2>0 and y2<=60).is(true)
            end
        end)

function Dungeon:createRandomRooms(count)
    local r
    self.rooms = {}
    while count > 0 do
        count = count -1
        local timeout = 100
        local placed = false
        while not placed do
            timeout = timeout - 1
            r = Room:random(self.tileCountX,self.tileCountY, 5,14, self:getDungeon())
            if r:isAllowed(self) then
                placed = true
                table.insert(self.rooms,r)
                r:paint(#self.rooms)
            elseif timeout <= 0 then
                placed = true
            end
        end
    end
    return self.rooms
end

function Dungeon:createRoomsFromXYWHA(roomTable, runner)
    local makeRoom = function(result,roomDef)
        return result + Room:fromXYWHA(roomDef, runner:getDungeon())
    end
    self.rooms = ar.reduce(roomTable, Array{}, makeRoom)
end

That last one troubles me a bit because I’m not sure who’s calling it, and there may be an issue. I make a note. It’ll have to do with the learning level, I think. I don’t want to divert while I’m thinking about this runner-dungeon swap.

OK, I think I’m ready. Only took 5 minutes or so to get to the point where I’m ready to see what explodes. Of course something does:

Dungeon:220: attempt to call a nil value (method 'getDungeon')
stack traceback:
	Dungeon:220: in method 'createRandomRooms'
	DungeonBuilder:154: in method 'defineDungeonLayout'
	DungeonBuilder:52: in method 'buildLevel'
	GameRunner:58: in method 'createLevel'
	Main:64: in function 'setup'

That was a simple mistake, I was in Dungeon and changed the parameter from self.runner to self:getDungeon(). Should have been self. Now all but one test runs:

20: TileArbiter: monster can step on dead monster -- Room:22: Room requires a dungeon instance

That one is a test that I missed when finding all the references. Tests are green, game works. Better test learning level … I’m “glad” I did that:

Room:33: attempt to index a nil value (global 'runner')
stack traceback:
	Room:33: in field 'init'
	... false
    end

I forgot to change the announcement line:

function Room:init(x,y,w,h, dungeon, paint, announcement)
    assert(dungeon:is_a(Dungeon), "Room requires a dungeon instance")
    if paint == nil then paint = true end
    self.x1 = x
    self.y1 = y
    self.x2 = x + w - 1
    self.y2 = y + h - 1
    self.dungeon = dungeon
    if paint then
        self:paint()
    end
    if announcement then
        self:announce(runner:getDungeon(), announcement)
    end
end

Should be:

        self:announce(self.dungeon, announcement)

We’re good. Tests run, learning level works. Commit: Room now receives dungeon not runner, in preparation for calling from DungeonBuilder.

Time to take a break, sip some chai, etc.

RESTrospective

Always good to lean back after a moderately long thread of changes like that, take up the bigger pictures. I highly recommend Chaikhana Spicy Masala chai concentrate for your morning iced chai latte, by the way. Very close to Starbucks`, and I mean that as a recommendation. Anyway …

Back at the Rooms …

We can build the rooms in DungeonBuilder now, almost certainly. We might run into something but probably nothing difficult. There is one glitch, which is that as written, Dungeon is saving the rooms:

function Dungeon:createRandomRooms(count)
    local r
    self.rooms = {}
    while count > 0 do
        count = count -1
        local timeout = 100
        local placed = false
        while not placed do
            timeout = timeout - 1
            r = Room:random(self.tileCountX,self.tileCountY, 5,14, self)
            if r:isAllowed(self) then
                placed = true
                table.insert(self.rooms,r)
                r:paint(#self.rooms)
            elseif timeout <= 0 then
                placed = true
            end
        end
    end
    return self.rooms
end

We’re thinking of changing this:

function DungeonBuilder:defineDungeonLayout(count)
    self.dungeon:createRandomRooms(count)
    self.dungeon:connectRooms(self:getRooms())
    self.dungeon:convertEdgesToWalls()
end

We plan to move the createRandomRooms (and the other calls) to be internal to DungeonBuilder. Does Dungeon really need to know the rooms, and if so, what does it do with them?

A code scan tells me that Dungeon does save them but mostly does not use them. But it has an accessor getRooms that is used in other classes, notably GameRunner:

function GameRunner:getRooms()
    return self.dungeon:getRooms()
end

function GameRunner:movePlayerToRoom1()
    local tile = self:getRooms()[1]:centerTile()
    tile:moveObject(self.player)
end

function GameRunner:placeDarkness()
    local r1 = self:getRooms()[1]
    local tile = r1:centerTile()
    local darkTile = tile:getNeighbor(vec2(-2,0))
    local dark = Darkness(darkTile)
end

function GameRunner:placePlayerInRoom1()
    local r1 = self:getRooms()[1]
    local tile = r1:centerTile()
    self:placePlayer(tile)
end

function GameRunner:randomRoomTile(roomNumberToAvoid)
    local room = nil
    if roomNumberToAvoid < #self:getRooms() then
        room = self:getRooms()[roomNumberToAvoid]
    end
    return self:getDungeon():randomRoomTile(room)
end

That seems to be the extent of it. I suspect that at least some of what’s above should move out of GameRunner, but I don’t think we can encompass that in one go. We’d better arrange for Dungeon to continue to have the rooms collection.

Now, one thing we could do would be to give DungeonBuilder an accessor and a member variable Rooms, and forward from Dungeon to the builder. However … we’d probably really want for the builder to be ephemeral. We’d create one, build the dungeon, get the dungeon back and drop the builder. What are some options?

Jam it back

We could send the rooms to Dungeon after building: self.dungeon:setRooms(rooms) sort of thing.

Hold onto the builder

We could have the GameRunner hold onto the DungeonBuilder and access rooms from there … but Dungeon still wants to know them, so that’s a nogo.

Build directly into Dungeon.rooms

We could have the Dungeon create an empty rooms collection when it’s created, and then fetch it and fill it in DungeonBuilder. That would require the room building code to change a bit, but not much.

The other ideas I had don’t hold water, such as returning a structured object from DungeonBuilder. Its callers want a Dungeon.

I think we’ll go with the direct build idea. Let’s change Dungeon to work that way internally:

function Dungeon:init(runner, tileCountX, tileCountY)
    self.runner = runner
    self.tileCountX = tileCountX or assert(false, "must provide tile count")
    self.tileCountY = tileCountY or assert(false, "must provide tile count")
    self.map = nil -- set in createTiles
    self.rooms = {} -- new
end

Ah, a couple were already loading into self.rooms but this one is tricky:

function Dungeon:createRoomsFromXYWHA(roomTable, runner)
    local makeRoom = function(result,roomDef)
        return result + Room:fromXYWHA(roomDef, runner:getDungeon())
    end
    self.rooms = ar.reduce(roomTable, Array{}, makeRoom)
end

We can’t replace self.rooms, we need to build into it. Will this work?

function Dungeon:init(runner, tileCountX, tileCountY)
    self.runner = runner
    self.tileCountX = tileCountX or assert(false, "must provide tile count")
    self.tileCountY = tileCountY or assert(false, "must provide tile count")
    self.map = nil -- set in createTiles
    self.rooms = Array{}
end

function Dungeon:createRoomsFromXYWHA(roomTable, runner)
    local makeRoom = function(result,roomDef)
        return result + Room:fromXYWHA(roomDef, runner:getDungeon())
    end
    ar.reduce(roomTable, self.rooms, makeRoom)
end

I think it might. Test. Works.

Also an hour and fifteen minute interruption for a video call that had to be on the iPad and ran late. Definitely need to reboot my mind.

Commit: Dungeon:createRoomsFromXYWHA honors pre-initialized self.rooms.

I should mention that some of these rooms creators are returning the rooms collection, but I’m checking to see if anyone is saving them. I think I put those in expecting to go another way. What was I saying about plans?

Now I’ve got to refresh my mind after that long delay.

Saving Rooms Member

Our basic sub-issue is ensuring that the Dungeon is pre-initializing the rooms collection, and that all the methods use that existing one rather than replacing it. My thinking is that that will let me avoid having the DungeonBuilder do anything too weird: it can just fetch the rooms from the Dungeon and update them, and any existing reference should be OK. I think that’s better than jamming the rooms back in a few different places, one of which we’d surely forget.

function Dungeon:createRandomRooms(count)
    local r
    self.rooms = {}
    while count > 0 do
        count = count -1
        local timeout = 100
        local placed = false
        while not placed do
            timeout = timeout - 1
            r = Room:random(self.tileCountX,self.tileCountY, 5,14, self)
            if r:isAllowed(self) then
                placed = true
                table.insert(self.rooms,r)
                r:paint(#self.rooms)
            elseif timeout <= 0 then
                placed = true
            end
        end
    end
    return self.rooms
end

This is the next reference to self.rooms in the tab. I think we can just remove the init, and the return. Let’s try. Works. Commit: Dungeon:createRandomRooms updates self.rooms without replacing it.

OK, the subtask, ensure that Dungeon only ever has one room collection, is complete. Why is that important? Well, when we begin moving methods over, we can fetch the existing collection and fill it in, just as the code already does, and no matter what other odd things may happen, we almost certainly can’t break anything. I prefer that to needing to put the rooms collection back.

Move a Room Creation Method

OK, I think we’re ready to move over the room creation. We’re starting here:

function DungeonBuilder:defineDungeonLayout(count)
    self.dungeon:createRandomRooms(count)
    self.dungeon:connectRooms(self:getRooms())
    self.dungeon:convertEdgesToWalls()
end

We want that to be this:

function DungeonBuilder:defineDungeonLayout(count)
    self:createRandomRooms(count) -- <===
    self.dungeon:connectRooms(self:getRooms())
    self.dungeon:convertEdgesToWalls()
end

If we test now we’ll see it fail because we don’t have that method. It does. We can do this tiny step, which is so small as to seem silly:

function DungeonBuilder:createRandomRooms(count)
    self.dungeon:createRandomRooms(count)
end

However, the tests run again Was it silly? I think not: I actually made two mistakes before I had that right. First, I called it Dungeon:createRandomRooms rather than DungeonBuilder. Second, I left out the count parameter.

Tiny steps make things easy. I can even commit now: DungeonBuilder has a place to put createRandomRooms.

Now we can “just” copy and paste that method over here:

function DungeonBuilder:createRandomRooms(count)
    local rooms = self:getRooms() -- <===
    local r
    while count > 0 do
        count = count -1
        local timeout = 100
        local placed = false
        while not placed do
            timeout = timeout - 1
            r = Room:random(self.tileCountX,self.tileCountY, 5,14, self)
            if r:isAllowed(self) then
                placed = true
                table.insert(rooms,r) -- <===
                r:paint(rooms)        -- <===
            elseif timeout <= 0 then
                placed = true
            end
        end
    end
end

That doesn’t quite work yet, but the rooms bit is covered. We need to deal with room:isAllowed and with the tileCount stuff. And … in looking at the code in more detail, I note that we’re still allowing the Dungeon to do these things:

function DungeonBuilder:dcPrepare()
    TileLock = false
    self.dungeon:createTiles()
    self.dungeon:clearLevel()
    DungeonContents = DungeonContentsCollection()
    MonsterPlayer(self.RUNNER)
    Announcer:clearCache()
end

We should get those calls moved over as well. Choice is whether to push this forward or revert and do those. I think we’re just two small steps away from this working, so will continue. I make a reminder note about dcPrepare.

function DungeonBuilder:createRandomRooms(count)
    local rooms = self:getRooms() -- <===
    local r
    while count > 0 do
        count = count -1
        local timeout = 100
        local placed = false
        while not placed do
            timeout = timeout - 1
            r = Room:random(self.dungeon.tileCountX,self.dungeon.tileCountY, 5,14, self.dungeon) -- <===
            if r:isAllowed(self.dungeon) then -- <===
                placed = true
                table.insert(rooms,r) -- <===
                r:paint(rooms)        -- <===
            elseif timeout <= 0 then
                placed = true
            end
        end
    end
end

Tests and game run. Commit: createRandomRooms done in DungeonBuilder, not Dungeon.

RESTrospective

OK, heads up. What now? We could move the other room creator methods. There are just two of them, I think.

Or, we could work on the items in dcPrepare.

A third possibility would be to attack the other lines in our current create:

function DungeonBuilder:defineDungeonLayout(count)
    self:createRandomRooms(count)
    self.dungeon:connectRooms(self:getRooms())
    self.dungeon:convertEdgesToWalls()
end

I’m torn. I feel more fresh on room creation so am tempted to improve those other methods. But finishing this one method would feel more like a result, and would serve as a sort of template for how to do it.

Continuing with defineDungeonLayout

Let’s see about convertEdgesToWalls, expecting it to be easier.

function Dungeon:convertEdgesToWalls()
    for key,tile in self.map:pairs() do
        tile:convertEdgeToWall()
    end
end

Let’s try this, pulling it out of some orifice:

function DungeonBuilder:defineDungeonLayout(count)
    self:createRandomRooms(count)
    self.dungeon:connectRooms(self:getRooms())
    self:convertEdgesToWalls()
end

function DungeonBuilder:convertEdgesToWalls()
    for key,tile in self.dungeon.map:pairs() do
        tile:convertEdgeToWall()
    end
end

I expect this to work. It does. Commit: DungeonBuilder has convertEdgesToWalls, used in defineDungeonLayout.

Now I should be able to replace all the calls to that method in dungeon to call builder, unless there is a wild one somewhere.

There’s just this:

function DungeonBuilder:defineLearningLayout()
    self:createLearningRooms()
    self:connectLearningRooms()
    self:convertEdgesToWalls()
end

And I delete the one in Dungeon, to be sure. Tests Run. Commit: all convertEdgesToWalls done in DungeonBuilder, removed from Dungeon.

As always, we could stop now. I’m not deadly tired yet despite that it is nearly 1300 hours. I’ll see about the remaining one:

function DungeonBuilder:defineDungeonLayout(count)
    self:createRandomRooms(count)
    self.dungeon:connectRooms(self:getRooms())
    self:convertEdgesToWalls()
end

What does that connect look like?

function Dungeon:connectRooms(rooms)
    for i,r in ipairs(rooms) do
        if i > 1 then
            r:connect(self, rooms[i-1])
        end
    end
end

Should be easy-peasy.

function DungeonBuilder:defineDungeonLayout(count)
    self:createRandomRooms(count)
    self:connectRooms(self:getRooms())
    self:convertEdgesToWalls()
end

function DungeonBuilder:connectRooms(rooms)
    local rooms = self.dungeon:getRooms()
    for i,r in ipairs(rooms) do
        if i > 1 then
            r:connect(self.dungeon, rooms[i-1])
        end
    end
end

Tests run. Commit: defineDungeonLayout no longer calls back to Dungeon. createRandomRooms, connectRooms, and convertEdgesToWalls removed from Dungeon.

This is a good place to wrap.

Wrapping Up

We’ve narrowed the scope of the Room methods, to need only the dungeon, not the Runner. We’ve got three more methods moved out of GameRunner. We have another method, the XYWHA one, that is more ready to be moved over. We have a few cards:

Dung:createRoomsFromXYWHA caller runner learning?

DB:dcPrepare uses dungeon

Those aren’t deeply clear. If I were going to delay for a long time to work on this, I’d make better ones, especially for the first. As things stand, looking there tomorrow will suffice to set us on the right path, I think. And if not … there’ll be a lesson.

I feel like we’ve crossed a watershed. I think we’re on the downhill slope now, with most of the building stuff removed from Dungeon, at least the top and mid-level things.

I’m still wondering whether there should be another component broken out, something smaller than Dungeon that would support the basic Tile access, for building purposes. I think we’ll just wait to see what the code tells us. If we see a seam, we’ll deal with it then.

Things going well here in the Dungeon. Would that it were so elsewhere.

See you next time!