Dungeon 276

Today: Recovering from a couple of days on Nyquil. Is my brain operating?? We’ll see.

I’ve been fighting some horrible things with Nyquil and Dayquil and similar nostrums for a couple of days now. I think the drugs have worn off, and I feel much as I imagine you humans feel on a regular day. I’m not feeling super strong, so I’ll undertake some easy opportunities.

This may be unwise, but I’m going to put off checking out whether the responsibility shifting is done between GameRunner, Dungeon, DungeonBuilder, DungeonAnalyzer, and TileFinder. I think things are better, but I’d really like to spend a mornig when I’m on top of my game to maybe ever draw a diagram or something.

If I had to pick a single design error in the Dungeon program, it would be the fact that low level objects tend to know about higher level objects. I think we’ve got Tile pretty close, because if we’re even passed a dungeon, we cast it to TileFinder:

function Tile:init(mapPoint,kind, dungeonOrTileFinder)
    assert(mapPoint:is_a(MapPoint), "Tile requires MapPoint")
    self.mapPoint = mapPoint
    self.kind = kind
    self.tileFinder = dungeonOrTileFinder:asTileFinder()
    self:initDetails()
end

TileFinder itself only has one operational method:

TileFinder = class()

function TileFinder:init(dungeon)
    self.dungeon = dungeon
end

function TileFinder:getTile(x,y)
    return self.dungeon:privateGetTileXY(x,y)
end

function TileFinder:asTileFinder()
    return self
end

I do have one thought in mind for this. You may recall that parts of the code think of tile locations as a vector(x,y) and others as separate x and y variables. There are weird methods like privateGetTileXY set up to accommodate this infelicity.

The “right” thing to do is surely to pick one way and stick with it. And if we were to debate which is right, we’d surely come down either on the vector type, or possibly a wrapping type of our own, Point or something. The experts among us would probably argue that if we were going to change it, we should go with Point, because vector is a system type and we should no more be passing them around than we would raw integers.

And the experts would almost certainly be right.

Now without actually looking, that change seems likely to me to be pretty pervasive, and to involve changing a lot of tests and code. We might ease things by making some of our methods polymorphic with regard to their parameters. For example, we could have two getTile versions here in TileFinder, one accepting the x and y, and one accepting a Point or vector.

Modern languages often allow us to declare multiple versions of the same method, varying only by the method signature, i.e. its parameter list. We do not have that convenience in Lua. However, with a little jumping through our own orifice, we can create the desired behavior. In fact, I’m tempted to do that, with an eye to more people using TileFinder and giving them the freedom to use either form, at least until we converge on a single form.

Reflecting, though, that’s not really ideal. If we did that, it would make it easier for people to use either the vector or x,y form, and we were just arguing that we should converge on the Point object.

Now our TileFinder uses privateGetTileXY, and only that. What does that look like?

function Dungeon:privateGetTileXY(x,y)
    return self.map:atXYZ(x,0,y) or Tile:edge(x,y, self:asTileFinder())
end

Well, among other things, it looks like the very bottom of things before we defer to the Map. Let’s ask ourselves some questions:

Does this belong in Dungeon?

I think we can argue both sides. It certainly seems that there should be a single point of translation between the game’s thinking about tile and the map’s thinking. And all the references to the map are in Dungeon.

We could also argue that we should pull out this particular abstraction, the game’s two-d coordinate thinking, and the map’s three-d thinking that allows us to support Hex maps (in principle if not currently in practice).

This second argument suggests a small object providing the two operations used in Dungeon (fetching a tile and providing a set of tiles to iterate via map:pairs()). That object could be TileFinder. Right now, TileFinder has the Dungeon but it only uses it to refer back to the privateGetTileXY method. An alternative would be to make TileFinder a small object holding the map, and implementing the access methods to the map, including any coordinate transformations needed. Then dungeon, when it needs a tile or a collection, would defer to its TileFinder, and when sent asTileFinder it could just return its existing one.

Well. That’s interesting. I certainly didn’t plan to get to this place in Idea Space.

Let’s at least explore the possibility of doing this. It may not be as simple as it seems, if it even really seems simple.

Can Dungeon Have a Live TileFinder?

Dungeon has a map:

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{}
    Bus:subscribe(self,self.nearestContentsQueries, "requestinfo")
end

The dungeon is created here:

function GameRunner:defineDungeonBuilder(providedLevel)
    if providedLevel then
        self.dungeonLevel = providedLevel
    else
        self.dungeonLevel = self.dungeonLevel + 1
        if self.dungeonLevel > 4 then self.dungeonLevel = 4 end
    end
    self.dungeon = nil
    self.dungeon = Dungeon(self, self.tileCountX, self.tileCountY)
    local numberOfMonsters = 6
    local builderView = self.dungeon:asBuilderView()
    self.builder = DungeonBuilder(self, builderView, self.dungeonLevel, self.playerRoom, numberOfMonsters)
    return self.builder
end

DungeonBuilder does the building:

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

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

function DungeonBuilder:createTiles()
    Maps:cartesian()
    local map = Maps:map(self.view:tileCountX()+1,self.view:tileCountY()+1, Tile,TileEdge, self.view:asTileFinder())
    self.view:setMap(map)
end

That’s where the map comes from. In a new scheme, we’d probably have a TileFinder that had the map. But it is seeming tricky. And it gets deeper:

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.view:tileCountX(),self.view:tileCountY(), 5,14, self.view:asTileFinder())
            if r:isAllowed() then -- <===
                placed = true
                table.insert(rooms,r) -- <===
                r:paint()        -- <===
            elseif timeout <= 0 then
                placed = true
            end
        end
    end
end

function Room:random(spaceX, spaceY, minSize, maxSize, tileFinder)
    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,tileFinder, false)
end

Ah, this is good news. The Room paints tiles, but it gets them via TileFinder. Maybe it’s not too bad. What about createTiles? Recall, that looks like this:

function DungeonBuilder:createTiles()
    Maps:cartesian()
    local map = Maps:map(self.view:tileCountX()+1,self.view:tileCountY()+1, Tile,TileEdge, self.view:asTileFinder())
    self.view:setMap(map)
end

OK, if we wished, we could defer this to our new expanded TileFinder. I don’t think I’d worry much about someone calling the createTiles method.

Suppose We Wanted to Do This

Suppose we thought this was a good direction, moving tile access out of Dungeon and down into a live TileFinder (rather than the current façade version). We could work though all the calls and objects and possibly figure out all the changes that were needed and then plan how to do them.

Honestly, while that would be ideal, in some sense, I find that we often do not have the time or mental space to work out a big design like that, nor will we ever have the time to do it. Instead, while I do like to browse in the code, as we’ve just done, to get a sense of things, I prefer to think of a small change that I can make that moves us in approximately the right direction.

What is the “right” direction? Let’s try this:

Move all map access into a live TileFinder, the owner of the map and its creation and access. Modify dungeon to use only TileFinder to access the tiles.

Over time, try to ensure that objects that only talk to the dungeon for tile access talk to someone else lower in the food chain.

Oh hell, I’m going to try it. I’m a bit concerned that a lot of tests will break, but let’s see if we can move really slowly.

I think we’ll have to modify DungeonBuilder as well, because it creates the map.

First, in Dungeon, find all accesses to self.map and make them a method call.

It starts out nasty:

function Dungeon:availableTilesAtDistanceFromTile(startingTile, targetTile, desiredDistance)
    local atCorrectDistance = function(tile)
        return targetTile:distanceFrom(tile) == desiredDistance
    end
    local tileIsFloor = function(tile)
        return tile:isFloor()
    end
    local neighbors = startingTile:reachableTilesInMap(self.map)
    local correctDistance = ar.filter(neighbors, atCorrectDistance)
    local floorTiles = ar.filter(correctDistance, tileIsFloor)
    return #floorTiles > 0 and floorTiles or { startingTile }
end

Don’t care, if we have a map we can misuse it for a while. But clearly this method should be moved somewhere, in due time.

function Dungeon:availableTilesAtDistanceFromTile(startingTile, targetTile, desiredDistance)
    local atCorrectDistance = function(tile)
        return targetTile:distanceFrom(tile) == desiredDistance
    end
    local tileIsFloor = function(tile)
        return tile:isFloor()
    end
    local neighbors = startingTile:reachableTilesInMap(self:map())
    local correctDistance = ar.filter(neighbors, atCorrectDistance)
    local floorTiles = ar.filter(correctDistance, tileIsFloor)
    return #floorTiles > 0 and floorTiles or { startingTile }
end

I’m just going to change all of the self.map to self:maps().

function Dungeon:createHexTiles(tileCountX, tileCountY)
    Maps:hex()
    self.tileCountX = tileCountX
    self.tileCountY = tileCountY
    self.map = Maps:map(self.tileCountX+1,self.tileCountY+1, Tile,TileRoom, self. runner)
end

I’m going to break this once and for all, until further notice:

function Dungeon:createHexTiles(tileCountX, tileCountY)
    error("createHexTiles method intentionally broken.")
    Maps:hex()
    self.tileCountX = tileCountX
    self.tileCountY = tileCountY
    self.map = Maps:map(self.tileCountX+1,self.tileCountY+1, Tile,TileRoom, self. runner)
end

I change the other accesses to self:map() and implement:

function Dungeon:map()
    return self.map -- to be deferred to TileFinder
end

I expect this to work. Reality doesn’t care about my expectation:

Dungeon:273: attempt to call a table value (method 'map')
stack traceback:
	Dungeon:273: in function <Dungeon:272>
	(...tail calls...)
	Tile:296: in method 'getSurroundingInfo'
	Tile:156: in method 'convertEdgeToWall'
	DungeonBuilder:108: in method 'convertEdgesToWalls'
	DungeonBuilder:211: in method 'defineDungeonLayout'
	DungeonBuilder:62: in method 'buildTestLevel'
	GameRunner:65: in method 'createTestLevel'
	Tests:23: in field '_before'
	CodeaUnit:44: in method 'test'
	Tests:36: in local 'allTests'
	CodeaUnit:16: in method 'describe'
	Tests:13: in function 'testDungeon2'
	[string "testDungeon2()"]:1: in main chunk
	CodeaUnit:139: in method 'execute'
	Main:13: in function 'setup'

Oh, duh. I can’t have a member variable named map any more, if I have that method. Darn. OK, call it getMap, replace.

OK, we’re good. Commit: Make access to Dungeon.map via Dungeon:getMap().

Now what?

What about TileFinder?

I’ve been talking about making TileFinder live, with access to the map. That’s one possibility. Another would be to create another small object, TileAccessor or something, make that work, then provide it to TileFinder users as a second step.

I think I like that idea, it seems more incremental. And in the interim, we can probably pass a TileAccessor to TileFinder and have everything work.

I’m just going to do this rather than TDD it. Think of this as a spike to find out how awful this is.

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{}
    self.tileAccessor = TileAccessor()
    Bus:subscribe(self,self.nearestContentsQueries, "requestinfo")
end

This will suffice to drive out the class.

Dungeon:116: attempt to call a nil value (global 'TileAccessor')

TileAccessor = class()

The system will run now. Could commit if we wanted. Let’s do some actual work though:

function Dungeon:setMap(aMap)
    self.map = aMap
end

That will become this:

function Dungeon:setMap(aMap)
    self.TileAccessor:setMap(aMap)
end

That will break looking for that method in TileAccessor:

Dungeon:301: attempt to index a nil value (field 'TileAccessor')

function TileFinder:setMap(aMap)
    self.map = aMap
end

Note that while I said I wasn’t going to TDD this thing, what I really meant was that I wasn’t going to write new tests for it. Instead, existing tests will guide what we do with it.

Now we should get a problem with the map returned from getMap, I reckon.

Dungeon:301: attempt to index a nil value (field 'TileAccessor')
stack traceback:
	Dungeon:301: in method 'setMap'

Well it would be ideal if I hadn’t capitalized TileAccessor here:

function Dungeon:setMap(aMap)
    self.tileAccessor:setMap(aMap)
end

Now then … Sheesh. Test tells me that I put the method in TileFinder, not Accessor. I meant:

function TileAccessor:setMap(aMap)
    self.map = aMap
end

NOW THEN …

TileFinder:111: attempt to index a nil value (field 'map')
stack traceback:
	TileFinder:111: in method 'tilePairs'

OK, fair enough, the BuilderView refers to dungeon.map not getMap.

function BuilderView:tilePairs()
    return self.dungeon.getMap():pairs()
end

We’re still getting driven by the tests but not quite in the direction I expect. I’m glad I have the tests to guide me.

Dungeon:214: attempt to index a nil value (local 'self')
stack traceback:
	Dungeon:214: in field 'getMap'
	TileFinder:111: in method 'tilePairs'
	DungeonBuilder:77: in method 'clearLevel'
	DungeonBuilder:202: in method 'dcPrepare'
	DungeonBuilder:61: in method 'buildTestLevel'
	GameRunner:65: in method 'createTestLevel'
	Tests:23: in field '_before'
	CodeaUnit:44: in method 'test'
	Tests:36: in local 'allTests'
	CodeaUnit:16: in method 'describe'
	Tests:13: in function 'testDungeon2'
	[string "testDungeon2()"]:1: in main chunk
	CodeaUnit:139: in method 'execute'
	Main:13: in function 'setup'

Now that is downright weird.

function Dungeon:getMap()
    return self.map -- to be deferred to TileFinder
end

If self is nil, we must have referred to the class? Oh, dot vs comma error. Should be:

function BuilderView:tilePairs()
    return self.dungeon:getMap():pairs()
end

TileFinder:111: attempt to index a nil value
stack traceback:
	TileFinder:111: in method 'tilePairs'
	DungeonBuilder:77: in method 'clearLevel'
	DungeonBuilder:202: in method 'dcPrepare'
	DungeonBuilder:61: in method 'buildTestLevel'
	GameRunner:65: in method 'createTestLevel'
	Tests:23: in field '_before'
	CodeaUnit:44: in method 'test'
	Tests:36: in local 'allTests'
	CodeaUnit:16: in method 'describe'
	Tests:13: in function 'testDungeon2'
	[string "testDungeon2()"]:1: in main chunk
	CodeaUnit:139: in method 'execute'
	Main:13: in function 'setup'

OK, this is coming from a test. The failing line is the one above, self.dungeon:getMap():pairs(). I’m not sure whether the dungeon is missing, or the map. Let’s assert.

function BuilderView:tilePairs()
    assert(self.dungeon, "dungeon missing")
    assert(self.dungeon:getMap(), "map missing")
    return self.dungeon:getMap():pairs()
end

I expect “map missing” but want to be sure.

TileFinder:112: map missing

OK. We need to track down through the traceback:

This instrumentation gives the message “no map set”. We must have a setMap that needs refactoring.

function BuilderView:setMap(aMap)
    print("BuilderView setting map ", aMap)
    self.dungeon:setMap(aMap)
    assert(self.dungeon:getMap(), "no map set")
end

Ah. I failed to fix this:

function Dungeon:getMap()
    return self.map -- to be deferred to TileFinder
end

Should be:

function Dungeon:getMap()
    return self.tileAccessor:getMap()
end

I wish I had a save point, because now I need to remove the prints and asserts. Oh well. Now what?

Dungeon:214: attempt to call a nil value (method 'getMap')
stack traceback:
	Dungeon:214: in method 'getMap'
	TileFinder:112: in method 'tilePairs'
	DungeonBuilder:77: in method 'clearLevel'

What have I done now? I’m not progressing, and I’ve not been saving. Can’t really commit, things are broken.

Oh, right, this is what was supposed to drive out a method:

function Dungeon:getMap()
    return self.tileAccessor:getMap()
end

And …

function TileAccessor:getMap()
    return self.map
end

All the tests run but one and the game works. Great, it was supposed to be that simple. Now that test:

2: Dungeon helps monster moves -- Tests:530: attempt to index a nil value (field 'map')

That should be trivial:

        _:test("Dungeon helps monster moves", function()
            local tiles, x1,x2
            local dungeon = runner:getDungeon()
            for key,tile in dungeon.map:pairs() do
                tile:testSetRoom()
            end

That’s what happens when you don’t have a policy of always using accessors, kids. Don’t be like Ron.

        _:test("Dungeon helps monster moves", function()
            local tiles, x1,x2
            local dungeon = runner:getDungeon()
            for key,tile in dungeon:getMap():pairs() do
                tile:testSetRoom()
            end

This is still too invasive, but I’m holding my breath to get to green.

And we’re green. Commit: Dungeon defers all map access to its TileAccessor instance. More methods to move.

I’m about used up. I plead Nyquil hangover. But let’s see if we can push a bit more capability into the TileAccessor. The basics are good. But we have this Dungeon:getMap method. Who’s calling that, and why?

function Dungeon:availableTilesAtDistanceFromTile(startingTile, targetTile, desiredDistance)
    local atCorrectDistance = function(tile)
        return targetTile:distanceFrom(tile) == desiredDistance
    end
    local tileIsFloor = function(tile)
        return tile:isFloor()
    end
    local neighbors = startingTile:reachableTilesInMap(self:getMap())
    local correctDistance = ar.filter(neighbors, atCorrectDistance)
    local floorTiles = ar.filter(correctDistance, tileIsFloor)
    return #floorTiles > 0 and floorTiles or { startingTile }
end

This one we leave alone for now.

function Dungeon:drawMap(tiny)
    for pos,tile in self:getMap():pairs() do
        tile:draw(tiny)
    end
end

This one should defer thusly:

function Dungeon:drawMap(tiny)
    for pos,tile in self:tilePairs() do
        tile:draw(tiny)
    end
end

And

function Dungeon:tilePairs()
    return self.tileAccessor:tilePairs()
end

And

function TileAccessor:tilePairs()
    return self.map:pairs()
end

This is good. The job of producing the tile pairs is now an agreed matter between TileAccessor and the map. Other than that one test, no one knows details. Fix the test:

        _:test("Dungeon helps monster moves", function()
            local tiles, x1,x2
            local dungeon = runner:getDungeon()
            for key,tile in dungeon:tilePairs() do
                tile:testSetRoom()
            end

Test. We’re good. Commit: TileAccessor now produces tilePairs and no one else knows how.

Nearly done, but I became aware of an issue last night. Check this picture of the starting configuration:

Note the position of the NPC and the Lever. If the room is only three tiles wide, or three tiles high, which are both possible, the NPC or Lever can block the door. And it is possible, though unlikely, that it’s the only room exit. That would make for a boring game.

So let’s move both those items a bit.

function DungeonBuilder:placeNPC()
    local r1 = self:getRooms()[1]
    local tile = r1:centerTile()
    local npcTile = tile:getNeighbor(vec2(2,0))
    NPC(npcTile)
end

This places NPC two over and zero up. Let’s make it one up.

function DungeonBuilder:placeNPC()
    local r1 = self:getRooms()[1]
    local tile = r1:centerTile()
    local npcTile = tile:getNeighbor(vec2(2,1))
    NPC(npcTile)
end

Similarly:

function DungeonBuilder:placeLever()
    local r1 = self:getRooms()[1]
    local tile = r1:centerTile()
    local leverTile = tile:getNeighbor(vec2(0,2))
    Lever(leverTile, "Lever 1")
end

We’ll make that:

function DungeonBuilder:placeLever()
    local r1 = self:getRooms()[1]
    local tile = r1:centerTile()
    local leverTile = tile:getNeighbor(vec2(-1,2))
    Lever(leverTile, "Lever 1")
end

Test. Looks good:

OK, commit that and let’s sum up. Commit: Offset NPC and Lever so as not to block doors.

Summary

I have to give myself credit for still being alive. I think it’s possible that I made more silly mistakes than usual, but I always make some (unlike you, I’m sure …) and it could just be statistical variation.

Either way, my tests led me out of the woods in every case. As often happens, the obvious mistakes were harder to find because I always assume that I haven’t misplaced a dot for a colon or named a method the same as a member variable or forgotten to update an accessor. But the tests don’t lie.

And I think we’ve got a good thing going. We may not be entirely done, but we’ve encapsulated the relationship between Tile and Map notions mostly into TileAccessor. We’re not there yet, and probably won’t be satisfied until we’ve replaced all the TileFinders with TileAccessors, or at least given them TileFinders instead of a Dungeon. I think the first choice is better, deferring twice doesn’t seem to me to give us any benefit.

What I think is interesting about today is that we started off generally looking around and right away stumbled on something worth doing. After a bit of design thinking, it seemed doable, so we did it. And the code is better for it and will become better still, I feel sure.

Fun. Now I’m going to rest. See you next time!

D2.zip