Sprite animation handler

Question

When a user presses a key in my game, their avatar can move down, left, right or up. By move, I mean their avatar animates while various other things move around them on the screen. This is accomplished by raising an event from the InputHandler class.

public class InputHandler
{
    public delegate void ActionListener(Actions action);

    public event ActionListener ActionRequested;

    public void ProcessKeyboard(KeyboardState keyboardState)
    {
        var keys = keyboardState.GetPressedKeys().ToList();

        if (!keys.Any())
        {
            ActionRequested(Actions.Idle);
            return;
        }

        foreach (var action in keys.Select(GetAction))
        {
            ActionRequested(action);
        }
    }
}

The Player class listens for the ActionRequested event:

public class Player
{
    private readonly IEnumerable<Sprite> sprites;

    public Player(IEnumerable<Sprite> sprites)
    {
        this.sprites = sprites;
    }

    public void Listen(Actions action)
    {
        switch (action)
        {
            case Actions.Idle:
                Idle();
                return;

            case Actions.MoveDown:
            case Actions.MoveLeft:
            case Actions.MoveRight:
            case Actions.MoveUp:
                Animate(action);
                return;
        }
    }

    public void Idle()
    {
        foreach (var sprite in sprites)
        {
            sprite.Idle();
        }
    }

    public void Animate(Actions action)
    {
        foreach (var sprite in sprites)
        {
            sprite.Animate(action);
        }
    }
}

When the Animate method is called on the Sprite class, it gets the Animation for that Action (MoveDown, MoveLeft, etc.), gets the next frame from that Animation and resets the previous animation (so it doesn't start halfway through the next time the button is pressed). The Idle method moves the frame to the left-most position in the sprite sheet (so the avatar is standing still and not frozen mid-animation). Think of the frame as a viewport for the sprite sheet.

public class Sprite
{
    private Rectangle frame;
    private readonly IDictionary<Actions, Animation> animations;
    private Animation prevAnimation;

    public Sprite(Rectangle frame, IDictionary<Actions, Animation> animations)
    {
        this.frame = frame;
        this.animations = animations;
    }

    public void Idle()
    {
        frame.X = 0;
    }

    public void Animate(Actions action)
    {
        Animation animation;

        if (!animations.TryGetValue(action, out animation))
        {
            return;
        }

        if (!animation.IsReady())
        {
            return;
        }

        frame = animation.GetNextFrame();

        if (animation == prevAnimation)
        {
            return;
        }

        if (prevAnimation != null)
        {
            prevAnimation.Reset();
        }

        prevAnimation = animation;
    }
}

The Animation class itself is really just a list of frames (Rectangles). The IsReady method updates the timer (only one frame can be returned every 100ms or so, so the avatar doesn't act like a cracked out monkey). The GetNextFrame method updates the current frame number and returns the next one in the sequence.

public class Animation
{
    private readonly IEnumerable<Rectangle> frames;
    private int currentFrame = 1;
    private double frameTimer = 0;
    private const double frameSpeed = 0.2;

    public Animation(IEnumerable<Rectangle> frames)
    {
        this.frames = frames;
    }

    public void Reset()
    {
        currentFrame = 1;
        frameTimer = 0;
    }

    public bool IsReady()
    {
        if (frameTimer == 0)
        {
            frameTimer += frameSpeed;
            return true;
        }

        if (frameTimer < frameInterval)
        {
            frameTimer += frameSpeed;
            return false;
        }

        frameTimer = 0;
        return false;
    }

    public Rectangle GetNextFrame()
    {
        if (currentFrame == frames.Count())
        {
            currentFrame = 1;

            return frames.FirstOrDefault();
        }

        currentFrame++;

        return frames.ElementAtOrDefault(currentFrame - 1);
    }
}

Finally here's a typical sprite sheet, which I got from Liberated Pixel Cup. Each row is an animation for one direction (up, left, down right), and the far left column (frame.X = 0) is the standing position. The actual animations don't include this first column because it makes it look all janky.

This works pretty well and I'm happy with it for the most part. I have a few issues that I'm not sure how to improve:

1. InputHandler raises an event for what is essentially a non-action when there is no input detected, which means it is almost constantly telling the Player to Idle.
2. I don't like that there's a hard-coded requirement in the Idle method of the Sprite class for every sprite sheet to reserve its left-most column for an avatar's standing position. Not every sprite sheet is a humanoid anyways.
3. I don't like that the Animate method is responsible for resetting the previous animation - I have NO idea how else to do this, since I can't detect when a user stops pressing a key.
4. I'm not sure if the IsReady method of the Animation class should update frameTimer or just check the value of frameTimer and return true of false.
5. Similarly, I'm not sure if the GetNextFrame method should update currentFrame or just return the next frame in the sequence.

Answer 1

Looks pretty neat overall, I merely glanced at your code (favorited, I want to look at this more deeply, as I'm [playing with | learning] XNA myself, your XNA posts are very useful!).

One thing I've noticed, I don't think you need this switch block:

public void Listen(Actions action)
{
    switch (action)
    {
        case Actions.Idle:
            Idle();
            return;

        case Actions.MoveDown:
        case Actions.MoveLeft:
        case Actions.MoveRight:
        case Actions.MoveUp:
            Animate(action);
            return;
    }
}

Instead, I'd probably do this:

public void Listen(Actions action)
{
    Animate(action);
}

...and in Animate(Actions) I'd add the call to Idle() and exit if (action == Actions.Idle). "Being idle" does involve choosing a location on the sprite sheet, I don't see why it would be handled in Listen when everything else is in Animate.

Also in switch blocks you should use break;, and let the function return by itself.

Answer 2

Your GetNextFrame should have fewer if statements. In other words, you should merge some of them together. The ones that return early should especially be in an if ..then .. else if.. then structure, like this. It works either way, but this looks cleaner and is more to the point about what the code does.

public Rectangle GetNextFrame(GameActions gameAction)
{
    var sequence = GetSequence(gameAction);
    if (gameAction == GameActions.Idle) {
        frameTimer = 0;

        prevFrame.X = 0;
        prevGameAction = gameAction;

        return prevFrame;
    } else if(frameTimer != 0) {
        UpdateTimer();

        return prevFrame;
    } else if (sequence == null) {
        return prevFrame;
    }

    UpdateTimer();

    Rectangle nextFrame;

    if (gameAction != prevGameAction)
    {
        nextFrame = sequence.First();
    }
    else
    {
        nextFrame = sequence.Next(prevFrame);
    }

    prevFrame = nextFrame;
    prevGameAction = gameAction;

    return nextFrame;
}

Your Animate method has some extra lines in it as well where the if statements can be merged together.

public void Animate(Actions action)
{
    Animation animation;

    if (!animations.TryGetValue(action, out animation) || !animation.IsReady())
    {
        return;
    }

    frame = animation.GetNextFrame();

    if (animation == prevAnimation)
    {
        return;
    }

    if (prevAnimation != null)
    {
        prevAnimation.Reset();
    }

    prevAnimation = animation;
}

That one if statement with the OR clause || does the exact same thing that your code did, but with fewer lines and in a cleaner manner.