feat: create ping pong animation mode

[aaneto]

Allows the creation of animations using the PingPong mode. This is a WIP solution, there may be alternative implementations.

A possible implementation would be to insert state into the PingPong mode, so that users of the Repeat and Once AnimationMode's would not pay in storage for the PingPong logic.

The problem with this approach is that users would either:

• See the internal logic inside the PingPong mode
• Not be able to construct PingPong directly (AnimationMode fields are made private to improve API).

Closes #11

[jcornaz]

Hi, thanks for the contribution :-)

The general approach sounds good to me.

The problem with this approach is that users would either:

• See the internal logic inside the PingPong mode
• Not be able to construct PingPong directly (AnimationMode fields are made private to improve API).

I don't really understand these drawbacks. How does the user see the internal logic inside the PingPong mode? What would it mean to construct it directly?

---

Now, I would like to request two changes:

1. Can you make it a non-breaking change?
2. Can you add a test ?

If you don't see how to make it a non-breaking change here is recipe you may follow:

1. Create a new Mode enum. It may actually remain private, the users don't need it as they can set the mode by calling a function (once, repeator ping_pong)
2. Change the private fields and implementations to use this new enum
3. Leave as-is the existing AnimationMode and any function that use it in its signature. But mark them as #[deprecated]

[aaneto]

Following the suggestion, I've written tests for the ping pong behavior. As for this being a breaking change, I faced an issue:

• Making the AnimationMode private would break the interface.
• Changing the inner type of AnimationMode would also break the interface, since it is currently public.

Maybe I missed some essential part of your suggestion, but as it looks to me, the best I can do is prevent future problems by making the AnimationMode pub(crate).

[aaneto]

About the construction of PingPong, summarizing we could have:

enum AnimationMode {
  ...
  PingPong({ going_backward: bool })
}

So only PingPong AnimationMode's would actually store a boolean, however, I don't think that 'optimization' would be relevant.

[jcornaz]

I've written tests for the ping pong behavior

Awsome thanks 👍

Making the AnimationMode private would break the interface.

Yes, but I didn't want to make it private. I wanted to deprecate it and create a new one that could be private.

Changing the inner type of AnimationMode would also break the interface, since it is currently public.

Arf yes... That's a bummer. I should have kept the fields private. It is still possible to have it a non-breaking change, but that'll be a bit more convoluted. I'll try to look at it myself.

About the construction of PingPong, summarizing we could have:

enum AnimationMode {
  ...
  PingPong({ going_backward: bool })
}

Ah. No, we don't add the boolean.

---

Thank you for the contribution and the changes. I'll try to make it a non-breaking change, and merge soon.

[aaneto]

Ok @jcornaz ! Just wanted to give back a little :) Hope this helps.

[jcornaz]

It does help. Thanks 👍

[jcornaz]

After reconsideration, making it a non-breaking change would bring too much mess for the benefits.

So I'm taking it it as-is. I'll use that opportunity, to also hide has many implementation details as I can (like the fields of the animation asset), so that we can avoid the same problem next time.

Thanks you very much @aaneto!