Dungeon 258
Trying again. Last time was a disaster. I’ll sum up.
This gets to be a long slog. Feel free to skip ahead, maybe all the way to the Summary at the bottom. Mostly this is just nitty-gritty moving code around.
This morning I tried moving createTestLevel from GameRunner into DungeonMaker. I ran into a problem that I couldn’t find or explain. My best guess was that somehow I got an extra instance of Dungeon that was uninitialized. I’m going to try the same thing again, more carefully.
My original thinking was that my testing is pretty solid, and that I should be able to just move code over, run it, see what breaks, move that over (or otherwise deal with it), rinse repeat. It didn’t work, so I’ll try again. I will be a bit more prone to write new tests this time: I wrote none this morning, thinking that my tests were good enough.
Here goes. We have this:
We interrupt this program to save you many lines of confusion along with the author.
Many lines and much confusion deleted.
I give up, again. I can see that we’re creating too many Dungeon instances, and that different objects have different instances. That just won’t do. But I don’t see quite how it’s happening, and the fixes I’ve tried so far are atrocious, slamming values into other people’s objects.
Revert.
New plan: Let’s have dungeons get created with their dimensions and then populate themselves with tiles right then and there. I decide to remove DungeonMaker entirely, it’s just a couple of tests and a few lines of code.
Let’s just change the Dungeon init and find all the necessary changes. Might work.
function Dungeon:init(runner, tileCountX, tileCountY)
self.runner = runner
self.tileCountX = tileCountX or assert(false, "must provide tile count")
self.tileCountY = tileCountY or assert(false, "must provide tile count")
self:createTiles()
end
Search quickly for Dungeon(to fix up as much as we can.
function GameRunner:createNewDungeon()
self.dungeon = Dungeon(self, self.tileCountX, self.tileCountY)
end
Test, looking for things to convert to the new scheme. I’m hoping there are few.
Dungeon:251: attempt to perform arithmetic on a nil value (field 'tileCountX')
stack traceback:
Dungeon:251: in method 'createTiles'
Dungeon:125: in field 'init'
... false
end
setmetatable(c, mt)
return c
end:24: in global 'Dungeon'
GameRunner:266: in method 'createNewDungeon'
GameRunner:10: in field 'init'
... false
end
setmetatable(c, mt)
return c
end:24: in global 'GameRunner'
Tests:22: in field '_before'
CodeaUnit:44: in method 'test'
Tests:36: in local 'allTests'
CodeaUnit:16: in method 'describe'
Tests:12: in function 'testDungeon2'
[string "testDungeon2()"]:1: in main chunk
CodeaUnit:139: in method 'execute'
Main:11: in function 'setup'
Check that test:
_:before(function()
_TileLock = TileLock
TileLock = false
_tweenDelay = tween.delay
tween.delay = fakeTweenDelay
_Runner = Runner
_bus = Bus
Bus = EventBus()
Runner = GameRunner()
_dc = DungeonContents
DungeonContents = DungeonContentsCollection()
Runner:getDungeon():createTiles(Runner.tileCountX, Runner.tileCountY)
end)
Change the before:
_:before(function()
_TileLock = TileLock
TileLock = false
_tweenDelay = tween.delay
tween.delay = fakeTweenDelay
_Runner = Runner
_bus = Bus
Bus = EventBus()
Runner = GameRunner()
_dc = DungeonContents
DungeonContents = DungeonContentsCollection()
end)
The tiles are supposed to be created when GameRunner is created. Now we get:
Tile:92: Invariant Failed: Tile must receive Runner
stack traceback:
[C]: in function 'error'
Tile:92: in field 'init'
... false
end
Oh my. This is nasty. Tricky code, the logic that decides what kind of Tile to build way down low:
function BaseMap:createRectangle(xWidth, zHeight, klass, ...)
self.tab = {}
for z = 1,zHeight do
for x = 1,xWidth do
local pt = self.mapType(x,z)
local item = klass(pt, ...)
self:atPointPut(pt,item)
end
end
end
I think, but am not sure, that we don’t have a legitimate runner going into klass … but printing klass doesn’t print Tile, and Tile has a __tostring.
Some quick checking tells me that the problem is that we have somehow provided a different GameRunner and not put it into Runner. I comment out the check for the invariant.
It Works!
Back away slowly. Here’s what I’ve changed:
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:createTiles()
end
function Dungeon:createTiles()
Maps:cartesian()
self.map = Maps:map(self.tileCountX+1,self.tileCountY+1, Tile,TileEdge, self. runner)
end
I changed the above to accept (and require) the dungeon size when the Dungeon is created, and to use that size when tiles are created.
function GameRunner:dcPrepare()
TileLock = false
DungeonContents = DungeonContentsCollection()
self:createNewDungeon()
local dungeon = self.dungeon
if HexMode then
assert(false, "broken")
dungeon:createHexTiles(self.tileCountX, self.tileCountY)
end
dungeon:clearLevel()
Announcer:clearCache()
end
Here, I just removed the code that creates tiles, and flagged HexMode as broken. (In the interest of figuring this out, I check with myself and got permission to break HexMode. I’m still negotiating over removing it entirely.)
Here:
_:before(function()
_TileLock = TileLock
TileLock = false
_tweenDelay = tween.delay
tween.delay = fakeTweenDelay
_Runner = Runner
_bus = Bus
Bus = EventBus()
Runner = GameRunner()
_dc = DungeonContents
DungeonContents = DungeonContentsCollection()
end)
I removed the code that created tiles, since it is no longer necessary. There is a major code smell here, which is that this setup, and many others, is way too complicated. That tells me that there’s something wrong, that the objects may not have their responsibilities divided up well … or something. I’m honestly not sure if complicated setups are always due to the same thing, but they’re generally a bad sign and should at least cause one to look around.
Another test had the same issue and I corrected it as well.
Finally, in Tile, I had to remove this check:
function Tile:init(mapPoint,kind, runner)
assert(mapPoint:is_a(MapPoint), "Tile requires MapPoint")
--[[
if runner ~= Runner then
print(runner, " should be ", Runner)
error("Invariant Failed: Tile must receive Runner")
end
--]]
self.mapPoint = mapPoint
self.kind = kind
self.runner = runner
self:initDetails()
end
At this point my tests all run, and the game plays correctly. I commit: Dungeon now creates tiles when it’s created.
Where Are We?
Well, we’re 2.5 sessions less far than I had hoped to be. We are, however, in a somewhat better place.
One of Kent Beck’s dicta is to say that when we’re faced with a difficult change, make the change easy, then make the easy change. What I’ve just done is a poor attempt at that. Now that Dungeon creates its tiles when the Dungeon itself is created, we have a bit narrower surface of behavior to deal with. However, I freely grant that making this change was based on a guess.
It’s clear enough, I think, that Dungeon is simpler now that it knows the Dungeon size from the beginning. And, given that it knows, it’s simpler to creat the tiles at once, rather than later. But will this make it easier to build the DungeonMaker? That, I don’t know for sure, but if Dungeon is a bit simpler, it surely can’t make things worse. Or so I reason.
Along the way, I found two things to worry about. One was whether we ever create new tiles other than when the Dungeon is created. Ideally we wouldn’t. The other issue was that invariant in Tile. I put that in because somewhere, someone was passing in a GameRunner that wasn’t the one in Runner, the “singleton” location for the currently live GameRunner.
Let’s put the invariant back and find the problem. It’s surely just a test that isn’t doing quite what we want.
I hate it when I get a super long traceback:
Tile:93: Invariant Failed: Tile must receive Runner
stack traceback:
[C]: in function 'error'
Tile:93: in field 'init'
... false
end
setmetatable(c, mt)
return c
end:24: in local 'klass'
BaseMap:33: in method 'createRectangle'
BaseMap:25: in field 'init'
... false
end
setmetatable(c, mt)
return c
end:24: in function <... false
end
setmetatable(c, mt)
return c
end:20>
(...tail calls...)
Dungeon:249: in method 'createTiles'
Dungeon:125: in field 'init'
... false
end
setmetatable(c, mt)
return c
end:24: in global 'Dungeon'
GameRunner:265: in method 'createNewDungeon'
GameRunner:10: in field 'init'
... false
end
setmetatable(c, mt)
return c
end:24: in global 'GameRunner'
Tests:22: in field '_before'
CodeaUnit:44: in method 'test'
Tests:35: in local 'allTests'
CodeaUnit:16: in method 'describe'
Tests:12: in function 'testDungeon2'
[string "testDungeon2()"]:1: in main chunk
CodeaUnit:139: in method 'execute'
Main:11: in function 'setup'
But Tests:22 tells me what I need to know, I think. That’s here:
_:before(function()
_TileLock = TileLock
TileLock = false
_tweenDelay = tween.delay
tween.delay = fakeTweenDelay
_Runner = Runner
_bus = Bus
Bus = EventBus()
Runner = GameRunner()
_dc = DungeonContents
DungeonContents = DungeonContentsCollection()
end)
Ah. GameRunner creates a new dungeon, and is not stored into Runner until the call returns. I know that I put that check in because the problem was occurring. We could leave it out, or change it to a warning. Or we could cause GameRunner to store itself in Runner right away:
function GameRunner:init(testX, testY)
Runner = self
self.tileSize = 64
self.tileCountX = testX or 85 -- if these change, zoomed-out scale
self.tileCountY = testY or 64 -- may also need to be changed.
self:createNewDungeon()
self.cofloater = Floater(50,25,4)
self.musicPlayer = MonsterPlayer(self)
self.dungeonLevel = 0
self.requestNewLevel = false
self.playerRoom = 1
Bus:subscribe(self, self.createNewLevel, "createNewLevel")
Bus:subscribe(self, self.darkness, "darkness")
end
That makes the error go away. Now if anyone ever creates a GameRunner, it becomes the official one. Let’s find creators and modify them not to save the result.
OK all the tests and code that create a GameRunner do already save to Runner. But this:
Runner = GameRunner()
Is 10X better than this:
GameRunner()
The latter completely hides the fact that we’ve set Runner. Ah, I have some good news. The Main needs to know the current GameRunner, to trigger drawing and such. We can make its knowledge local to it. All the other references to Runner are in tests. We can make those local as well.
Remove the Runner setting in GameRunner:init. Remove the invariant. Test. All green. Does the game run?
GameRunner:548: attempt to index a nil value (global 'Runner')
stack traceback:
GameRunner:548: in method 'scaleForTinyMap'
GameRunner:347: in method 'drawTinyMap'
OperatingModes:17: in method 'drawTinyMap'
GameRunner:342: in method 'drawTinyMapOnTopOfLargeMap'
GameRunner:288: in method 'draw'
Main:100: in function 'draw'
No. This is why I always run it. I don’t fully trust my tests.
function GameRunner:drawTinyMap()
pushMatrix()
self:scaleForTinyMap()
Runner:drawMap(true)
popMatrix()
end
I missed this one. It’s more than a little odd anyway. Try self. There are two occurrences. Change both to self. Game runs. Commit: Remove global Runner from system. Woot!
Let’s look back.
Retrospective
I typoed “restrospective”. Sort of a break with reflection? Perhaps a new book to write. An omen. Or maybe just a typo.
Anyway … I’ve used about 2 1/2 sessions to separate dungeon building from dungeon operation, and the net result is that I’ve removed my initial DungeonMaker, and instead spent a couple of hours improving the Dungeon class so that it creates tiles at the very beginning, and then improving tests and code to simplify testing, and to by the way, reassure myself that we don’t need the global Runner nor the check in Tile to be sure that we have the right runner.
We can be rather sure now that we do have the right one, because we create the dungeon from the GameRunner and pass it in. I think the only way the problem occurred at all was probably due to the creation and destruction of temporary GameRunners in testing.
So I think we’re at a better place, and I am somewhat hopeful that these changes will make it easier to separate out building the dungeon, but I’ve removed what little direct progress was made.
More to the point, I burned at least four hours yesterday trying to make the DungeonMaker accept the next step, which seemed to be moving a very simple bit of code over, and calling right back. I would probably have done better to immediately revert and try something else, but I had a bug in my thinking, or bad luck.
Often, in refactoring, it makes sense to make part of the change, and then use the failing tests to guide further changes. (With a smarter code browser, there are often better ways, but we’re here in iPadLand and things are tough.) So that was my plan, make the initial change, follow my nose through the tests to make it work. And it started out OK and then I got a failure that caused me to follow my nose further and further into some kind of Nose Hell, until finally, way too late, I would give up and revert.
I don’t know whether it’s just a poor idea ever to make a change and run the tests and let them tell you what to fix. I don’t think it’s a poor idea all the time, but it sure didn’t work yesterday. Maybe I wasn’t careful enough.
Be that as it may, I’m now going once more into the breach, to create a DungeonBuilder (formerly called Maker).
As I did before, I’ll create at least some tests for it.
DungeonBuilder
My first cut with test is this:
-- DungeonBuilder
-- RJ 20220307
local _bus
function testDungeonBuilder()
CodeaUnit.detailed = false
_:describe("DungeonBuilder", function()
_:before(function()
_bus = Bus
Bus = EventBus()
end)
_:after(function()
Bus = _bus
end)
_:test("creation", function()
local runner = GameRunner()
local builder = DungeonBuilder(runner)
_:expect(2).is(2)
end)
end)
end
DungeonBuilder = class()
function DungeonBuilder:init(runner)
self.runner = runner
end
Now the current new scheme that the GameRunner creates the dungeon. We have this:
function GameRunner:init(testX, testY)
self.tileSize = 64
self.tileCountX = testX or 85 -- if these change, zoomed-out scale
self.tileCountY = testY or 64 -- may also need to be changed.
self:createNewDungeon()
self.cofloater = Floater(50,25,4)
self.musicPlayer = MonsterPlayer(self)
self.dungeonLevel = 0
self.requestNewLevel = false
self.playerRoom = 1
Bus:subscribe(self, self.createNewLevel, "createNewLevel")
Bus:subscribe(self, self.darkness, "darkness")
end
function GameRunner:createNewDungeon()
self.dungeon = Dungeon(self, self.tileCountX, self.tileCountY)
end
Let’s inline that:
function GameRunner:init(testX, testY)
self.tileSize = 64
self.tileCountX = testX or 85 -- if these change, zoomed-out scale
self.tileCountY = testY or 64 -- may also need to be changed.
self.dungeon = Dungeon(self, self.tileCountX, self.tileCountY)
self.cofloater = Floater(50,25,4)
self.musicPlayer = MonsterPlayer(self)
self.dungeonLevel = 0
self.requestNewLevel = false
self.playerRoom = 1
Bus:subscribe(self, self.createNewLevel, "createNewLevel")
Bus:subscribe(self, self.darkness, "darkness")
end
Test. Grr. There’s another use of that function. Back that out.
Who are the senders of that? We have init. What else?
function GameRunner:dcPrepare()
TileLock = false
DungeonContents = DungeonContentsCollection()
self:createNewDungeon()
local dungeon = self.dungeon
if HexMode then
assert(false, "broken")
dungeon:createHexTiles(self.tileCountX, self.tileCountY)
end
dungeon:clearLevel()
Announcer:clearCache()
end
This is good news and not so good news. The good news is that with this code we create a newDungeon every time we create a level. That’s better than trying to clear out a level and reuse it. The bad news is that GameRunner needs to be told what kind of dungeon to create, and there are about four possibilities, a “new level”, a “hex level”, a “learning level”, and a “test level”.
Meh. I really liked those few moments when GameRunner created the Dungeon right off the bat. Let’s remove that and see what happens.
Only 20 tests fail. I think this is because they now assume that there is a dungeon.
There were two locations needing this kind of change:
_:before(function()
_TileLock = TileLock
_tweenDelay = tween.delay
tween.delay = fakeTweenDelay
_bus = Bus
Bus = EventBus()
runner = GameRunner()
runner:createTestLevel(0) -- <-- added
_dc = DungeonContents
DungeonContents = DungeonContentsCollection()
TileLock = false -- <-- moved to after level creation.
end)
Now all the tests run, and the game does not:
GameRunner:373: attempt to index a nil value (field 'dungeon')
stack traceback:
GameRunner:373: in method 'hasMonsterNearPlayer'
MusicPlayer:78: in method 'checkForDanger'
MusicPlayer:83: in field 'callback'
...in pairs(tweens) do
c = c + 1
end
return c
end
:158: in upvalue 'finishTween'
...in pairs(tweens) do
c = c + 1
end
return c
end
:589: in function <...in pairs(tweens) do
c = c + 1
end
return c
end
:582>
I am not sure about this. Let’s check Main: I’m not sure the game is actually started. OK, it is:
...
TileLock = false
Bus = EventBus()
currentRunner = GameRunner()
currentRunner:createLevel(12)
Inventory:subscribe()
...
Let’s look at 373 …
function GameRunner:hasMonsterNearPlayer(range)
return self.dungeon:hasMonsterNearPlayer(range)
end
OK, no question there. self.dungeon is nil. We’re still showing the tests at this point, and apparently some other stuff is running.
This is not technical debt: this is inferior design, showing me that the code is hard to change.
I took a wild guess and guessed right. In GameRunner:init, we have this:
function GameRunner:init(testX, testY)
self.tileSize = 64
self.tileCountX = testX or 85 -- if these change, zoomed-out scale
self.tileCountY = testY or 64 -- may also need to be changed.
--self:createNewDungeon()
self.cofloater = Floater(50,25,4)
--self.musicPlayer = MonsterPlayer(self)
self.dungeonLevel = 0
self.requestNewLevel = false
self.playerRoom = 1
Bus:subscribe(self, self.createNewLevel, "createNewLevel")
Bus:subscribe(self, self.darkness, "darkness")
end
I just commented out the music player. It checks to see whether to play the regular theme or the monster theme. I’ll have to move that into dcPrepare to get the music back, I guess. That works, but I found another use of Runner in Main. Changed to currentRunner. Tests and game seem good. Commit: fixed issues with music player and touch. GameRunner no longer creates dungeon right off the bat.
Does This Seem Chaotic and Bizarre?
It almost seems that I’ve backed out every change I’ve made in the past few days. First we don’t create a dungeon in GameRunner init, then we do, then we don’t. And so on.
There’s more than a germ of truth there. I am pushing the clay here and there to see whether I can see a better shape. If I were smarter, or had a better memory, or a better pair partner than a sleeping cat, I might go more directly from where we are to better. As it stands, my moves are tentative and often have ramifications beyond what I anticipate.
This is because the design isn’t as crisp as it could be. There was that global Runner that random methods used, and that had to be set up carefully so that they all got the same one. The dungeon member needs to be created at the right times, and while doing it right away seemed good, it turns out that it wasn’t. I’m still not sure we’re just right. Currently, there are two ways for a dungeon to be created (not counting the special ones). Let’s look.
function WayDown:actionWith(aPlayer)
if aPlayer:provideWayDownKey() then
Bus:publish("createNewLevel")
else
Bus:publish("addTextToCrawl", self, {text="You must have a key to go further!"})
end
end
The WayDown publishes “createNewLevel” on the Bus when the player goes down. That’s fielded here:
function GameRunner:init(testX, testY)
self.tileSize = 64
self.tileCountX = testX or 85 -- if these change, zoomed-out scale
self.tileCountY = testY or 64 -- may also need to be changed.
--self:createNewDungeon()
self.cofloater = Floater(50,25,4)
self.dungeonLevel = 0
self.requestNewLevel = false
self.playerRoom = 1
Bus:subscribe(self, self.createNewLevel, "createNewLevel")
Bus:subscribe(self, self.darkness, "darkness")
end
So when the Bus sees “createNewLevel”, it will call that method on the GameRunner. You’d think that we could simply publish “createNewLevel” from Main to start the game , but that turns out not to work.
The reason is somewhat intricate. The createNewLevel message just sets a flag, and the flag is checked when the player has completed her turn. Then, and only then, can we create a new level.
I’m not sure if this is a design flaw or not. The game is set up now so that Main will run the tests if CodeaUnit is present, but a game is always initiated and running, so if there are no tests, or if you touch the screen with the tests displayed, the running game is shown, already rolling.
That’s probably not a good thing. If there were an initial crawl, for example, it would have scrolled off before I could touch the tests to make the game show.
I think we should leave this alone. We are here to create a DungeonBuilder. Let’s focus on that and deal with other design issues instantly, if we see how, or later, if we don’t. This one is a later.
Lesson? Be careful when diverting to clean up a design problem. It might help, but it might not be a step in the direction you’re currently trying to go. Judgment in the light of experience.
But to the question of whether this seems chaotic and bizarre, yes it does. We haven’t broken anything permanently, but what we can see here is a design that has been left along too long, tolerating too many quick fixes. What we have here is perilously close to legacy code. Fortunately I do have some decent tests, although not enough. Since I almost always actually run the game a bit before committing, we know that I don’t fully trust the tests.
I’ll try to see if I can write a test more frequently when the game breaks but the tests don’t.
But right now we are here to improve the building. Let’s get back to that.
DungeonBuilder
Current design is that the GameRunner has the only instance of the current Dungeon. Clearly a DungeonBuilder must either have the instance while it works, or forward lots of messages to GameRunner to effect changes. The latter is exactly what we don’t want.
Where do we set self.dungeon now, in GameRunner? Just one place:
function GameRunner:createNewDungeon()
self.dungeon = Dungeon(self, self.tileCountX, self.tileCountY)
end
Sweet. Let’s create our DungeonBuilder right there as well.
function GameRunner:createNewDungeon()
self.dungeon = Dungeon(self, self.tileCountX, self.tileCountY)
self.builder = DungeonBuilder(self,dungeon)
end
We’ll need to enhance our builder test and code.
_:test("creation", function()
local runner = GameRunner()
_:expect(runner.builder).is(nil)
runner:createNewDungeon()
local builder = runner.builder
_:expect(builder, "no builder").isnt(nil)
_:expect(builder.runner, "no runner").is(runner)
_:expect(builder.dungeon, "no dungeon").isnt(nil)
_:expect(2).is(2)
end)
Test. Fails saying “no dungeon”. Class doesn’t conform yet. Change:
function DungeonBuilder:init(runner, dungeon)
self.runner = runner
self.dungeon = dungeon
end
Curiously this still fails, saying “no dungeon”. Helps to do it right:
function GameRunner:createNewDungeon()
self.dungeon = Dungeon(self, self.tileCountX, self.tileCountY)
self.builder = DungeonBuilder(self,self.dungeon)
end
Test. Tests run. Commit: createNewDungeon now creates initialized DungeonBuilder with dungeon.
Here, on the other hand, is a diversion that seems worth it, if only because it’s easy.
I notice that the tests are now running in two to three seconds, formerly one second. I suspect it’s because we default to a larger size dungeon when we create the GameRunner.
function GameRunner:init(testX, testY)
self.tileSize = 64
self.tileCountX = testX or 85 -- if these change, zoomed-out scale
self.tileCountY = testY or 64 -- may also need to be changed.
--self:createNewDungeon()
self.cofloater = Floater(50,25,4)
self.dungeonLevel = 0
self.requestNewLevel = false
self.playerRoom = 1
Bus:subscribe(self, self.createNewLevel, "createNewLevel")
Bus:subscribe(self, self.darkness, "darkness")
end
Setting the various before clauses to smaller dimensions gets the tests back to 1 second. Commit: Smaller dungeons in tests.
OK. Now in createNewLevel, we have a dungeon and the corresponding builder. I want to document those variables in init. I think it may help avoid confusion later.
function GameRunner:init(testX, testY)
self.tileSize = 64
self.tileCountX = testX or 85 -- if these change, zoomed-out scale
self.tileCountY = testY or 64 -- may also need to be changed.
self.cofloater = Floater(50,25,4)
self.dungeonLevel = 0
self.requestNewLevel = false
self.playerRoom = 1
Bus:subscribe(self, self.createNewLevel, "createNewLevel")
Bus:subscribe(self, self.darkness, "darkness")
self.dungeon = nil -- created in createNewDungeon
self.builder = nil -- created in createNewDungeon
end
Test. Commit: show members dungeon and builder in GameRunner:init.
Now let’s quit messing about and do something in the DungeonBuilder.
That happens in the various level creation methods. Prior to today, I was trying the Learning Level, because it seemed easier. Today I’m going to work with the primary level, because it’s more important. That’s createLevel. It’s huge but I plan not to mind that:
Later: This may be a mistake, though I don’t think it is. If I’m right that I can convert this one, then the other conversions will be easy. If I’m wrong … it’s gonna hurt. We won’t know for a couple of days.
function GameRunner:createLevel(count)
-- determine level
self.dungeonLevel = self.dungeonLevel + 1
if self.dungeonLevel > 4 then self.dungeonLevel = 4 end
self:dcPrepare()
-- customize rooms and connections
self.dungeon:createRandomRooms(count)
self.dungeon:connectRooms(self:getRooms())
-- paint dungeon correctly
self.dungeon:convertEdgesToWalls()
self:placePlayerInRoom1()
-- customize contents
self:placeWayDown()
self:placeSpikes(15)
self:placeLever()
--self:placeDarkness()
self:placeNPC()
self:setupMonsters(6)
self.keys = self:createThings(Key,5)
self:createThings(Chest,5)
self:createLoots(10)
self:createDecor(30)
self:createButtons()
-- prepare crawl
self.cofloater:startCrawl()
Announcer:sayAndSave(self:crawlMessages(self.dungeonLevel))
self:startTimers()
self:dcFinalize()
end
I think dcPrepare needs a hard look. Yes:
function GameRunner:dcPrepare()
TileLock = false
DungeonContents = DungeonContentsCollection()
self:createNewDungeon()
local dungeon = self.dungeon
if HexMode then
assert(false, "broken")
dungeon:createHexTiles(self.tileCountX, self.tileCountY)
end
dungeon:clearLevel()
self.musicPlayer = MonsterPlayer(self)
Announcer:clearCache()
end
We create a new dungeon here and then populate.
I’m going to use Chet’s favorite trick and dispatch back from the builder.
I rename the existing method to buildLevel and create a new one:
function GameRunner:createLevel(count)
self:createNewDungeon()
self.builder:buildLevel(count)
end
function GameRunner:buildLevel(count)
...
I need to remove the createNewDungeon from dcPrepare:
function GameRunner:dcPrepare()
TileLock = false
DungeonContents = DungeonContentsCollection()
if HexMode then
assert(false, "broken")
self.dungeon:createHexTiles(self.tileCountX, self.tileCountY)
end
self.dungeon:clearLevel()
self.musicPlayer = MonsterPlayer(self)
Announcer:clearCache()
end
And now I should fail not understanding buildLevel in DungeonBuilder.Not quite. I can’t remove that creation, tests fail. For now I’ll lazy init, which I think should get us back in the air.
function GameRunner:dcPrepare()
TileLock = false
DungeonContents = DungeonContentsCollection()
if not self.dungeon then
self:createNewDungeon()
end
if HexMode then
assert(false, "broken")
self.dungeon:createHexTiles(self.tileCountX, self.tileCountY)
end
self.dungeon:clearLevel()
self.musicPlayer = MonsterPlayer(self)
Announcer:clearCache()
end
GameRunner:139: attempt to call a nil value (method 'buildLevel')
stack traceback:
GameRunner:139: in method 'createLevel'
Main:64: in function 'setup'
As expected.
function DungeonBuilder:buildLevel(roomCount)
self.runner:buildLevel(roomCount)
end
Tests run. Game runs. Commit: DungeonBuilder now gets first shot at building dungeon, then calls back to GameRunner.
I should have added Woot! to that commit message. It has been rather a slog to get here. I believe that now we can begin moving code from the new GameRunner:buildLevel née createLevel, into DungeonBuilder. It should be “just”1 a matter of moving bits of it and making them run.
I guess I’ll try moving the call to dcPrepare over.
function DungeonBuilder:buildLevel(roomCount)
self.runner:dcPrepare()
self.runner:buildLevel(roomCount)
end
(With the call removed from GameRunner.)
Tests run, game runs. Commit: dcPrepare is called separately from DungeonBuilder, still in GameRunner.
Now let’s call the one in DungeonBuilder:
function DungeonBuilder:dcPrepare()
self.runner:dcPrepare()
end
Not a big improvement but now we have a place to stand.
Here’s the real method:
function GameRunner:dcPrepare()
TileLock = false
DungeonContents = DungeonContentsCollection()
if not self.dungeon then
self:createNewDungeon()
end
if HexMode then
assert(false, "broken")
self.dungeon:createHexTiles(self.tileCountX, self.tileCountY)
end
self.dungeon:clearLevel()
self.musicPlayer = MonsterPlayer(self)
Announcer:clearCache()
end
There are issues with moving this into builder, and we should always expect that. We have that hack with createNewDungeon. We really dare not do that inside the builder, because the gameRunner knows the dungeon and creates a new builder.
The HexMode stuff is just there to irritate me, I think. I’ll remove that from the original and make a card for it. Then I’ll lose the card.
I think the clearLevel should be done when the dungeon is created, automatically. And we are committing to creating a new one for each level.
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:createTiles()
self:clearLevel()
end
And removing that stuff:
function GameRunner:dcPrepare()
TileLock = false
DungeonContents = DungeonContentsCollection()
if not self.dungeon then
self:createNewDungeon()
end
self.musicPlayer = MonsterPlayer(self)
Announcer:clearCache()
end
I expect everything to continue to work. Let’s find out why I’m mistaken.
We’re good. Commit: DungeonBuilder refers back to GameRunner with new dcPrepare method. Moved clearLevel into Dungeon:init.
Obviously I missed a chance to commit earlier. Oh well.
Now I’m wondering about the music player. There is no use of that member variable. The MonsterPlayer runs independently. So:
function GameRunner:dcPrepare()
TileLock = false
DungeonContents = DungeonContentsCollection()
if not self.dungeon then
self:createNewDungeon()
end
MonsterPlayer(self)
Announcer:clearCache()
end
Can we move the DungeonContents down?
function GameRunner:dcPrepare()
TileLock = false
if not self.dungeon then
self:createNewDungeon()
end
DungeonContents = DungeonContentsCollection()
MonsterPlayer(self)
Announcer:clearCache()
end
Yes. Commit: “reorder dcPrepare, drop member variable for music player.” Could have done two commits there, too. Slow down, Ron, you’re doing fine.
Now if we could just get rid of that extra creation. I just did that as a convenience to avoid changing tests. Let’s remove it and do it right.
There were just three places that needed to createNewDungeon, two in tests and one in createTestLevel. I suspect there’ll be a problem with the learning level. Let me try it. Yes. Add the create to that method as well. Now dcPrepare is this:
function GameRunner:dcPrepare()
TileLock = false
DungeonContents = DungeonContentsCollection()
MonsterPlayer(self)
Announcer:clearCache()
end
We can move that to DungeonBuilder:
function DungeonBuilder:dcPrepare()
TileLock = false
DungeonContents = DungeonContentsCollection()
MonsterPlayer(self.runner)
Announcer:clearCache()
end
Important to say self.runner there. I forgot that and confused myself briefly.
I left GameRunner: dcPrepare in place, since the other level builders use it.
It’s after 1 PM here, so even taking my trip to the post office into account, I should wrap up. We have a builder that is actually doing some building:
DungeonBuilder = class()
function DungeonBuilder:init(runner, dungeon)
self.runner = runner
self.dungeon = dungeon
end
function DungeonBuilder:buildLevel(roomCount)
self:dcPrepare()
self.runner:buildLevel(roomCount)
end
function DungeonBuilder:dcPrepare()
TileLock = false
DungeonContents = DungeonContentsCollection()
MonsterPlayer(self.runner)
Announcer:clearCache()
end
So far it’s just doing easy stuff but it is doing stuff. It’s beginning to bear the load. This is good. Let’s sum up.
Summary
I’ve taken three full runs at moving dungeon building into a separate class, and two of those runs have ended in a revert. Each time I learned a little, and each learning resulted in a tiny change to the way things work, removing some of the wrinkles.
Why so many wrinkles? I’ve not been doing a superior job. Over time, in my zeal to make this program work and do cool new things, I wound up with redundant and halfway measures, so that some things were done in multiple places, and in other instances things were done more by luck than by design. I’ve done things one shouldn’t be proud of.
However, it’s not too late. Moving methods around, and changing dungeon creation has improved many of these poor moves.
As for the process today, it was odd that I first set up to do dungeon creation in init and made that work, only to decide that I didn’t like it. Some would argue that I “should” have seen the better design right away. I am all about not worrying about “should”. I saw what I saw. Having moved the creation into init, I then saw that it was too soon. Had I not tried it, I might have speculated that it was too soon, or used some innate design sense reaching back over 200 versions of this program to some perfect clarity.
Those magical facilities are not available to me. I often need to see the code in a new form to decide that the new form isn’t as good as I need, so I move things around. Now they are better. Are they just what I need? Surely not, but they are better.
And tomorrow, we’ll make it betterer. Do stop by and watch.
-
When anyone says something will “just” be some simple thing, run away. ↩