Dungeon 199
Let’s pretend we care about that inefficiency in drawingOrder. Also a momentary loss of momentum.
We have this nifty little function that allows us to draw dungeon contents in low-to-high order:
function DungeonContentsCollection:drawingOrder()
local order = {} -- maps tile to array sorted by object level.
for object,tile in pairs(self.contentMap) do
local ord = order[tile] or {}
table.insert(ord,object)
if #ord > 1 then
table.sort(ord, function(a,b) return a:zLevel() < b:zLevel() end)
end
order[tile] = ord
end
return order
end
This function makes a table with as many elements as there are occupied tiles in the dungeon, probably around 50, and each table has an inner table of the objects on that tile, sorted in increasing z-level order. Most of the inner tables only have one element.
This function is used in every draw cycle, so it’s being called 120, or 60, times per second, depending on how fast your iPad is. That’s a lot.
Let’s pretend that we care. We don’t, because the game runs just fine, but in some other world, this could be too much. So let’s make it more efficient.
I Used To Have One Idea
Up until a few moments ago, I had a really nice idea about this problem. The drawingOrder collection changes when we move something in or out of it, and I have figured out a really neat way to keep two collections, the object->tile one that we have now, and the drawing order one, and update the drawing order incrementally when someone changes the collection. Something like:
- If an incoming item is already in the o->t collection, remove that t from the order collection;
- Take any new o->t1 and find the order entry for that t1, and insert the new o into its collection;
I had figured out a really nice insertion loop, in my head, and if it turned out as nice as my head version, it would be quite smooth, and it would only update the old subcollection and the new one. Super.
Then I Got Another Idea
Somewhere up near the top of this article, I was thinking about what to write, something like “drawing order has to change whenever the object to tile table changes” and somehow it came to me …
Duh. Let’s just cache the drawingOrder collection, and nil it out whenever we get a change. Our existing code will refresh it, and will only run once, the first time we draw after something moves.
Part of me is thinking “Bah! The other idea was so nice.” Most of me is thinking “Yay! This is easier and surely good enough.”
Thinking. Really useful. Anyway, here goes:
Oh, No!
I just put in the caching stuff and it didn’t work, in a very odd way: all the dungeon items would appear once and then flicker out, until I moved, then everything would pop into visibility for one draw cycle, then gone again.
That was daunting, but I am rather sure I know what I did wrong. Then, however, I reverted, as was appropriate, and explored a bit. I happened across a Chest, bumped it open, and voila! the Health did not appear on top of the Chest as it is supposed to.
I don’t mind telling you that my stomach dropped. The existing drawingOrder is supposed to deal with that. This is the sort of thing that takes the wind out of your sails.
Nothing for it but to set aside the caching and see if there is in fact a problem with Chests and Loots.
Don’t you hate when that happens? I do.
What did we decide the zLevels should be? Maybe they’re not set right. Here’s the info:
- We’re drawing contents on top of tiles, so z level need not accommodate z-level room for tiles.
- Player should be on top. She likes it that way. She gets z-level 10.
- Monsters next: 9.
- Chests go under whatever they contain: make them 1.
- Everything else can probably have any level other than those. We’ll give them level 5, to leave room for change.
Does Chest implement zLevel()? It does not! Yay! Thur’s yer problum raht thur.
function Chest:zLevel()
return 1
end
Why did I think it was working? I remember being all callooh-callay about it. Of course, it was chance. The sort could list them either way.
I wonder if I could write a test for this. If a defect slips through our tests, it’s the wise thing to do.
Here’s my first cut:
_:test("Chest-Loot Order", function()
local dc,ord, entry
dc = DungeonContentsCollection()
local chest = Chest()
dc:moveObjectToTile(666)
ord = dc:drawingOrder()
entry = ord[666]
_:expect(#entry).is(1)
_:expect(entry[1]).is(chest)
end)
That gives me this error:
5: Chest-Loot Order -- Chest:7: attempt to index a nil value (local 'tile')
Ah. We can’t really do this one without real tiles, can we? I’ll move this test over to one with a more robust setup.
I’ve got a setup with the full boat, and this slightly modified test runs so far:
_:test("Chest-Loot Drawing Order", function()
local ord, entry, tile
tile = dungeon:getTile(vec2(9,9))
local chest = Chest(tile,Runner)
DungeonContents:moveObjectToTile(tile)
ord = DungeonContents:drawingOrder()
entry = ord[tile]
_:expect(#entry).is(1)
_:expect(entry[1]).is(chest)
end)
There’s more to do. However, since adding DungeonContents save-restore to the test above, I get this failure:
1: Spikes -- Spikes:97: attempt to index a nil value (global 'DungeonContents')
Following my nose, that’s readily fixed by making sure that the Spikes test gets a DungeonCollection: it was using an unsaved one. Now extend the test we came here to do:
_:test("Chest-Loot Drawing Order", function()
local ord, entry, tile, loot
tile = dungeon:getTile(vec2(9,9))
local chest = Chest(tile,Runner)
DungeonContents:moveObjectToTile(tile)
ord = DungeonContents:drawingOrder()
entry = ord[tile]
_:expect(#entry).is(1)
_:expect(entry[1]).is(chest)
chest:open()
ord = DungeonContents:drawingOrder()
entry = ord[tile]
_:expect(#entry).is(2)
_:expect(entry[1]).is(chest)
loot = entry[2]
_:expect(loot:is_a(Loot)).is(true)
end)
I had to check the loot’s is_a, because the Chest just creates its Loot, and doesn’t return it. We could change that. Let’s do:
function Chest:open()
if self:isOpen() then return end
self.opened = true
self.pic = self.openPic
return Loot(self:getTile(), "Health", 4,10) -- returned for testing
end
Now we can make the test a bit nicer:
_:test("Chest-Loot Drawing Order", function()
local ord, entry, tile, loot
tile = dungeon:getTile(vec2(9,9))
local chest = Chest(tile,Runner)
DungeonContents:moveObjectToTile(tile)
ord = DungeonContents:drawingOrder()
entry = ord[tile]
_:expect(#entry).is(1)
_:expect(entry[1]).is(chest)
loot = chest:open()
ord = DungeonContents:drawingOrder()
entry = ord[tile]
_:expect(#entry).is(2)
_:expect(entry[1]).is(chest)
_:expect(entry[2]).is(loot)
end)
That’s nice. This test will also serve to test the caching, since if we don’t flush the cache, it’ll fail with only one item in the second phase.
Now Where Were We?
Oh yes, caching the drawing order.
function DungeonContentsCollection:drawingOrder()
local order = {} -- maps tile to array sorted by object level.
for object,tile in pairs(self.contentMap) do
local ord = order[tile] or {}
table.insert(ord,object)
if #ord > 1 then
table.sort(ord, function(a,b) return a:zLevel() < b:zLevel() end)
end
order[tile] = ord
end
return order
end
Let’s init a variable to hold the table:
function DungeonContentsCollection:init()
self.contentMap = {}
self.orderTable = nil
end
And a method to make it dirty:
function DungeonContentsCollection:dirty()
self.orderTable = nil
end
And use it in the init, and in the other places that dirty it:
function DungeonContentsCollection:init()
self.contentMap = {}
self:dirty()
end
function DungeonContentsCollection:moveObjectToTile(object,tile)
--requires DungeonObject to be defined way forward in tabs.
self.contentMap[object] = tile
self:dirty()
end
function DungeonContentsCollection:remove(object)
self.contentMap[object] = nil
self:dirty()
end
So far so good. Eeek, I forgot to commit that other thing.
Commit: fix defect causing loot not to be on top of chest.
Now the caching. How about this:
function DungeonContentsCollection:drawingOrder()
local order -- maps tile to array sorted by object level.
if not self.orderTable then
order = {}
for object,tile in pairs(self.contentMap) do
local ord = order[tile] or {}
table.insert(ord,object)
if #ord > 1 then
table.sort(ord, function(a,b) return a:zLevel() < b:zLevel() end)
end
order[tile] = ord
end
self.orderTable = order
end
return self.orderTable
end
I think this ought to do it. Test, then explore. Tests run. I open three chests and (of course) the Health is on top. No surprise, we have a solid test for this now.
Commit: drawingOrder collection now cached, only dirtied when something changes to a new tile.
Now let’s see whether we can draw the monsters and player from the drawingOrder collection.
Player first, I think. GameRunner draws this way:
function GameRunner:draw()
font("Optima-BoldItalic")
self:drawLargeMap()
self:drawTinyMapOnTopOfLargeMap()
self:drawMapContents()
self:drawButtons()
self:drawInventory()
self:drawPlayerOnBothMaps()
self:drawMonstersOnSomeMaps()
self:drawMessages()
end
function GameRunner:drawMonstersOnSomeMaps()
pushMatrix()
self:scaleForLocalMap()
self.monsters:draw()
popMatrix()
-- don't show them on the small map for now
end
function GameRunner:drawPlayerOnBothMaps()
pushMatrix()
self:scaleForLocalMap()
self.player:drawExplicit(false)
popMatrix()
pushMatrix()
self:scaleForTinyMap()
self.player:drawExplicit(true)
popMatrix()
end
Hm. Looks like monsters might be easier. Here’s our DC:draw:
function DungeonContentsCollection:draw()
pushMatrix()
pushStyle()
for tile,entries in pairs(self:drawingOrder()) do
if tile:isVisible() then
local gc = tile:graphicCenter()
for i,object in ipairs(entries) do
--zLevel(object:zLevel()) -- breaks visibility??
spriteMode(CENTER)
object:draw(false, gc)
end
end
end
popStyle()
popMatrix()
end
As I look at this I’m wondering where the scaling is coming from. Ah:
function GameRunner:drawMapContents()
pushMatrix()
self:scaleForLocalMap()
DungeonContents:draw()
popMatrix()
end
This is good as far as it goes. Let’s see what Monsters do.
I’m thinking about how we’ll condition drawing on some maps and not others, so that all drawing can pass through here.
function Monster:draw()
-- ignored
end
function Monster:drawExplicit()
local r,g,b,a = tint()
if r==0 and g==0 and b==0 then return end
self:drawMonster()
self.behavior.drawSheet(self)
end
function Monster:drawMonster()
if not self:getTile().currentlyVisible then return end
pushMatrix()
pushStyle()
spriteMode(CENTER)
noTint()
local center = self:getTile():graphicCenter()
translate(center.x,center.y)
self.behavior.flip(self)
self.animator:draw()
popStyle()
popMatrix()
end
This is a bit intricate, isn’t it? We ignore the regular draw code, because gameRunner calls drawExplicit, allowing the contents loop not to draw us.
Let’s see what happens if we do two things
- Call
drawExplicitfromMonster:draw - Don’t call the monster drawing code in GameRunner.
I expect things will mostly work, but that monsters will show up on the small map. Maybe,
What I see is that monsters work fine on the big map, and do not display on the small one. I’m not sure why they don’t. Oh, well, we never draw them on the small map at all, and we only call drawContents on the large map, which makes sense.
I now have Monster:draw looking like this:
function Monster:draw()
local r,g,b,a = tint()
if r==0 and g==0 and b==0 then return end
self:drawMonster()
self.behavior.drawSheet(self)
end
I early-outed the drawExplicit.
I think I can nuke the tint stuff. It’s there to keep monsters from appearing outside the lighted area. I think that’s no longer needed. Seems to work. Vestige of previous implementation.
Remove drawExplicit and the reference. That’s here:
function Monsters:draw(tiny)
if not tiny then
for i,m in ipairs(self.table) do
m:drawExplicit()
end
end
end
We can remove this and the reference:
function GameRunner:drawMonstersOnSomeMaps()
pushMatrix()
self:scaleForLocalMap()
self.monsters:draw()
popMatrix()
-- don't show them on the small map for now
end
And the call to that, of course.
Commit: Monsters now drawn with other dungeon objects.
I’m wondering now why we need the Monsters class at all. A quick check reminds me that we use the class to create random monsters, and to move them when it’s their turn. OK, stand down.
Now let’s see about drawing the princess. How does that work? Similarly to the Monsters, I imagine:
function GameRunner:drawPlayerOnBothMaps()
pushMatrix()
self:scaleForLocalMap()
self.player:drawExplicit(false)
popMatrix()
pushMatrix()
self:scaleForTinyMap()
self.player:drawExplicit(true)
popMatrix()
end
What’s that flag all about?
function Player:drawExplicit(tiny)
OperatingMode:drawInLargeAndSmallScale(tiny,self)
end
function Player:drawInLargeAndSmallScale(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 self:isDead() then tint(0) end
sx,sy = self:setForMap(tiny)
self:drawSprite(sx,sy)
popStyle()
popMatrix()
if not tiny then
self.attributeSheet:draw()
end
end
We use the flag to turn off drawing the attribute sheet. Good idea, we’d otherwise have a tiny one up by the small map. I’m wondering about what OperatingMode does.
Ah, neat.
function PlayerMode:drawInLargeAndSmallScale(tiny, sender)
sender:drawInLargeAndSmallScale(tiny)
end
function DevMode:drawInLargeAndSmallScale(tiny, sender)
if not tiny then
sender:drawInLargeAndSmallScale(tiny)
end
end
This makes sure we draw only once when we have the dev map up. OK. Let’s first change GameRunner to explicitly draw the player on the small map, but not on the large:
function GameRunner:drawPlayerOnBothMaps()
--[[
pushMatrix()
self:scaleForLocalMap()
self.player:drawExplicit(false)
popMatrix()
--]]
pushMatrix()
self:scaleForTinyMap()
self.player:drawExplicit(true)
popMatrix()
end
This will make there be no player shown on the main map.
Now let’s make the Player draw function work by calling drawExplicit:
function Player:draw()
self:drawExplicit(false)
end
This should bring her back. And it does.
Make these changes permanent.
function GameRunner:draw()
font("Optima-BoldItalic")
self:drawLargeMap()
self:drawTinyMapOnTopOfLargeMap()
self:drawMapContents()
self:drawButtons()
self:drawInventory()
self:drawPlayerOnSmallMap()
self:drawMessages()
end
function GameRunner:drawMapContents()
pushMatrix()
self:scaleForLocalMap()
DungeonContents:draw()
popMatrix()
end
function GameRunner:drawPlayerOnSmallMap()
pushMatrix()
self:scaleForTinyMap()
self.player:drawExplicit(true)
popMatrix()
end
Commit: Player now drawn as part of dungeon contents rather than with special code.
Let’s sum up.
Summary: More Collapsing
As we expect–or at least hope for–when we do a design improvement, the system collapses nicely down to a smaller more reasonable shape. Today, we removed the need for monsters to be drawn with special code, and sent the player drawing through the now-standard code for drawing everything in the dungeon. We retained the special code that draws the player on the small map, so that the user can know where they are.
Most of our work with the new DungeonContents object has been to reduce code elsewhere. We did add a few lines to make it cache its drawing order collection, saving literally microseconds. We did that because we could, not because a performance measurement showed it was necessary.
Unnecessary but nice.
My sails luffed for a few minutes, when I discovered that the Loot wasn’t on top of the Chest after all, but that was a trivial fix to return the needed explicit z level. I rather wonder whether it was in there at some point and got lost. But no matter, it’s there now.
Once that turned out to be a trivial problem rather than some conceptual issue with the whole premise of my existence, the wind came back up and we sailed smoothly through the rest of the morning.
I think this is the last article regarding the DungeonContents design improvement, so let’s take a look at the main lessons I’ve learned, and that I offer to you:
- Good tests really matter
- In doing this, I ran into a few problems where my tests weren’t adequate, and the resulting confusion took me more time, and at least once caused me to release something that didn’t work as well as I thought it did.
-
When I had good tests, things went much more smoothly. Writing the tests takes time, but it often saves serious debugging time and, for me at least, always gives a much higher level of confidence, and a lot less tension.
-
I learn this lesson often. Sometimes I wonder if I’m slow or something.
- A design improvement can have pervasive influence
- The good side of this is that when we make a design improvement, all the code around it can take advantage of the improvement, and become smaller, simpler, more clear.
-
The not so good side is that sometimes we have to make pervasive changes. In our case, we didn’t have to do that very often, but we did need to make a change to each class that wanted to take advantage of the new scheme. But see below.
- Incremental change is (almost?) always possible
- There are now over 30 individual commits to Head as part of the DungeonContents design change, and after almost? every single one of them, the game was in good condition to run. If my tests had been better, and I had been a bit more patient, there’d have been no glitches at all.
-
Even so, 30 commits in 3 or 4 days isn’t bad for an old man, and probably 28 or them were perfect, with none having serious issues.
-
I believe that literally any refactoring can be done this incrementally. I imagine there could be some existence proof that one cannot, and I’m sure that if the code gets bad enough, it would take a lot of work to get ready for the refactoring. But I have never seen a design that couldn’t be improved incrementally if one had the will to do it.
-
That’s important, because it is the means whereby we get design improvements done while under pressure to deliver features. See Refactoring - Not on the backlog! for more about that.
Incremental, small steps toward “better”. This is the way.
See you next time, for something almost certainly not completely different.