Should Sprite use index based animations

[DanRStevens]

Related: #528 (Warning flag -Weffc++)

In file included from NAS2D/Resource/Sprite.cpp:11:
NAS2D/Resource/Sprite.h:30:8: warning: ‘class NAS2D::Sprite’ has pointer data members [-Weffc++]
30 | class Sprite
| ^~~~~~
NAS2D/Resource/Sprite.h:30:8: warning: but does not declare ‘NAS2D::Sprite(const NAS2D::Sprite&)’ [-Weffc++]
NAS2D/Resource/Sprite.h:30:8: warning: or ‘operator=(const NAS2D::Sprite&)’ [-Weffc++]
NAS2D/Resource/Sprite.h:68:43: note: pointer member ‘NAS2D::Sprite::mCurrentAction’ declared here
68 | const std::vectorAnimationSet::Frame* mCurrentAction{nullptr};
| ^~~~~~~~~~~~~~

This warning is implying we should probably have (copy/move constructor/assignment) special member functions for this class. The reason being it contains a pointer field which points to internal data, so if the class is copied using the implicit copy constructor, the new object will have a pointer referencing data from the old object. In this case it's not a particular problem, since that data is actually stored in the referenced AnimationSet class, which has separate shared lifetime. Implicit in the safety of this, is the assumption the AnimationSet will be stored in a long lived cache, which has lifetime beyond that of the Sprite class.

---

This brings up a few design points. Should we need that internal pointer? Currently we use the pointer to cache the result of lookups into a std::map, which happens in the method void Sprite::play(const std::string& action). If we had used a std::vector based store for the data, then we could have stored an index instead of a pointer, and would still have similar efficiency. It also means the implicit special operations would work correctly, without generating any warnings.

Related, is potential errors for animation lookups. Currently we do the animation name to animation data lookup at runtime, on demand in the play method, which means any lookup errors will be deferred until the animation is actually used. If there is a missing animation sequence, it would be very easy for that to be missed until after a game update is released, resulting in crashes for the user.

An alternative would be to force the lookups to be done early, such as when loading the animation data, and storing the result of lookups into a table of index values. Any needed animations would be found at that time, forcing early reporting of errors.

The lookup tables (or structs with named fields) would have a fixed number of entries, with fixed meaning for each entry, and would reference the animations that are actually used. The sprite data itself could contain extra animations, and be in any order.

[cugone]

Point 1:

It depends on the use case. If the Sprite class owns the AnimationSet then sure, making a deep copy makes sense but if it just references a pre-existing AnimationSet then no it shouldn't. Particularly if AnimationSet is, like you said, cached elsewhere. My implementation does not own the underlying Material or SpriteSheet and instead references them via a pointers owned by the Renderer (which outlives everything as it is not destroyed until program termination.)

Point 2:

Loading the data at runtime via data-driven design is fine (See previous implementation) and almost required in some cases instead of hard coding any values. It allows for faster iteration because changing simple animation values (like timing) doesn't require re-compiling the entire program.

I do not cache the animations on play, but instead on load; logging any errors at that time.