Dungeon 245
I’m thinking of adding a shark to the game that you can only escape by jumping over it. Today, however, a bit more on Maps.
Seriously, I’m getting tired of this particular passel of code. That said, it offers many opportunities to improve code, so as fodder for keeping me off the streets, it’s hard to beat. Winter is coming: I don’t want to be on the streets.
Anyway, I am morally obliged to get a hex map running to the point where there can be a player, monsters, and treasures in it. I’m not promising to make rooms, walls, or even interesting terrain. The point of the hex exercise was to demonstrate that even unreasonable demands can generally be met if we keep our design in good shape.
But/and … let’s reflect on what has happened with the hex map so far. I built an experiment, called D3, and created a bunch of objects that I thought would serve as a “library” of objects that could support both square- and hex-tiled maps. That library has evolved already, even though it’s only 15 days, or less than a week’s work for a normal person, since I started that experiment.
And in that time I’ve written about 15 articles. Surely I get some work credit for that. Surely?1
The D3 code has been moved into the Dung game, and is now being used, in cartesian mode, everywhere in the game. That’s rather good if I do say so myself. We even managed to get the game to display a hex map without crashing. That got us our next tranche2 of funding, which will keep Dark Dungeons of Glory, Inc. going for a good long time.
Yesterday in working with the map code enough to get a sense of how to use it, I decided to have an well-known object, MapStrategy, that could hold a HexStrategy or CartesianStrategy, which would mediate the decisions about what kind of map, coordinates, and directions would be created. That would allow any map-using code to operate without knowing what kind of map was in place.
I don’t quite like how that’s done. The time to improve it is now, before we get even more code in place. If I had a more powerful editor in Codea, I’d be more willing to let it ride, but since I can’t trust “replace”, I can’t even do a power-rename, much less any general refactoring operations. It’s all hand-crafted here chez Dung, and mostly I like it that way. But not when sweeping changes need to be made.
What Have We, What Want We?
MapStrategy is a global variable. It can contain nothing, which would be bad. It can contain an instance of HexStrategy or CartesianStrategy, which is good. Those two classes have the same protocol, and can therefore deal with all the differences between square and hex tiles.
I think the names are awkward here. And there’s an issue that we ought to discuss.
Right now, the Strategy objects support three methods:
map–create a map of given dimensionsdirection–return a list of all the directions you can move on the current mappoint–create a point object given x,y and optional z coordinates
I think we may have some others, direction(n) to return a specific direction, and allDirections, which will return all eight directions to the Tiles that surround a square tile. You can’t move to the corners, but you can interrogate them.
As things stand, if you don’t initialize the MapStrategy global, the game will crash. We can of course fix that by initializing it. But I’m wondering about the structure. It seems OK, but would it be better if the object, let’s call it Maps, had the protocol, but used one of Hex or CartesianStrategy to do the work, forwarding to whichever one it had?
In other words, should Maps be an xStrategy, or should it be an object that has an xStrategy? The latter seems to me to be more like what people expect will be done with a strategy, and it would allow for the raw Maps object to have methods hex() and cartesian() to prime it one way or another.
I’ve asked my sounding board for ideas, but I’m going with the simpler case, and will ensure that the variable always has something in it when the game starts.
I do want to rename things. I want to rename MapStrategy to Maps, And … dammit, I’m changing my mind. I’m going with the forwarding class, because I can see that it’ll improve the naming for the user.
Why don’t I remove the whiplash above? Because I want to show that we think as we work, and we change our mind as we see better ideas. We don’t figure out ages in advance, we let reality tell us what seems best right now.
Reality might lie, and we might misperceive reality, but with any luck we are smarter now than we were a paragraph ago. Let’s do this.
The Current Plan
I’m going to make Maps a class, with functions for map and point and all those in the tests. However, since the class name is already global, I’m going to have it work as a class. You don’t instantiate it, you just say Maps:point(x,y) and there you are with a Point.
Let’s go revise some tests.
Here’s the beginning of the strategy tests:
local _strat
function testMapClasses()
CodeaUnit.detailed = false
_:describe("Test Map Classes", function()
_:before(function()
_strat = MapStrategy
end)
_:after(function()
MapStrategy = _strat
end)
_:test("MapPoint Directions", function()
MapStrategy = CartesianStrategy()
local sq = MapStrategy:point(1,1)
local dirs = sq:directions()
_:expect(#dirs).is(4)
_:expect(dirs[1]).is(MapDirection(1,0,0))
MapStrategy = HexStrategy()
local hx = MapStrategy:point(1,2,-3)
_:expect(hx._isHex,"isHex").is(true)
dirs = hx:directions()
_:expect(#dirs).is(6)
_:expect(dirs[4]).is(MapDirection(-1,1,0))
end)
The before and after will need to change. We’ll have Maps all the time, but we’ll need to save and restore the strategy. Let’s assume that we’ll have methods for getting and setting the strategy, used only in tests.
_:before(function()
_strat = Maps:getStrategy()
end)
_:after(function()
Maps:setStrategy(_strat)
end)
This will break a raft of tests. Might as well let it drive us.
TestMapClasses:12: attempt to index a nil value (global 'Maps')
stack traceback:
TestMapClasses:12: in field '_before'
codeaUnit:44: in method 'test'
TestMapClasses:19: in local 'allTests'
codeaUnit:16: in method 'describe'
TestMapClasses:9: in function 'testMapClasses'
[string "testMapClasses()"]:1: in main chunk
codeaUnit:139: in field 'execute'
Tests:622: in function 'runCodeaUnitTests'
Main:8: in function 'setup'
CodeaUnit can’t cope if your before and after hurl. No huhu, we’ll fix it readily:
local _ms
Maps = class()
function Maps:init()
error("cannot instantiate Maps")
end
function Maps:getStrategy()
return _ms
end
function Maps:setStrategy(strategy)
_ms = strategy
end
Note that because I don’t allow Maps to be instantiated, it needs local variables for its storage. So far it has _ms, which I’ve given a short name because it’ll get used a lot. We’ll see if that turns out to be confusing.
Let’s test again just to see if anything breaks. Nothing does, because no one is using Maps yet. Now to change the tests.
_:test("MapPoint Directions", function()
MapStrategy = CartesianStrategy()
local sq = MapStrategy:point(1,1)
local dirs = sq:directions()
_:expect(#dirs).is(4)
_:expect(dirs[1]).is(MapDirection(1,0,0))
MapStrategy = HexStrategy()
local hx = MapStrategy:point(1,2,-3)
_:expect(hx._isHex,"isHex").is(true)
dirs = hx:directions()
_:expect(#dirs).is(6)
_:expect(dirs[4]).is(MapDirection(-1,1,0))
end)
This is the first test we have, and I’ll change it, but I want another one that is prior to this one. Let’s make it be that Maps defaults to cartesian. The test:
_:test("Maps defaults to cartesian", function()
local dirs = Maps:directions()
_:expect(#dirs,"did not default to cartesian").is(4)
end)
Run, see it break.
1: Maps defaults to cartesian -- TestMapClasses:20: attempt to call a nil value (method 'directions')
Naively, we know that Maps needs to have directions, so let’s implement it naively:
function Maps:directions()
return _ms:directions()
end
Run test. Error as expected:
1: Maps defaults to cartesian -- MapStrategy:76: attempt to index a nil value (upvalue '_ms')
We need to init _ms. We want cartesian default. So:
local _ms = CartesianStrategy()
This should make that first test run, and it does. We have a design decision to make here. The code above creates a CartesianStrategy instance. We could just as well use the class, since, like Maps, it has no instance-related behavior, no member variables, etc. Probably safer to make the instance, we’ll let it be.
Now back to the formerly first test, which I’ll change thus:
_:test("MapPoint Directions", function()
Maps:cartesian()
local sq = Maps:point(1,1)
local dirs = sq:directions()
_:expect(#dirs).is(4)
_:expect(dirs[1]).is(MapDirection(1,0,0))
Maps:hex()
local hx = Maps:point(1,2,-3)
_:expect(hx._isHex,"isHex").is(true)
dirs = hx:directions()
_:expect(#dirs).is(6)
_:expect(dirs[4]).is(MapDirection(-1,1,0))
end)
I expect this to work. That’s because I forgot that I didn’t implement the strategy-setting methods yet.
function Map:cartesian()
_ms = CartesianStrategy()
end
function Map:hex()
_ms = HexStrategy()
end
Now maybe it’ll work. No, typo. I have Map: above, needed Maps:. What even is Map? Oh, right, it’s the underlying class for accessing the map. Maybe I should worry about the name similarity here. Also, I’m seriously considering making all these names private to the map family, if I can figure a way to do it. For now, we let it ride.
Let’s convert some more tests. I do a global replace in Sublime, surely breaking some lines. I’ll just tick thru the tests:
2: MapPoint Directions -- TestMapClasses:26: attempt to call a nil value (method 'point')
Yep, we haven’t done that yet …
function Maps:point(...)
return _ms:point(...)
end
3: Map Directions -- TestMapClasses:41: attempt to call a nil value (method 'map')
Similarly:
function Maps:map(...)
return _ms:map(...)
end
3: Map Directions -- Map:62: attempt to index a nil value (global 'MapStrategy')
Hm, that’s interesting:
function Map:directions()
return MapStrategy:directions()
end
That should of course now say Maps. The tests all run, but now it’s time to look for all uses of MapStrategy and make sure they are replaced.
I just have this one, now edited:
_:test("CartesianStrategy", function()
Maps:cartesian()
local map = Maps:map(2,3,FakeClass)
local ct = 0
for k,t in map:pairs() do
ct = ct + 1
end
_:expect(ct,"number of cells").is(6)
local dirs = map:directions()
_:expect(#dirs).is(4)
end)
That works. But I gotta ask ya, how the heck are we building the real map? We surely aren’t going through Maps. Something else must still be wired up. We don’t want to allow that, we want to funnel everyone through our new official scheme.
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
OK, what’s going on here? We’re working with the low-level Map object, which does have built in that moderately hacky way to build cartesian and hex maps. I want to rename that class, to keep people away from it.
Let’s rename it BaseMap. I’m a bit concerned here, in that it seems possible that we could have used Map where we have Maps, but Maps seems simpler. Rename and see what breaks. We’ll also search for Map( and Map:
function Dungeon:createTiles(tileCountX, tileCountY)
Maps:cartesian()
self.tileCountX = tileCountX
self.tileCountY = tileCountY
self.map = Maps:map(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 = Maps:point(pos.x,pos.y)
self.map:atPointPut(pt,aTile)
end
function CartesianStrategy:map(w,h,klass,...)
return BaseMap:cartesianMap(w,h,klass,...)
end
function HexStrategy:map(w,h,klass,...)
return BaseMap:hexMap(w,h,klass,...)
end
These changes break the PathFinder tests.
1: Initial Map -- PathFinder:310: attempt to index a nil value (global 'Map')
function FakeMapTile:create(maxX,maxY)
return Map:cartesianMap(maxX, maxY, FakeMapTile)
end
We’d do best to rig this test to set up cartesian at the beginning. First let’s make it work:
function FakeMapTile:create(maxX,maxY)
Maps:cartesian()
return Maps:map(maxX, maxY, FakeMapTile)
end
That makes all the tests pass, but I want to test extensively in the game. I’m not entirely sure everyone is going through the new strategy properly. I think we’ll also search for references to the other inner classes, and we should seriously consider renaming them or otherwise hiding them.
Following a PathFinder, I discover that it does not correctly go down the WayDown, instead just floating on top of it. I, the Princess, was able to go down to the Segundo level with no problem. Let’s see what’s up with PathFinder.
A little searching around finds this method:
function Monster:pathComplete()
self.runner:removeMonster(self)
end
Clearly that’s supposed to happen. Who sends that?
function PathMonsterStrategy:execute(dungeon)
local newTile = self.map:nextAfter(self.monster:getTile())
if newTile then
newTile:moveObject(self.monster)
else
self.monster:pathComplete()
end
end
The monster asks its (path) map for the next tile and if it doesn’t get one it dissolves. What does nextAfter do?
function PathMap:nextAfter(tile)
local cell = self:getCellFromTile(tile)
local prev = cell.parent
if not prev then return tile end
return self.dungeon:getTile(vec2(prev.x,prev.y))
end
This can’t work. We return the same tile if we have no prev. (The path is drawn backward from the target, so the prev points to the next cell you want if you’re moving to the target.) We should do this:
function PathMonsterStrategy:execute(dungeon)
local oldTile = self.monster:getTile()
local newTile = self.map:nextAfter(oldTile)
if newTile ~= oldTile then
newTile:moveObject(self.monster)
else
self.monster:pathComplete()
end
end
Arrgh, I should probably have committed the other stuff. Let’s make sure this works, then commit.
This fails oddly:
Dungeon:315: attempt to index a nil value (local 'tile1')
stack traceback:
Dungeon:315: in function <Dungeon:314>
(...tail calls...)
Monster:283: in method 'chooseMove'
Monsters:117: in method 'move'
GameRunner:497: in method 'playerTurnComplete'
Player:311: in method 'turnComplete'
Player:203: in method 'keyPress'
GameRunner:426: in method 'keyPress'
Main:107: in function 'keyboard'
Let’s look at that. A brief look makes me think this isn’t PathFinder, but another monster trying to decide whether to attack. Dungeon 315:
function Dungeon:manhattanBetween(tile1, tile2)
return tile1:manhattanDistance(tile2)
end
That’s from here:
function Monster:chooseMove()
-- return true if in range of player
if self:isDead() then return false end
self:executeMoveStrategy()
return self:manhattanDistanceFromPlayer() <= 10
end
And that calls this:
function Entity:manhattanDistanceFromPlayer()
return self.runner:playerManhattanDistance(self:getTile())
end
Someone has a nil tile. I guess it could be the PathFinder. Test again. It happens again.
Let’s look at the pathfinder strategy again.
function PathMonsterStrategy:execute(dungeon)
local oldTile = self.monster:getTile()
local newTile = self.map:nextAfter(oldTile)
if newTile ~= oldTile then
newTile:moveObject(self.monster)
else
self.monster:pathComplete()
end
end
I’m not sure what happened, perhaps it is that we destroyed the monster and then he had no tile? Let’s put this code back the way it was:
function PathMonsterStrategy:execute(dungeon)
local oldTile = self.monster:getTile()
local newTile = self.map:nextAfter(oldTile)
if newTile then
newTile:moveObject(self.monster)
else
self.monster:pathComplete()
end
end
Then let’s change nextAfter to return the nil. I think this should not fix the bug.
function PathMap:nextAfter(tile)
local cell = self:getCellFromTile(tile)
local prev = cell.parent
if not prev then return nil end
return self.dungeon:getTile(vec2(prev.x,prev.y))
end
Yes, same problem. I don’t think this is new. I suspect this has been with us for a while, and I don’t mind saying that it would anger me as the product owner or other stakeholder in this game, to know that the PathFinder has been broken for a long time.
It is possible that we broke this recently, but I doubt it. Let’s revert out this return, leaving us with a PathFinder who won’t die, and commit the rest.
OK, it sticks there on the WayDown and you can attack it and go down. So the game isn’t as intended but it’s not fatally broken. Commit: Everyone using new Maps strategy.
Now I’d like to know whether I broke this. Now that I’ve committed today’s code, I can back up quite a ways, although not all the way to the beginning of time, because something weird happened when I got the new iPad. I’ll back up to then and check the PathFinder.
Good news, we have the same bug from long ago. Nothing we did lately.
Time to break, think, clear the head bones …
Popping Up A Level
OK, as far as the map stuff is concerned, we’re in reasonable shape. I’m not sure we’re entirely done. We may still have people referring to the MapPoint and similar classes.
Tell you what, let’s make some of these classes local to the tabs they’re in:
local CartesianStrategy = class()
local HexStrategy = class()
Test. Commit: CartesianStrategy and HexStrategy are now local classes.
What else? In searching around, I find that Tile creates a lot of points:
function Tile:hall(x,y, runner)
assert(not TileLock, "Attempt to create room tile when locked")
return Tile(MapPoint:cartesian(x,y),TileHall, runner)
end
These should be calling Maps:point. I fix them and test. All is well. Commit: Tile no longer incorrectly uses MapPoint.
PathFinder uses it as well. Fixed.
I find this:
function Hex:boundaryLine(angleIndex)
local rad = MapPoint:screenRadius()
local bTop = rad*vec2(0.8660254038, 0.5) -- cos30 = 0.8660254038 = sqrt(3)/2, sin(30)
local bBot = rad*vec2(0.8660254038, -0.5)
local ang = angleIndex*math.pi/3.0
local vTop = bTop:rotate(ang)
local vBot = bBot:rotate(ang)
return {vTop.x, vTop.y, vBot.x, vBot.y}
end
We should send that through Maps, which might allow us to make MapPoint local, if not today then soon.
I discover that that method isn’t even implemented. It’s in the Hex code for drawing. Hex is used in test but the drawing must not be tested, as is common. I’ll fix it and put it on Maps for now.
No. It’s wrong and we don’t need these functions, they are not used and not tested. Delete them. Git can get them back if we need them.
Commit: removed unused untested Hex drawing methods.
So I can’t quite hide MapPoint yet. I guess that’s OK. I think I’d prefer more hiding. Let’s talk about that.
I don’t usually concern myself with private classes and the like. I’m used to Smalltalk, where everything was wide open. But there are two forces acting here that suggest some privacy.
First of all, the program is getting large, and some way of grouping classes separately would be useful. Even Smalltalk had ways of doing that, but Codea doesn’t help us much on that front.
Second, because I’m converting from an old way to a new way of doing things, and changing the new way as I go, I want to be sure that the code is all using the new ways, that there are no little corners or crevices where the old ways are still practiced. That can only lead to trouble.
Renaming classes, and making them local, are pretty heavy hammers, but they are the ones I’ve got here in iPad programming land. So I’d like to do as much as I reasonably can. But for now, I think I’ve done that, even though I remain unsatisfied.
But the PathFinder!
Yeah, what about that? It has been broken since at least 4 July, when I did the first commit on this iPad. I could go to the old iPad and look for the problem. Maybe I’ll do that when I break here. I’m pretty sure that what has happened is that we accessed the PathFinder monster after it had declared itself hors de combat, and someone has tried to decide whether he should be attack or be attacked. And I’m rather sure that he used to be able to go down the WayDown without that problem. Very curious. We should write a test for it when we find the problem.
But it’s nearly lunch time. Let’s sum up.
Summary
We’ve improved the mapping strategy objects, in a good way, I think. It wasn’t challenging, but it was tedious. We have to accept that, I think, given that our need is to put an entirely new substrate under the game. But the changes mean that most of the things that go on do not need changing, just a relatively few methods that touch bottom.
I do think we’l need to create a few more methods on the strategy objects, notably the ones that calculate distances, and perhaps some other convenience ones. With this in place as nicely as it is, those new methods should be straightforward to implement. We’ll see.
This is, however, taking longer than I had hoped. I think I said that the earliest date for playing on the hexes was this week. There’s still tomorrow and Saturday, since we do often “work” on Saturdays, but it feels tight. I’m refusing to feel pressure over an imaginary game product, but in honesty, I can feel it looming. Weird things, these humans.
But all in all, it’s coming along nicely.
But I sure want to put in that shark.
See you next time!