[Bug] Confusing playMode replacement in 2D animation getKeyFrame Method

[ZCHawk]

In 'com.badlogic.gdx.graphics.g2d.Animation'
there is a method called getKeyFrame with an additional looping param, I think the parameter looping can override the Animation playMode temporarily when it get its keyFrames, the current code are following:

public T getKeyFrame (float stateTime, boolean looping) {
		// we set the play mode by overriding the previous mode based on looping
		// parameter value
		PlayMode oldPlayMode = playMode;
		if (looping && (playMode == PlayMode.NORMAL || playMode == PlayMode.REVERSED)) {
			if (playMode == PlayMode.NORMAL)
				playMode = PlayMode.LOOP;
			else
				playMode = PlayMode.LOOP_REVERSED;
		} else if (!looping && !(playMode == PlayMode.NORMAL || playMode == PlayMode.REVERSED)) {
			if (playMode == PlayMode.LOOP_REVERSED)
				playMode = PlayMode.REVERSED;
			else
				playMode = PlayMode.LOOP;
		}

		T frame = getKeyFrame(stateTime);
		playMode = oldPlayMode;
		return frame;
}

In Addition, the current PlayMode enum has six values where the code is

public enum PlayMode {
    NORMAL, REVERSED, LOOP, LOOP_REVERSED, LOOP_PINGPONG, LOOP_RANDOM,
}

In the first if part, it says when looping=true and playMode is NORMAL or REVERSED, the playMode is changed to corresponding loop mode LOOP AND LOOP_REVERSED.

Next, in the second else if part, when looping=false and playMode is LOOP_REVERSED, the playMode will be replaced to PlayMode.REVERSED, which also makes sense, but it turns the other three loop PlayMode into LOOP, which confuses me.

In my Opinion, all the 4 loop modes shall keep the Animation playing forever. the other 2 non-loop modes will stop the Animation and offer the last frame. But when you are in LOOP_PINGPONG or LOOP_RANDOM mode, and you want a non-loop output, I'm not quite sure what kind of output you are looking for, may be no changes shall be made? throw an Exception seems unfriendly. But a replacement from LOOP to NARMAL shall be made which is consistant with the first part.

So perhaps the second part can be written like this:

else if (!looping && !(playMode == PlayMode.NORMAL || playMode == PlayMode.REVERSED)) {
    if (playMode == PlayMode.LOOP_REVERSED)
        playMode = PlayMode.REVERSED;
    else if (playMode == PlayMode.LOOP) // replace the LOOP mode by NORMAL
        playMode = PlayMode.NORMAL;
    // ignore LOOP_PINGPONG and LOOP_RANDOM mode even if you want non-loop output
}

Moreover, there are three tests for getKeyFrame(float stateTime, boolean looping), but all of them require looping=true, so the second part might has never been tested.

[tommyettinger]

Yeah, this looks like a bug, and good catch on the crummy testing. The code you gave looks good.

Animation is pretty much a mess as it is; there's no support for the common case of an animated sprite, for instance, and the long-ago change to generic Animation<T> didn't provide any sort of fallback for a Sprite that can have its TextureRegion change. This and Sprite are probably some of the most commonly-changed classes if they get used at all... That and SpriteBatch.

[ZCHawk]

Thanks for reply,
I think this Animation class for 2D only supply a basic timeline manage function, In a well-performed Sprite Animation system, it may need a higher management layer for Animation control. Refer to other game engine, a designed animation management may be better. I myself currently add a tween anim lib and a skeleton anim lib during game-dev.

Anyway, I'll just push my code.