No promises, just a look at Tile and its friends.

I did some refactoring yesterday after writing D-316, and probably we’ll see the results of that as we go. There were six commits, four just named “tidying”, reflecting a couple of added comments about constraints, but mostly reordering methods. I found an unused method and removed it, and added a new method contentsDo in Tile, and used it in doQueries, which became this:

function Tile:doQueries()
    self:contentsDo(function(obj) obj:query() end)
end

That’s supported by the new contentsDo:

function Tile:contentsDo(f)
    for _,obj in ipairs(self:getContents()) do
        f(obj)
    end
end

I wanted to use the FP functions to do that, but we do not have an apply or do FP function, and map won’t work because you can’t collect nils. Someday, I’ll enhance FP, but yesterday was not that day, and that day may never come.

Tentative Plan

My tentative plan for today is to check to see whether any objects actually hold onto their tile, and to check users of self:currentTile() to see what they’re up to. That might lead to a useful idea on narrowing the breadth of the Tile class, which still has 59 methods after yesterday’s deletion of one.

I’ll start with a search for self.tile, hoping to find few or none. To my delight, there are no occurrences of “self.tile:” or “self.tile “ or “self.tile.”. That makes me fairly certain that no dungeon objects are holding on to their tile. Now we’ll look at self:currentTile(). I’m sure there are a bunch of those.

And there are. I’l show them here, so that you can browse with me. I’m looking for any patterns of usage, just opening my mind to ideas.

function Entity:remove()
    self:currentTile():removeContents(self)
end

This one I like. It seems necessary and is not invasive. I wonder, though, why it is so low in the hierarchy rather than up in DungeonObject. Don’t we remove other objects? So I will divert for a moment and check that out. I think we may have two ways to accomplish the same thing. Loot must get removed, let’s look there.

Sure enough, I find this:

function Loot:actionWith(aPlayer)
    self:currentTile():removeContents(self)
    self.item:addToInventory()
end

Same line of code. Duplication, who knew? Let’s move the remove function up to DungeonObject and use it here.

function Loot:actionWith(aPlayer)
    self:remove()
    self.item:addToInventory()
end

function DungeonObject:remove()
    self:currentTile():removeContents(self)
end

Test. Green. Also tested some loot and decor just to be sure. Commit: move remove function to DungeonObject, use in loot.

Now I’ll look for other removeContents to see if there are other candidates. I find that Key does the same. Fix that. Test. Commit: Change Key to use DO:remove().

Worth it?

You may wonder whether those two changes were worth doing. I think they were. There used to be two textually different ways to remove yourself from the dungeon, although they were equivalent. Now there is just one way and everyone who wants to be removed uses it. In addition, every occurrence of currentTile is a reference to a very broad object, and we’d on a quest to limit those. So let’s get back to it:

function Entity:manhattanDistanceFromTile(tile)
    return self:currentTile():manhattanDistance(tile)
end

I let that one ride. I think when we cover the Tile by calling just one method on it like that, it’s probably OK. And we might be able to return a more narrow instance that only knows the few methods we use that way.

This next one is a bit more irritating:

function Monster:manhattanDistanceFromPlayer()
    return self.runner:playerManhattanDistance(self:currentTile())
end

Let’s see how that’s done.

function GameRunner:playerManhattanDistance(aTile)
    return self:getDungeon():manhattanToPlayer(aTile)
end

function Dungeon:manhattanToPlayer(tile)
    return self:manhattanBetween(tile, self:playerTile())
end

function Dungeon:manhattanBetween(tile1, tile2)
    return tile1:manhattanDistance(tile2)
end

I don’t mind those much, as they are all one-liners, drilling down. We should note, however, that if DungeonObjects knew the Dungeon, the call to the runner could be eliminated. We’re not on that quest just now, and I don’t feel like accepting a side quest. It’s Sunday and will be time for breakfast soon.

So:

function Monster:__tostring()
    return string.format("Monster (%d,%d)", self:currentTile():pos().x,self:currentTile():pos().y)
end

There are a few of these __tostring methods, and I do want them to display their position. They’re not in the main stream, so even though I do have an idea for them, namely moving the duplication part of these up to DungeonObject, I think they’re low priority.

This is interesting:

function Monster:basicMaintainRangeToPlayer(dungeon)
    local tiles = dungeon:availableTilesSameDistanceFromPlayer(self:currentTile())
    self:basicRandomMove(tiles)
end

I was wondering where this method gets its dungeon and the answer is that it ultimately asks the runner, here:

function Entity:getDungeon()
    return self.runner:getDungeon()
end

We can remove a method from GameRunner, I think. Back to this:

function Monster:manhattanDistanceFromPlayer()
    return self.runner:playerManhattanDistance(self:currentTile())
end

function GameRunner:playerManhattanDistance(aTile)
    return self:getDungeon():manhattanToPlayer(aTile)
end

We can inline that:

function Monster:manhattanDistanceFromPlayer()
    return self:getDungeon():manhattanToPlayer(self:currentTile())
end

Test that. We’re good. Commit: Monster gets distance to player from dungeon not thru Runner.

There’s another caller of that method tho:

        _:test("distance from avatar", function()
            local x,y = 100,100
            local player = Player:privateFromXY(104,104, runner)
            runner.player = player
            local tile = runner:getTile(vec2(x,y))
            local d = runner:playerManhattanDistance(tile) -- manhattan
            _:expect(d).is(8)
        end)

So let’s just defer that by fetching the dungeon:

        _:test("distance from avatar", function()
            local x,y = 100,100
            local player = Player:privateFromXY(104,104, runner)
            runner.player = player
            local tile = runner:getTile(vec2(x,y))
            local d = runner:getDungeon():manhattanToPlayer(tile) -- manhattan
            _:expect(d).is(8)
        end)

Now we can remove that method. Commit: remove unused playerManhattanDistance method.

That side quest was valuable as it let us remove a method. So far this morning we have moved a couple of things, removed some lines, and little else. The program is overall just a bit simpler.

Back to currentTile.

There are a bunch of uses in the monster move strategy but they are all just passing the current tile into distance calculations and such. I think they are OK.

Here’s something interesting:

function Monster:drawMonster()
if not self:currentTile():isVisible() then return end
    pushMatrix()
    pushStyle()
    spriteMode(CENTER)
    noTint()
    local center = self:currentTile():graphicCenter()
    translate(center.x,center.y)
    self.behavior.flip(self)
    self.animator:draw()
    popStyle()
    popMatrix()
end

We have two calls to currentTile in the same method. Would this be better?

function Monster:drawMonster()
    local currentTile = self:currentTile()
    if not currentTile:isVisible() then return end
    pushMatrix()
    pushStyle()
    spriteMode(CENTER)
    noTint()
    local center = currentTile:graphicCenter()
    translate(center.x,center.y)
    self.behavior.flip(self)
    self.animator:draw()
    popStyle()
    popMatrix()
end

I’m saying yes, but on another day I might say no. Testl Commit: use local currentTile in drawMonster.

I note the call to currentTile here:

function Monster:xScaleToFaceTowardPlayer()
    local dir = self.runner:playerDirection(self:currentTile())
    return ((dir.x > 0) and self.facing) or -self.facing
end

This is called, ultimately, from inside drawMonster, but it’s a bit of a slog from the self.behavior.flip(self) down to this method. I think we’ll leave that for next time, with a sticky note, and go see about brekkers.

Let’s sum up.

Summary

Today we removed complexity, or, channeling Colin Chapman, we added simplicity. Not much, just a bit.

What we have seen so far is that there are a few special methods, like remove, that reference the current Tile, and those were improved by using that method in all the classes that needed to remove themselves from the Dungeon.

Other uses of the current tile were fairly righteous, and we unwound one reference to the runner because we had a more direct way to get the distance to the player.

There are a raft of methods about selecting useful tiles relative to oneself, and those may be candidates for breaking out of Tile somehow, but it was too much for a Sunday.

Overall, we’ve simplified the system, and done seven commits in less than an hour. A good morning.

And a good morning to you, as well.