Well, I guess there’s nothing for it but to forge ahead getting a hex map to draw. I rather regret committing to this idea.

I do think that putting in the hex map answers an important question about the incremental, iterative, evolution of design approach that I espouse: what if there is a requirement out of the blue? I hold that a decent design for the current product is a good place to be, even for some pretty random requirements.

That said, our current design could be better. Hopefully, by the time we can actually draw and walk on a hex map, it will be better, but I can imagine things that would make it easier than it is now. For example:

What if we had envisioned a Tile, not as a set of squares all adjacent, but instead as a bunch of holders of a graphical chunks, a place where contents and creatures could “be”, and connected in arbitrary ways to other tiles.

If that were the case, then our hexes would “just” be another kind of graphical chunk, and hex directions just another kind of connection. That would, we imagine, have made it much easier to plug hexes in.

We should emphasize imagine, because that never happened, and given the assumptions of the game, it likely never would have. We’re doing a riff on roguelike games, and they are pretty relentlessly square-tile games. Had we been riffing on Colossal Cave, we might have catered for a more general arrangement. Be that as it may, we’re here now, and we’re not having a great deal of trouble, but we do seem to have a lot of loose ends to deal with. We’ll see how that goes. So far, things have been easier than they seemed. Maybe that will continue.

Let’s get to it.

A Round Tuit

I think we should get rid of the second structure for the tiles, doing everything through the new classes, Map and MapPoint. That should tie off a lot of weird things that could happen–and some of them did, in yesterday’s experiment.

Ah. Thinking of yesterday reminds me of another issue. To set up our initial hex map experiment yesterday, we ruthlessly cut most everything we could out of the learning level creation, and then caused it to draw a hex map. In the course of trying to get that to run, we bumped into code that was expecting monsters, code that was trying to run the princess, and other surprises.

Remember a while back, I got briefly interested in a “Making App” that the dev team (me) would use to create the game levels and such. If we had that today, there would probably maybe possibly be a way to configure the monsters and player and such, so that I could readily configure an empty hex map. And with a great deal of luck, that Making App might even deal with there being no player and no monsters and no treasures, etc. If that were the case, it might be easier to rig up a test map than it is.

Does that say we “should” have done that? It’s far from clear that we should, because had we done that, we’d likely have fewer general capabilities in the game, as effort would have moved over that way. Our actual configuration, the layout of rooms and halls, has been pretty stable, so I don’t believe we’d be further ahead to day if we had that app.

Should we create one now? Again, I think not. We may want to do that later, but to get a simple hex map in place, a single custom-made map creation function is probably sufficient. It might be fun–and I emphasize might–be fun to put in at least some configuration values that the Making App could tweak.

Anyway we want to remove that self.tiles in Dungeon, and push all the things done with that structure through the new Map code. Let’s see who does what with whom.

self.tiles

Here, I think, are all the references to the tiles structure:

        _:test("Dungeon helps monster moves", function()
            local tiles, x1,x2
            local dungeon = Runner:getDungeon()
            local tiles = dungeon.tiles
            for row = 1,#tiles do
                for col = 1,#tiles[row] do
                    tiles[row][col]:testSetRoom()
                end
            end

        _:test("Dungeon has tiles", function()
            _:expect(#dungeon.tiles).is(26)
            _:expect(dungeon.tileCountX).is(25)
            _:expect(dungeon.tileCountY).is(15)
        end)

function Dungeon:init(runner)
    self.runner = runner
    -- declare unset member variables. needs improvement.
    self.tiles = nil
    self.tileCountX = nil
    self.tileCountY = nil
end

function Dungeon:clearLevel()
    for i,row in ipairs(self.tiles) do
        for j,tile in ipairs(row) do
            tile:convertToEdge()
        end
    end
end

function Dungeon:convertEdgesToWalls()
    for i,row in ipairs(self.tiles) do
        for j,tile in ipairs(row) do
            tile:convertEdgeToWall()
        end
    end
end

function Dungeon:createTiles(tileCountX, tileCountY)
    self.tileCountX = tileCountX
    self.tileCountY = tileCountY
    self.map = Map:cartesianMap(self.tileCountX+1,self.tileCountY+1, Tile,TileEdge, self. runner)
    self.tiles = {}
    for x = 1,tileCountX+1 do
        self.tiles[x] = {}
        for y = 1,tileCountY+1 do
            local tile = self.map:atXYZ(x,0,y)
            self:defineTile(tile)
        end
    end
end

function Dungeon:defineTile(aTile)
    assert(not TileLock, "attempt to set tile while locked")
    local pos = aTile:pos()
    self.tiles[pos.x][pos.y] = aTile
    local pt = MapPoint:cartesian(pos.x,pos.y)
    self.map:atPointPut(pt,aTile)
end


function Dungeon:resetVisibility()
    for i,row in ipairs(self.tiles) do
        for j,tile in ipairs(row) do
            tile:clearVisible()
        end
    end
end

        _:test("Initial Dungeon", function()
            local dungeonClass = FakeMapDungeon
            local tiles = FakeMapTile:create(10,5)
            local dungeon = dungeonClass(tiles)
            _:expect(#dungeon.tiles).is(10)
        end)

function PathMap:initCells()
    local tiles = self.dungeon.tiles -- Sorry, Demeter!
    self.cells = {}
    for x = 1,#tiles do
        self.cells[x] = {}
        for y = 1, #tiles[1] do
            self.cells[x][y] = MapCell(x,y, self, self.dungeon)
        end
    end
end

FakeMapDungeon = class()

function FakeMapDungeon:init(tiles, runner, player)
    self.tiles = tiles
end

function FakeMapDungeon:getTile(pos)
    return self.tiles[pos.x][pos.y]
end

Aside from the PathMap stuff for the PathFinder, most of this doesn’t look too daunting.

How Should We Proceed?

I see at least two options:

  1. Step through each of these and recode it to operate on the primary Map structure.
  2. Create a new auxiliary structure, store it in self.tiles, and make it respond to a method at(x,y), and paste that in wherever we now have [x][y].
  3. As above but make it smart enough to respond to [x][y].

I did say at least, supposing that I’d have another idea by the time I was done with the second one. I think it would be possible to do the third one, using Lua’s __index and __newindex metamethods, but it would be pretty exotic.

The second option would probably be a bit faster. The first would probably result in better and more clear code. I do think we’ll need a way to cause the iterations to proceed in y within x order, as most of the loops do that and might be sensitive to it. We can try it without doing that.

Let’s go with the first option. It seems that it will lead us to better code. No real sense starting out with hackery if we don’t have to.

Iterating the Map

One thing we clearly need is the ability to iterate over the map. And the map is now stored in random order, hashed by the concatenation of the subscripts. If we need the y within x order, we’ll create it, but we’ll try to do without.

Let’s see. If in Dungeon we wanted to iterate the map in the natural way right now, we would write something like

for key,tile in pairs(self.map.tab) do
    ...
end

In fact we do that at least once. Let’s implement pairs directly on Map, so that we can use the more reasonable

for key,tile in pairs(self.map) do
    ...
end

function Map:pairs()
    return pairs(self.tab)
end

I also used it here:

function Map:draw()
    for k,obj in self:pairs() do
        obj:draw()
    end
end

Tests run, game runs. Commit: new pairs function on Map.

There is at least one loop referring to self.map.tab. Find and fix.

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

That does not work. I am not as smart as I think I am, apparently.

Dungeon:256: attempt to index a function value (local 'tile')
stack traceback:
	Dungeon:256: in method 'drawMap'
	GameRunner:316: in method 'drawMap'
	GameRunner:308: in method 'drawLargeMap'
	GameRunner:286: in method 'draw'
	Main:102: in function 'draw'

I was sure I had done that before and that it would work. Oh. I called it wrong. Duh.

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

Right, that works. The other form tried to iterate directly on the map, delivering, I suppose, methods and member variables.

OK, let’s start ticking through.

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

I should have committed the draw, shouldn’t I? Anyway test this. Looks good. Commit: Dungeon:clearLevel and draw methods use Map:pairs rather than tiles.

Now, before:

function Dungeon:convertEdgesToWalls()
    for i,row in ipairs(self.tiles) do
        for j,tile in ipairs(row) do
            tile:convertEdgeToWall()
        end
    end
end

And after:

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

Looks good. Commit: Dungeon:convertEdgesToWalls use Map:pairs()

These micro commits include running the tests, seeing that the game runs, and then committing the code in WorkingCopy. The commit part takes perhaps 30 seconds, and I want to run the tests to the point that the game starts after every change. So, as usual, frequent commits don’t cost me any notable amount of time, and ensure that the game gets slightly better every few minutes.

If I get interrupted, the best possible code is in the repo. And, the switch to WorkingCopy gives me a moment to refresh and think, should I care to do so.

We are now down to this method:

function Dungeon:createTiles(tileCountX, tileCountY)
    self.tileCountX = tileCountX
    self.tileCountY = tileCountY
    self.map = Map:cartesianMap(self.tileCountX+1,self.tileCountY+1, Tile,TileEdge, self. runner)
    self.tiles = {}
    for x = 1,tileCountX+1 do
        self.tiles[x] = {}
        for y = 1,tileCountY+1 do
            local tile = self.map:atXYZ(x,0,y)
            self:defineTile(tile)
        end
    end
end

We can just remove all that last bit:

function Dungeon:createTiles(tileCountX, tileCountY)
    self.tileCountX = tileCountX
    self.tileCountY = tileCountY
    self.map = Map:cartesianMap(self.tileCountX+1,self.tileCountY+1, Tile,TileEdge, self. runner)
end

Nice. Next:

function Dungeon:defineTile(aTile)
    assert(not TileLock, "attempt to set tile while locked")
    local pos = aTile:pos()
    self.tiles[pos.x][pos.y] = aTile
    local pt = MapPoint:cartesian(pos.x,pos.y)
    self.map:atPointPut(pt,aTile)
end

I think we just don’t do that:

function Dungeon:defineTile(aTile)
    assert(not TileLock, "attempt to set tile while locked")
    local pos = aTile:pos()
    local pt = MapPoint:cartesian(pos.x,pos.y)
    self.map:atPointPut(pt,aTile)
end

Oops. Having done those two, I’ve broken self.tiles entirely. I should have saved those for last. Let’s see what’s left, but we can’t commit this now until we’re done. No, hell, do it right. Revert.

Why? Because now I can work them one by one and commit when each one works. If I have removed the tiles structure, the game is broken until I complete the changes. Incremental is better. I might want a nap or something.

Next:

function Dungeon:privateGetTileXY(x,y)
    if x<=0 or x>self.tileCountX or y<=0 or y>self.tileCountY then
        return Tile:edge(x,y, self.runner)
    end
    return self.tiles[x][y]
end

After:

function Dungeon:privateGetTileXY(x,y)
    if x<=0 or x>self.tileCountX or y<=0 or y>self.tileCountY then
        return Tile:edge(x,y, self.runner)
    end
    return self.map:atXYZ(x,0,y)
end

I do wonder if that’s still in use. Probably in a test? Yes, in lots of tests.

Test. Runs. Commit: privateGetTile uses Map.

Next:

function Dungeon:resetVisibility()
    for i,row in ipairs(self.tiles) do
        for j,tile in ipairs(row) do
            tile:clearVisible()
        end
    end
end

That becomes:

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

This deserves a bit more testing to see whether any odd lighting occurs, but I’m sure it won’t. Looks fine. Commit: resetVisibility uses Map.

Now:

function Dungeon:setVisibility()
    for i,row in ipairs(self.tiles) do
        for j,tile in ipairs(row) do
            tile:setVisible()
        end
    end
end

As usual:

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

Test. OK. Commit: setVisibility uses Map.

We are now down to the code that defines the tiles member variable, except for the code in PathFinder. I think that’s only in the tests. I’m prepared to leave it broken, but it would be better to have a commit with nothing broken. Let’s see what’s up in PathFinder.

Here’s the big violation:

function PathMap:initCells()
    local tiles = self.dungeon.tiles -- Sorry, Demeter!
    self.cells = {}
    for x = 1,#tiles do
        self.cells[x] = {}
        for y = 1, #tiles[1] do
            self.cells[x][y] = MapCell(x,y, self, self.dungeon)
        end
    end
end

I had a search showing self.tiles, and it missed a few references to dungeon.tiles, so I’m now just showing .tiles and it’s showing me a few other things to consider.

Let’s do the easy ones first and then revisit PathFinder.

Ticking Along Still

        _:test("Dungeon helps monster moves", function()
            local tiles, x1,x2
            local dungeon = Runner:getDungeon()
            local tiles = dungeon.tiles
            for row = 1,#tiles do
                for col = 1,#tiles[row] do
                    tiles[row][col]:testSetRoom()
                end
            end
            dungeon:setTestPlayerAtPosition(vec2(30,30))
            local monsterTile = dungeon:getTile(vec2(34,34))
            _:expect(dungeon:manhattanToPlayer(monsterTile)).is(8)
            tiles = dungeon:availableTilesCloserToPlayer(monsterTile)
            x1,x2 = dungeon:getTile(vec2(33,34)), dungeon:getTile(vec2(34,33))
...

I think we can replace that with our standard loop.

        _: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
            dungeon:setTestPlayerAtPosition(vec2(30,30))

So far so good. Commit: test Dungeon helps monster moves” uses Map.

OK, we’re back with PathFinder. Here’s some relevant code from that 350 line tab:

        _:test("Initial Dungeon", function()
            local dungeonClass = FakeMapDungeon
            local tiles = FakeMapTile:create(10,5)
            local dungeon = dungeonClass(tiles)
            _:expect(#dungeon.tiles).is(10)
        end)
        
        _:test("Initial Map", function()
            local dungeonClass = FakeMapDungeon
            local tiles = FakeMapTile:create(50,50)
            local dungeon = dungeonClass(tiles)
            local targ = dungeon:getTile(vec2(10,10))
            local testing = true
            local map = PathMap(dungeon, targ, testing)
            _:expect(map:cell(1,1).previous).is(nil)
            _:expect(map:cell(50,50).previous).is(nil)
        end)

This brings to mind that the PathFinder uses a PathMap instance, which has some pretty square ideas built into it. I need to review more.

function PathMap:init(dungeon, tile, testing)
    self.dungeon = dungeon
    self:initCells()
    self.visited = false
    if not testing then
        self:target(tile)
    end
end

function PathMap:initCells()
    local tiles = self.dungeon.tiles -- Sorry, Demeter!
    self.cells = {}
    for x = 1,#tiles do
        self.cells[x] = {}
        for y = 1, #tiles[1] do
            self.cells[x][y] = MapCell(x,y, self, self.dungeon)
        end
    end
end

In the code above, we see that PathMap is making a 2d array of MapCell, whatever that is, that mirrors the old tiles array. Let’s look at that creation method:

function MapCell:init(x,y,map,dungeon)
    self.x = x
    self.y = y
    self.map = map
    self.dungeon = dungeon
    self.visited = false
    self.accessible = true
end

function MapCell:getLegalCell(x,y)
    return self.map:getLegalCell(x,y)
end

function MapCell:isAccessible()
    return self.accessible and self.dungeon:cellIsAccessible(self:posVec())
end

function MapCell:manhattan(cell)
    return manhattan(self:posVec(),cell:posVec())
end

function MapCell:neighborIndices()
    return { vec2(0,1), vec2(1,0), vec2(0,-1), vec2(-1,0) }
end

function MapCell:neighbors()
    local near = {}
    for i,nxy in ipairs(self:neighborIndices()) do
        local n = self:getLegalCell(self.x+nxy.x, self.y+nxy.y)
        if n then
            table.insert(near,n)
        end
    end
    return near
end

function MapCell:neighborIndices()
    return { vec2(0,1), vec2(1,0), vec2(0,-1), vec2(-1,0) }
end

I’m not reading that, I’m just scanning it, to get the drift. One drift I getr is that there are two copies of neighborIndices. Fortunately they seem to be the same. I’ll delete the one and commit: remove redundant MapCell:neighborIndices.

Right. Since the PathMap stuff seems to be using cell indices, maybe when it checks the dungeon we can plug in the right values.

function PathMap:getLegalCell(x,y)
    if x < 1 or y < 1 or x > #self.cells or y > #self.cells[1] then
        return nil
    else
        local cell = self:cell(x,y)
        if cell:isAccessible() then
            return cell
        else
            return nil
        end
    end
end

function PathMap:cell(x,y)
    return self.cells[x][y]
end

Could we perhaps create, instead of the cells arrays, a copy of the dungeon’s Map, containing our MapCells instead of the original Tiles? Then we could change the cell method above to use atXYZ as we’ve done in other places.

I am fearful that using the atXYZ as we have so far leaves us in danger that methods will not properly index into hex maps, because we’re explicitly writing (x,0,z) which is right for square tiles but not for hexes. Make a note of that, will you?

This is a tricky change. I think it’ll take the rest of my allocated morning to do it. It’s tempting to defer it, but that leaves PathFinder broken and we can’t ship.

We’re on a clean commit. Let’s go forward, and if things go bad, we can revert and reassess the situation.

PathFinder Mods

I think we want the PathMap to be a mirror of the dungeon map. Let’s start there.

function pathMap:initCells()
    for key,tile in self.dungeon.map:pairs() do
        
    end
end

Now, we have to make our new map hashed same as the old map, or else unwind the random looping we get over the hash. But can we extract the x,y and use them in what will mostly appear to be a flat map to the MapCells? Let’s try that.

function PathMap:initCells()
    self.cells = {}
    for key,tile in self.dungeon.map:pairs() do
        self.cells[key] = MapCell(tile:pos().x, tile:pos().y, self, self.dungeon)
    end
end

Let’s see what the MapCell does, we’ll need to fix it, I think.

function MapCell:init(x,y,map,dungeon)
    self.x = x
    self.y = y
    self.map = map
    self.dungeon = dungeon
    self.visited = false
    self.accessible = true
end

function MapCell:getLegalCell(x,y)
    return self.map:getLegalCell(x,y)
end

function MapCell:isAccessible()
    return self.accessible and self.dungeon:cellIsAccessible(self:posVec())
end

I think we can let that ride, but need to check getLegalCell.

function PathMap:getLegalCell(x,y)
    if x < 1 or y < 1 or x > #self.cells or y > #self.cells[1] then
        return nil
    else
        local cell = self:cell(x,y)
        if cell:isAccessible() then
            return cell
        else
            return nil
        end
    end
end

function PathMap:cell(x,y)
    return self.cells[x][y]
end

cells is now a hashmap mirroring a Map table. (We might want to provide a better way to do that.) So we need to convert that x,y to the appropriate kind of key.

Would we prefer a real Map at this point? Possibly, but let’s see if we can short-circuit it.

function PathMap:cell(x,y)
    local coord = MapPoint:cartesian(x,y)
    local key = coord:key()
    return self.cells[key]
end

Let’s run this and see why it breaks. Unfortunately I’ll have to play until I can rez a PathFinder.

Well, the PathFinder tests break, so that’s probably good. Let’s see why.

3: Initial Map -- PathFinder:158: attempt to index a nil value (field 'map')

That seems to be most of the problem.

I think our FakeDungeon is the problem.

Urrgh. Messier than I’d like:

FakeMapDungeon = class()

function FakeMapDungeon:init(tiles, runner, player)
    self.tiles = tiles
end

function FakeMapDungeon:cellIsAccessible(pos)
    return true
end

function FakeMapDungeon:getTile(pos)
    return self.tiles[pos.x][pos.y]
end

FakeMapTile = class()

function FakeMapTile:create(maxX,maxY)
    local tiles = {}
    for x = 1,maxX do
        tiles[x] = {}
        for y = 1, maxY do
            tiles[x][y] = FakeMapTile(x,y)
        end
    end
    return tiles
end

function FakeMapTile:init(x,y)
    self.x = x
    self.y = y
end

function FakeMapTile:pos()
    return vec2(self.x,self.y)
end

This code is seriously dedicated to the proposition that the map is made of tiles that can be indexed by x and y.

I think the thing to do is to revert, and start making the tests run with changes to the Fake objects. That will drive out some changes to the real ones, I think.

I’m not sure that’ll work better, but we’re not losing much, just our hacked copy of the Map. So revert.

Fake Map Stuff

        _:test("Initial Test Tiles", function()
            local tiles = FakeMapTile:create(10,5)
            _:expect(#tiles).is(10)
            _:expect(#tiles[1]).is(5)
        end)
        
        _:test("Initial Dungeon", function()
            local dungeonClass = FakeMapDungeon
            local tiles = FakeMapTile:create(10,5)
            local dungeon = dungeonClass(tiles)
            _:expect(#dungeon.tiles).is(10)
        end)
        
        _:test("Initial Map", function()
            local dungeonClass = FakeMapDungeon
            local tiles = FakeMapTile:create(50,50)
            local dungeon = dungeonClass(tiles)
            local targ = dungeon:getTile(vec2(10,10))
            local testing = true
            local map = PathMap(dungeon, targ, testing)
            _:expect(map:cell(1,1).previous).is(nil)
            _:expect(map:cell(50,50).previous).is(nil)
        end)
        
        _:test("One Frontier Step", function()
            local dungeonClass = FakeMapDungeon
            local tiles = FakeMapTile:create(50,50)
            local dungeon = dungeonClass(tiles)
            local targ = dungeon:getTile(vec2(10,10))
            local testing = true
            local map = PathMap(dungeon, targ, testing)
            map:initCells()
            map:targetXY(10,10)
            _:expect(map:queue()[1]).is(map:cell(10,10))
            map:step()
            _:expect(#map:queue()).is(4)
            local queue = map:queue()
            _:expect(queue).has(map:cell(9,10))
            _:expect(queue).has(map:cell(10,9))
            _:expect(queue).has(map:cell(11,10))
            _:expect(queue).has(map:cell(10,11))
            _:expect(map:cell(9,10).parent).is(map:cell(10,10))
            _:expect(map:cell(10,11).distance).is(1)
        end)

That’s more than enough context to get the drift. When we create our FakeMap, we want it to contain either

  • A hashed table using indices like the Map uses, or
  • An actual Map.

An actual Map would be better. To make that work in the real program we’ll need a map copying function in Map, but that’s better than doing it somewhere else, so despite the hassle, let’s go that way.

function FakeMapTile:create(maxX,maxY)
    local tiles = {}
    for x = 1,maxX do
        tiles[x] = {}
        for y = 1, maxY do
            tiles[x][y] = FakeMapTile(x,y)
        end
    end
    return tiles
end

We want to return a Map of FakeMapTile. What does Map do? It creates instances passing in the MapPoint, not the x and y. So we will have to accommodate that. Let’s just go for it, we can always revert again.

function FakeMapTile:create(maxX,maxY)
    return Map:cartesianMap(maxX, maxY, FakeMapTile)
end

function FakeMapTile:init(mapPoint)
    self.x = mapPoint:x()
    self.y = mapPoint:z()
end

Let’s test now, see what breaks. It’ll be a lot.

1: Initial Test Tiles  -- Actual: 0, Expected: 10
1: Initial Test Tiles -- PathFinder:21: attempt to get length of a nil value (field 'integer index')
2: Initial Dungeon  -- Actual: 0, Expected: 10

These are probably mostly checking for information we are doing without. Our new Map doesn’t remember its size. Ignore those first two.

3: Initial Map -- PathFinder:316: attempt to index a nil value (field '?')

That test is:

        _:test("Initial Map", function()
            local dungeonClass = FakeMapDungeon
            local tiles = FakeMapTile:create(50,50)
            local dungeon = dungeonClass(tiles)
            local targ = dungeon:getTile(vec2(10,10))
            local testing = true
            local map = PathMap(dungeon, targ, testing)
            _:expect(map:cell(1,1).previous).is(nil)
            _:expect(map:cell(50,50).previous).is(nil)
        end)

And 316 is:

function FakeMapDungeon:getTile(pos)
    return self.tiles[pos.x][pos.y]
end

The test has led us to just the place we need to change. Nice.

function FakeMapDungeon:getTile(pos)
    return self.tiles:atXYZ(pos.x, 0, pos.y)
end

I think that’s right.

3: Initial Map -- PathFinder:175: attempt to index a nil value (field '?')

Same test, new location:

function PathMap:cell(x,y)
    return self.cells[x][y]
end

Same change, I hope:

function PathMap:cell(x,y)
    return self.cells:atXYZ(x,0,y)
end

3: Initial Map -- PathFinder:175: attempt to call a nil value (method 'atXYZ')

Not quite. What is cells then, if not a Map? Ah, we have put it back to the old way and must bring it up to speed:

function PathMap:initCells()
    local tiles = self.dungeon.tiles -- Sorry, Demeter!
    self.cells = {}
    for x = 1,#tiles do
        self.cells[x] = {}
        for y = 1, #tiles[1] do
            self.cells[x][y] = MapCell(x,y, self, self.dungeon)
        end
    end
end

I’m reluctant to push the copy down to Map at this point, because I’m not sure what we want it to create. I think in general we’d provide a wrapper class to wrap existing tiles, but our MapCell expects to access the cells directly. So:

function PathMap:initCells()
    self.cells = {}
    for key,tiles in self.dungeon.map:pairs() do
        self.cells[key] = MapCell(x,y, self, self.dungeon)
    end
end

Something else will of course break.

Shouldn’t I be looking ahead, figuring out what to change and changing it, to make the test run? Well, such is not the way. The way is to run the tests and let them lead us to the next change.

Of course you are not sworn to the way, so you can do as you wish.

3: Initial Map -- PathFinder:158: attempt to index a nil value (field 'map')

That is us, “discovering” that FakeMapDungeon doesn’t return map.

function FakeMapDungeon:init(tiles, runner, player)
    self.tiles = tiles
    self.map = tiles
end

Hack for now.

3: Initial Map -- PathFinder:171: attempt to call a nil value (method 'atXYZ')

Method missing in the Fake, I expect … Oh, my bad. I didn’t push down to get a real Map, so I have to compute the key myself:

function PathMap:cell(x,y)
    local pt = MapPoint:cartesian(x,y)
    local key = pt:key()
    return self.cells[key]
end

Haven’t I already made that mistake once today? Speaks for getting a Map up in here, but I’m really not ready.

4: One Frontier Step  -- Actual: 0, Expected: 4
4: One Frontier Step  -- Actual: table: 0x2848fd100, Expected: table: 0x284826540

Well we got further but we also spilled out about a million messages. These are the first two, from this:

        _:test("One Frontier Step", function()
            local dungeonClass = FakeMapDungeon
            local tiles = FakeMapTile:create(50,50)
            local dungeon = dungeonClass(tiles)
            local targ = dungeon:getTile(vec2(10,10))
            local testing = true
            local map = PathMap(dungeon, targ, testing)
            map:initCells()
            map:targetXY(10,10)
            _:expect(map:queue()[1]).is(map:cell(10,10))
            map:step()
            _:expect(#map:queue()).is(4)
            local queue = map:queue()
            _:expect(queue).has(map:cell(9,10))
            _:expect(queue).has(map:cell(10,9))
            _:expect(queue).has(map:cell(11,10))
            _:expect(queue).has(map:cell(10,11))
            _:expect(map:cell(9,10).parent).is(map:cell(10,10))
            _:expect(map:cell(10,11).distance).is(1)
        end)

The good news is that we did get cell(10,10) for the beginning of the queue. Then, however, we expected four items in the queue and got none. That means that step didn’t find anything. Let’s look at it:

function PathMap:step()
    local cell = self:removeQueue()
    local neighbors = cell:neighbors()
    for i,n in ipairs(neighbors) do
        self:addQueue(n, cell)
    end
end

So we didn’t get any neighbors. Why?

function MapCell:neighbors()
    local near = {}
    for i,nxy in ipairs(self:neighborIndices()) do
        local n = self:getLegalCell(self.x+nxy.x, self.y+nxy.y)
        if n then
            table.insert(near,n)
        end
    end
    return near
end

Seems like getLegalCell was returning nil:

function MapCell:getLegalCell(x,y)
    return self.map:getLegalCell(x,y)
end

And that’ll be:

function PathMap:getLegalCell(x,y)
    if x < 1 or y < 1 or x > #self.cells or y > #self.cells[1] then
        return nil
    else
        local cell = self:cell(x,y)
        if cell:isAccessible() then
            return cell
        else
            return nil
        end
    end
end

We can’t count on those comparisons at the top. We’ll need to fetch the cell and check for nil there, I think.

function PathMap:getLegalCell(x,y)
    local cell = self:cell(x,y)
    if not cell then
        return nil
    else
        if cell:isAccessible() then
            return cell
        else
            return nil
        end
    end
end

I’m getting nervous, this is taking a while. But so far the tests seem to be leading well.

However, I get the same error. OK, I’m going to have to toss in a couple of prints, I think.

function PathMap:getLegalCell(x,y)
    local cell = self:cell(x,y)
    if not cell then
        print("no cell", x, y)
        return nil
    else
        if cell:isAccessible() then
            print("found one", x,y)
            return cell
        else
            print("inaccessible cell", x, y)
            return nil
        end
    end
end

I am not expecting to see “found one”, of course.

no cell	0.0	1.0
no cell	1.0	0.0
no cell	0.0	-1.0
no cell	-1.0	0.0

Yes, well, I don’t think I like those indexes much. Let’s instrument cell

function PathMap:cell(x,y)
    local pt = MapPoint:cartesian(x,y)
    local key = pt:key()
    print("key ", key)
    return self.cells[key]
end

The keys look OK:

key 	0,0,1
key 	1,0,0

And so on. But there are no cells like that. This is disturbing.

I want to dump the cells table. Or is it just created oddly?

Ah:

function PathMap:initCells()
    self.cells = {}
    for key,tiles in self.dungeon.map:pairs() do
        self.cells[key] = MapCell(x,y, self, self.dungeon)
    end
end

We’re giving them all nil x and y. That can’t be helping. Fix that.

function PathMap:initCells()
    self.cells = {}
    for key,tile in self.dungeon.map:pairs() do
        local x = tile.mapPoint:x()
        local y = tile.mapPoint:z() -- note z
        self.cells[key] = MapCell(x,y, self, self.dungeon)
    end
end

From this I discover that FakeTile thingie doesn’t know mapPoint …

function FakeMapTile:init(mapPoint)
    self.mapPoint = mapPoint
    self.x = mapPoint:x()
    self.y = mapPoint:z()
end

This is feeling ragged. I think we’re still progressing, but I freely grant that while the tests may know what’s up, I do not. I’m just following my nose.

Running the tests now, I get a massive dump from all those prints. Remove them.

We’re down to one error:

8: target accepts tile -- PathFinder:328: attempt to index a number value (local 'mapPoint')

Pathfinder 328 is:

function FakeMapTile:init(mapPoint)
    self.mapPoint = mapPoint
    self.x = mapPoint:x()
    self.y = mapPoint:z()
end

Best check the test, it’s surely doing something weird:

        _:test("target accepts tile", function()
            local dungeonClass = FakeMapDungeon
            local tiles = FakeMapTile:create(50,50)
            local dungeon = dungeonClass(tiles)
            local targ = dungeon:getTile(vec2(10,10))
            local testing = true
            local map = PathMap(dungeon, targ, testing)
            local t = FakeMapTile(10,10)
            map:target(t)
            map:run()
            t = FakeMapTile(11,10)
            local g = map:nextAfter(t)
            local pos = g:pos()
            _:expect(pos).is(vec2(10,10))
        end)

Ah. Can’t create FakeMapTiles that way any more.

        _:test("target accepts tile", function()
            local dungeonClass = FakeMapDungeon
            local tiles = FakeMapTile:create(50,50)
            local dungeon = dungeonClass(tiles)
            local targ = dungeon:getTile(vec2(10,10))
            local testing = true
            local map = PathMap(dungeon, targ, testing)
            local pt = MapPoint:cartesian(10,10)
            local t = FakeMapTile(pt)
            map:target(t)
            map:run()
            t = FakeMapTile(11,10)
            local g = map:nextAfter(t)
            local pos = g:pos()
            _:expect(pos).is(vec2(10,10))
        end)

Duh. Happens multiple times. Fix the 11,10 one.

Tests run except that I ignored one that was dumping mondo errors and the first two, which are checking an array that doesn’t exist. Remove those two, change the other one back.

Tests run, Game runs, PathFinder rezzes and wanders. Commit: no references to dungeon.tiles in system.

Now we should be able to go back and remove the creation of tiles from Dungeon.

function Dungeon:createTiles(tileCountX, tileCountY)
    self.tileCountX = tileCountX
    self.tileCountY = tileCountY
    self.map = Map:cartesianMap(self.tileCountX+1,self.tileCountY+1, Tile,TileEdge, self. runner)
end

function Dungeon:defineTile(aTile)
    assert(not TileLock, "attempt to set tile while locked")
    local pos = aTile:pos()
    local pt = MapPoint:cartesian(pos.x,pos.y)
    self.map:atPointPut(pt,aTile)
end

Test. I find one defect, but it may be an old one. If you die, you find yourself back in room 1, but it’s all dark. As soon as you move, it lights up fine. I’m going to commit anyway, with a note. That bug could be old, but we did change illumination.

Commit: old tiles structure completely removed from system. Teleport to room 1 after going down leaves you in darkness until you move.

It’s about noon, and I’ve been at this since shortly after 0800, so it’s past time to break. Let’s sum up.

Sum(up)

We, that is to say I decided to completely remove the dependency on the old tiles structure before pushing forward with building a hex map. That went very smoothly, guided by tests just as we like.

It got scary there with the PathFinder, because I had made enough changes that I couldn’t remember them all, and I had never really formed a “big picture” of how that object’s many fake objects worked. Naturally, they all had to be adjusted to follow Map style instead of tile style. That took a long time, but the tests basically led me from place to place until at the end, everything worked.

One area in which the tests didn’t help, no fault of mine, is that when you get a message that says a method wasn’t understood or something came back nil, you can’t be sure just what you were looking at, so I had to remember that if the tests were failing, the problems were in the Fake object.

I’m a bit uncomfortable with the atXYZ method, because our user methods are doing the y = z trick atXYZ(x,0,y) and I think there are even some cases where we do it the other way around, pulling the z coordinate out of a MapPoint. That should all be made transparent, or maybe I mean opaque, with everything going on inside the Map and MapPoint classes. We’ll see about that in the future, with any luck.

It’s tempting to make a Map:copy() function, but we only have one use for it, in PathFinder. Nonetheless, it’s not being done in the right place.

There are a few dungeon.map references, which are certainly Demeter violations, tell don’t ask violations.

The code is better than when we started, because of having removed that tiles appendix, but it’s not as tidy as we might wish.

Overall, we’re in good shape, and tomorrow I expect we’ll take another run at having a simple hex map in the game.

Which reminds me. I don’t think I like that parallelogram hex map that we got last time, so I think I want to create the map with rings as we did in the spike. I think it should have no effect on anything. At least not when we’re done.

See you next time!