Trade: pack AnimationTrackData internals better.

Instead of storing Animation::TrackViewStorage directly it now contains
the view pointers, strides and size (where the size is shared by both
keys and values) together with packing the non-pointer values into
existing paddings. Together with reducing the keyframe count to 32 bits
and strides to 16 bits (which is consistent with MeshData and
SceneData), this reduces the size from 80 bytes to 48.

Not using TrackViewStorage also means we can directly accept the
key/value views in constructors, significantly improving the usability.
This also makes it possible to add support for (constexpr) offset-only
track data and thus easy serializability, again similarly to
MeshAttributeData and SceneFieldData.

src/Magnum/Animation/Track.h

@@ -427,6 +427,15 @@ template<class K, class V, class R
         Extrapolation _before, _after;
 };
 
+}
+
+#ifndef DOXYGEN_GENERATING_OUTPUT
+/* For a friend declaration in TrackViewStorage */
+namespace Trade { class AnimationTrackData; }
+#endif
+
+namespace Animation {
+
 /**
 @brief Type-erased track view storage
 
@@ -518,6 +527,11 @@ template<class K> class TrackViewStorage {
 
     private:
         template<class, class, class> friend class TrackView;
+        #ifndef DOXYGEN_GENERATING_OUTPUT
+        /* This one creates TrackViewStorage instances on the fly using the
+           below constructor */
+        friend Trade::AnimationTrackData;
+        #endif
 
         explicit TrackViewStorage(const Containers::StridedArrayView1D<K>& keys, const Containers::StridedArrayView1D<typename std::conditional<std::is_const<K>::value, const void, void>::type>& values, Interpolation interpolation, void(*interpolator)(), Extrapolation before, Extrapolation after) noexcept: _keys{keys}, _values{values}, _interpolator{interpolator}, _interpolation{interpolation}, _before{before}, _after{after} {
             CORRADE_ASSERT(keys.size() == values.size(), "Animation::TrackView: expected key and value view to have the same size but got" << keys.size() << "and" << values.size(), );

src/Magnum/Trade/AnimationData.cpp

@@ -33,6 +33,25 @@
 
 namespace Magnum { namespace Trade {
 
+AnimationTrackData::AnimationTrackData(const AnimationTrackType type, const AnimationTrackType resultType, const AnimationTrackTarget targetName, const UnsignedLong target, const Animation::TrackViewStorage<const Float>& view) noexcept: _type{type}, _resultType{resultType}, _targetName{targetName}, _interpolation{view.interpolation()}, _before{view.before()}, _after{view.after()}, _target{target}, _size{UnsignedInt(view.size())}, _keysStride{Short(view.keys().stride())}, _valuesStride{Short(view.values().stride())}, _keysData{view.keys().data()}, _valuesData{view.values().data()}, _interpolator{view.interpolator()} {
+    #ifndef CORRADE_TARGET_32BIT
+    CORRADE_ASSERT(view.size() <= 0xffffffffu,
+        "Trade::AnimationTrackData: expected keyframe count to fit into 32 bits but got" << view.size(), );
+    #endif
+    CORRADE_ASSERT(view.keys().stride() >= -32768 && view.keys().stride() <= 32767,
+        "Trade::AnimationTrackData: expected key stride to fit into 16 bits but got" << view.keys().stride(), );
+    CORRADE_ASSERT(view.values().stride() >= -32768 && view.values().stride() <= 32767,
+        "Trade::AnimationTrackData: expected value stride to fit into 16 bits but got" << view.values().stride(), );
+}
+
+Animation::TrackViewStorage<const Float> AnimationTrackData::track() const {
+    return Animation::TrackViewStorage<const Float>{
+        /* We're sure the views are correct, circumventing the asserts */
+        Containers::StridedArrayView1D<const Float>{{nullptr, ~std::size_t{}}, static_cast<const Float*>(_keysData), _size, _keysStride},
+        Containers::StridedArrayView1D<const char>{{nullptr, ~std::size_t{}}, static_cast<const char*>(_valuesData), _size, _valuesStride},
+        _interpolation, _interpolator, _before, _after};
+}
+
 AnimationData::AnimationData(Containers::Array<char>&& data, Containers::Array<AnimationTrackData>&& tracks, const Range1D& duration, const void* importerState) noexcept: _dataFlags{DataFlag::Owned|DataFlag::Mutable}, _duration{duration}, _data{std::move(data)}, _tracks{std::move(tracks)}, _importerState{importerState} {}
 
 AnimationData::AnimationData(Containers::Array<char>&& data, std::initializer_list<AnimationTrackData> tracks, const Range1D& duration, const void* importerState): AnimationData{std::move(data), Implementation::initializerListToArrayWithDefaultDeleter(tracks), duration, importerState} {}
@@ -47,11 +66,19 @@ AnimationData::AnimationData(const DataFlags dataFlags, const Containers::ArrayV
 
 AnimationData::AnimationData(Containers::Array<char>&& data, Containers::Array<AnimationTrackData>&& tracks, const void* importerState) noexcept: _dataFlags{DataFlag::Owned|DataFlag::Mutable}, _data{std::move(data)}, _tracks{std::move(tracks)}, _importerState{importerState} {
     if(!_tracks.isEmpty()) {
+        const auto duration = [](const AnimationTrackData& track) {
+            if(!track._size) return Range1D{};
+
+            /* We're sure the view is correct, circumventing the assert */
+            Containers::StridedArrayView1D<const Float> keys{{nullptr, ~std::size_t{}}, static_cast<const Float*>(track._keysData), track._size, track._keysStride};
+            return Range1D{keys.front(), keys.back()};
+        };
+
         /* Reset duration to duration of the first track so it properly support
            cases where tracks don't start at 0 */
-        _duration = _tracks.front()._view.duration();
+        _duration = duration(_tracks.front());
         for(std::size_t i = 1; i != _tracks.size(); ++i)
-            _duration = Math::join(_duration, _tracks[i]._view.duration());
+            _duration = Math::join(_duration, duration(_tracks[i]));
     }
 }
 
@@ -101,20 +128,21 @@ UnsignedLong AnimationData::trackTarget(UnsignedInt id) const {
     return _tracks[id]._target;
 }
 
-const Animation::TrackViewStorage<const Float>& AnimationData::track(UnsignedInt id) const {
+Animation::TrackViewStorage<const Float> AnimationData::track(UnsignedInt id) const {
     CORRADE_ASSERT(id < _tracks.size(),
-        "Trade::AnimationData::track(): index" << id << "out of range for" << _tracks.size() << "tracks", _tracks[0]._view);
+        "Trade::AnimationData::track(): index" << id << "out of range for" << _tracks.size() << "tracks", _tracks[0].track());
-    return _tracks[id]._view;
+    return _tracks[id].track();
 }
 
-const Animation::TrackViewStorage<Float>& AnimationData::mutableTrack(UnsignedInt id) {
+Animation::TrackViewStorage<Float> AnimationData::mutableTrack(UnsignedInt id) {
+    /* EW those casts! */
     CORRADE_ASSERT(_dataFlags & DataFlag::Mutable,
         "Trade::AnimationData::mutableTrack(): the animation is not mutable",
-        reinterpret_cast<const Animation::TrackViewStorage<Float>&>(_tracks[id]._view));
+        reinterpret_cast<const Animation::TrackViewStorage<Float>&>(static_cast<const Animation::TrackViewStorage<const Float>&>(_tracks[0].track())));
     CORRADE_ASSERT(id < _tracks.size(),
         "Trade::AnimationData::mutableTrack(): index" << id << "out of range for" << _tracks.size() << "tracks",
-        reinterpret_cast<const Animation::TrackViewStorage<Float>&>(_tracks[0]._view));
+        reinterpret_cast<const Animation::TrackViewStorage<Float>&>(static_cast<const Animation::TrackViewStorage<const Float>&>(_tracks[0].track())));
-    return reinterpret_cast<const Animation::TrackViewStorage<Float>&>(_tracks[id]._view);
+    return reinterpret_cast<const Animation::TrackViewStorage<Float>&>(static_cast<const Animation::TrackViewStorage<const Float>&>(_tracks[id].track()));
 }
 
 Containers::Array<char> AnimationData::release() {

src/Magnum/Trade/AnimationData.h

@@ -338,7 +338,7 @@ Convenience type for populating @ref AnimationData. Has no accessors, as the
 data are then accessible through @ref AnimationData APIs.
 @experimental
 */
-class AnimationTrackData {
+class MAGNUM_TRADE_EXPORT AnimationTrackData {
     public:
         /**
          * @brief Default constructor
@@ -347,7 +347,7 @@ class AnimationTrackData {
          * initialization of the track array for @ref AnimationData, expected
          * to be replaced with concrete values later.
          */
-        explicit AnimationTrackData() noexcept: _type{}, _resultType{}, _targetName{}, _target{}, _view{} {}
+        explicit AnimationTrackData() noexcept: _type{}, _resultType{}, _targetName{}, _interpolation{}, _before{}, _after{}, _target{}, _size{}, _keysStride{}, _valuesStride{}, _keysData{}, _valuesData{}, _interpolator{} {}
 
         /**
          * @brief Type-erased constructor
@@ -356,11 +356,14 @@ class AnimationTrackData {
          * @param targetName    Track target name
          * @param target        Track target ID
          * @param view          Type-erased @ref Animation::TrackView instance
+         *
+         * Expects that @p view key and value strides both fit into signed
+         * 16-bit values and that keyframe count fits into 32 bits.
          */
         /** @todo stop taking TrackViewStorage and instead directly take the
             key/value views, interpolator/interpolation and extrapolation --
             it's just 6 overloads and makes usage much better */
-        explicit AnimationTrackData(AnimationTrackType type, AnimationTrackType resultType, AnimationTrackTarget targetName, UnsignedLong target, const Animation::TrackViewStorage<const Float>& view) noexcept: _type{type}, _resultType{resultType}, _targetName{targetName}, _target{target}, _view{view} {}
+        explicit AnimationTrackData(AnimationTrackType type, AnimationTrackType resultType, AnimationTrackTarget targetName, UnsignedLong target, const Animation::TrackViewStorage<const Float>& view) noexcept;
 
         /** @overload
          *
@@ -409,16 +412,23 @@ class AnimationTrackData {
          * @brief Type-erased @ref Animation::TrackView instance
          * @m_since_latest
          */
-        Animation::TrackViewStorage<const Float> track() const { return _view; }
+        Animation::TrackViewStorage<const Float> track() const;
 
     private:
         friend AnimationData;
 
         AnimationTrackType _type, _resultType;
         AnimationTrackTarget _targetName;
-        /* 4-byte padding */
+        Animation::Interpolation _interpolation;
+        Animation::Extrapolation _before, _after;
+        /* 1 byte padding */
         UnsignedLong _target;
-        Animation::TrackViewStorage<const Float> _view;
+        UnsignedInt _size;
+        Short _keysStride;
+        Short _valuesStride;
+        const void* _keysData;
+        const void* _valuesData;
+        void(*_interpolator)();
 };
 
 /**
@@ -683,7 +693,7 @@ class MAGNUM_TRADE_EXPORT AnimationData {
          * checked version below to access a concrete @ref Animation::TrackView
          * type.
          */
-        const Animation::TrackViewStorage<const Float>& track(UnsignedInt id) const;
+        Animation::TrackViewStorage<const Float> track(UnsignedInt id) const;
 
         /**
          * @brief Mutable track data storage
@@ -693,7 +703,7 @@ class MAGNUM_TRADE_EXPORT AnimationData {
          * animation is mutable.
          * @see @ref dataFlags()
          */
-        const Animation::TrackViewStorage<Float>& mutableTrack(UnsignedInt id);
+        Animation::TrackViewStorage<Float> mutableTrack(UnsignedInt id);
 
         /**
          * @brief Track data
@@ -707,7 +717,7 @@ class MAGNUM_TRADE_EXPORT AnimationData {
          * use the view or you need to release the data array using
          * @ref release() and manage its lifetime yourself.
          */
-        template<class V, class R = Animation::ResultOf<V>> const Animation::TrackView<const Float, const V, R>& track(UnsignedInt id) const;
+        template<class V, class R = Animation::ResultOf<V>> Animation::TrackView<const Float, const V, R> track(UnsignedInt id) const;
 
         /**
          * @brief Mutable track data
@@ -717,7 +727,7 @@ class MAGNUM_TRADE_EXPORT AnimationData {
          * animation is mutable.
          * @see @ref dataFlags()
          */
-        template<class V, class R = Animation::ResultOf<V>> const Animation::TrackView<Float, V, R>& mutableTrack(UnsignedInt id);
+        template<class V, class R = Animation::ResultOf<V>> Animation::TrackView<Float, V, R> mutableTrack(UnsignedInt id);
 
         /**
          * @brief Release data storage
@@ -812,15 +822,15 @@ namespace Implementation {
 
 template<class V, class R> inline AnimationTrackData::AnimationTrackData(AnimationTrackTarget targetName, UnsignedLong target, const Animation::TrackView<const Float, const V, R>& view) noexcept: AnimationTrackData{Implementation::animationTypeFor<V>(), Implementation::animationTypeFor<R>(), targetName, target, view} {}
 
-template<class V, class R> const Animation::TrackView<const Float, const V, R>& AnimationData::track(UnsignedInt id) const {
+template<class V, class R> Animation::TrackView<const Float, const V, R> AnimationData::track(UnsignedInt id) const {
-    const Animation::TrackViewStorage<const Float>& storage = track(id);
+    const Animation::TrackViewStorage<const Float> storage = track(id);
     CORRADE_ASSERT(Implementation::animationTypeFor<V>() == _tracks[id]._type, "Trade::AnimationData::track(): improper type requested for" << _tracks[id]._type, (static_cast<const Animation::TrackView<const Float, const V, R>&>(storage)));
     CORRADE_ASSERT(Implementation::animationTypeFor<R>() == _tracks[id]._resultType, "Trade::AnimationData::track(): improper result type requested for" << _tracks[id]._resultType, (static_cast<const Animation::TrackView<const Float, const V, R>&>(storage)));
     return static_cast<const Animation::TrackView<const Float, const V, R>&>(storage);
 }
 
-template<class V, class R> const Animation::TrackView<Float, V, R>& AnimationData::mutableTrack(UnsignedInt id) {
+template<class V, class R> Animation::TrackView<Float, V, R> AnimationData::mutableTrack(UnsignedInt id) {
-    const Animation::TrackViewStorage<Float>& storage = mutableTrack(id);
+    const Animation::TrackViewStorage<Float> storage = mutableTrack(id);
     CORRADE_ASSERT(Implementation::animationTypeFor<V>() == _tracks[id]._type, "Trade::AnimationData::mutableTrack(): improper type requested for" << _tracks[id]._type, (static_cast<const Animation::TrackView<Float, V, R>&>(storage)));
     CORRADE_ASSERT(Implementation::animationTypeFor<R>() == _tracks[id]._resultType, "Trade::AnimationData::mutableTrack(): improper result type requested for" << _tracks[id]._resultType, (static_cast<const Animation::TrackView<Float, V, R>&>(storage)));
     return static_cast<const Animation::TrackView<Float, V, R>&>(storage);

src/Magnum/Trade/Test/AnimationDataTest.cpp

@@ -49,6 +49,10 @@ struct AnimationDataTest: TestSuite::Tester {
     void constructTrackResultType();
     void constructTrackTemplate();
     void constructTrackDefault();
+    #ifndef CORRADE_TARGET_32BIT
+    void constructTrackWrongSize();
+    #endif
+    void constructTrackWrongStride();
 
     void construct();
     void constructNotOwned();
@@ -93,6 +97,10 @@ AnimationDataTest::AnimationDataTest() {
               &AnimationDataTest::constructTrackResultType,
               &AnimationDataTest::constructTrackTemplate,
               &AnimationDataTest::constructTrackDefault,
+              #ifndef CORRADE_TARGET_32BIT
+              &AnimationDataTest::constructTrackWrongSize,
+              #endif
+              &AnimationDataTest::constructTrackWrongStride,
 
               &AnimationDataTest::construct,
               &AnimationDataTest::constructImplicitDuration,
@@ -249,6 +257,75 @@ void AnimationDataTest::constructTrackDefault() {
     CORRADE_COMPARE(data.track().values().data(), nullptr);
 }
 
+#ifndef CORRADE_TARGET_32BIT
+void AnimationDataTest::constructTrackWrongSize() {
+    CORRADE_SKIP_IF_NO_ASSERT();
+
+    /* This should be fine */
+    AnimationTrackData{AnimationTrackTarget::Rotation3D, 16, Animation::TrackView<const Float, const Quaternion>{
+        {nullptr, 0xffffffffu},
+        {nullptr, 0xffffffffu},
+        Animation::Interpolation::Constant
+    }};
+
+    std::ostringstream out;
+    Error redirectError{&out};
+    AnimationTrackData{AnimationTrackTarget::Rotation3D, 16, Animation::TrackView<const Float, const Quaternion>{
+        {nullptr, 0x100000000ull},
+        {nullptr, 0x100000000ull},
+        Animation::Interpolation::Constant
+    }};
+    CORRADE_COMPARE(out.str(),
+        "Trade::AnimationTrackData: expected keyframe count to fit into 32 bits but got 4294967296\n");
+}
+#endif
+
+void AnimationDataTest::constructTrackWrongStride() {
+    CORRADE_SKIP_IF_NO_ASSERT();
+
+    char toomuch[2*(32768 + sizeof(Vector2))];
+
+    /* These should be fine */
+    AnimationTrackData{AnimationTrackTarget::Scaling2D, 1, Animation::TrackView<const Float, const Vector2>{
+        Containers::StridedArrayView1D<Float>{Containers::arrayCast<Float>(toomuch), 2, 32767},
+        Containers::StridedArrayView1D<Vector2>{Containers::arrayCast<Vector2>(toomuch), 2, 32768}.flipped<0>(),
+        Animation::Interpolation::Constant
+    }};
+    AnimationTrackData{AnimationTrackTarget::Scaling2D, 1, Animation::TrackView<const Float, const Vector2>{
+        Containers::StridedArrayView1D<Float>{Containers::arrayCast<Float>(toomuch), 2, 32768}.flipped<0>(),
+        Containers::StridedArrayView1D<Vector2>{Containers::arrayCast<Vector2>(toomuch), 2, 32767},
+        Animation::Interpolation::Constant
+    }};
+
+    std::ostringstream out;
+    Error redirectError{&out};
+    AnimationTrackData{AnimationTrackTarget::Scaling2D, 1, Animation::TrackView<const Float, const Vector2>{
+        Containers::StridedArrayView1D<Float>{Containers::arrayCast<Float>(toomuch), 2, 32768},
+        Containers::StridedArrayView1D<Vector2>{Containers::arrayCast<Vector2>(toomuch), 2, 32767},
+        Animation::Interpolation::Constant
+    }};
+    AnimationTrackData{AnimationTrackTarget::Scaling2D, 1, Animation::TrackView<const Float, const Vector2>{
+        Containers::StridedArrayView1D<Float>{Containers::arrayCast<Float>(toomuch), 2, 32769}.flipped<0>(),
+        Containers::StridedArrayView1D<Vector2>{Containers::arrayCast<Vector2>(toomuch), 2, 32767},
+        Animation::Interpolation::Constant
+    }};
+    AnimationTrackData{AnimationTrackTarget::Scaling2D, 1, Animation::TrackView<const Float, const Vector2>{
+        Containers::StridedArrayView1D<Float>{Containers::arrayCast<Float>(toomuch), 2, 32767},
+        Containers::StridedArrayView1D<Vector2>{Containers::arrayCast<Vector2>(toomuch), 2, 32768},
+        Animation::Interpolation::Constant
+    }};
+    AnimationTrackData{AnimationTrackTarget::Scaling2D, 1, Animation::TrackView<const Float, const Vector2>{
+        Containers::StridedArrayView1D<Float>{Containers::arrayCast<Float>(toomuch), 2, 32767},
+        Containers::StridedArrayView1D<Vector2>{Containers::arrayCast<Vector2>(toomuch), 2, 32769}.flipped<0>(),
+        Animation::Interpolation::Constant
+    }};
+    CORRADE_COMPARE(out.str(),
+        "Trade::AnimationTrackData: expected key stride to fit into 16 bits but got 32768\n"
+        "Trade::AnimationTrackData: expected key stride to fit into 16 bits but got -32769\n"
+        "Trade::AnimationTrackData: expected value stride to fit into 16 bits but got 32768\n"
+        "Trade::AnimationTrackData: expected value stride to fit into 16 bits but got -32769\n");
+}
+
 void AnimationDataTest::construct() {
     struct Data {
         Float time;