Dungeon 73

A tricky graphics issue. And about those walls. Is there a problem?

Bryan has suggested that the princess should be drawn a bit higher in the tile she’s on, so that she’ll extend a bit into the one above, giving a more 3d appearance to the view. I think he’s right. And you’d think it would be pretty simple.

Here’s the code for drawing the princess:

function Player:draw(tiny)
    local sx,sy
    local dx = 0
    local dy = 10
    pushMatrix()
    pushStyle()
    spriteMode(CENTER)
    local center = self:graphicCenter() + vec2(dx,dy)
    translate(center.x,center.y)
    if not self.alive then tint(0)    end
    sx,sy = self:setForMap(tiny)
    self:drawSprite(sx,sy)
    popStyle()
    popMatrix()
    if not tiny then
        self.attributeSheet:draw()
    end
end

OK, not exactly lovely, but pretty straightforward. and dx and dy are her offsets into the tile. As written, it looks like this:

So we should just be able to move her upward by tweaking dy:

But here’s what happens when I set dy to 30:

She’s clipped. The reason is that tiles are drawn bottom up left to right, and tile contents are drawn when the tile is drawn, so the tile above her is drawn on top of her.

I’ve had good luck with the zLevel feature, which draws things further “forward” or “backward” into the screen. But when we try that, we see this:

Most of the tile above is now blacked out. This is due, I’m told, to a known deficiency in OpenGL, which is that it doesn’t properly handle transparency when using z order. The black area shown is part of the princess’s transparent background. Whether that explanation is accurate or not, this clearly won’t do.

One very messy “solution” would be to draw the tiles from the top down instead of bottom up. That would almost certainly work, but it would be awkward to code and it only solves this problem, not whatever thing next needs to overlap a tile.

It would be possible to go over the whole map once to draw the tiles and then again to drawn the contents. We’ve already seen that drawing is quite slow, and we’re already going over them twice, so going over all the tiles a third time just won’t do.

The only solution I see just now is to have the map drawing ignore contents, and to cache the tiles that have contents, drawing just those in a separate pass. That’s a blatant hack, but when it comes to graphics glitches, sometimes that’s what we need.

Let’s do it.

Caching Tiles with Contents

Here’s our GameRunner draw:

function GameRunner:draw()
    font("Optima-BoldItalic")
    self:drawLargeMap()
    self:drawButtons()
    self:drawTinyMap()
    self:drawMessages()
end

function GameRunner:drawLargeMap()
    pushMatrix()
    self:scaleForLocalMap()
    self:drawMap(false)
    popMatrix()
end

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:draw(tiny)
end

Hm. That commented line at the bottom. That’s there to keep the player from being drawn on the small map. Does that mean that the code already has something special in mind for the player?

A review of her drawing code says no, she’s just drawn the same as anyone else. But it does look as if she could be drawn directly.

What if we did something like this?

1. Change princess draw to do nothing, so that when she’s drawn from tile contents, she ignores the call.
2. Give her a new draw function that can be called separately.
3. Call that function about where that comment is.

That might just work and frankly I think it’s a lot better.

function Player:draw()
    -- ignored
end

function Player:drawExplicit(tiny)
    local sx,sy
    local dx = 0
    local dy = 30
    pushMatrix()
    pushStyle()
    spriteMode(CENTER)
    local center = self:graphicCenter() + vec2(dx,dy)
    translate(center.x,center.y)
    if not self.alive then tint(0)    end
    sx,sy = self:setForMap(tiny)
    self:drawSprite(sx,sy)
    popStyle()
    popMatrix()
    if not tiny then
        self.attributeSheet:draw()
    end
end

Now she shouldn’t draw at all. Well, I don’t get quite what I expected:

AdjustedSprite:16: expect userdata, got nil
stack traceback:
	[C]: in function 'sprite'
	AdjustedSprite:16: in method 'draw'
	Tile:168: in method 'drawSprites'
	Tile:148: in method 'draw'
	GameRunner:146: in method 'drawMap'
	GameRunner:136: in method 'drawLargeMap'
	GameRunner:115: in method 'draw'
	Main:30: in function 'draw'

I don’t think that has anything to do with us. I think there’s a problem in the tile sprite logic.

I backed out my change, and the error didn’t occur. I put the change back, and the error didn’t occur. Let’s see what’s up in the code:

function AdjustedSprite:draw()
    pushMatrix()
    scale(self.scaleVec.x, self.scaleVec.y)
    rotate(self.degrees)
    sprite(self.icon)
    popMatrix()
end

Some entry in the table isn’t getting a value. I think we’ll need to catch the case and log it somehow. Oh. There’s a diagnostic in the WallTiles tab: “Asset ‘Pit_Column_64’ does not exist.” That’s clear enough even for me. I must not have copied that one down. I will do so. Done. Easy.

Now where was I? Oh yes, now the princess doesn’t draw. So let’s draw her explicitly:

function GameRunner:draw()
    font("Optima-BoldItalic")
    self:drawLargeMap()
    self.player:drawExplicit(false)
    self:drawButtons()
    self:drawTinyMap()
    self:drawMessages()
end

Curiously enough, that doesn’t draw anything. Oh. The scaling is surely wrong. She’s probably drawn somewhere out of sight. That’s not the right place.

function GameRunner:drawLargeMap()
    pushMatrix()
    self:scaleForLocalMap()
    self:drawMap(false)
    self.player:drawExplicit(false)
    popMatrix()
end

And here she is, overlapping.

However … notice the key and the chest in that picture. It seems that it would be nice if they were raised up a bit too, especially the chest. If we were to do that, this scheme wouldn’t be useful. We’d have to go to a more robust scheme, like caching and drawing all the contents last.

We’ll leave that for another day, making sure that our product owner knows it might take us an hour to do it. Maybe even two, but I think one. We would probably have to fix up the AdjustedSprite to include the offset, although presently all the various contents are drawn separately. Anyway that’s for another day.

Commit: Princess spans part of tile above.

Now What?

You may recall that I noticed an occasional wall tile that didn’t look right.

That wall tile with the single stone in the lower right. Why was it selected, instead of the tile without that stone? To find out, I need to see all the tiles, even those currently hidden, so that I can see what configuration of tiles is causing that. It could even be “right”.

Found it. F4 should be “Wall_49”. After three attempts to figure out where F4 was in the array, it now looks better.

Commit: fixed misplaced wall tile.

OK, I think that’s nearly right. One more thing. The tiny map now draws in the color of the floor tiles, because it’s drawing the floor tiles. Let’s see what we can do about this.

I’ve created a moderately egregious hack in Tile:drawSprites:

function Tile:drawSprites(tiny)
    local sprites = self:getSprites(self:pos(), tiny)
    local center = self:graphicCenter()
    if tiny then
        if self.kind ~= TileRoom or not self.seen then return end
        pushMatrix()
        pushStyle()
        rectMode(CENTER)
        translate(center.x, center.y)
        fill(255)
        rect(0,0,64,64)
        popStyle()
        popMatrix()
        return
    end
    for i,sp in ipairs(sprites) do
        pushMatrix()
        pushStyle()
        translate(center.x,center.y)
        if i > 1 and self.tintColor.r > 50 then noTint() end
        sp:draw()
        popStyle()
        popMatrix()
    end
end

I found that the scaling and translating was confusing me enough that I couldn’t see quickly how to color the map background white. The reason turns out to be that we draw the entire map, even the edge bits, which are black. So instead I decided to make the map draw a white rectangle for every visible tile of room type. That code does that job. Now let’s commit it and clean it.

Extract Method:

function Tile:drawSprites(tiny)
    local sprites = self:getSprites(self:pos(), tiny)
    local center = self:graphicCenter()
    if tiny then
        self:drawTinySprites()
        return
    end
    for i,sp in ipairs(sprites) do
        pushMatrix()
        pushStyle()
        translate(center.x,center.y)
        if i > 1 and self.tintColor.r > 50 then noTint() end
        sp:draw()
        popStyle()
        popMatrix()
    end
end

function Tile:drawTinySprites()
    if self.kind ~= TileRoom or not self.seen then return end
    pushMatrix()
    pushStyle()
    rectMode(CENTER)
    translate(center.x, center.y)
    fill(255)
    rect(0,0,64,64)
    popStyle()
    popMatrix()
end

Extract another:

function Tile:drawSprites(tiny)
    local sprites = self:getSprites(self:pos(), tiny)
    local center = self:graphicCenter()
    if tiny then
        self:drawTinySprites()
        return
    end
    self:drawLargeSprites()
end

function Tile:drawLargeSprites()
    for i,sp in ipairs(sprites) do
        pushMatrix()
        pushStyle()
        translate(center.x,center.y)
        if i > 1 and self.tintColor.r > 50 then noTint() end
        sp:draw()
        popStyle()
        popMatrix()
    end
end

That return is just a very poor substitute for else:

No, that’s all wrong. Revert.

Go again.

Extract Method. An automated extract would have told me that I needed the parameter. (Probably I could have fixed the previous versions, but let’s not.)

function Tile:drawSprites(tiny)
    local sprites = self:getSprites(self:pos(), tiny)
    local center = self:graphicCenter()
    if tiny then
        drawTinySprites(center)
        return
    end
    for i,sp in ipairs(sprites) do
        pushMatrix()
        pushStyle()
        translate(center.x,center.y)
        if i > 1 and self.tintColor.r > 50 then noTint() end
        sp:draw()
        popStyle()
        popMatrix()
    end
end

function Tile:drawTinySprites(center)
    if self.kind ~= TileRoom or not self.seen then return end
    pushMatrix()
    pushStyle()
    rectMode(CENTER)
    translate(center.x, center.y)
    fill(255)
    rect(0,0,64,64)
    popStyle()
    popMatrix()
end

Move the sprite fetch down,

function Tile:drawSprites(tiny)
    local center = self:graphicCenter()
    if tiny then
        self:drawTinySprites(center)
        return
    end
    local sprites = self:getSprites(self:pos(), tiny)
    for i,sp in ipairs(sprites) do
        pushMatrix()
        pushStyle()
        translate(center.x,center.y)
        if i > 1 and self.tintColor.r > 50 then noTint() end
        sp:draw()
        popStyle()
        popMatrix()
    end
end

Inline Temp.

function Tile:drawSprites(tiny)
    local center = self:graphicCenter()
    if tiny then
        self:drawTinySprites(center)
        return
    end
    for i,sp in ipairs(self:getSprites(self:pos(), tiny)) do
        pushMatrix()
        pushStyle()
        translate(center.x,center.y)
        if i > 1 and self.tintColor.r > 50 then noTint() end
        sp:draw()
        popStyle()
        popMatrix()
    end
end

Replace early return with if-else:

function Tile:drawSprites(tiny)
    local center = self:graphicCenter()
    if tiny then
        self:drawTinySprites(center)
    else
        for i,sp in ipairs(self:getSprites(self:pos(), tiny)) do
            pushMatrix()
            pushStyle()
            translate(center.x,center.y)
            if i > 1 and self.tintColor.r > 50 then noTint() end
            sp:draw()
            popStyle()
            popMatrix()
        end
    end
end

Extract Method.

function Tile:drawSprites(tiny)
    local center = self:graphicCenter()
    if tiny then
        self:drawTinySprites(center)
    else
        self:drawLargeSprites(center)
    end
end

function Tile:drawLargeSprites(center)
    for i,sp in ipairs(self:getSprites(self:pos(), tiny)) do
        pushMatrix()
        pushStyle()
        translate(center.x,center.y)
        if i > 1 and self.tintColor.r > 50 then noTint() end
        sp:draw()
        popStyle()
        popMatrix()
    end
end

Better. I notice that the game has sped up noticeably. Certainly not due to the refactoring. We are skipping a lot of tiles now, however, since we only draw room ones in the tiny map and we used to draw them all.

I wonder what would happen if we did that in the main map? My guess is that the game would get slower as we explored more of the map. Anyway, that’s not our quest.

Our quest, better code, has completed.

Commit: improved drawSprites / tiny map drawing.

The method name drawTinySprites is misleading. How about drawMapCell?

function Tile:drawSprites(tiny)
    local center = self:graphicCenter()
    if tiny then
        self:drawMapCell(center)
    else
        self:drawLargeSprites(center)
    end
end

function Tile:drawMapCell(center)
    if self.kind ~= TileRoom or not self.seen then return end
    pushMatrix()
    pushStyle()
    rectMode(CENTER)
    translate(center.x, center.y)
    fill(255)
    rect(0,0,TileSize,TileSize)
    popStyle()
    popMatrix()
end

I also gave the magic 64 number its proper name, TileSize.

Commit: renaming.

That’ll do for a Sunday morning. Let’s sum up.

Summary

Elevating the princess

We set out to move the princess higher on her tile and found a nearly reasonable way to do that: we draw her separately, after all the tiles are drawn. To do that, we overrode her draw method, and added a new one.

This was arguably a reasonable way to do this, but it only applies to the princess. If we were asked to raise up other sprites, we’d need to go about it in a more robust way.

Mistake in wall tiles

I had seen a wall tile that seemed wrong. I went back to the little Tiler app that I wrote, scrolled through to see the configuration that looked wrong, and sure enough it was. I selected the correct tile, and edited my local copy of the file in the dungeon program.

It took me three tries to get it right, because the file doesn’t include indexes. I’ve changed the generator to show the indexes as comments now:

...
asset.Wall_1, -- 0XEF
asset.Wall_49, -- 0XF0
asset.Wall_73, -- 0XF1
asset.Wall_4, -- 0XF2
asset.Wall_4, -- 0XF3
asset.Wall_49, -- 0XF4
asset.Wall_73, -- 0XF5
asset.Wall_4, -- 0XF6
asset.Wall_4, -- 0XF7
asset.Wall_66, -- 0XF8
...

That will make it easier to do a single patch next time. I’d copy the file over but the raw file has excess info in it to accommodate that the Tiler program doesn’t have access to the dungeon programs data.

Anyway, the Tiler program has shown its worth again … and it’s clearly in danger of needing to be maintained. Recall that I left it pretty crufty on the grounds that it was a one-off thing. So far, it hasn’t needed anything like real maintenance, but it’s a near thing.

Should I improve its factoring in expectation of maintenance? I would always recommend no. If it needs maintenance, improve it then. Otherwise the improvement is clearly premature.

Of course if we expected that the next improvement was going to be an emergency and have to be done quickly, the investment might pay off. As things stand, I never recommend improving code that’s just lying around bothering no one.

Tiny Map

The tiny map had become almost invisible due to the dark tiles we’re using now. I changed it to just draw white squares for room tiles, and not to draw other tiles at all. That has clearly sped up the program substantially: the crawl goes faster.

This tells me that it would be useful to find a way to optimize the drawing of the whole map, since we attempt to draw tiles that are not on screen. I don’t need that optimization, so I’m not going to chase it.

In cleaning up that code, I went down the wrong path and left too many variables unbound in some inner functions. I could have fixed that, but I chose instead to revert and refactor again. It’s nice when you have a commit point only a few minutes and one concept old. I should learn to do that more often.

All in all …

Things went fairly smoothly this morning, with only the usual number of typos.

See you next time!

D2.zip