Dungeon 274

Let’s see whether I am more clever than yesterday, and whether my ‘make the change easy’ mods were useful.

Yesterday I was not at my best–not to say that my best is any great shakes–so I bailed after making some changes intended to make the next steps easier. This was driven by Kent Beck’s advice:

To make a difficult change, make the change easy, then make the easy change.

I was thinking about the random room tile code, which now starts like this:

function DungeonBuilder:randomRoomTileAvoidingRoomNumber(roomNumberToAvoid)
    local avoidedRoom = nil
    if roomNumberToAvoid < #self:getRooms() then
        avoidedRoom = self:getRooms()[roomNumberToAvoid]
    end
    return self.dungeon:randomRoomTileAvoidingRoom(avoidedRoom)
end

I found methods with the same name and different signatures and operation, which made it difficult to move that randomRoomTileAvoidingRoom code over into DungeonBuilder, where I believe it belongs.

First, let’s look for references. I have a concern.

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

function Dungeon:farawayOpenRoomTile(room, target)
    local candidate
    local result = self:randomRoomTileAvoidingRoom(room)
    for i = 1,20 do
        candidate = self:randomRoomTileAvoidingRoom(room)
        result = self:furthestFrom(target, result, candidate)
    end
    return result
end

function Dungeon:randomRoomTileAvoidingRoom(roomToAvoid)
    local x1,y1, x2,y2 = 0,0,0,0
    if roomToAvoid then
        x1,y1, x2,y2 = roomToAvoid:corners()
    end
    return self:randomRoomTileAvoidingCoordinates(x1,y1, x2,y2)
end

function Monster:getRandomMonsters(runner, number, level, roomNumberToAvoid)
    local t = self:getMonstersAtLevel(level)
    local result = {}
    for i = 1,number do
        local mtEntry = t[math.random(#t)]
        local tile = runner:randomRoomTileAvoidingRoomNumber(roomNumberToAvoid)
        local monster = Monster(tile, runner, mtEntry)
        table.insert(result, monster)
    end
    return result
end

function Decor:createRequiredItemsInEmptyTiles(items, runner)
    return ar.map(items, function(item)
        local tile = runner:randomRoomTileAvoidingRoomNumber(666)
        return Decor(tile,item)
    end)
end

Wow. That’s rather nasty. Decor and Monster are relying on GameRunner for this method. These methods are happening at build time (I’m pretty sure that we don’t rez monsters, other than PathMonster, during game play.)

I think we can only move that top level method down by passing a builder to the Monster and Decor

But these objects … this can’t be the only reference they have that is relying on runner. (I don’t mind if they know the runner during play, but during build, I’d like to unwind them.)

There is an option. GameRunner just forwards to Dungeon. We can defer from there to builder and just allow the weird connection for now. Let’s try that.

Deferring the Method

We start with this:

function Dungeon:randomRoomTileAvoidingRoom(roomToAvoid)
    local x1,y1, x2,y2 = 0,0,0,0
    if roomToAvoid then
        x1,y1, x2,y2 = roomToAvoid:corners()
    end
    return self:randomRoomTileAvoidingCoordinates(x1,y1, x2,y2)
end

We move the method to DungeonBuilder:

function DungeonBuilder:randomRoomTileAvoidingRoom(roomToAvoid)
    local x1,y1, x2,y2 = 0,0,0,0
    if roomToAvoid then
        x1,y1, x2,y2 = roomToAvoid:corners()
    end
    return self:randomRoomTileAvoidingCoordinates(x1,y1, x2,y2)
end

We think we’d like to defer to it … oh my. The Dungeon doesn’t know its builder. Only GameRunner has a builder.

Ah. I think I see what is going wrong. These methods, even the ones I’ve already moved, are not really part of dungeon building, they are part of something we might call dungeon searching. It’s quite likely the case that they should be moved out of Dungeon, but moving them into DungeonBuilder isn’t the right thing.

Reflection

This is a case of the programmer wanting to do something, and the code wanting something else. It wouldn’t be all that hard for the runner method to spawn a DungeonBuilder and call it. But the fact that all these (few) methods want to talk with the runner means that we’d be forcing a connection that doesn’t exist.

This is hard to explain. I could do this. I know pretty nearly exactly how: spawn a builder and defer to it. But it doesn’t “feel” right. It’s not that it’s hard: it will be pretty easy. So I’m not resisting doing something that’s difficult: I’m resisting doing something that’s easy enough but feels wrong.

What should we do? I can see these options:

1. Leave things as they are, and move to find other things that may need to be in DungeonBuilder, moving it more toward completion.
2. Move the dungeon searching methods back to Dungeon, so that the newly recognized problem will be localized, and then see about completing DungeonBuilder.
3. Create some new object, maybe DungeonAnalyzer, and start putting these methods in it, then move to advance both DungeonBuilder and DungeonAnalyzer.

If we were to schoose the first option, we leave the code less cohesive than it could be, with these analysis methods separated between builder and dungeon. If we were to choose the last option, we would be faced with methods from two sources, dungeon and builder, that need to be moved to the new analyzer. Those methods bounce back and forth between dungeon and builder and the changes would likely get tricky.

I think we should do this: First, move the dungeon searching/analysis methods back to Dungeon, and perhaps mark them or rename them for visibility, and then look at whether moving them again to an analyzer is more profitable, or whether continuing with builder is the thing to do.

I have to admit that I’m very tempted by the DungeonAnalyzer notion. It seems to me that it can be an ephemeral object. It doesn’t need to hang around. We can create one when we need it, call it one time, and throw it away. Or, of course we could cache one, but the cost of building and tossing is low and the style is better (according to my lights, YLMV).

Let’s try it. I’ve just reverted out what I did above, starting the move from Dungeon to Builder. We’re looking at a clean slate. Now we just have to clear our mind and see what needs doing.

One more thing pops into my mind as I think: what kind of access to the Dungeon will this new DungeonAnalyzer need? It clearly will need the rooms. Can it otherwise live with a TileFinder?

I think it all comes down to this method:

function Dungeon:randomRoomTileAvoidingCoordinates(x1,y1,x2,y2)
    local x,y, pos, tile
    repeat
        pos = self:randomPositionAvoiding(x1,y1, x2,y2, self.tileCountX, self.tileCountY)
        tile = self:getTile(pos)
    until tile:isOpenRoom()
    return tile
end

That only relies on getTile (of some format), so quite likely we can limit our access to something very narrow, like what TileFinder provides.

Let’s get started.

DungeonAnalyzer

Are you surprised that we’re doing this when just a few paragraphs above I said that I thought we should do something a bit different? That’s because in the paragraphs following it, I changed my mind, having not stopped thinking after writing that paragraph.

I don’t sanitize my articles, making them look like some god of programming wrote them, never making a mistake, a misstep, or changing their mind. I make them look like what programming really is to me, a never-ending series of looking, trying, deciding, and thinking, all in a tight bundle of activity.

None of my decisions are so great that I am not ready to change them in the light of evidence¹.

So at this moment right here, I propose to create a new object, DungeonAnalyzer, and to move some of these tile finding methods over to it. I am going to rely on existing tests to do this, at least until there’s evidence that I need direct tests. I will, however, start with a test frame built into the class’s tab, because that will make it seem less of a hassle to add a test when I start feeling the need.

-- Dungeon Analyzer
-- 20220402

function testDungeonAnalyzer()
    CodeaUnit.detailed = false
    
    _:describe("DungeonAnalyzer", function()
        
        _:before(function()
        end)
        
        _:after(function()
        end)
        
        _:test("creation", function()
            local DA = DungeonAnalyzer()
        end)
        
    end)
end


DungeonAnalyzer = class()

function DungeonAnalyzer:init()
end

My plan is to start at the bottom this time, and move these methods:

function Dungeon:randomRoomTileAvoidingCoordinates(x1,y1,x2,y2)
    local x,y, pos, tile
    repeat
        pos = self:randomPositionAvoiding(x1,y1, x2,y2, self.tileCountX, self.tileCountY)
        tile = self:getTile(pos)
    until tile:isOpenRoom()
    return tile
end

function Dungeon:randomPositionAvoiding(x1,y1, x2,y2, maxX, maxY)
    local x,y
    x = randomValueAvoiding(x1,x2,maxX)
    y = randomValueAvoiding(y1,y2,maxY)
    return vec2(x,y)
end

It’ll go like this:

function DungeonAnalyzer:randomRoomTileAvoidingCoordinates(x1,y1,x2,y2)
    local x,y, pos, tile
    repeat
        pos = self:randomPositionAvoiding(x1,y1, x2,y2, self.tileCountX, self.tileCountY)
        tile = self:getTile(pos)
    until tile:isOpenRoom()
    return tile
end

function DungeonAnalyzer:randomPositionAvoiding(x1,y1, x2,y2, maxX, maxY)
    local x,y
    x = randomValueAvoiding(x1,x2,maxX)
    y = randomValueAvoiding(y1,y2,maxY)
    return vec2(x,y)
end

Now I’ll remove the second method from Dungeon, and forward the first:

function Dungeon:randomRoomTileAvoidingCoordinates(x1,y1,x2,y2)
    return DungeonAnalyzer(self):randomRoomTileAvoidingCoordinates(x1,y1,x2,y2)
end

I know some things will need changing. Let’s do the ones I can think of:

function DungeonAnalyzer:randomRoomTileAvoidingCoordinates(x1,y1,x2,y2)
    local x,y, pos, tile
    repeat
        pos = self:randomPositionAvoiding(x1,y1, x2,y2, self.dungeon.tileCountX, self.dungeon.tileCountY)
        tile = self.dungeon:getTile(pos)
    until tile:isOpenRoom()
    return tile
end

This is a bit invasive, but it ought to work, or else provide me with some surprising information. Test. Tests run. Commit: DungeonAnalyzer has randomRoomTileAvoidingCoordinates.

Now let’s think about whether we’d like to pass in something a bit less terrifying than the whole dungeon. Right now we just need those two values and a TileFinder. Let’s improve Dungeon:

function Dungeon:randomRoomTileAvoidingCoordinates(x1,y1,x2,y2)
    return self:dungeonAnalyzer():randomRoomTileAvoidingCoordinates(x1,y1,x2,y2)
end

Now we “just” need to make one …

function Dungeon:dungeonAnalyzer()
    return DungeonAnalyzer(self:asTileFinder(), self.tileCountX, self.tileCountY)
end

And to change DA:

function DungeonAnalyzer:init(finder, tileCountX, tileCountY)
    self.finder = finder
    self.tileCountX = tileCountX
    self.tileCountY = tileCountY
end

function DungeonAnalyzer:randomRoomTileAvoidingCoordinates(x1,y1,x2,y2)
    local x,y, pos, tile
    repeat
        pos = self:randomPositionAvoiding(x1,y1, x2,y2, self.tileCountX, self.tileCountY)
        tile = self.finder:getTile(pos.x,pos.y)
    until tile:isOpenRoom()
    return tile
end

I had to change the getTile call to pass the x and y. I really wish I had chosen an approach and stuck with it.

Tests run. Commit: DungeonAnalyzer receives only tile finder and tile counts.

Now let’s see who calls our method and see about moving them over.

function Dungeon:randomRoomTileAvoidingRoom(roomToAvoid)
    local x1,y1, x2,y2 = 0,0,0,0
    if roomToAvoid then
        x1,y1, x2,y2 = roomToAvoid:corners()
    end
    return self:randomRoomTileAvoidingCoordinates(x1,y1, x2,y2)
end

This is the only one. Move it:

function DungeonAnalyzer:randomRoomTileAvoidingRoom(roomToAvoid)
    local x1,y1, x2,y2 = 0,0,0,0
    if roomToAvoid then
        x1,y1, x2,y2 = roomToAvoid:corners()
    end
    return self:randomRoomTileAvoidingCoordinates(x1,y1, x2,y2)
end

Defer to it:

function Dungeon:randomRoomTileAvoidingRoom(roomToAvoid)
    return self:dungeonAnalyzer():randomRoomTileAvoidingCoordinates(x1,y1, x2,y2)
end

Test. Tests run. Remove:

function Dungeon:randomRoomTileAvoidingCoordinates(x1,y1,x2,y2)
    return self:dungeonAnalyzer():randomRoomTileAvoidingCoordinates(x1,y1,x2,y2)
end

Test again. Tests run. Commit: randomRoomTileAvoidingRoom done by DungeonAnalyzer.

Do you feel that we could have done that in one step? Possibly. Two steps made it easier. I am easily distracted by cats wanting to go out, plus I prefer to juggle only one ball at a time.

Now callers of the method we just moved:

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

I think we’ll allow that one, which means our new deferring method may need to remain in Dungeon for a while. What else?

Here’s one:

function Dungeon:farawayOpenRoomTile(room, target)
    local candidate
    local result = self:randomRoomTileAvoidingRoom(room)
    for i = 1,20 do
        candidate = self:randomRoomTileAvoidingRoom(room)
        result = self:furthestFrom(target, result, candidate)
    end
    return result
end

Let’s move it.

function DungeonAnalyzer:farawayOpenRoomTile(room, target)
    local candidate
    local result = self:randomRoomTileAvoidingRoom(room)
    for i = 1,20 do
        candidate = self:randomRoomTileAvoidingRoom(room)
        result = self:furthestFrom(target, result, candidate)
    end
    return result
end

We’ll do the deferral now.

function Dungeon:farawayOpenRoomTile(room, target)
    return self:dungeonAnalyzer():farawayOpenRoomTile(room, target)
end

I noticed the furthestFrom. Had I not, the tests would have found it. Look for senders. There’s just the one, it’s an Explaining Method. Move it:

function DungeonAnalyzer:furthestFrom(targetTile, tile1, tile2)
    local d1 = tile1:distanceFrom(targetTile)
    local d2 =tile2:distanceFrom(targetTile)
    if d2 > d1 then return tile2 else return tile1 end
end

I expect things to work. The game works but a test fails:

4: furthest from -- Dungeon:74: attempt to call a nil value (method 'furthestFrom')

        _:test("furthest from", function()
            local tile, near, far, furthest
            tile = Tile:room(0,0,runner:tileFinder())
            near = Tile:room(10,10,runner:tileFinder())
            far = Tile:room(50,50,runner:tileFinder())
            furthest = dungeon:furthestFrom(tile, near, far)
            _:expect(furthest).is(far)
        end)

We should really move this test into DungeonAnalyzer. Cut from where it is. Paste. This is why I made the test frame, it makes it easier to do a test. I need a runner for this and another change.

After a bit of fiddling, I just replicate the before and after and local variables from the other test:

local dungeon
local _bus
local _TileLock
local _dc
local runner

function testDungeonAnalyzer()
    CodeaUnit.detailed = false
    
    _:describe("DungeonAnalyzer", function()
        
        _:before(function()
            _bus = Bus
            Bus = EventBus()
            _dc = DungeonContents
            DungeonContents = DungeonContentsCollection()
            runner = GameRunner(25,15)
            runner:createTestLevel(0)
            dungeon = runner:getDungeon()
            _TileLock = TileLock
            TileLock = false
        end)
        
        _:after(function()
            Bus = _bus
            TileLock = _TileLock
            DungeonContents = _dc
        end)
        
        _:test("creation", function()
            local DA = DungeonAnalyzer()
        end)
        
        _:test("furthest from", function()
            local finder = runner:tileFinder()
            local analyzer = runner:getDungeon():dungeonAnalyzer()
            local tile, near, far, furthest
            tile = Tile:room(0,0,finder)
            near = Tile:room(10,10,finder)
            far = Tile:room(50,50,finder)
            furthest = analyzer:furthestFrom(tile, near, far)
            _:expect(furthest).is(far)
            Bus = _bus
        end)
    end)
end

Tests all run, as does the game. I am surprised, however, to find a Mimic in the starting room. Is that allowed? Let’s look. I think this ought to be OK, but I’m not dead certain.

No, when we create random monsters we say to avoid Room 1. The room could have been expanded, that does happen. But I’m still not certain we’re OK.

I can commit the test changes anyway: move test to dungeonAnalyzer.

Now let’s see about those room checks.

I instrument this method:

function DungeonAnalyzer:randomRoomTileAvoidingRoom(roomToAvoid)
    local x1,y1, x2,y2 = 0,0,0,0
    if roomToAvoid then
        x1,y1, x2,y2 = roomToAvoid:corners()
    end
    if roomToAvoid then
        print("avoiding room ", roomToAvoid, x1,y1, x2,y2)
    end
    return self:randomRoomTileAvoidingCoordinates(x1,y1, x2,y2)
end

The prints are all OK. I’m confident that we haven’t broken anything.

It’s 1100 hours and I’ve been at this for a while. We’re at a solid point, so let’s sum up.

Summary

The only thing I like better than when a plan comes together is when a plan pushes back and we get an even better plan. We have now created two new central objects in the system, plus two Façade objects:

TlleFinder

This is a Façade covering Dungeon, with just a single method, getTile, which allows us to build objects with very limited access to the dungeon. It gets used in DungeonBuilder and Tile, as well as DungeonAnalyzer. It deserves more use, as time goes on.

BuilderView

This is another Façade covering Dungeon, used to forward messages to Dungeon from DungeonBuilder. It changes over time as things move in and out, but always serves to limit what the Builder can access.

DungeonBuilder

This object is receiving more and more of the methods that are used in building the Dungeon. (Thus the name.) Over time, I hope to get all the dungeon building over in this object, but each move generally reduces the breadth of GameRunner or Dungeon, always a good thing.

DungeonAnalyzer

This new object is intended to contain methods used, typically during building, to find tiles with properties desired by objects being built. No monsters in room 1, Spikes only in hallways, and so on. So far it only has five methods, but it will get more. This helper class is ephemeral: you create one, ask it your question, and drop it.

What I like about today is that the overall design of the system has begun to change again, in addition to the builder idea. Now we have a new idea, analyzer. If this new object holds water, and I believe it will, it’ll be something that we “should” have thought of sooner, but now that we have thought of it, we can build it, improve it, and use it just as if we had been smarter back in the past.

We can’t make ourselves smart in the past, it’s too late. But we can make our lives look much as they would, had we been smarter in the past. It just takes a bit of time and refactoring.

A fun day. Better than yesterday, for sure.

What will happen next time? I can’t wait to find out.