More with the Animator. Maybe we can get it plugged in today. That would be nice.

OK, where were we? Our last few tests of Animator class ensured that each Animator instance has entries for “dead”, “hit”, and “moving”, and accepts entries for “attack”, “hide”, and “idle”. The latter three default to use the “moving” entry if they are not provided.

We also have this test for the oneShot call:

        _:test("Create Animator from monster table entry", function()
            local entry = {
                attack={"attack"},
                dead={"dead"},
                hide={"hide01"},
                hit={"hit"},
                idle={"idle"},
                moving={"moving"},
            }
            local animator = Animator(entry)
            animator:oneShot("hide")
            local animation = animator.animation
            _:expect(animation.frames[1]).is("hide01")
        end)

And this implementation:

function Animator:oneShot(name)
    self.animation = Animation:oneShot(self:getAnimation(name))
end

I originally had in mind that Animator would hold a single instance of Animation, and reuse it. Clearly the code above does not do that: it creates a new Animation. And I think I like that.

Animation also implements cycleStep, which is the method that repeats the given animation over and over until told to do something else. Shall I just implement that on Animator, or test for it. Let’s test, because just now, cycleStep is the default step and I think we’d prefer not to use the default. It’s likely to confuse me sooner or later.

I enhance the oneShot test:

        _:test("Create Animator from monster table entry", function()
            local entry = {
                attack={"attack"},
                dead={"dead"},
                hide={"hide01"},
                hit={"hit"},
                idle={"idle"},
                moving={"moving01"},
            }
            local animator = Animator(entry)
            animator:oneShot("hide")
            local animation = animator.animation
            _:expect(animation.frames[1]).is("hide01")
            animator:cycle("moving")
            animation = animator.animation
            _:expect(animation.frames[1]).is("moving01")
        end)

This’ll fail looking for cycle:

3: Create Animator from monster table entry -- TestAnimation:52: attempt to call a nil value (method 'cycle')

We put cycle into Animator, similar to oneShot:

function Animator:cycle(name)
    self.animation = Animation:cycle(self:getAnimation(name))
end

And this will fail because Animation doesn’t have cycle either:

3: Create Animator from monster table entry -- TestAnimation:119: attempt to call a nil value (method 'cycle')

And we do that:

function Animation:cycle(frames)
    return Animation(frames, self.cycleStep)
end

Once I spell “cycle” correctly, that works. I think we are nearly ready to go live with this thing. Let’s commit what we have: Animator / Animation now support cycle and oneShot.

The main thing we’re missing is the timing. Animations have a tween that makes them cycle. Where is that done now? Let’s look. Probably in Monster.

function Monster:init(tile, runner, mtEntry)
    if not MT then self:initMonsterTable() end
    tile:moveObject(self)
    self.runner = runner
    self.mtEntry = mtEntry or self:randomMtEntry()
    self:initAnimations()
    self.level = self.mtEntry.level or 1
    self.facing = self.mtEntry.facing or 1
    self.attackVerbs = self.mtEntry.attackVerbs or {"strikes at"}
    self:initAttributes()
    self.swap = 0
    self.move = 0
    self.attributeSheet = AttributeSheet(self)
    self.deathSound = asset.downloaded.A_Hero_s_Quest.Monster_Die_1
    self.hurtSound = asset.downloaded.A_Hero_s_Quest.Monster_Hit_1
    self.pitch = 1
    self.animationTimes = self.mtEntry.animationTimes or {0.25,0.25}
    self:startAllTimers()
end

function Monster:initAnimations()
    local mtE = self.mtEntry
    self.dead = mtE.dead
    self.hit = mtE.hit
    self.moving = mtE.moving
    self.attack = mtE.attack or self.moving
    self.idle = mtE.idle or self.moving
    self.hide = mtE.hide or self.moving
    self:setAnimation(self.moving)
end

function Monster:startAllTimers()
    self:setAnimationTimer()
end

function Monster:setAnimationTimer()
    if self.animationTimer then tween.stop(self.animationTimer) end
    local times = self.animationTimes
    self.animationTimer = self:setTimer(self.chooseAnimation, times[1], times[2])
end

function Monster:chooseAnimation()
    self.animations:step()
    self:setAnimationTimer()
end

I think we’d like to have all of this handled by Animator. But here’s a topic that may be important–at least I think it is.

Small Steps

As constant readers know, I like to work in very small steps, and I like to release working tested code Very Frequently. Basically every couple of hours, and that might be too slow.

So I’m looking at two ways to go here.

I think many, if not most developers would choose to get Animator and Animation ready to handle everything that Monster is now doing, initialization, timing, and so on. Once the objects are ready, plug them in.

Another approach exists, and today I prefer it.

We could plug Animator in now, making the minimal number of changes necessary to get it working. We’d almost certainly leave the timing in Monster, just changing the calls that Monster makes to its animator. Then, later, be that a minute or an hour or a day or a month later, we move the rest of the functionality over, whatever makes sense.

This second approach has one thing in its favor, and there’s one objection that some folks might raise. I’ll raise it on their behalf.

The good thing is that it would get Animator released and in use. Since we prefer the way it encapsulates information and behavior, that’s a system improvement.

The objection is that we’ll have to change things twice.

Hey, if we do this, we’ll have to go patching around in Monster, all the places it makes animation decisions. Then, when “later” comes, we’ll have to go back to all those places and change them again. Changing once is better than changing twice.

Well, what if changing twice is actually better than changing once. When we “change once”, we make big changes, and because we have more isolated code in our new classes that has never been in use, we’re at risk of surprises during the install when it finally comes. Changing twice means we face the problems independently.

First, we’ll face whatever problems there are in the new objects, and despite our testing, there may be some. Then, later, we’ll face the design problem of moving timing over.

I prefer one thing at a time, and since I see my work, as GeePaw Hill would put it, as changing code, I’m not at all troubled by doing the smallest bits I can imagine.

Anyway, I’m going for it.

The thing will be to replace Monsters animationsvariable with an Animator. Presently it is setting that from time to time.

It’s 0852. I’ll start here:

function Monster:initAnimations()
    local mtE = self.mtEntry
    self.dead = mtE.dead
    self.hit = mtE.hit
    self.moving = mtE.moving
    self.attack = mtE.attack or self.moving
    self.idle = mtE.idle or self.moving
    self.hide = mtE.hide or self.moving
    self:setAnimation(self.moving)
end

That goes like this:

function Monster:initAnimations()
    local mtE = self.mtEntry
    self.animator = Animator(mtEntry)
    self:setAnimation(self.moving)
end

Now we follow our nose to setAnimation:

function Monster:setAnimation(animations)
    self.animations = Animation(animations)
end

That needs to call Animator. The default used to be “cycle”, so:

function Monster:setAnimation(animations)
    self.animator:cycle(animations)
end

Notice that I’m using a new member variable here, animator, both because the name is better and to give me a variable to check to see whether I’m done.

Now we need to deal a bit with the timing.

function Monster:chooseAnimation()
    self.animations:step()
    self:setAnimationTimer()
end

We should defer to our Animator, who doesn’t know how to step, but soon will:

function Monster:chooseAnimation()
    self.animator:step()
    self:setAnimationTimer()
end

function Animator:step()
    self.animation:step()
end

I suspect much of this will work right now, but let’s look for other references in Monster to animations.

function Monster:drawMonster()
    if not self.tile.currentlyVisible then return end
    pushMatrix()
    pushStyle()
    spriteMode(CENTER)
    noTint()
    local center = self.tile:graphicCenter()
    translate(center.x,center.y)
    self:flipTowardPlayer()
    self.animations:draw()
    popStyle()
    popMatrix()
end

We need to tell our Animator to do that, and it has to defer to the current animations:

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

function Animator:draw()
    self.animation:draw()
end

There’s just one more:

function Monster:setDeathAnimation()
    self.animations = Animation:oneShot(self.dead)
end

Easy-peasy:

function Monster:setDeathAnimation()
    self.animator:oneShot(self.dead)
end

I gotta try this to see whether it works. A couple of tests break:

1: Create the monsters -- TestAnimation:109: attempt to index a nil value (local 'mtEntry')

That tells me that I didn’t hook up the mtEntry in initAnimations.

function Monster:initAnimations()
    local mtE = self.mtEntry
    self.animator = Animator(mtEntry)
    self:setAnimation(self.moving)
end

See what happens when you try to save a few characters of typing?

function Monster:initAnimations()
    local mtEntry = self.mtEntry
    self.animator = Animator(mtEntry)
    self:setAnimation(self.moving)
end

TestAnimation:160: attempt to get length of a nil value (field 'frames')
stack traceback:
	TestAnimation:160: in field 'iterate'
	TestAnimation:181: in method 'step'
	TestAnimation:139: in method 'step'
	Monster:236: in field 'callback'
	...in pairs(tweens) do
    c = c + 1
  end
  return c
end

I think I need to rename that tab from TestAnimation to Animation. Anyway:

Ah. I manage to guess what this is. We’ve changed the rules for Animator. You call it with the name of the frames you want, because it has them tucked away. So let’s check our references to monster’s animator.

function Monster:initAnimations()
    local mtEntry = self.mtEntry
    self.animator = Animator(mtEntry)
    self:setAnimation(self.moving)
end

This should say:

function Monster:initAnimations()
    local mtEntry = self.mtEntry
    self.animator = Animator(mtEntry)
    self:setAnimation("moving")
end

Let’s modify setAnimation:

function Monster:setAnimation(animations)
    self.animator:cycle(animations)
end

To:

function Monster:setAnimation(animationName)
    self.animator:cycle(animationName)
end

And the death one too:

function Monster:setDeathAnimation()
    self.animator:oneShot(self.dead)
end

To:

function Monster:setDeathAnimation()
    self.animator:oneShot("dead")
end

Now let’s see.

Same error occurs at run time.

OK. I am confused. And I’m grateful that I didn’t try to do more work on Animator before trying to plug it in. I’ll do a little work to debug this but I’m mindful that I may have to consider reverting and doing it again.

The message is that frames is nil in iterate:

TestAnimation:160: attempt to get length of a nil value (field 'frames')
stack traceback:
	TestAnimation:160: in field 'iterate'
	TestAnimation:181: in method 'step'
	TestAnimation:139: in method 'step'
	Monster:236: in field 'callback'
	...in pairs(tweens) do
    c = c + 1
  end
  return c
end

How can that happen?

function Animation:oneShot(frames)
    return Animation(frames,self.oneShotStep)
end

function Animation:cycle(frames)
    return Animation(frames, self.cycleStep)
end

If can only happen if we don’t give it frames. We’re probably in cycle, though I can’t be sure.

function Animator:cycle(name)
    self.animation = Animation:cycle(self:getAnimation(name))
end

function Animator:getAnimation(name)
    return self.animations[name] or self:defaultAnimation()
end

function Animator:defaultAnimation()
    return self.moving
end

Huh. Looks good to me, but I wrote it. Let’s do some instrumenting.

Ah. I forgot the calls to set animation in the strategy.

function MimicMonsterStrategy:selectMove(range)
    if range > 2 then
        self.monster:setAnimation(self.monster.moving)
        return "basicMoveRandomly"
    else
        self.monster:setAnimation(self.monster.attack)
        return "basicMoveTowardPlayer"
    end
end

That becomes:

function MimicMonsterStrategy:selectMove(range)
    if range > 2 then
        self.monster:setAnimation("moving")
        return "basicMoveRandomly"
    else
        self.monster:setAnimation("attack")
        return "basicMoveTowardPlayer"
    end
end

Now I think we’re probably OK.

Nope:

Monster:318: attempt to index a nil value (field 'moving')
stack traceback:
	Monster:318: in method 'photo'
	AttributeSheet:26: in method 'draw'
	Monster:282: in method 'drawSheet'
	Monster:263: in method 'drawExplicit'
	Monsters:76: in method 'draw'
	GameRunner:216: in method 'drawMap'
	GameRunner:202: in method 'drawLargeMap'
	GameRunner:176: in method 'draw'
	Main:34: in function 'draw'

What the?

Oh too clever:

function Monster:photo()
    if self:isAlive() then
        return self.moving[1]
    else
        return self.dead
    end
end

We want to show the monster in a living form or a dead one. We’ll need to defer that to Animator, but it should be easy enough:

function Monster:photo()
    return self.animator:photo()
end

function Animator:photo()
    return self.animation:photp()
end

function Animation:photo()
    return self.frames[1]
end

Run again, then talk.

LOL. I typed photp. Unlikely to work.

function Animator:photo()
    return self.animation:photo()
end

Game works. Attributes appear. Monster dies. I’ll wander to find a Mimic, just to check it out.

mimic down

Yes! The mimic operates in its normal mode, moves to attack mode, then goes down. Just as we had intended.

Commit: Monster animation uses new Animator object.

It’s 0943. So that took 40 minutes, minus some time spent wandering around the house. About 50 lines changed or written. One line of code and a thousand words, more or less. 2000 in the article so far. That counts code words.

Not bad for an old guy. Now the timing. Are we up for it? Yes, I think we are. Worst case we have to revert.

Here’s how the timing works now:

...
    self.animationTimes = self.mtEntry.animationTimes or {0.25,0.25}
...

function Monster:setAnimationTimer()
    if self.animationTimer then tween.stop(self.animationTimer) end
    local times = self.animationTimes
    self.animationTimer = self:setTimer(self.chooseAnimation, times[1], times[2])
end

function Monster:setTimer(action, time, deltaTime)
    if not self.runner then return end
    local t = time + math.random()*deltaTime
    return tween.delay(t, action, self)
end

function Monster:startAllTimers()
    self:setAnimationTimer()
end

I’m wondering who sends that last one.

function GameRunner:startTimers()
    self.monsters:startAllTimers()
    self.player:startAllTimers()
end

GameRunner won’t start the timers unless they should be running. That makes me think that the check for runner in setTimer is redundant.

So. The Animator should fetch and save away the timing interval:

function Animator:init(mtEntry)
    self.animations = {}
    self.animations.dead = mtEntry.dead or error("need dead animation")
    self.animations.hit  = mtEntry.hit  or error("need hit animation")
    self.animations.moving = mtEntry.moving or error("need moving animation")
    self.animations.attack = mtEntry.attack
    self.animations.hide = mtEntry.hide
    self.animations.idle = mtEntry.idle
    self.animationTimes = mtEntry.animationTimes or { 0.23, 0.27 }
    self.animation = Animation()
end

I put a little variability into the default, so that all the monsters don’t cycle in perfect harmony.

Now let’s start the timer and keep it going.

function Monster:startAllTimers()
    self.animator:startTimer()
end

And ….

function Animator:startTimer()
    if self.timer then tween.stop(self.timer) end
    local times = self.animationTimes
    local time = math.random(times[1],times[2])
    tween.delay(time, self.stepAnimation, self)
end

function Animator:stepAnimation()
    self:step()
    self:startTimer()
end

function Animator:step()
    self.animation:step()
end

Let’s see why this explodes.

Well, it explodes because I can’t call random with two floats. Exploring tells me that those numbers in animationTimes are the minimum time and the maximum deviation after that time. The current code is this:

function Monster:setTimer(action, time, deltaTime)
    if not self.runner then return end
    local t = time + math.random()*deltaTime
    return tween.delay(t, action, self)
end

Let’s duplicate that, but I’m not sure that I like it.

function Animator:startTimer()
    if self.timer then tween.stop(self.timer) end
    local times = self.animationTimes
    local time = times[1] + math.random()*times[2]
    tween.delay(time, self.stepAnimation, self)
end

OK, that’s working. Now we can remove some timer stuff from Monster. These are the moments we live for.

function Monster:chooseAnimation()
    self.animator:step()
    self:setAnimationTimer()
end

function Monster:setAnimationTimer()
    if self.animationTimer then tween.stop(self.animationTimer) end
    local times = self.animationTimes
    self.animationTimer = self:setTimer(self.chooseAnimation, times[1], times[2])
end

function Monster:setTimer(action, time, deltaTime)
    if not self.runner then return end
    local t = time + math.random()*deltaTime
    return tween.delay(t, action, self)
end

OK. Tests run, game runs. Commit: Animator handles monster animation timing.

So. Let’s sum up.

Summary

My favorite part of today is this sequence:

function AttributeSheet:draw()
    local m = self.monster
...
    self:drawParchment()
    self:drawPhoto(m:photo())

function Monster:photo()
    return self.animator:photo()
end

function Animator:photo()
    return self.animation:photo()
end

function Animation:photo()
    return self.frames[1]
end

This reminds me of some of the best code we see in Smalltalk. You call a method that calls a method that calls a method that returns a constant … and some good thing is the result.

Of course, people who aren’t used to tiny methods are driven crazy by this, because they too often feel compelled to drill down rather than accept that photo somehow returns a photo and move on to whatever they were doing, nothing to see here, get moving buddy.

The only smart thing I did today was to put the Animator into play before it supported timing, letting it take on the load of displaying the right picture but not doing the timing, and then, when that worked and was committed, putting in the timer-related code.

The overall result of creating the Animator / Animation objects is that we have moved all the responsibility for displaying monsters over into those classes, simplifying the Monster class accordingly, removing about 27 lines and a half-dozen methods or more.

Increased cohesion: Monster does one fewer thing, and the Animation classes handle animation selection and animation display, respectively.

Reduced coupling. Monster has an object to talk to, and rarely talks to it, while the Animator and Animation doo all the lifting.

Righteous changes, in my opinion. Smooth and fairly easy. One glitch when I forgot that the Monster Strategy can change the animation. We might do well to move all the animation decisions over there. We could even do it with the EventBus … that might be sweet.

That will be for another day. Tomorrow? I’m not at all sure what we’ll want to do then. Stop by and find out!

D2.zip