Dungeon 160
Improving the Animation idea. Moar cohesion, less coupling. At least that’s the plan.
It’s Sunday, 0915. I never know how much time I’ll have to play on Sunday, as we have a standard Sunday brekkers and watch old people tv when the household is all awake. We’ll do our best.
The Animation class has moved some responsibility our of Monster, but there’s still more. The Monster holds on to all the individual animation tables, and we call for those by creating a new instance of Animation that holds a particular table. It would be “better”, I think, if the Animation had an indexed table of named animation tables, and we could trigger a particular table and cycling behavior more symbolically.
I think we’ll move in that direction. One possible outcome is that we’ll find that we have too much responsibility in Animation, and that we’ll split the class. We’ll see.
Let’s get to it.
Giving Animation All the Animations
The most complicated monster entry is the Mimic, which has all the animations we know about and more:
m = {name="Mimic", level=1, health={10,10}, speed={10,10}, strength=10, facing=-1, strategy=MimicMonsterStrategy,
attackVerbs={"bites", "chomps", "gnaws"},
dead={"mdead01","mdead02","mdead03","mdead04","mdead05","mdead06","mdead07","mdead08","mdead09","mdead10"},
hit={"mhurt01", "mhurt02", "mhurt03", "mhurt04", "mhurt05", "mhurt06", "mhurt07", "mhurt08", "mhurt09", "mhurt10"},
moving={"mwalk01", "mwalk02", "mwalk03", "mwalk04", "mwalk05", "mwalk06", "mwalk07", "mwalk08", "mwalk09", "mwalk10"},
attack={"mattack01", "mattack02", "mattack03", "mattack04", "mattack05", "mattack06", "mattack07", "mattack08", "mattack09", "mattack10"},
idle={"midle01", "midle02", "midle03", "midle04", "midle05", "midle06", "midle07", "midle08", "midle09", "midle10"},
hide={"mhide01", "mhide02", "mhide03", "mhide04", "mhide05", "mhide06", "mhide07", "mhide08", "mhide09", "mhide10"},
}
The official list of names is attack, dead, hide, hit, idle, moving.
Presently we initialize like this:
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:setAnimation(animations)
self.animations = Animation(animations)
end
My goal is to set monster.animations just once, with an instance of Animation, and never to replace it. Let’s begin by providing a new Animation creation method, fromMtEntry:
I’m doing to TDD this, because I have some nice tests for Animation and want to keep them useful. In addition, I suspect we’re going to change the object’s protocol, so keeping the tests “alive” will probably be of value. We’ll see.
I start with this much:
_:test("Create from monster table entry", function()
local entry = {
attack={"attack"},
dead={"dead"},
hide={"hide"},
hit={"hit"},
idle={"idle"},
moving={"moving"},
}
local animation = Animation:fromMtEntry(entry)
_:expect(animation.hide).is("hide")
end)
I’ve already made some design “decisions” here. I keep them tentative in my mind, but we need to type something specific. We’ll see as we go forward whether we like these choices, and if we don’t, we’ll improve them.
The creation method is fromMtEntry, as planned. The Animation will only look at the six keywords, so we don’t need to provide anything else. And I’m proposing that the Animation will have a way to get a given animation, and that it will be a method of that name. I think this is a bit odd, and I am open to regretting it.
To make this run, I’ll just code this much:
function Animation:fromMtEntry(entry)
self.animations = {}
self.animations.hide = entry.hide
end
function Animation:hide()
return self.animations.hide
end
That’s enough to show me that the test is wrong, and should be:
...
local animation = Animation:fromMtEntry(entry)
_:expect(animation:hide()).is("hide")
I would like this to run correctly, please.
3: Create from monster table entry -- TestAnimation:49: attempt to index a nil value (local 'animation')
Oh, duh, that’s not how we create the object. More chai, quick, before it’s too late.
function Animation:fromMtEntry(entry)
self.animations = {}
self.animations.hide = entry.hide
return Animation(animations)
end
Now we’ll create the table we want and pass it in to the Animation object. But the Animation object has that other protocol, because it thinks of itself as running just one animation using one particular cycle type.
We have a different idea here. This object is new. I think it’s an Animator, not an Animation.
Change:
_:test("Create from monster table entry", function()
local entry = {
attack={"attack"},
dead={"dead"},
hide={"hide"},
hit={"hit"},
idle={"idle"},
moving={"moving"},
}
local animator = Animator:fromMtEntry(entry)
_:expect(animator:hide()).is("hide")
end)
Coming slowly into my view are now two objects, one that manages the set of animations for a monster, and one that runs one of them. These two objects will collaborate in a way as yet unknown.
So I need a separate class, Animator.
Animator = class()
function Animator:fromMtEntry(entry)
self.animations = {}
self.animations.hide = entry.hide
return Animator(animations)
end
function Animator:hide()
return self.animations.hide
end
I really think this will work. And it does but the test is wrong:
3: Create from monster table entry -- Actual: table: 0x2852711c0, Expected: hide
We do want it to return the table of all the animations. I just happened to build tables with only one, since I didn’t really know what I wanted. I rather wish I had built just the “hide” one.
Test becomes:
_:expect(animator:hide()[1]).is("hide")
That runs. Sweet. However.
How do we want this baby to work when it goes into action? We’ll want to drive it similarly to how we drive the existing Animation class, with a request for an animation, by name, and an interation method. Those are oneShotStep or cycleStep in Animation.
So we’ll want to provide those two bits of information to the Animator, and it will take care of running the animation for us, in a way yet to be discovered.
And I do think of it as “discovery”. Yes, whatever is done, I will have decided and created, but I think of it more as trying things, not randomly but with whatever judgment I may have at a given moment, and then selecting, discovering, the things I like best.
We’ve already changed this design a time or two and we’ll be doing more as we discover more and more about what we like.
So, what might we want to say to the Animator? Let’s see what we’re saying now.
Here’s one example:
function Monster:setDeathAnimation()
self.animations = Animation:oneShot(self.dead)
end
I think we’ll want to say something like this:
function Monster:setDeathAnimation()
self.animator:oneShot("dead")
end
So that drives me to want to create that method on Animator and decide how I’ll know what it did. I am an insider when I write the test, so I can look at my Animator and see if it is set up properly.
_:test("Create from monster table entry", function()
local entry = {
attack={"attack"},
dead={"dead"},
hide={"hide01"},
hit={"hit"},
idle={"idle"},
moving={"moving"},
}
local animator = Animator:fromMtEntry(entry)
animator:oneShot("hide")
local animation = animator.animation
_:expect(animation.frames[1]).is("hide01")
end)
Because I’m going to use the string “hide” as a key into the Animator’s thinking, I changed the entry in the hide table to “hide01”, lest I accidentally get the right answer for the wrong reason.
I am now assuming that Animator will have an instance of Animation. I rip its frames out and inspect the first element. In production code, this would be a violation of the Cheerful Advice of Demeter, but here we’re in the doctor’s office and we can prod around a bit.
Let’s let ourselves be test-driven:
3: Create from monster table entry -- TestAnimation:49: attempt to call a nil value (method 'oneShot')
As I work, I realize that now that Animator is a separate class, I don’t need the factory method (which was also wrong anyway) and replace it:
Here again, the fact that I’m using these classes, right along with creating them, helps me shape them into something I won’t mind using.
_:test("Create 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)
Animator = class()
function Animator:init(mtEntry)
self.animations = {}
self.animations.hide = mtEntry.hide
self.animation = Animation()
end
function Animator:oneShot(name)
self.animation:oneShot(self:getAnimation(name))
end
function Animator:getAnimation(name)
return self.animations[name] or self:defaultAnimation()
end
Now, I’ve changed here how Animation works. I think I’m wrong to do that. Let’s keep creating new ones, and saving them in Animator. That will keep juggling the old and new down a bit.
function Animator:oneShot(name)
self.animation = Animation:oneShot(self:getAnimation(name))
end
We’ll have a bit of an issue with initialization but that can come later.
Test runs. And the game should run as well, since we aren’t yet using Animator. And the game does run. Here’s the test an Animator so far:
_: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)
Animator = class()
function Animator:init(mtEntry)
self.animations = {}
self.animations.hide = mtEntry.hide
self.animation = Animation()
end
function Animator:getAnimation(name)
return self.animations[name] or self:defaultAnimation()
end
function Animator:oneShot(name)
self.animation = Animation:oneShot(self:getAnimation(name))
end
We’ll need a default animation in there before we can let this thing loose, but we are a ways from that. Breakfast is nearly under way, so I’ll need to move quickly and briefly here. Let’s consider the default situation with monsters who don’t have all the fancy animations that Mimic has:
That’s implemented here, though not exactly defined anywhere:
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
Let’s write a test reflecting what we have here. The code above says that we must have dead, hit, and moving. Let’s take that as given.
_:test("Animator defaults", function()
local entry = {
dead={"dead"},
hit={"hit"},
moving={"moving"},
}
local animator = Animator(entry)
_:expect(animator:getAnimation("attack")).is(moving)
_:expect(animator:getAnimation("hide")).is(moving)
_:expect(animator:getAnimation("idle")).is(moving)
end)
I think we may want to diagnose whether any of the big three are missing, but that’s for the next test. Right now it’ll fail:
4: Animator defaults -- TestAnimation:78: attempt to call a nil value (method 'defaultAnimation')
You may recall that I put in a call to that method, even though I had no test for it. I didn’t even mention it, but I did it because as I coded the method, I wondered “what if there isn’t one”.
We can use that. So implement:
function Animator:defaultAnimation()
return self.moving
end
This should fix the test. And it does. Now let’s ensure that we get the big three.
The test for this is one that I’ve not done before with CodeaUnit, to my recollection:
_:test("Animator diagnoses missing dead, hit, moving", function()
local f = function()
Animator({})
end
_:expect(f).throws("need dead animation")
end)
We provide CodeaUnit expect with a function. It calls that function and expects an error with the message “need dead animation”. If it get it, the test passes. And with this code, it does pass:
function Animator:init(mtEntry)
self.animations = {}
self.animations.dead = mtEntry.dead or error("need dead animation")
self.animations.hide = mtEntry.hide
self.animation = Animation()
Forgive me, but I’m going to fill in the correct code for this init now.
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.animation = Animation()
end
However, for my sins, I’ll do all three checks for missing animations.
_:test("Animator diagnoses missing dead", function()
local entry = {
hit={"hit"},
moving={"moving"},
}
local f = function()
Animator(entry)
end
_:expect(f).throws("need dead animation")
end)
_:test("Animator diagnoses missing hit", function()
local entry = {
dead={"dead"},
moving={"moving"},
}
local f = function()
Animator(entry)
end
_:expect(f).throws("need hit animation")
end)
_:test("Animator diagnoses missing moving", function()
local entry = {
dead={"dead"},
hit={"hit"},
}
local f = function()
Animator(entry)
end
_:expect(f).throws("need moving animation")
end)
Tests all run. Game works. Commit: Initial implementation of Animator class.
It’s about time for Sunday festivities, so let’s sum up.
Summary
We’re TDDing a new class, Animator, whose responsibilities will be to hold on to all the animations for a given monster, and to play a desired animation (or sequence, if I have my eventual way), with a desired cycle style, cyclic, one shot, or other styles if we think of them. Possibly reversed, for example. We might use that to have the Mimic go back into hiding mode.
We have done a lot of designing here. We started with the idea of expanding the Animation class, and then discovered that the changes would be substantial and would break existing code before we’re ready. So we went to a design with two classes, Animator and Animation. (I suspect those names are too similar, and maybe we’ll change one or both.)
Because of the initial assumption that all the work would be done by expanding Animation class, we started with a factory method, then “discovered”, i.e. realized, that we didn’t need a factory method yet, there’s probably just one way to build an Animator.
I didn’t keep a list of all the design decisions that got made, and changed, as we did this little bit of code. It might be fun, sometime, to make a note of every single one, if it were possible. But programming is essentially a continuous process of making design decisions, and if we can keep them small, we never get far off track. We make one, decide we don’t like it, and change it, all before there’s much impact at all.
Made small enough and frequently enough, design decisions are almost free. Made large enough, and infrequently, and they become a rigid frame that keeps us from improving the program.
Lets keep them small and frequent.
See you next time!