More on improving control over Dungeon creation. And a RAT. And a bear bite, kind of.

We’ve reached a point in the Decor creation where I’m beginning to feel that it’s cracked. The code isn’t really great yet, but I feel we’re not bad on that score, and that the essence of the problem is solved. This, as constant readers know, is the danger point where I get bored and move on to something else.

If we were really building a product, this could be bad. Since we’re just looking for code to learn from, moving around can even be good. In real software work, we typically have a large code base, and we’re called upon to add and change things all over, so we’re always encountering problems and code where we’re not totally up to speed.

Besides … I’ve learned that we can draw lessons from any code. Sometimes, rarely, it is really good and we can talk about why it’s so nearly perfect. More commonly, it is decent, mediocre, or downright poor code, which gives us plenty of room for improvement and learning.

As for Decor, we’re not quite there. I want to figure out some scheme for selecting tiles, where we can at least see how we’d do likely alternatives that may arise. Which leads us to the topic of “generality”. I’ve mentioned before that, to me, generality comes in two forms, the kind where some very simple code handles all our cases quite nicely, and the kind where we add complexity to the code to make it handle more cases. I like the first kind, and am not so fond of the second.

So, whatever we do to provide room selection to our Decor creation, we’ll want to try for the simple kind of generality, not the complicated kind. That concern will be flavored, however, by the fact that different tile selections quite probably do require rather different kinds of searches.

Or do they? I think they do, but this is something I’d like to be mistaken about. We’ll find out … someday.

Let’s review our tests and code. Things are pretty straightforward for now.

Code Review

I always start a day, or a new task, with a bit of code review, to refresh my memory on how things work. Doing so with the code in front of me gives me an opportunity to spot things that might be useful but that I hadn’t remembered, and it gives me a very nice overview of how good … or not so good … the relevant code is.

We’re working down a path where the dungeon’s Decor layout can be controlled by an array of strings, each string specifying a Decor in a simple format:

"Skeleton1;health(5,10);r1"

This isn’t terrible to type, and will be easy to generate with code in some long-distant future world where we build a tool to help do the job. We have a very simple Parse object to deal with this string:

function Parse:decorDefinition(aString)
    local pat = "[^;]+"
    local iter = string.gmatch(aString,pat)
    local parse = {}
    for w in iter do
        table.insert(parse, self:parseElement(w))
    end
    if #parse ~= 3 then return InvalidParse() end
    return self(parse[1], parse[2], parse[3])
end

function Parse:parseElement(aString)
    -- kind(min,max) parens optional default 0
    local iter = string.gmatch(aString, "%w+")
    return ParseElement(iter(), tonumber(iter() or 0), tonumber(iter() or 0))
end

function Parse:init(decorElement, lootElement, roomElement)
    self.valid = true
    self.decor = decorElement
    self.loot = lootElement
    self.room = roomElement
end

Right now the Parse object is custom-made for Decor. You and I can both see how this could be generalized, and it’s tempting to do it, but my better angels tell me to wait until I have a second potential user, and generalize with that real problem in mind. So we’ll wait. Let me note here a couple of things we might do:

  • Allow for more than 2 values inside the parens;
  • Allow the values inside the parens to be non-numbers;
  • Allow for other than three main fields;
  • Allow the fields to have different names from decor, loot, and room.

These are all easy enough. If we were working on a library for parsing, I could imagine doing them now, speculating about what people would want to do. I could even imagine doing them because I see how. And, of course it’s tempting to do something smarter than the current parsing. The code above just splits the input string on semicolon breaks, and then splits the individual bits into “word” strings, alphanumerics. Seriously trivial parsing. Surely we need a grammar, lexing, and so on … but no, we don’t. This will serve until it doesn’t, so we’ll leave it alone.

I should mention GeePaw Hill’s RAT 🐀, Rework Avoidance Theory. Many folx, including yours truly sometimes, would look at this parsing and think that we should beef it up right now, because if we don’t, we’ll just have to rework it in the future. We think “rework is bad”. Fact is, rework is good.

When we make something of wood, we don’t go from rough wood to smooth and shiny in one pass. We mill, we grind, we sand, and sand, and sand, and we seal and varnish and sand and varnish … the process of getting a marvelous finish involves many passes over the work.

In code, it’s the same, and we have the advantage that we can pause at any point between passes over the work, knowing that it will serve until it doesn’t, at which point we can make it better. For this to work, our code needs to be clear, and it needs to be modular, and it needs all those other good code qualities … but it doesn’t need to be the final version.

The RAT 🐀 in the code appears when we decide to do a lot of extra work now, so as to avoid it in the future. Get serious. The future may never come. We should never do extra work now to avoid work in the future. We should put such work off as long as we can, and no longer. Don’t avoid rework. Cherish it and prepare for it. Just don’t do it until we need to.

Back to the code. Our main test looks like this:

        _:test("Decor from definition string", function()
            local def = "Skeleton1;health(5,10);r1"
            local parsed = Parse:decorDefinition(def)
            _:expect(parsed.valid).is(true)
            local decorKind = Decor:legalKind(parsed.decor.kind)
            _:expect(decorKind).is("Skeleton1")
            local lootElement = parsed.loot
            local loot = Loot(nil, lootElement.kind, lootElement.min, lootElement.max)
            _:expect(loot:is_a(Loot)).is(true)
            _:expect(loot.kind).is("health")
            _:expect(loot.min).is(5)
            _:expect(loot.max).is(10)
        end)

What we can see here is that our central parsing is properly dealing with the Decor kind and the Loot kind, and that it isn’t dealing with the tile selection at all. We just have an “r1” there, reminding us that our main room selection scheme is “a room tile not in room 1”.

How should we do room selection. Well, both Decor and Loot have agreed terms for selecting kinds. Decor knows words like “Skeleton2”, and Loot knows words like “health” and “catPersuasion”. That’s the good news. The not so good news is that these words aren’t very well documented, but as we look at the code, we can readily find the tables. Perhaps they should be identified so that some imaginary Making App and this app can share the definitions, and so on. We don’t do that now, because we don’t need it now, and because of the RAT 🐀, we’re not going to do it now so that we don’t have to do it later. Later is good. Never is better.

OK, let’s think what we want for our first version of tile selection.

Tile Selection

I can only speculate about what kinds of tile selection may be needed, but that’s probably enough to give us all the clue we need:

  • Room tile avoiding room(n)
  • Room tile (any)
  • Hallway tile
  • Room tile against a wall, avoiding room(n)
  • …

And like that. We might also want something like

  • Tile avoiding room with WayDown
  • Tile avoiding room with NCP(name)

Pretty clearly these can all be handled with a string and optional parenthesized parameters, but we see here that the parameters may well want to be other than numeric, such as “WayDown”. We have no need of that today, but we want to be aware. Today, I think we only have a couple of choices in play. Let’s look around.

I run across this code in creating the learning level:

    self:createRoomsFromXYWHA(t, self.RUNNER)
    local r2 = self:getRooms()[2]
    local lootTile = r2:tileAt(2,2)
    local loot = Loot(lootTile, "health", 5,5)
    loot:setMessage("This is a Health Power-up of 5 points.\nStep onto it to add it to your inventory.")

Here we select a specific room, and a specific tile inside that room. Nice find to keep in mind, even though the learning level is hand-crafted right now. We’d want the Level Design folx to take this over, if we had any such people.

The current code does this:

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

The 666 is a trick to allow me to use the method that finds a tile, avoiding a room, since there is no room 666.

It seems that so far, we only have the one method, randomRoomTileAvoidingRoomNumber, and the need for another one that picks a room and actual coordinates.

Let’s just make up a string and deal with it. Then we’ll see about improving the name.

            local def = "Skeleton1;health(5,10);avoid(1)"

And then to test that …

        _:test("Decor from definition string", function()
            local def = "Skeleton1;health(5,10);avoid(1)"
            local parsed = Parse:decorDefinition(def)
            _:expect(parsed.valid).is(true)
            local decorKind = Decor:legalKind(parsed.decor.kind)
            _:expect(decorKind).is("Skeleton1")
            local lootElement = parsed.loot
            local loot = Loot(nil, lootElement.kind, lootElement.min, lootElement.max)
            _:expect(loot:is_a(Loot)).is(true)
            _:expect(loot.kind).is("health")
            _:expect(loot.min).is(5)
            _:expect(loot.max).is(10)
            local builder = GameRunner():defineDungeonBuilder()
            local re = parsed.room
            local tile = builder:findTile(re.kind,re.p[1])
        end)

I’ve created a builder to work on, which troubles me a bit, it’s a big deal, but it should be OK. And notice that I didn’t want to fetch room.min to get the parameter. So we’ll get an error on that first, and then the missing findTile, I think.

3: Decor from definition string -- DungeonBuilder:100: attempt to index a nil value (field 'p')

Change the parsing. I think we’ll just go to parameters in an array for everyone.

function ParseElement:init(kind,min,max)
    self.kind = kind
    self.min = min or 0
    self.max = max or 0
    self.p = {self.min, self.max}
end

That should do it … we’ll just have our ParseElement go both ways.

3: Decor from definition string -- DungeonBuilder:101: attempt to call a nil value (method 'findTile')

Perfect. Now the thing about findTile is that it needs to be prepared for a string and a parameter. For now, the parameter is forced to a number … we know this cannot last forever, but it can sure last for now.

function DungeonBuilder:findTile(kind,number)
    if kind=="avoid" then
        return self:randomRoomTileAvoidingRoom(number)
    else -- default
        return self:randomRoomTileAvoidingRoom(666)
    end
end

I went beyond my brief with the else clause, but in my defense, I had to return something in all cases, didn’t I? I’ll add a test for that ASAP. I expect the test might run now, but it doesn’t assert anything.

3: Decor from definition string -- DungeonAnalyzer:93: attempt to index a number value (local 'roomToAvoid')

Hm, what happened here? Ah. We are supposed to have a room here. Must have called the wrong method. Yes, should be:

function DungeonBuilder:findTile(kind,number)
    if kind=="avoid" then
        return self:randomRoomTileAvoidingRoomNumber(number)
    else -- default
        return self:randomRoomTileAvoidingRoomNumber(666)
    end
end

Test expecting a pass. But no:

3: Decor from definition string -- Tiles:132: attempt to index a nil value (field 'map')

Yes. We just don’t have enough setup for this to work. I think we needed at least dcPrepare. When I do that, the tests do not stop running. I’m going to let them run a bit and see if I can get a stack overflow or something.

No luck. I think Lua sometimes does tail recursion and the bad news with tail recursion is you never get a stack overflow.

Let’s reflect, we have an important topic here.

Reflection

We have a test whose setup has started to get complicated, and so far, we haven’t made it complicated enough to work. This is where, too often, I decide that I can make something work without proper testing and just go ahead. And I do make it work: it’s just that the code has a hole in its testing, and the code is too complicated to test. These are not good things.

Sometimes I’ll create a fake object to let me test. That gets me out of the bind but still leaves me with code that is too complex to be tested in place.

So I don’t want to give up. And so far the test setup isn’t too horrid. I think I’ll try a few prints, to see where we’re going wrong. Maybe the fix will be easy.

I need a commit to come back to. Comment out the nasty bits in the test, so that I can save my little parse change. Commit.

OK, now put the changes back and a few prints.

I think we have no tiles to search. I think we needed to build the level. Yes, now I get a failure:

3: Decor from definition string -- DungeonBuilder:229: attempt to compare number with nil

That should be a simple ordinary everyday as usual mistake. Yes, I need to give it a room count:

        _:test("Decor from definition string", function()
            local def = "Skeleton1;health(5,10);avoid(1)"
            local parsed = Parse:decorDefinition(def)
            _:expect(parsed.valid).is(true)
            local decorKind = Decor:legalKind(parsed.decor.kind)
            _:expect(decorKind).is("Skeleton1")
            local lootElement = parsed.loot
            local loot = Loot(nil, lootElement.kind, lootElement.min, lootElement.max)
            _:expect(loot:is_a(Loot)).is(true)
            _:expect(loot.kind).is("health")
            _:expect(loot.min).is(5)
            _:expect(loot.max).is(10)
            local builder = GameRunner():defineDungeonBuilder()
            builder:buildLevel(5)
            local re = parsed.room
            local tile = builder:findTile(re.kind,re.p[1])
        end)

I think we’re back on track. Didn’t lose too much steam but did lose some. Test expecting it to run now.

Eww. Tests run but we get a flameout:

Dungeon:239: attempt to index a nil value (local 'tile1')
stack traceback:
	Dungeon:239: in function <Dungeon:238>
	(...tail calls...)
	Monsters:114: in function <Monsters:112>
	(...tail calls...)
	MusicPlayer:78: in method 'checkForDanger'
...

I think I need to save and restore some stuff in the test setup. This is getting nasty again, and I was just getting my steam back up. Make it work first, then make it right.

I’m torn. I really want to test this all the way down to finding the tile, even though I’m pretty sure that in a properly set-up dungeon it’ll work. But I’m not at all sure what has gone wrong here, and I’m becoming concerned that it will require lots more setting up and protecting and so on.

A narrow escape! This code works, does not flame out, and the game runs:

        _:test("Decor from definition string", function()
            local def = "Skeleton1;health(5,10);avoid(1)"
            local parsed = Parse:decorDefinition(def)
            _:expect(parsed.valid).is(true)
            local decorKind = Decor:legalKind(parsed.decor.kind)
            _:expect(decorKind).is("Skeleton1")
            local lootElement = parsed.loot
            local loot = Loot(nil, lootElement.kind, lootElement.min, lootElement.max)
            _:expect(loot:is_a(Loot)).is(true)
            _:expect(loot.kind).is("health")
            _:expect(loot.min).is(5)
            _:expect(loot.max).is(10)
            local builder = GameRunner(20,20):defineDungeonBuilder()
            builder:buildTestLevel(5)
            local re = parsed.room
            local tile = builder:findTile(re.kind,re.p[1])
        end)

The changes were to copy another test in this same suite, to give the GameRunner dimensions, and to give the buildLevel some rooms. Now I can check the tile, though there isn’t much to check:

            _:expect(tile:isRoom()).is(true)

This should pass. It loops. Gotta fix that. I know where it is.

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

This loops forever if it can’t find an open room tile.

Let’s give it a finite duration … and then what? I guess for now I’ll assert.

function DungeonAnalyzer:randomRoomTileAvoidingCoordinates(x1,y1,x2,y2)
    local x,y, pos, tile
    local count = 0
    repeat
        pos = self:randomPositionAvoiding(x1,y1, x2,y2, self.tileCountX, self.tileCountY)
        tile = self.finder:getTile(pos)
        count = count + 1
    until tile:isOpenRoom() or count > 1000
    assert(count<1000, "could not find open room tile")
    return tile
end

The failure seems to be randomish. Let’s see. Yes, after ten or more tries:

3: Decor from definition string -- DungeonAnalyzer:89: could not find open room tile

My dungeon is pretty small. Maybe if I make it larger? That seems to make the intermittent failure go away. Of course it might just be more unlikely. But I let my test dungeon go to full size and gave it 10 rooms, so there should be plenty of spaces.

I think my steam is gone. Let’s reflect a bit.

Loss of Steam

Yesterday, I wrote about Ease. I suggested that when we don’t feel ease, style, and grace, there’s something wrong. Here I lost my feeling of ease as soon as it became clear that my test was going to need to have a fully set up dungeon to play with. That’s not easy to roll up offhand, and poof, the steam goes out of my tank, the wind goes out of my sails, my charge pipe gets a crack in it, and the chain comes off my sprocket.

Now, I might have been satisfied to use a Fake to be sure that I was calling randomRoomTileAvoidingRoom, but if I had, the code would have been wrong, because I needed to call ...Number. If I’d done a Fake, I’d quite likely have left that defect in.

Yes, people who like static type checking: Your static type checking language would have caught that problem. A, I don’t have a static type checking language in Codea, and B, I don’t want one anyway. YMMV.

But the code is working as intended now, so I can t least commit: dungeon definition string can include “avoid(1)” to define type of tile needed.

However, this morning’s work has resulted in about five lines of production code, not exactly a world’s record, at least not on the high side.

But remember the motto from the Ease article:

If it’s hard to write, I’m not done yet. Make it easy to write. It’s always possible.

So I’m not done yet. But I am tired, and out of ideas for now. I might be good for pairing with someone else on their problem. I don’t want to work on this one, at least not for a while. And when you don’t want to work on something, my advice is “don’t”.

We’ll hit it again next time.