Fix frame number reset for animated gifs

[fedus]

The animated gif sprite does not correctly reset the frame number back to 0 after the first loop.

In fact, when the frame count is set to zero in the animated gif sprite's check_frame_bounds method, the subsequent call to update_frame_num() will actually increase it to frame 1 before we render the sprite, resulting in frame 0 to be skipped after the first loop.

By setting a reset flag, a modified update_frame_num() method will set the frame number to zero correctly.

This error doesn't occur with other sprites because we usually just change the frame-delta, and don't fiddle with the frame number directly.

[partofthething]

Thanks for the PR! Sorry it took so long for me to review it... must've missed the notification. Anyway this is a great fix, I definitely see the problem.

If we change the check_frame_bounds method of AnimatedGif to self._frame_num = -self._frame_delta instead of 0 you can avoid the extra _reset_frame_count if statement and make this faster. Can you update this to do that instead?

[fedus]

Hi,

no worries about the delay :) Nonetheless, I had to properly re-read my pull request and reconstruct what is happening.

So it seems like there's good and bad news!

The bad one first:

I think I misrepresented the problem in my initial pull request! The problem was not that the first (0) frame was not shown for the animated GIF, but the last one (Nth frame for a GIF with N frames).

Let's have a look at what is happening for a GIF with 5 total frames, currently at the penultimate frame (3 of 4, due to the 0-4 numbering of arrays), and entering the render() function:

-> Render() function entered with _frame_num = 3
Here, the sprite itself and its phrase are rendered, and following this, the sprite "ticks".
-> Let's see what happens in the tick() function
We first see what's going on with update_frame_num(): in here, _frame_num is increased to 4 (last frame).
Then, the overridden check_frame_bounds() of the GIF sprite is called. In there, it checks whether _frame_num is equal to N-1 (N-1 equals 4 in our case). This is true, so it sets _frame_num = 0.
-> Tick exits, and at the next render we render frame 0, skipping frame 4 (or N-1)!

My initial solution suggested setting a flag in the check_frame_bounds() method so that we would stay at frame N-1 for the moment and render it. Following this, the next call to update_frame_num() would reset it to 0. My idea at the time was to keep the bound-checking in the check_frame_bounds() method and updating of the frame number in the update_frame_num(). Yet, thinking back, it seems that this adds unnecessary complication and makes the code less readable.

Now the good one:

It seems that you have (inadvertently?) fixed the problem yourself in more recent commits. More specifically, you added the following to the update_frame_num() function of the Sprite class:

            if self._frame_num == len(self.frames):
                # loop around if we overstepped the bounds
                # This should never happen if reverse_frame_loop is true
                self._frame_num = 0

Also, in the check_frame_bounds() method of Sprite we have:

        elif self.reverse_frame_loop and self._frame_num == len(self.frames) - 1:
            self._frame_delta = -1

So in essence, and notwithstanding any thinking error from my part, we can simply remove the GIF sprites overriding check_frame_bounds() method and it should work just fine!

Bottom line

I will change my pull request to removing the GIF sprite's overriding check_frame_bounds() method as a fix later today or tomorrow.

Of course any feedback is welcome! Thanks :)

[partofthething]

Ahh, gotcha. Ok well that sounds great. Thanks.

[partofthething]

Still waiting! ;)