Dungeon 132

There’s a defect! Also, I’d like to do something about creating a monster who leads you to the WayDown. And there’s inventory. But first, the defect.

Some folks call them “bugs”. I do, too, sometimes, but I prefer the term “defect”, because it reminds me that they don’t creep into the code in the dark, through crevices in the firewall. They are put in, one by one, meticulously and carefully, by the programmer, in this case, me. OK, probably not meticulously and carefully, and certainly not on purpose, but I own the responsibility for each and every one.

I do not, however, subscribe to the sometimes popular theory that we should punish programmers for defects. At least not much. Some level of defect insertion is probably inevitable, but I would argue that whatever defect you’re thinking of right now could have been avoided by some change in your team’s behavior, and your own.

Better testing, perhaps. Less pressure from management, perhaps.¹ Improved toolset.² Not so much: try harder. That trick never works.

The Defect

The defect is this: Sometimes, when the player has moved far to the left of the dungeon, there are graphical glitches. There can be duplicate columns of tiles, and I’ve seen flickering objects. I’m sure when that started to happen, and I can guess what might be going on.

I recently changed the game to keep the player always centered. It used to be that she’d stay centered until the edge of the dungeon met the edge of the screen. Then the screen would stop scrolling, and she would walk over that way. I decided to remove that “feature” because I thought I could make better use of the screen space, and because it makes drawing a bit simpler. One of these days, maybe, I’d like to make the game scale properly onto other devices.

There’s no doubt in my mind that that change was the cause of the defect.

My guess, and it’s only a rough one at this point, is that there’s a problem in the code that provides tiles to be drawn. There are a few kinds of tile: Room, Wall, and Edge. As a rule, you never see an Edge tile, because the dungeon is surrounded by Wall tiles.

Edge tiles ought to be plain black or background color, whatever that is, and should therefore make the screen appear empty whenever we’ve brought the edges in toward the center, that is, when we’ve walked close to the dungeon bounds.

I’m not sure why I’d see duplication. It’s hard to reproduce, but when it happens, it looks like this:

The room appears to have space off to the left, but when you try to move, you cannot. And the space there is mysteriously just like the space we’re on. All the tiles are the same. Those columns are duplicated on the screen. We can see on the little map that we are in fact up against the wall of the room. A bit of debugging print tells me that the princess is at coordinates (1,45), that is, in the left-most column of the dungeon.

This troubles me for a new reason. I think rooms should be laid down such that their sides will all be of type Wall. So we should limit the coordinates of rooms to a lower value of 2, and an upper value of one less than whatever the dungeon upper limit is, which I think is 85x64. However, even so, we shouldn’t be seeing the floor repeated, though that is the effect I see on the left (but I’ve not seen it on any of the other three sides).

Let’s look and see what happens when we draw the dungeon tiles.

function GameRunner:drawMap(tiny)
    fill(0)
    stroke(255)
    strokeWidth(1)
    for i,row in ipairs(self.tiles) do
        for j,tile in ipairs(row) do
            tile:draw(tiny)
        end
    end
    self.player:drawExplicit(tiny)
end

Well, there’s your trouble right there. You’re assuming that if you draw all the tiles, you’ll hit every square on the screen. That’s not the case now. We used to only scroll until the leftmost tile was at the far left, and so on. Now we scroll further, and we’re not drawing anything other than tiles.

In the case in hand, we’re at 1,45, so when we draw the column 1 tiles, they’ll go at screen center. Everything else is to the right of that.

But … why is there anything there? If we only draw the tiles starting at the center of the screen, why is there anything left, as it were, on the left? Where are these residual images coming from? Don’t we clear the background before drawing?

It turns out that we do not clear the background. I think that may in part relate to making the CodeaUnit tests show up in big red letters when the tests fail. Be that as it may, we need to clear the background.

function draw()
    background(0)
    --speakCodeaUnitTests()
    if CodeaUnit and CodeaTestsVisible then
        if Console:find("[1-9] Failed") or Console:find("[1-9] Ignored") then
            
            showCodeaUnitTests()
        end
    end
    Runner:draw()
end

I find that black works well, because the drawn background tiles are black.

I literally LOLed, quietly. Browsing to see what color Edge tiles are, I found this:

function Tile:drawLargeSprite(center)
    -- we have to draw something: we don't clear background. Maybe we should.
    local sp = self:getSprite(self:pos(), tiny)
    pushMatrix()
    pushStyle()
    textMode(CENTER)
    translate(center.x,center.y)
    if not self.currentlyVisible then tint(0) end
    sp:draw()
    --[[
    if self.roomNumber then
        fill(255)
        text(tostring(self.roomNumber), 0,0)
    end
    --]]
    popStyle()
    popMatrix()
end

Check that comment at the top. “Maybe we should.”

This tells us something about putting “TODO” in the code. Doesn’t work, at least for this guy.

The tint(0) tells me that black is the color. Fine with me, I’m a Winter. Black is my color.

OK, I think this is mostly done, but I want to deal with the room getting set up in column 1. Let’s commit, though: “Background set to black, fixes repeated tiles on left”.

I found that yesterday’s work wasn’t committed. I must have been even more muzzy than I realized. Anyway, no harm done.

Room Placement

Here’s room creation:

function GameRunner:createRandomRooms(count)
    local r
    self.rooms = {}
    while count > 0 do
        count = count -1
        local timeout = 100
        local placed = false
        while not placed do
            timeout = timeout - 1
            r = Room:random(self.tileCountX,self.tileCountY, 4,13, self)
            if self:hasRoomFor(r) then
                placed = true
                table.insert(self.rooms,r)
                r:paint(#self.rooms)
            elseif timeout <= 0 then
                placed = true
            end
        end
    end
end

Wow look at that nesting! This code could use some improvement. I also wonder why I used a while there rather than a for. Let’s see what Room:random has to say:

function Room:random(spaceX, spaceY, minSize, maxSize, runner)
    local w = math.random(minSize,maxSize)
    local h = math.random(minSize,maxSize)
    local x = math.random(1,spaceX-w)
    local y = math.random(1,spaceY-h)
    return Room(x,y,w,h,runner, false)
end

Well, that’s weird, isn’t it? We allow x and y to be anything from 1 to spaceX-w and spaceY-h, where the space variables are the dungeon size, 85x64. Surely we shouldn’t allow ourselves to place this room’s center at 1,1, which could happen. And … why are we subtracting w and h, not w and h over two (ish)?

Want to bet whether we have any tests for this? We have not.

We probably should be testing this logic, if not the method. Testing the method is tricky, since it makes four calls to math.random, but testing the logic should be relatively easy.

Let’s think about what we want to be true. We want to return a Room. Its width and height should each be from minSize to maxSize, inclusive. Its x and y coordinates should be such that the room’s outer edges are all at least one tile inside the spaceX, spaceY range.

The left edge of the room is at x - … no, wait!

function Room:init(x,y,w,h, runner, paint)
    if paint == nil then paint = true end
    self.x1 = x
    self.y1 = y
    self.x2 = x + w
    self.y2 = y + h
    self.runner = runner
    if paint then
        self:paint()
    end
end

This code is using the room’s lower left corner to place a room of size w,h. We only use the center after this, which comes from this:

function Room:center()
    return (self.x2+self.x1)//2, (self.y2+self.y1)//2
end

function Room:centerPos()
    local x,y = self:center()
    return vec2(x,y)
end

I am not pleased by this, despite that it has been working fine forever. I’m certain that if I change those two random calls to 2,whatever instead of 1, the wall issue will be resolved. And I guess we just use the center to decide where to draw hallways. We don’t use Rooms for much if at all in the course of the game. They’re there for convenience in setting up.

OK, I’m just going to hammer those two variables and leave the rest. Then I’ll rationalize my actions.

function Room:random(spaceX, spaceY, minSize, maxSize, runner)
    local w = math.random(minSize,maxSize)
    local h = math.random(minSize,maxSize)
    local x = math.random(2,spaceX-w) -- leave room for wall
    local y = math.random(2,spaceY-h) -- leave room for wall
    return Room(x,y,w,h,runner, false)
end

But what about the other side? Do we leave room for walls there? I think not.

Suppose we roll a width of 10. Then we roll an x between 2 and 85-10, or 75. If we roll x of 75, which can happen, then the highest floor tile in the room will be 75+10, or 85. That’s screen edge.

No … wait again! We paint the room like this:

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

function Room:correctTile(x,y, roomNumber)
    if false and (x == self.x1 or x == self.x2 or y == self.y1 or y == self.y2) then
        return Tile:wall(x,y, self.runner, roomNumber)
    else
        return Tile:room(x,y, self.runner, roomNumber)
    end
end

We actually set the boundary cells of the room to be walls!

We have to talk about this.

Legacy Code

What we have here is Legacy Code. It’s old, it’s untested, and it’s weird. Worse, we’ve seen strange behavior on the screen, that isn’t consistent with what (we think) the code does. I’m sure I’ve seen rooms with no left-side walls.

If it were not for that false there, the code would put walls all around the room. Instead of removing that, at some point in the past some programmer (I suspect I know which one) conditioned that code with false, which, if you notice it, tells you that rooms are Room tiles all the way to all the edges.

So that tells me, that if I want room for the system to put in the walls, which it now does, I want to origin my rooms at two or higher, and that I want the origin to be one less than the origin plus width or height.

I want the code to be this:

function Room:random(spaceX, spaceY, minSize, maxSize, runner)
    local w = math.random(minSize,maxSize)
    local h = math.random(minSize,maxSize)
    local x = math.random(2,spaceX-w - 1) -- leave room for wall
    local y = math.random(2,spaceY-h - 1) -- leave room for wall
    return Room(x,y,w,h,runner, false)
end

Also I want that correctTile thing fixed or gone.

And I want a test of the logic if not the object.

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

I’ll leave the method. But we need to reflect on all this.

First, though, a test. I start with a really stupid one, but it’s the one I’ve got:

        _:test("Room layout thinking", function()
            local screen = 85
            local minW,maxW = 4,13
            for w = minW,maxW do
                local minX = 1
                local maxX = screen - w
                _:expect(minX>1, "min").is(true)
                _:expect(maxX+w<screen, "max").is(true)
            end
        end)

I can of course see clearly that these expectations are not met. I’m already sure I need to adjust the random room code. But I want a test, even a dumb one.

21: Room layout thinking min -- Actual: false, Expected: true

I’m not sure why I did the loop over sizes, if it’s going to work it’s going to work.

This runs:

        _:test("Rnadom Room layout thinking", function()
            local screen = 85
            local minW,maxW = 4,13
            for w = minW,maxW do
                local minX = 2 -- one won't do
                local maxX = screen - w - 1 -- -w won't do
                _:expect(minX>1, "min").is(true)
                _:expect(maxX+w<screen, "max").is(true)
            end
        end)

Truly a silly test … except that it tells me exactly what I need to do. So I’m confident now with this:

function Room:random(spaceX, spaceY, minSize, maxSize, runner)
    local w = math.random(minSize,maxSize)
    local h = math.random(minSize,maxSize)
    local x = math.random(2,spaceX-w - 1) -- leave room for wall
    local y = math.random(2,spaceY-h - 1) -- leave room for wall
    return Room(x,y,w,h,runner, false)
end

Commit: make sure rooms leave room for walls at screen edge.

Let’s Reflect

What just happened? We had a long-standing issue in the game, namely that rooms at the edge of the screen could show up without walls. It never bothered me, even though I had seen it happening, at least on the left. But when we went after the screen duplication problem, I remembered the issue and looked. Sure enough, it was clear that the Room could use row and column 1, which is incorrect if we’re going to put a wall around it.

The upper values also allowed the room to extend all the way to the edges. I don’t recall so clearly whether I ever noticed it happening or not.

Now I don’t do this particular task well, deciding what values to use to keep x + w inside some range, because I get tangled up in zero-based vs one-based indexing, and subtraction is my second-worst form of arithmetic. So I would generally resort to a print statement somewhere to make it clear as a bell to the hole in my head whether we were out of range or not.

(I could, presumably, do the checks with assumed values of 1 or zero or something, but be that as it may, I am never sure until I draw a picture or something.)

So I wrote a test and made it run. It’s not a good test: it doesn’t even test the real code. I’m not sure just how to test the real code, or how to refactor it so that I can test it without using fake random numbers or looping a million times,

I’m sure we could figure out something. Fact is, I don’t think it’s worth it. I think we have a righteous change and the test did what it was supposed to do, gave me confidence in the change needed.

If you were here, we’d have done better. Without you, well, my best isn’t always the best, just my best.

OK, what else?

What Else?

I’d like to use my pathfinding stuff to make an Entity that will lead us to the WayDown. We’ll need a new monster movement strategy to do that.

And I’d like to follow up on yesterday’s spike about inventory. I don’t have much time left this morning, so we need to do something quick. And I’m not a clever man, so it should be easy.

Let’s begin by changing the Flee button. It presently lays down Health loots to lead the way to the WayDown. Instead of that, let’s spawn a monster with the WayDown strategy and get it to move.

function Player:flee()
    local map = self.runner:mapToWayDown()
    local next = self:getTile()
    while next do
        next = map:nextAfter(next)
        if next then self:markTile(next) end
    end
end

Let’s see what we’d like to say here. How about:

function Player:flee()
    local map = self.runner:mapToWayDown()
    local myTile = self:getTile()
    local monsterTile = map:nextAfter(myTile)
    Monsters:createPathMonster(self.runner, map, monsterTile)
end

The map can be anything, it just happens that we picked a map to the WayDown. So it’s a PathMonster not a WayDownMonster.

How do we usually create Monsters?

Here’s a close example to what we need:

function Monsters:createHangoutMonsters(runner, count, tile)
    local tab = Monster:getHangoutMonsters(runner, count, tile)
    for i,m in ipairs(tab) do
        table.insert(self.table, m)
    end
end

We’ll want this:

function Monsters:createPathMonster(runner, map, tile)
    table.insert(self.table, Monster:getPathMonster(runner, map, tile))
end

So far so good. Never do any work if you can help it: make some other object do it.

function Monster:getPathMonster(runner, map, tile)
    local mtEntry = {name="Poison Frog", level=3, health={4,8}, speed = {2,6}, strength={8,11},
    attackVerbs={"leaps at", "smears poison on", "poisons", "bites"},
    dead=asset.frog_dead, hit=asset.frog_hit,
    moving={asset.frog, asset.frog_leap}}
    local monster = Monster(tile, runner, mtEntry)
    monster:setMovementStrategy(PathMonsterStrategy(monster,map,tile))
end

I decided to make the Path monster a poison frog, since those are the ones who act as guardians.

There is no such movement strategy.

-- PathMonsterStrategy

PathMonsterStrategy = class()

function PathMonsterStrategy:init(monster, map, tile)
    self.monster = monster
    self.map = map
    self.tile = tile
end

function PathMonsterStrategy:execute(dungeon)
    local newTile = self.map:nextAfter(self.tile)
    self.monster:basicMoveToTile(newTile)
end

And …

function Monster:basicMoveToTile(tile)
    self.tile = self.tile:validateMoveTo(self,tile)
end

I suspect this isn’t strong enough. If the path leads through a tile with something that refuses monsters, we may get stuck. We’ll deal with that. Let’s see why this explodes.

When I press Flee, the poison frog appears, but:

Monsters:60: bad argument #1 to 'insert' (table expected, got nil)
stack traceback:
	[C]: in function 'table.insert'
	Monsters:60: in method 'createPathMonster'
	Player:136: in field '?'
	Button:58: in method 'performCommand'
	Button:53: in method 'touched'
	GameRunner:402: in method 'touched'
	Main:42: in function 'touched'

function Monsters:createPathMonster(runner, map, tile)
    table.insert(self.table, Monster:getPathMonster(runner, map, tile))
end

function Monster:getPathMonster(runner, map, tile)
    local mtEntry = {name="Poison Frog", level=3, health={4,8}, speed = {2,6}, strength={8,11},
    attackVerbs={"leaps at", "smears poison on", "poisons", "bites"},
    dead=asset.frog_dead, hit=asset.frog_hit,
    moving={asset.frog, asset.frog_leap}}
    local monster = Monster(tile, runner, mtEntry)
    monster:setMovementStrategy(PathMonsterStrategy(monster,map,tile))
end

Would help if we returned it.

function Monster:getPathMonster(runner, map, tile)
    local mtEntry = {name="Poison Frog", level=3, health={4,8}, speed = {2,6}, strength={8,11},
    attackVerbs={"leaps at", "smears poison on", "poisons", "bites"},
    dead=asset.frog_dead, hit=asset.frog_hit,
    moving={asset.frog, asset.frog_leap}}
    local monster = Monster(tile, runner, mtEntry)
    monster:setMovementStrategy(PathMonsterStrategy(monster,map,tile))
    return monster
end

Well, that was a defect but not the defect. The message says that the table is nil. That is more than a little surprising to me.

Oh. I don’t have the right object. We need to use the Monsters instance that GameRunner has.

I could violate Demeter, or just ask the GameRunner:

function Player:flee()
    local map = self.runner:mapToWayDown()
    local myTile = self:getTile()
    local monsterTile = map:nextAfter(myTile)
    self.runner:createPathMonster(map, monsterTile)
end

function GameRunner:createPathMonster(map, tile)
    self.monsters:createPathMonster(self,map,tile)
end

function Monsters:createPathMonster(runner,map,tile)
    table.insert(self.table, Monster:getPathMonster(runner,map,tile))
end

And now … the poison frog appears, but he does not move.

Something in his execute. Probably getting refused. I’m also a bit concerned because he starts two squares away from me, and I though I had asked for one.

A print tells me that the frog hasn’t been moved, but that it does seem to be asking for the right tile to move to.

A bit more printing tells me that our execute isn’t using the monster’s resulting tile. This fixes it …

function PathMonsterStrategy:execute(dungeon)
    local newTile = self.map:nextAfter(self.tile)
    print("path ", self.tile, newTile)
    self.monster:basicMoveToTile(newTile)
    self.tile = self.monster:getTile()
end

But it also makes me think that the strategy should be just using the monster tile, not its own. And shouldn’t all of the movement strategies be doing that?

One thing at a time.

function PathMonsterStrategy:init(monster, map)
    self.monster = monster
    self.map = map
end

function PathMonsterStrategy:execute(dungeon)
    local newTile = self.map:nextAfter(self.monster:getTile())
    self.monster:basicMoveToTile(newTile)
    self.tile = self.monster:getTile()
end

This works well enough to commit, I think, since the Flee button is just for testing anyway. Commit: Flee creates WayDown PathMonster.

I am sure that the monster can be stymied. In principle, we shouldn’t put the path through any cell it can’t enter, and right now we only check that the tile is a room tile. I think the validation logic will block the monster from moving through an object, so this code is bound to product a blocked monster.

Here’s a quick movie:

We’ll decide what to do about that anon. I have in mind that the real monster for this will be a cloud, so it may be that it can just go anywhere. That should be easy. And given that spikes go in hallways and that they block monsters, it’s possible that enough spikes would guarantee no valid path. We don’t even cater to that possibility in the Map yet.

Better make a sticky note about this. “Path must cross spikes. Kill blocked PathMonster? Fight?”

For now, if a PathMonster gets stuck, you can move around the obstacle and spawn another one.

Let’s sum up.

Summary

The Room object is seriously Legacy Code, but it is isolated and seems to work. We’ve had no real trouble with it, but for the wall issue, which didn’t affect game play. And it helped find the scrolling issue, the missing background.

The background now wipes out the big red tests. I think I’d better fix that. I bash something in and commit: red and yellow tests display properly.

This area needs work, for sure. The tests produce way too much useless information, and they fill up the screen. I keep needing to use a smaller font as I add tests. Not ideal.

I think the Room can be left alone as Legacy Code, as it is just used in setting things up, and all the real work goes on at the level of Tiles. We should take a look and see if we can do better, but it’s a low priority: we rarely pass through the area.

The PathFinding monster is neat and went in easily. We’ll improve him over the next few sessions, and I think he’ll be a fun addition.

That’ll do for a decent day. I hope you have one as well.

D2.zip