Untangling the knots. Let’s see if we can clean up the coupling between GameRunner, tile, and their friends.

I’m starting this article Wednesday afternoon, though I expect most of it will be done tomorrow. But I have a few moments and I wanted to try something.

Earlier today, I ran into a bit of trouble creating a new dungeon in the now-removed DungeonMaker class, because every Tile knows its room number. That’s set up rather late on, using Tile:paint, which gets called by the GameRunner, passing in a room number.

The room number is used in just one situation, selecting a random tile that is not in a given room. We use that, for example, to ensure that no monsters are spawned in the first room of the dungeon, where the princess spawns.

So if our purpose is to find a room tile that isn’t in a given room, one way is to have every tile know its room number, in case anyone asks. But at some small expense, we could avoid a given room by telling the functions that avoid rooms to avoid a given rectangle of tiles. And the Room objects know their rectangles. We don’t use them for much of anything, but we certainly could.

I’m thinking that if we made this change, the creation of the dungeon would be just a bit less complicated, and that would be a good thing as we move to try to separate creation from operating the game. I can see two ways to go.

The easy but inefficient way would be to change Room:getRoomNumber to search the list of rooms to see what room the tile’s coordinates are in. There are only a dozen rooms, typically, so it would be pretty quick. A more efficient but perhaps more complicated way would be to change the functions that now try to avoid a room and make them avoid a rectangle instead.

function Dungeon:randomRoomTile(roomToAvoid)
    while true do
        local pos = vec2(math.random(1, self.tileCountX), math.random(1,self.tileCountY))
        local tile = self:getTile(pos)
        if tile:getRoomNumber() ~= roomToAvoid and tile:isOpenRoom() then return tile end
    end
end

If this is the only usage, we can easily push the rectangle down, it’s in the room. So …

function Dungeon:randomRoomTile(roomToAvoid)
    local x1,y1, x2,y2 = 0,0,0,0
    if roomToAvoid then
        x1,y1, x2,y2 = roomToAvoid:corners()
    end
    local x,y
    while true do
        repeat
            x = math.random(1, self.tileCountX)
        until x < x1 or x > x2
        repeat
            y = math.random(1, self.tileCountY)
        until y < y1 or y > y2
        local pos = vec2(x,y)
        local tile = self:getTile(pos)
        if tile:isOpenRoom() then return tile end
    end
end

I’m allowing for someone calling this method with no room, if they don’t care to avoid anything. I get the room’s corners, and roll individual coordinates outside the room bounds. Check for openRoom and done.

This required a change here:

function GameRunner:randomRoomTile(roomNumberToAvoid)
    local room = nil
    if roomNumberToAvoid < #self.rooms then
        room = self.rooms[roomNumberToAvoid]
    end
    return self:getDungeon():randomRoomTile(room)
end

This takes room number and sends the room down to the dungeon. And I had to change this:

function GameRunner:placeWayDown()
    local r1 = self.rooms[1]
    local target = r1:centerTile()
    local tile = self:getDungeon():farawayOpenRoomTile(r1, target)
    self.wayDown = WayDown(tile)
end

function Dungeon:farawayOpenRoomTile(room, target)
    local candidate
    local result = self:randomRoomTile(room)
    for i = 1,20 do
        candidate = self:randomRoomTile(room)
        result = self:furthestFrom(target, result, candidate)
    end
    return result
end

We used to pass in the princess’s tile and imply the room number from that. Can’t do that any more, requiring the above changes.

Now we can go about removing the roomNumber member variable in Tile, but that’s going to break some tests.

function Tile:room(x,y, runner)
    assert(not TileLock, "Attempt to create room tile when locked")
    return Tile(x,y,TileRoom, runner)
end

function Tile:wall(x,y, runner)
    assert(not TileLock, "Attempt to create wall tile when locked")
    return Tile(x,y,TileWall, runner)
end

function Tile:edge(x,y, runner)
    return Tile(x,y,TileEdge, runner)
end

function Tile:init(x,y,kind, runner)
    if runner ~= Runner then
        print(runner, " should be ", Runner)
        error("Invariant Failed: Tile must receive Runner")
    end
    self.position = vec2(x,y)
    self.kind = kind
    self.runner = runner
    self:initDetails()
end

And now all the callers of those creation methods:

function Room:correctTile(x,y)
    return Tile:room(x,y, self.runner)
end

function Room:paint()
    for x = self.x1,self.x2 do
        for y = self.y1,self.y2 do
            self.runner:defineTile(self:correctTile(x,y))
        end
    end
end

This seems to be everyone referring to roomNumber. Now for a million tests to break. Ah, excellent. Just one:

2: Get a random room -- Dungeon:242: attempt to index a number value (local 'roomToAvoid')

        _:test("Get a random room", function()
            local tile = dungeon:randomRoomTile(1)
            _:expect(tile:getRoomNumber()).isnt(1)
            _:expect(tile:isOpenRoom()).is(true)
        end)

I’m just going to delete that test. So sue me. Or tweet me up if I don’t justify it. Tests run. I notice decor in the starting room. Is that supposed to happen? Yes, it is. We create decor in all rooms not numbered 666.

I have looked up on this and it is good. Commit: Tiles no longer have room number.

Let’s retro and then I’m for supper.

Retro

The change we wanted was to get rid of the room number in Tile, which narrows the Tile’s interface a bit and will, I believe, help us as we refactor to tease apart dungeon creation from dungeon operation. Even if it doesn’t, it’s one less member variable to worry about in creating and working with Tiles.

I did remove one test, which checked to see whether we could find a room tile that wasn’t in room 1. That might be the only test for whether that function works., and we did just change it.

OK, now I feel badly. Let’s go back and look at that changed code and see what we can do to test it.

function Dungeon:randomRoomTile(roomToAvoid)
    local x1,y1, x2,y2 = 0,0,0,0
    if roomToAvoid then
        x1,y1, x2,y2 = roomToAvoid:corners()
    end
    local x,y
    while true do
        repeat
            x = math.random(1, self.tileCountX)
        until x < x1 or x > x2
        repeat
            y = math.random(1, self.tileCountY)
        until y < y1 or y > y2
        local pos = vec2(x,y)
        local tile = self:getTile(pos)
        if tile:isOpenRoom() then return tile end
    end
end

OK, I’m going to refactor this method, like this:

function Dungeon:randomRoomTile(roomToAvoid)
    local x1,y1, x2,y2 = 0,0,0,0
    if roomToAvoid then
        x1,y1, x2,y2 = roomToAvoid:corners()
    end
    return self:randomRoomTileAvoiding(x1,y1, x2,y2)
end

function Dungeon:randomRoomTileAvoiding(x1,y1,x2,y2)
    local x,y
    while true do
        repeat
            x = math.random(1, self.tileCountX)
        until x < x1 or x > x2
        repeat
            y = math.random(1, self.tileCountY)
        until y < y1 or y > y2
        local pos = vec2(x,y)
        local tile = self:getTile(pos)
        if tile:isOpenRoom() then return tile end
    end
end

Test this with a run. No actual test yet. Now refactor again, starting here:

function Dungeon:randomRoomTileAvoiding(x1,y1,x2,y2)
    local x,y
    while true do
        repeat
            x = math.random(1, self.tileCountX)
        until x < x1 or x > x2
        repeat
            y = math.random(1, self.tileCountY)
        until y < y1 or y > y2
        local pos = vec2(x,y)
        local tile = self:getTile(pos)
        if tile:isOpenRoom() then return tile end
    end
end

Let’s get a random position:

function Dungeon:randomRoomTileAvoiding(x1,y1,x2,y2)
    local x,y
    while true do
        local pos = self:randomPositionAvoiding(x1,y1, x2,y2)
        local tile = self:getTile(pos)
        if tile:isOpenRoom() then return tile end
    end
end

function Dungeon:randomPositionAvoiding(x1,y1, x2,y2)
    repeat
        x = math.random(1, self.tileCountX)
    until x < x1 or x > x2
    repeat
        y = math.random(1, self.tileCountY)
    until y < y1 or y > y2
    return vec2(x,y)
end

And now we have a function that we can test all we wish.

But, you know, I’d rather pass in those high water marks as well.

function Dungeon:randomRoomTileAvoiding(x1,y1,x2,y2)
    local x,y
    while true do
        local pos = self:randomPositionAvoiding(x1,y1, x2,y2, self.tileCountX, self.tileCounntY)
        local tile = self:getTile(pos)
        if tile:isOpenRoom() then return tile end
    end
end

function Dungeon:randomPositionAvoiding(x1,y1, x2,y2, maxX, maxY)
    repeat
        x = math.random(1, maxX)
    until x < x1 or x > x2
    repeat
        y = math.random(1, maxY)
    until y < y1 or y > y2
    return vec2(x,y)
end

And, you know what, I’d like to avoid that duplication. And I’m glad I looked because I forgot to make x and y local.

function Dungeon:randomPositionAvoiding(x1,y1, x2,y2, maxX, maxY)
    local x,y
    x = self:randomValueAvoiding(x1,x2, maxX)
    y = self:randomValueAvoiding(y1,y2,maxY)
    return vec2(x,y)
end

function Dungeon:randomValueAvoiding(v1,v2,vMax)
    local v
    repeat
        v = math.random(1, vMax)
    until v < v1 or v > v2
end

Nasty typo above, self.tileCounntY. Fixed.

Hm that loops. Revert to here:

function Dungeon:randomPositionAvoiding(x1,y1, x2,y2, maxX, maxY)
local x,y
    repeat
        x = math.random(1, maxX)
    until x < x1 or x > x2
    repeat
        y = math.random(1, maxY)
    until y < y1 or y > y2
    return vec2(x,y)
end

Extract again:

function Dungeon:randomPositionAvoiding(x1,y1, x2,y2, maxX, maxY)
    local x,y
    x = self:randomValueAvoiding(x1,x2,maxX)
    repeat
        y = math.random(1, maxY)
    until y < y1 or y > y2
    return vec2(x,y)
end

function Dungeon:randomValueAvoiding(x1,x2,maxX)
    local x
    repeat
        x = math.random(1, maxX)
    until x < x1 or x > x2
    return x
end

I’m pretty sure that last time I forgot that final return. Test. Works. Use extracted method again:

function Dungeon:randomPositionAvoiding(x1,y1, x2,y2, maxX, maxY)
    local x,y
    x = self:randomValueAvoiding(x1,x2,maxX)
    y = self:randomValueAvoiding(y1,y2,maxY)
    return vec2(x,y)
end

Test. Works. Rename variables:

function Dungeon:randomValueAvoiding(vLow,vHigh,vMax)
    local v
    repeat
        v = math.random(1, vMax)
    until v < vLow or v > vHigh
    return v
end

Test. Works. I’m called to supper, still no test. But all the others run. Commit: refactored randomRoomTile to death to improve testability.

I’ll retro and work on the test tomorrow. Tonight, pasta with Italian sausage and other yummies.

Thursday 0833

So, looking back, I set out to make a small improvement, keeping a room number in each tile of the dungeon. This value was used to avoid putting things in specific rooms, generally room 1, where the player starts out.

The basic scheme was to pass in, not the room number to avoid, but the room’s corners, so that the avoidance code could avoid the rectangle instead. That went fairly smoothly.

I did find it necessary to delete one test that was checking room number in the test. It was also perhaps the only test that actually checked to see if we were really skipping the room requested. During a brief retrospective, I realized that that test was arguably important, and that we didn’t really have any tests to determine whether the right rectangle was being skipped. So I accepted the mission of testing that code.

It wasn’t easy to test. It looked like this:

function Dungeon:randomRoomTile(roomToAvoid)
    local x1,y1, x2,y2 = 0,0,0,0
    if roomToAvoid then
        x1,y1, x2,y2 = roomToAvoid:corners()
    end
    local x,y
    while true do
        repeat
            x = math.random(1, self.tileCountX)
        until x < x1 or x > x2
        repeat
            y = math.random(1, self.tileCountY)
        until y < y1 or y > y2
        local pos = vec2(x,y)
        local tile = self:getTile(pos)
        if tile:isOpenRoom() then return tile end
    end
end

It wasn’t quite that nasty when I started, but it was close. How does one test a method like that? Only with difficulty. Think about how much setup it needs. Yucch. The method does at least these things:

  • Set up coordinates to avoid
  • Compute an x coordinate
  • Loop if it’s not outside the x range
  • Compute a y coordinate
  • Loop if it’s not outside the y range
  • Get the tile at those coordinates
  • Check to see if the tile is OK
  • Loop way back if it isn’t

We prefer methods that either call other methods to get several things done, or do one thing. So we refactored, using Extract Method and a small amount of recasting, down to this:

function Dungeon:randomRoomTile(roomToAvoid)
    local x1,y1, x2,y2 = 0,0,0,0
    if roomToAvoid then
        x1,y1, x2,y2 = roomToAvoid:corners()
    end
    return self:randomRoomTileAvoiding(x1,y1, x2,y2)
end

function Dungeon:randomRoomTileAvoiding(x1,y1,x2,y2)
    local x,y
    while true do
        local pos = self:randomPositionAvoiding(x1,y1, x2,y2, self.tileCountX, self.tileCountY)
        local tile = self:getTile(pos)
        if tile:isOpenRoom() then return tile end
    end
end

function Dungeon:randomPositionAvoiding(x1,y1, x2,y2, maxX, maxY)
    local x,y
    x = self:randomValueAvoiding(x1,x2,maxX)
    y = self:randomValueAvoiding(y1,y2,maxY)
    return vec2(x,y)
end

function Dungeon:randomValueAvoiding(vLow,vHigh,vMax)
    local v
    repeat
        v = math.random(1, vMax)
    until v < vLow or v > vHigh
    return v
end

This is nearly clean. I do see one change I’d like to make this morning, changing this:

function Dungeon:randomRoomTileAvoiding(x1,y1,x2,y2)
    local x,y
    while true do
        local pos = self:randomPositionAvoiding(x1,y1, x2,y2, self.tileCountX, self.tileCountY)
        local tile = self:getTile(pos)
        if tile:isOpenRoom() then return tile end
    end
end

To this:

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

I find that a bit nicer. Now all this stuff about avoiding the room comes down to this one function, called twice:

function Dungeon:randomValueAvoiding(vLow,vHigh,vMax)
    local v
    repeat
        v = math.random(1, vMax)
    until v < vLow or v > vHigh
    return v
end

Now we see that this surely works, and we can clearly write tests for it. However, last night I was wondering whether we really had to try multiple times to find a random number outside an interval. A bit of searching on the Internet found an idea which I plan to adapt here.

Yes, experienced programmers use the internet to look up how to do things. Sometimes we grab what we find. Sometimes we grab the idea and build on it. But we do look. It’s called research. In the olden days, we had to read books and magazines, and keep files of things we wanted to remember. Now the Internet remembers for us.

Anyway …

function Dungeon:randomValueAvoiding(vLow,vHigh,vMax)
    local numberToSkip = vHigh-vLow + 1
    local v = math.random(1, vMax - numberToSkip)
    if v >= vLow then
        v = v + numberToSkip
    end
    return v
end

Now if you’re like me, that code almost obviously works. Plus or minus one. Now we have a good reason to write a test for it. Here goes.

        _:test("randomValueAvoiding", function()
            local r, ok
            for try = 1,100 do
                r = dungeon:randomValueAvoiding(3,5,10)
                ok = (0 < r and r < 3) or (5 < r and r < 11)
                _:expect(ok).is(true)
            end
        end)

That seems like a good test. And it passes. Voila!

Mission Accomplished

We set out to remove the room number concept from the Tile , and it turns out that we removed it from the Dungeon object as well. And we have a reasonable test to assure us that it still all works.

You may be wondering–I know that I was–whether this random number scheme could be extended to avoiding multiple ranges. I convinced myself before rolling out of bed that it can be, with a series of checks and adjustments, one interval at a time. I don’t really want to do it: I think it would involve merging intervals and such. If it became necessary, I might go back to the loop until you find one form.

But that’s for a day that may never come. Today we have a nice little scheme that calls down and down until it basically returns a random number. Nice. We like that.

Does that last function belong on Dungeon?

No, it does not. It’s a generally useful random number function. We have a couple of other general functions, over in Main:

-- Generally Useful Functions

function clamp(lo,val,hi)
    if lo <= hi then
        return math.max(math.min(val,hi), lo)
    else
        return math.max(math.min(val,lo), hi)
    end
end

function sign(x)
    return (x < 0 and -1) or (x == 0 and 0) or (x > 0 and 1)
end

Let’s move it over there.

        _:test("randomValueAvoiding", function()
            local r, ok
            for try = 1,100 do
                r = randomValueAvoiding(3,5,10)
                ok = (0 < r and r < 3) or (5 < r and r < 11)
                _:expect(ok).is(true)
            end
        end)

function Dungeon:randomPositionAvoiding(x1,y1, x2,y2, maxX, maxY)
    local x,y
    x = randomValueAvoiding(x1,x2,maxX)
    y = randomValueAvoiding(y1,y2,maxY)
    return vec2(x,y)
end

function randomValueAvoiding(vLow,vHigh,vMax)
    local numberToSkip = vHigh-vLow + 1
    local v = math.random(1, vMax - numberToSkip)
    if v >= vLow then
        v = v + numberToSkip
    end
    return v
end

All continues to work. I think we’re done here.

Summary

Is there a lot of benefit to these changes? I’d have to say no … but there is some, in that we’ve removed one of the odd things that tie the GameRunner, Dungeon, Tile and other objects into a somewhat nasty tangle.

But we have a bigger picture to consider. We’re starting to work on designed dungeons, layouts that can be fully defined by the human designers (me, with designer hat on (and trust me, it’s a lovely one)), rather than be created randomly.

We ran into this bump in working on designed dungeons. It was the last straw in an attempt to create a DungeonMaker class all in one go. And two things have come out of that bump.

First, we removed the bump. Since it got in the way once already, we can be pretty sure that it would do so again. I like to focus refactoring on code that I’d be working on anyway, when I’m thinking like a product developer. Going off and refactoring random things that aren’t really being worked on is arguably a waste of time when we’re focused on delivering the most capability we can in the shortest time we can. Refactoring things that are in the way, on the other hand, speeds us up, at least if we are intelligent about it.

Of course, in these articles, sometimes I refactor things because I want to write about how we refactor things. I can always justify diverting from the product frame of mind, because the learning and practice frame of mind is more valuable to me.

To some extent, I’d suggest that it’s valuable to everyone, but when you’re working for a living, you have to be judicious.

The second thing to come out of the DungeonMaker spike was a commitment to move in smaller steps. DungeonMaker seemed to be too big a step. It looks like we’ll need to move more incrementally to untangle the mess that is GameRunner and its collaborators.

And we’re not without progress on that task. We have this new formulation of the learning level code:

function GameRunner:createLearningRooms()
    local w = 12
    local h =  8
    local t = {
        {2,2, w,h},
        {15,2, w,h},
        {28,2, w,h},
        {41,2, w,h},
        {2,11, w,h},
        {15,11, w,h},
        {28,11, w,h},
        {41,11, w,h},
        {2,20, w,h},
        {15,20, w,h},
        {28,20, w,h},
        {41,20, w,h},
    }
    self.rooms = self:makeRoomsFromXYWH(t)
end

function GameRunner:makeRoomsFromXYWH(tab)
    return map(tab, function(e)
        return Room(e[1],e[2],e[3],e[4], self, true)
    end)
end

This may look cryptic, but I think it opens an interesting door. It has separated out a tabular representation of some rooms from the code that creates them. When we build a textual language, or, if we were totally bonkers, a graphical tool for defining a dungeon (wouldn’t that be fun?), producing a control table to drive some general building routines will be just the thing to allow us to “compile” a dungeon definition.

All that’s for future days. Today, we have a bit of improvement to our initial cut at designed dungeons. And that makes us happy.

See you next time!

D2.zip