Dungeon 268

What are you trying to prove, Ron?

Thanks for asking. I have two primary purposes with these articles. Well, main purposes: I guess you can’t have two primary things. Be that as it may …

There’s no denying that I do these programming exercises and articles for my own benefit. It’s a pleasure to exercise the meager skills that I have left, and it distracts me from humanity’s apparently intentional damage to itself, and to the world. If things are bringing you down, I suggest something that needs a bit fo concentration may help you clear your mind.

However, I choose what I work on and write about with a somewhat higher purpose. Both the faux and true Agile ways of working require developers to work in small steps, certainly of no larger than iteration / Sprint size, but ideally very much smaller steps. For many, perhaps most developers, this is not a natural way of working, and in fact most of us have been advised to fully plan our entire effort, and design it down to the nubbins, and only then begin programming.

With that background, it can seem impossible to work in the way that modern methods seem to demand. So I do these projects of mine with very little overt up-front planning and design, instead working with what I know, inevitably making mistakes of design and implementation, and yet, somehow surviving with the aid of tests and refactoring.

Recent articles on the Dung program have been working toward a rather large goal, to separate the building of the dungeon from the operation during game play. Now you could argue that separation of building from using is ever so obvious a thing to do, so that only a fool would do such a thing, but as the local fool, I can tell you that with everything on my plate, what I did seemed reasonable at the time.

And what you do in an Agile situation will seem reasonable to you as well. And, unless I miss my guess, sometime in the future of a decision, you’ll realize that another way would have been better. To many teams, there are only two things to do:

1. Live with the pain. Too late now, nothing to be done about it.
2. Request time for a major refactoring. Probably have it refused, go to 1. Possibly get the time, probably find it’s not enough. Possibly tick everyone off. Go to 1.

There is a third way: work incrementally, doing small improvements, aiming in the direction of a desired large change, grabbing time in chunks of a day, a morning, even an hour. Over time, almost any design change can be done that way. I want to demonstrate that, over and over, in various situations. My hope is that some readers will one day spot something that clicks with them, and induces them to give incremental iterative development a real try.

I believe that as we practice these things, our work lives become better, as we will feel more in control of things. In these times–and any times–that’s a good thing.

So that’s what I’m trying to prove. Let’s get to it.

What Shall We Improve Today?

I think we were in the process of looking at calls from DungeonBuilder to Dungeon, to move those building methods out of Dungeon, as that’s our overall desire.

So I’ll search, in DungeonBuilder, for self.dungeon.

The first one still irritates me:

function DungeonBuilder:init(runner, dungeon, levelNumber, playerRoomNumber, numberOfMonsters)
    self.RUNNER = runner
    self.dungeon = dungeon
    self.levelNumber = levelNumber
    self.playerRoomNumber = playerRoomNumber
    self.numberOfMonsters = numberOfMonsters
end

Ideally, DungeonBuilder should create the dungeon rather than be handed one. There are probably all kinds of constraints on what could be handed in, and if someone gave us a dirty dungeon to build on, the result would not likely be quite what we expected.

But we can’t really create the dungeon in the builder so long as there are calls back to the dungeon instance, since a dungeon has a gamerunner and there’s no telling what the thing might do.

Additionally, the Dungeon currently knows things that should really be parameters to the builder:

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.map = nil -- set in createTiles
    self.rooms = Array{}
end

I think we have to leave that situation until we’re sure we have unwired all the connections. So we’re looking for smaller game.

Some other references are there in preparation for the final stages, returning the dungeon from a build call:

function DungeonBuilder:buildLevel(roomCount)
    ...
    return self.dungeon
end

function DungeonBuilder:buildTestLevel(roomCount)
    ...
    return self.dungeon
end

function DungeonBuilder:buildLearningLevel()
    ...
    return self.dungeon
end

Those are fine and as intended. What else?

function DungeonBuilder:connectRooms(rooms)
    local rooms = self:getRooms()
    for i,r in ipairs(rooms) do
        if i > 1 then
            r:connect(self.dungeon, rooms[i-1])
        end
    end
end

This is OK, it’s just that rooms want to know the dungeon in order to connect. Except …

function Room:connect(dungeon, aRoom)
    if math.random(1,2) == 2 then
        self:hvCorridor(dungeon, aRoom)
    else
        self:vhCorridor(dungeon, aRoom)
    end
end

function Room:hvCorridor(dungeon, aRoom)
    local startX,startY = self:center()
    local endX,endY = aRoom:center()
    dungeon:horizontalCorridor(startX,endX,startY)
    dungeon:verticalCorridor(endY,startY,endX)
end

So room is using a dungeon method to draw corridors. What does that look like?

function Dungeon:horizontalCorridor(fromX, toX, y)
    local prev,curr
    local increment = 1
    if fromX > toX then
        increment = -1
    end
    prev = self:privateGetTileXY(fromX,y)
    for x = fromX,toX,increment do
        curr = self:privateGetTileXY(x,y)
        curr = self:setHallAndDoors(prev,curr, "EW")
        prev = curr
    end
end

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

function Dungeon:setHallAndDoors(prevTile,currTile, dir)
    if not currTile:isRoom() then
        currTile:convertToHall()
        if prevTile:isRoom() then
            currTile:setCanBeDoor(dir)
        end
    else -- curr is room, may need to set prev
        if prevTile:isHall() then
            prevTile:setCanBeDoor(dir)
        end
    end
    return currTile
end

It looks to me as if we could move this without too much difficulty. Would it be better to have DungeonBuilder know how to draw a hallway, or to move the capability to Room? I’m inclined to think it could be in Room, since a hallway is a lot like a room, though it does have a different Tile type.

In the fullness of time, we could give the Room the DungeonBuilder instead of the Dungeon, and it could do its Tile access through the Builder. You may wonder why I might want to do that …

It seems to me that I am lazy, and do not always see the future perfectly. So if an object has access to another object via a member variable, I am prone to use any possibly useful methods in that second object, and even to add things to it when it would help the first object. That’s not really ideal, especially when the object you’re using is at a higher level of abstraction than the one you started with. Dungeon is a larger concept than Room, and using it for a low-level idea like getting a tile seems wrong.

In fact, it seems so wrong that I have been asking whether Dungeon itself needs something in between it and the Map. Or whether the Map needs to be more helpful.

I’m speculating too far into the mists. Let’s see whether we can move the hallway code over into Room, that should be a step forward.

Regroup and drill in:

Moving Hallways to Room

This seems like it’s going to be a bit tricky, so I will go over what I just looked at again, this time thinking more seriously about how to move the code.

We want to change this:

function Room:hvCorridor(dungeon, aRoom)
    local startX,startY = self:center()
    local endX,endY = aRoom:center()
    dungeon:horizontalCorridor(startX,endX,startY)
    dungeon:verticalCorridor(endY,startY,endX)
end

Let’s change it to say what we want, but to do it the old way:

function Room:hvCorridor(dungeon, aRoom)
    local startX,startY = self:center()
    local endX,endY = aRoom:center()
    self:horizontalCorridor(dungeon,startX,endX,startY)
    dungeon:verticalCorridor(endY,startY,endX)
end

function Room:horizontalCorridor(dungeon, startX,endX,startY)
    dungeon:horizontalCorridor(startX,endX,startY)
end

We’ve just made a place to stand. Tests should run, if I haven’t made some silly mistake. I’ll test, because, well, often I make silly mistakes. In this case, all good. Commit: make Room:horizontalCorridor method, in preparation for moving it from dungeon.

Yes a commit for a 5 line change with no behavior change. And now even when I mess up, I have my working scaffold set up. Now …

function Dungeon:horizontalCorridor(fromX, toX, y)
    local prev,curr
    local increment = 1
    if fromX > toX then
        increment = -1
    end
    prev = self:privateGetTileXY(fromX,y)
    for x = fromX,toX,increment do
        curr = self:privateGetTileXY(x,y)
        curr = self:setHallAndDoors(prev,curr, "EW")
        prev = curr
    end
end

function Dungeon:setHallAndDoors(prevTile,currTile, dir)
    if not currTile:isRoom() then
        currTile:convertToHall()
        if prevTile:isRoom() then
            currTile:setCanBeDoor(dir)
        end
    else -- curr is room, may need to set prev
        if prevTile:isHall() then
            prevTile:setCanBeDoor(dir)
        end
    end
    return currTile
end

Sure looks to me as if these can almost move over intact. I’ll copy/paste, in the absence of a refactoring tool up in this iPad.

function Room:horizontalCorridor(dungeon,fromX, toX, y)
    local prev,curr
    local increment = 1
    if fromX > toX then
        increment = -1
    end
    prev = dungeon:privateGetTileXY(fromX,y)
    for x = fromX,toX,increment do
        curr = dungeon:privateGetTileXY(x,y)
        curr = self:setHallAndDoors(prev,curr, "EW")
        prev = curr
    end
end

function Room:setHallAndDoors(prevTile,currTile, dir)
    if not currTile:isRoom() then
        currTile:convertToHall()
        if prevTile:isRoom() then
            currTile:setCanBeDoor(dir)
        end
    else -- curr is room, may need to set prev
        if prevTile:isHall() then
            prevTile:setCanBeDoor(dir)
        end
    end
    return currTile
end

This worked, first time. I don’t even believe it. Let’s see where else the Dungeon method is called, maybe I can remove it. That’s when I’m really sure the right stuff is happening.

Irritatingly, there are two tests calling the method, testing that the function draws the right stuff.

I wonder if I can change the tests like this:

        _:test("Horizontal corridor", function()
            local tile
            local msg
            local dungeon = runner:getDungeon()
            local room = Room(1,1,1,1, dungeon)
            room:horizontalCorridor(dungeon, 5,10, 7)
            tile = runner:getTile(vec2(4,7))
            _:expect(tile:isEdge()).is(true)
            tile = runner:getTile(vec2(11,7))
            _:expect(tile:isEdge()).is(true)
            r,x,y = checkRange(dungeon, 5,7, 10,7, Tile.isFloor)
            msg = string.format("%d %d", x, y)
            _:expect(r,msg).is(true)
        end)

That actually works. Nice. Fix the other one similarly. Tests run. Commit: Room now draws horizontal corridor rather than Dungeon.

Before I could remove the dungeon method, I had to do this:

function Room:vhCorridor(dungeon, aRoom)
    local startX,startY = self:center()
    local endX,endY = aRoom:center()
    dungeon:verticalCorridor(startY,endY,startX)
    self:horizontalCorridor(dungeon, endX,startX,endY) 
end

Now commit: Remove horizontalCorridor method from Dungeon, now done in Room.

Now I reckon we do just the same for vertical. The corresponding changes are easy enough, I only forget to pass in the additional dungeon parm about three times. Tests run, all seems good.

Commit: Room draws vertical corridors. verticalCorridor method removed from Dungeon.

Somehow a half hour passed between Visible commits. And one of the ones I would have sworn I did is not shown. Did I type it and then forget to do it? I usually write the commit message first in the article.

The current commit shows all my changes. I’ll chalk it up to driver error.

Now I think I can remove setHallsAndDoors from Dungeon. I do, everything works. Commit: remove setHallsAndDoors from Dungeon.

Let’s reflect.

RESTrospective

It’s good, when a chunk of work is done, to take a break, reflect, and see what we’ve learned and think about what to do next.

I have to assume that I failed to do at least one commit after claiming that I was doing it. So I’ll change my process. Henceforth, I’ll do the commit, and copy the message from Working Copy and paste it into Scrivener for the article, instead of the other way around. That will ensure that I do every commit I set out to do, and should mean that I won’t forget to do one.

Now I’d like to check and see what use of dungon is made in Room. Did you notice that it’s actually passed in to the hallway methods? That’s really better, passing a helper when it’s needed rather than making it a member variable.

It turns out that when we paint the room and set up the announcements, we need the dungeon.

function Room:init(x,y,w,h, dungeon, paint, announcement)
    assert(dungeon:is_a(Dungeon), "Room requires a dungeon instance")
    if paint == nil then paint = true end
    self.x1 = x
    self.y1 = y
    self.x2 = x + w - 1
    self.y2 = y + h - 1
    self.dungeon = dungeon
    if paint then
        self:paint()
    end
    if announcement then
        self:announce(self.dungeon, announcement)
    end
end

function Room:announce(dungeon, messageTable)
    local tile
    local one = Announcer:cachingOneShot(messageTable)
    for x = self.x1+1, self.x2-1 do
        tile = dungeon:getTile(vec2(x,self.y1))
        tile:addContents(Trigger(one))
        tile = dungeon:getTile(vec2(x,self.y2))
        tile:addContents(Trigger(one))
    end
    for y = self.y1, self.y2 do
        tile = dungeon:getTile(vec2(self.x1,y))
        tile:addContents(Trigger(one))
        tile = dungeon:getTile(vec2(self.x2,y))
        tile:addContents(Trigger(one))
    end
end

function Room:paint()
    for x = self.x1,self.x2 do
        for y = self.y1,self.y2 do
            local existing = self:getDungeon():getTile(vec2(x,y))
            existing:convertToRoom()
        end
    end
end

All we really need is a “tile getter”, some small object that can return the tile from the dungeon, not the entire dungeon. I’ll make a card:

Convert Room to require a TileGetter of some kind rather than a whole Dungeon instance.

That seems like a nice kind of object to have. It might just be a Map, or maybe a small object on top of a Map. For now, I’ll leave it open.

And I think that’ll do for the day.

Summary

We moved some key functionality, corridors, from Dungeon to Room, for a net reduction in Dungeon’s size, and a reduction of its responsibilities in building.

Good things. Small steps, even if I did forget to commit some of them for a while.