current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

So I've been thinking alot about simple/convenient ways to implement simple 2D animation using a bare-bones graphics library I'm using for school (Animation is way out of the scope of the class, I'm just experimenting.)

I thought of using a custom function passed to the class to allow the "user" of the code to just write their own code to do whatever they want to the sprite, then return control to the sprite's Update() function. Here's what I have so far:

// SpriteStuff.h

typedef bool (AnimationStep*)(Sprite* sprite) ANIMATIONSTEP

class Sprite {
    //...
    ANIMATIONSTEP m_currentAnimation;

    void BeginAnimation(ANIMATIONSTEP step);
    void EndAnimation();

    void Update();
    //...
};

void Sprite::BeginAnimation(ANIMATIONSTEP step) {
    if (step != NULL)
        m_currentAnimation = step;
}
void Sprite::EndAnimation() {
    m_currentAnimation = NULL;
}

void Sprite::Update() {
    //...
    if (m_currentAnimation != NULL) 
        if (!m_currentAnimation(this)) EndAnimation();
    //...
}

// (off in some other code...)
void DoAnimateThing() {
    mySprite->BeginAnimation(GeneralFallAnimationStep);
}

bool GeneralFallAnimationStep(Sprite* sprite){
    return sprite->y++ > SOME_LIMIT_SOMEWHERE;
}

// Could possibly end it early...
void CancelAnimationOnThing() {
    mySprite->EndAnimation();
}

It seems like a pretty simple design that will do what I need, but almost too simple. I'm doing something terribly wrong, huh?

EDIT: So I've gotten some good input on this, but now I'm curious... Any suggestions how controlling the speed of these animations? Can this be done well in the Sprite class, or should that be handled strictly by the ANIMATIONSTEP?

share|improve this question

2 Answers 2

up vote 2 down vote

Too simple? Hardly. That is pretty much a textbook implementation of animation using a callback-based event model. In a production-grade version there would be a few extra null checks and similar to check for programming mistakes, but the basic implemation could be very similar.

I'm assuming that elsewhere there is a main loop that periodically calls update on each relevant sprite. I'm also assuming that either update takes care of drawing the spite, or that code not shown is responsible for that.

One Weakness and Possible Solution

One thing though that may be problematic for complicated animations is that your design gives the function no scratch space in which to store any state information (complicated animations may want to know if this is the first or second time the sprite was at coordinate (457,758) for example).

An easy solution is to add an extra void * to the sprite. This would be an extra pointer the animation function can set to whatever it wants (usually an animation specific structure). Have BeginAnimation set this new pointer to NULL. Document that the pointer must either be left NULL, or allocated with new. EndAnimation would delete the pointer on the animation's behalf if it was not still NULL, since the animation is not given the opertunity to cleanup if EndAnimation is called directly.

There are cleaner solutions than the above, but the that one is easy to explain, and should work.

share|improve this answer
    
Your assumptions are correct; I left them out for brevity. And +1 for adding state. Great idea. –  wtfsven Oct 4 '11 at 22:11
up vote 1 down vote

I would say that is the perfect way of doing things if you were programming in C.

But since you tagged this as C++ I would say you are doing it wrong.

Rather than having a pointer to a function I would set an animation object off your sprite. Then you can call the animation step method on this object. Since it is an object it can hold state as required.

Change:

typedef bool (AnimationStep*)(Sprite* sprite) ANIMATIONSTEP

Into:

class ANIMATIONSTEP
{
    public:
        virtual bool animationStep(Sprite* sprite) = 0;
        virtual ~ANIMATIONSTEP() {}
};

Then change:

if (!m_currentAnimation(this)) EndAnimation();

Into:

if (!m_currentAnimation->animationStep(this)) EndAnimation();
share|improve this answer
    
An animation object is indeed a cleaner approach. The existing approach is not "wrong" in so far as it works, and is conceptually simple. On advantage of your approach is that adding state is cleaner than my suggestion. –  Kevin Cathcart Oct 5 '11 at 13:42
    
A disadvantage of your approach is that it does not work as-is, since you cannot have an instance of an abstract class in C++, only a reference or a pointer. Using a pointer implies passing ownership of the animation to the sprite. auto_ptr could be used, or if availible, unique_ptr is preferable. –  Kevin Cathcart Oct 5 '11 at 13:49
    
@Kevin Cathcart: I would still say it is wrong when used in a C++ context. In C it is perfectly valid. It looks simple now because it is unfinished. The trouble with function pointers approach is that to be generic (and thus useful) you also need to pass state information, this will require that you add a void* parameter (as not all animation functions will take the same state) stored in the sprite and passed to the animation function for decoding. This breaking of encapsulation is wrong for C++. –  Loki Astari Oct 5 '11 at 13:50
    
@Kevin Cathcart: The use of a smart pointer goes without saying for any owned pointer. It works as-as in the same way a function pointer works as-is. If you want to add functionality you have to write the function or the object (if you don't write them you can't use them). –  Loki Astari Oct 5 '11 at 13:53
    
The alternative is having a public void* member on the Sprite class. (That is admitedly an even worse violation of encapsulation.) I fully agree that in terms of good OO-design, your basic approach is the right approach. –  Kevin Cathcart Oct 5 '11 at 13:59

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.