More refactoring: I want more habitable code to work in. Maybe one tiny feature.

I’m on a quest to make the Tile object more valuable, while–I hope–offering a more narrow interface. There are still methods around the system that are unpacking x’s and y’s, who shouldn’t need to if information and function were better kept together (Cohesion). I’m just kind of following my nose on this process, cleaning up whatever I find.

I do have a feature in mind, a tiny one, that I tried last night on the other iPad. Faithful readers may recall that I tried reversing the ghost monster’s image. The idea was to make him always face toward the player (Princess), thus appearing to be more menacing, or at least aware of what he’s doing.

Faithful readers may recall that scaling him (-1,1) did flip him, but it also moved him one square to the right, because the sprite mode is CORNER. I tried adjusting to CENTER and that worked.

Let’s put that in now, then we’re sure to have a “feature” as part of our releases today.

function Monster:draw()
    pushMatrix()
    pushStyle()
    spriteMode(CORNER)
    local gx,gy = self.tile:graphicCoordinates()
    translate(gx,gy)
    -- scale(-1,1) -- flips him to wrong square
    sprite(self.sprite1, 0,0)
    translate(-gx,-gy)
    popStyle()
    popMatrix()
end

We want to change the mode to CENTER, and flip him only so that he’s facing the player. We’ll want to draw him at the center of the tile. tile:graphicCoordinates() returns the corner, so we’ll want a new Tile function. Let’s try this:

function Monster:draw()
    pushMatrix()
    pushStyle()
    spriteMode(CORNER)
    local gx,gy = self.tile:graphicCenter()
    translate(gx,gy)
    self:flipTowardPlayer()
    sprite(self.sprite1, 0,0)
    translate(-gx,-gy)
    popStyle()
    popMatrix()
end

function Monster:flipTowardPlayer()
    local dx = self.runner:playerDirection(self.tile)
    if dx > 0 then
        scale(-1,1)
    end
end

That flip will either be perfectly right or perfectly wrong. Now we need:

function Tile:graphicCenter()
    return (self:x()-1)*TileSize+TileSize//2,(self:y()-1)*TileSize+TileSize//2
end

I expect this to work. And it does.

monster facing

Commit: monster faces princess.

Refactoring Opportunities

Let’s first see what methods in Tile are still returning separate x and y values. We know that graphicCoordinates and graphicCenter do, but that’s the form the users actually want. We should, however, rename graphicCoordinates to graphicCorner I think.

Commit: rename graphicCoordinates to graphicCorner

That wasn’t too exciting. Checking further in Tile …

I find these:

function Tile:tileCoordinates()
    return self:x(),self:y()
end

function Tile:x()
    return self.position.x
end

function Tile:y()
    return self.position.y
end

Tile uses that first function here:

function Tile:directionTo(aTile)
    local tx,ty = aTile:tileCoordinates()
    return sign(tx-self:x()), sign(ty-self:y())
end

We can readily convert that to use pos:

function Tile:directionTo(aTile)
    local t = aTile:pos()
    return sign(t.x-self:x()), sign(t.y-self:y())
end

That justifies the x() and y() as well. Other uses of tileCoordinates:

function Monster:chooseMove()
    local nx,ny
    local tx,ty = self.tile:tileCoordinates()
    if self.runner:playerDistance(self.tile) <= 10 then
        nx,ny = self:proposeMoveTowardAvatar()
    else
        nx,ny = self:proposeRandomMove()
    end
    self.runner:moveMeIfPossible(self, tx+nx, ty+ny)
    self:setMotionTimer()
end

That one wants to be changed for another purpose, to make the moves instead of asking to make them. We’ll let that sit for a moment and look further:

function Monster:proposeRandomMove()
    local moves = {vec2(-1,0), vec2(0,1), vec2(0,-1), vec2(1,0)}
    local move = moves[math.random(1,4)]
    local tx,ty = self.tile:tileCoordinates()
    return tx + move.x, ty + move.y
end

Here again, I want to make the move, not just propose it. We’ll come back to these. What else is there?

function Player:tileCoordinates()
    return self.tile:tileCoordinates()
end

I am not at all sure that this is used. I see no calls to it above. Let’s remove it and see.

I note that I forgot to set sprite mode to center in Monster:draw:

function Monster:draw()
    pushMatrix()
    pushStyle()
    spriteMode(CENTER)
    local gx,gy = self.tile:graphicCenter()
    translate(gx,gy)
    self:flipTowardPlayer()
    sprite(self.sprite1, 0,0)
    translate(-gx,-gy)
    popStyle()
    popMatrix()
end

Commit: fixed monster centering, removing some uses of tileCoordinates.

Now back to the monster motion. Let’s review how the princess does it:

PlayerSteps = {a=vec2(-1,0), w=vec2(0,1), s=vec2(0,-1), d=vec2(1,0)}

function Player:keyPress(key)
    local step = PlayerSteps[key]
    if step then
        self.tile = self.tile:legalNeighbor(step)
    end
end

The Tile:legalNeighbor just returns the proposed tile if it is legal and the old tile if not:

function Tile:legalNeighbor(aVector)
    local newTile = self:getNeighbor(aVector)
    if newTile:isRoom() then
        return newTile
    else
        return self
    end
end

Let’s rename that argument:

function Tile:legalNeighbor(aStepVector)
    local newTile = self:getNeighbor(aStepVector)
    if newTile:isRoom() then
        return newTile
    else
        return self
    end
end

I think this next bit is a bit tricky, because I have to change these three methods consistently, so that nothing breaks. It’s no big deal to do them all, and I feel sure that I could do it, but it’s a better practice, in my opinion, to do things in smaller bites. First commit: renamed argument. Now I have a clean reversion point if I need it.

Here’s where we start:

function Monster:chooseMove()
    local nx,ny
    local tx,ty = self.tile:tileCoordinates()
    if self.runner:playerDistance(self.tile) <= 10 then
        nx,ny = self:proposeMoveTowardAvatar()
    else
        nx,ny = self:proposeRandomMove()
    end
    self.runner:moveMeIfPossible(self, tx+nx, ty+ny)
    self:setMotionTimer()
end

function Monster:proposeMoveTowardAvatar()
    local dx,dy = self.runner:playerDirection(self.tile)
    if math.random() < 0.5 then
        return 0,dy
    else 
        return dx,0
    end
end

function Monster:proposeRandomMove()
    local moves = {vec2(-1,0), vec2(0,1), vec2(0,-1), vec2(1,0)}
    local move = moves[math.random(1,4)]
    local tx,ty = self.tile:tileCoordinates()
    return tx + move.x, ty + move.y
end

The problem here is that the two branches of the if statement drop through and expect tx and ty to be provided. If I’m to change the two propose methods to “do” methods, they’ll not want to return anything. So let’s “defactor”:

function Monster:chooseMove()
    local nx,ny
    local tx,ty = self.tile:tileCoordinates()
    if self.runner:playerDistance(self.tile) <= 10 then
        nx,ny = self:proposeMoveTowardAvatar()
        self.runner:moveMeIfPossible(self, tx+nx, ty+ny)
    else
        nx,ny = self:proposeRandomMove()
        self.runner:moveMeIfPossible(self, tx+nx, ty+ny)
    end
    self:setMotionTimer()
end

Now I can change one branch at a time. Let’s commit: defactor Monster:chooseMove, moving our save point a bit forward.

Now let’s make the random move just do it:

function Monster:chooseMove()
    local nx,ny
    local tx,ty = self.tile:tileCoordinates()
    if self.runner:playerDistance(self.tile) <= 10 then
        nx,ny = self:proposeMoveTowardAvatar()
        self.runner:moveMeIfPossible(self, tx+nx, ty+ny)
    else
        nx,ny = self:makeRandomMove()
    end
    self:setMotionTimer()
end

function Monster:makeRandomMove()
    local moves = {vec2(-1,0), vec2(0,1), vec2(0,-1), vec2(1,0)}
    local move = moves[math.random(1,4)]
    self.tile = self.tile:legalNeighbor(move)
end

I reckon this works. It’s a bit hard to see unless it explodes, because by the time you can see a ghost, it’s following. But I can see on the expanded map that the ghosties are moving. So far so good. Now the other:

function Monster:chooseMove()
    local nx,ny
    if self.runner:playerDistance(self.tile) <= 10 then
        self:moveTowardAvatar()
    else
        self:makeRandomMove()
    end
    self:setMotionTimer()
end

But how should we modify the mover. With just the rename I have this:

function Monster:moveTowardAvatar()
    local dx,dy = self.runner:playerDirection(self.tile)
    if math.random() < 0.5 then
        return 0,dy
    else 
        return dx,0
    end
end

Let’s see if we can get closer:

function Monster:moveTowardAvatar()
    local dx,dy = self.runner:playerDirection(self.tile)
    if math.random() < 0.5 then
        self.tile = self.tile:legalNeighbor(vec2(0,dy))
    else 
        self.tile = self.tile:legalNeighbor(vec2(dx,0))
    end
end

This should work. I owe a commit. Eeek. Let’s test this. All good. Commit: refactoring to make monsters use direct movement to legal neighbor.

There are now no calls to tileCoordinates. Remove it, commit: removed tileCoordinates method. We still have the x,y thing going on here though:

function Monster:moveTowardAvatar()
    local dx,dy = self.runner:playerDirection(self.tile)
    if math.random() < 0.5 then
        self.tile = self.tile:legalNeighbor(vec2(0,dy))
    else 
        self.tile = self.tile:legalNeighbor(vec2(dx,0))
    end
end

We’re using that function a couple of times. It can surely return a vector, I’d think. Let’s see how it works:

function GameRunner:playerDirection(aTile)
    return aTile:directionTo(self.player:getTile())
end

function Tile:directionTo(aTile)
    local t = aTile:pos()
    return sign(t.x-self:x()), sign(t.y-self:y())
end

Let’s just make that baby return a vector, and work from there.

function Tile:directionTo(aTile)
    local t = aTile:pos()
    return vec2(sign(t.x-self:x()), sign(t.y-self:y()))
end

Users are:

function Monster:flipTowardPlayer()
    local dx = self.runner:playerDirection(self.tile)
    if dx > 0 then
        scale(-1,1)
    end
end

function Monster:moveTowardAvatar()
    local dx,dy = self.runner:playerDirection(self.tile)
    if math.random() < 0.5 then
        self.tile = self.tile:legalNeighbor(vec2(0,dy))
    else 
        self.tile = self.tile:legalNeighbor(vec2(dx,0))
    end
end

        _:test("player direction", function()
            local dx,dy
            local runner = GameRunner()
            local player = Player:privateFromXY(100,100, runner)
            runner.player = player
            local tile = Tile:edge(101,101)
            dx,dy = runner:playerDirection(tile)
            _:expect(dx).is(-1)
            _:expect(dy).is(-1)
            tile = Tile:edge(98,98)
            dx,dy = runner:playerDirection(tile)
            _:expect(dx).is(1)
            _:expect(dy).is(1)
        end)

We can fix the test this way:

        _:test("player direction", function()
            local dx,dy
            local runner = GameRunner()
            local player = Player:privateFromXY(100,100, runner)
            runner.player = player
            local tile = Tile:edge(101,101)
            dx,dy = runner:playerDirection(tile):unpack()
            _:expect(dx).is(-1)
            _:expect(dy).is(-1)
            tile = Tile:edge(98,98)
            dx,dy = runner:playerDirection(tile):unpack()
            _:expect(dx).is(1)
            _:expect(dy).is(1)
        end)

We can do the same for the usages if we care to, but we can do better:

function Monster:flipTowardPlayer()
    local dir = self.runner:playerDirection(self.tile)
    if dir.x > 0 then
        scale(-1,1)
    end
end

So that’s nice. I should commit but let’s see about this one:

function Monster:moveTowardAvatar()
    local dx,dy = self.runner:playerDirection(self.tile)
    if math.random() < 0.5 then
        self.tile = self.tile:legalNeighbor(vec2(0,dy))
    else 
        self.tile = self.tile:legalNeighbor(vec2(dx,0))
    end
end

We can just unpack here, or do this:

function Monster:moveTowardAvatar()
    local dxdy = self.runner:playerDirection(self.tile)
    if math.random() < 0.5 then
        self.tile = self.tile:legalNeighbor(vec2(0,dxdy.y))
    else 
        self.tile = self.tile:legalNeighbor(vec2(dxdy.x,0))
    end
end

I like that a bit better. Let’s test. And we’re good. Commit: playerDirection returns vec2, not a pair.

Let’s glance at Tile, but I think it’s nearly good:

It uses those x() and y() functions a lot, like here:

function Tile:graphicCorner()
    return (self:x()-1)*TileSize,(self:y()-1)*TileSize
end

What if we were to say that this function returns a vector and if you want it otherwise, unpack it? Let’s first express it with vectors:

function Tile:graphicCorner()
    local result = (self.position - vec2(1,1))*TileSize
    return result:unpack()
end

Then remove the local, which I just used for convenience:

function Tile:graphicCorner()
    return ((self.position - vec2(1,1))*TileSize):unpack()
end

And we can do the other … apparently not. Let’s commit: modified Tile:graphicCorner to use vector calculation.

Let me try the center one again.

function Tile:graphicCenter()
    return (self:x()-1)*TileSize+TileSize//2,(self:y()-1)*TileSize+TileSize//2
end

First cut:

function Tile:graphicCenter()
    local v11 = vec2(1,1)
    local offset = v11*(TileSize//2)
    local center = (self.position - v11)*TileSize
    local result = center + offset
    return result:unpack()
end

This isn’t obviously better, but it might be. We have some duplication:

function Tile:graphicCenter()
    local v11 = vec2(1,1)
    local offset = v11*(TileSize//2)
    local center = (self.position - v11)*TileSize
    local result = center + offset
    return result:unpack()
end

function Tile:graphicCorner()
    return ((self.position - vec2(1,1))*TileSize):unpack()
end

Let’s make these things return a vector and the callers unpack:

function Monster:draw()
    pushMatrix()
    pushStyle()
    spriteMode(CENTER)
    local center = self.tile:graphicCenter()
    translate(center.x,center.y)
    self:flipTowardPlayer()
    sprite(self.sprite1, 0,0)
    translate(-center.x,-center.y)
    popStyle()
    popMatrix()
end

That’ll be OK. And it’s the only use of that function so far.

function Tile:graphicCenter()
    local v11 = vec2(1,1)
    local offset = v11*(TileSize//2)
    local center = (self.position - v11)*TileSize
    local result = center + offset
    return result
end

Commit: Tile:graphicCenter returns vector.

The graphicCorner method is used more. We’ll change them:

function draw()
    pushMatrix()
    if CodeaUnit then showCodeaUnitTests() end
    if DisplayToggle then
        local center = Runner.player:graphicCorner()
        focus(center, 1)
    else
        scale(0.25)
    end
    Runner:draw()
    popMatrix()
end

function focus(center, zoom)
    local LOWX,LOWY = maxScrollValues()
    translate(clamp(LOWX, WIDTH/2-center.x, 0), clamp(LOWY, HEIGHT/2-center.y, 0))
end


function Tile:draw()
    pushMatrix()
    pushStyle()
    spriteMode(CORNER)
    tint(self:getTint())
    local sp = self:getSprite()
    local center = self:graphicCorner()
    sprite(sp, center.x,center.y, self.runner.tileSize)
    popStyle()
    popMatrix()
end

function Player:draw()
    local dx = -2
    local dy = -3
    pushMatrix()
    pushStyle()
    spriteMode(CORNER)
    local center = self:graphicCorner()
    sprite(asset.builtin.Planet_Cute.Character_Princess_Girl,center.x+dx,center.y+dy, 80,136)
    popStyle()
    popMatrix()
end

I think this does it. Those latter two methods might be made a bit nicer as well, but that’s not our mission. Let’s not confuse ourselves (i.e. me).

We’re all good. Commit: graphicCenter and graphicCorner now return vectors.

Are there still any calls to x() and y()? Just two:

function Tile:__tostring()
    return string.format("Tile[%d][%d]: %s", self:x(), self:y(), self.kind)
end

function Tile:directionTo(aTile)
    local t = aTile:pos()
    return vec2(sign(t.x-self:x()), sign(t.y-self:y()))
end

Let’s do this:

function Tile:__tostring()
    return string.format("Tile[%d][%d]: %s", self.position.x, self.position.y, self.kind)
end

And I think we’ll do that in the other as well. Makes it a bit more intricate but lets me remove those two methods.

I do have mixed feelings about this. the rationale for those methods, and pos, was to allow for internal changes to this object. I could have been right then, but my view now is that it’s stable, probably won’t change, and if it does, it is small enough to change without difficulty.

I could be wrong. But I’m gonna do it.

function Tile:directionTo(aTile)
    local t = aTile:pos()
    return vec2(sign(t.x-self.position.x), sign(t.y-self.position.y))
end

We could perhaps push the problem down to the sign function, but that’s for another day.

Now I think there are no references to x() and y().

Ah, I’m wrong. There’s this one:

function GameRunner:setTile(aTile)
    assert(not TileLock, "attempt to set tile while locked")
    self.tiles[aTile:x()][aTile:y()] = aTile
end

That’s a key method, at the core of how the tiles work. We could decode from pos(), which isn’t much worse. I think that reduces clarity a bit, reduces flexibility a bit, but I’d rather get rid of the two methods.

function GameRunner:setTile(aTile)
    assert(not TileLock, "attempt to set tile while locked")
    local pos = aTile:pos()
    self.tiles[pos.x][pos.y] = aTile
end

function GameRunner:setTile(aTile)
    assert(not TileLock, "attempt to set tile while locked")
    local pos = aTile:pos()
    self.tiles[pos.x][pos.y] = aTile
end

This lets me get rid of those two methods. I might be wise to call pos() inside Tile rather than refer directly to position. Let’s go back to that. Done … and commit: removed x and y methods from Tile.

Time for a bite. Then I’ll sum up.

S.U.

A small feature today gives the sole monster type the ability to face toward the player, which increases its menacing factor by at least 25 percent. It was easy to do, although I still managed to do it wrong the first time.

Some simple refactoring allowed us to remove more of the pesky x,y returns from the system, with a net simplification of code in all but one case. That case was this one:

function Tile:graphicCenter()
    local v11 = vec2(1,1)
    local offset = v11*(TileSize//2)
    local center = (self.pos - v11)*TileSize
    local result = center + offset
    return result
end

function Tile:graphicCorner()
    return (self.pos - vec2(1,1))*TileSize
end

The graphicCenter function is a bit long. However, we have one bit of refactoring yet available to us, like this:

function Tile:graphicCenter()
    local v11 = vec2(1,1)
    local offset = v11*(TileSize//2)
    local corner = self:graphicCorner()
    local result = corner + offset
    return result
end

That local variable should have been named corner right along. Now, v11 is used only once, so we can do Inline Variable:

function Tile:graphicCenter()
    local offset = vec2(1,1)*(TileSize//2)
    local corner = self:graphicCorner()
    local result = corner + offset
    return result
end

We apply Inline Variable once more:

function Tile:graphicCenter()
    local offset = vec2(1,1)*(TileSize//2)
    local result = self:graphicCorner() + offset
    return result
end

And once more:

function Tile:graphicCenter()
    local result = self:graphicCorner() + vec2(1,1)*(TileSize//2)
    return result
end

And once more:

function Tile:graphicCenter()
    return self:graphicCorner() + vec2(1,1)*(TileSize//2)
end

Let’s do one more thing: Extract Method:

function Tile:graphicCenter()
    return self:graphicCorner() + self:cornerToCenterOffset()
end

function Tile:cornerToCenterOffset()
    return vec2(1,1)*(TileSize//2)
end

I think that’s a bit better. Commit: refactor Tile:graphicCenter().

So we have simplified Tile substantially. It now never returns am x,y pair, and has a narrower and more consistent interface, so it’s both smaller and simpler. Good stuff.

I’m pretty sure that we’ve removed all the returns of paired x’s and y’s from the whole system, although there still hare uses of them in calling sequences, mostly in creation. Those are probably OK, I think.

It’s time for some more capability in the game, however. By Friday’s Zoom Ensemble, I want to have some cool stuff under my belt to show off.

Here are some things we should look at:

  • Adding more monsters than just pink ghosts;
  • Some kind of battle capability between player and monsters;
  • Some treasures, like weapons, health, and gold;
  • Locks and keys;
  • Scoring;
  • Multiple levels;
  • …

It goes on and on. Our purpose is just to have fun, and to create an extended example of how one does things like this, not to produce a real game. To do that would surely be more work than I want to put in. But for now, it’s fun.

See you next time!

D2.zip