From 5f1fc1047d7a82beceb15da72ddea27ab2e2b398 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Sun, 9 Apr 2023 09:53:34 +0200 Subject: [PATCH] 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 | 14 ++++ src/Magnum/Trade/AnimationData.cpp | 46 +++++++++--- src/Magnum/Trade/AnimationData.h | 38 ++++++---- src/Magnum/Trade/Test/AnimationDataTest.cpp | 77 +++++++++++++++++++++ 4 files changed, 152 insertions(+), 23 deletions(-) diff --git a/src/Magnum/Animation/Track.h b/src/Magnum/Animation/Track.h index c7f716964..b7ec6257f 100644 --- a/src/Magnum/Animation/Track.h +++ b/src/Magnum/Animation/Track.h @@ -427,6 +427,15 @@ template class TrackViewStorage { private: template 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& keys, const Containers::StridedArrayView1D::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(), ); diff --git a/src/Magnum/Trade/AnimationData.cpp b/src/Magnum/Trade/AnimationData.cpp index f8fa56134..d10995562 100644 --- a/src/Magnum/Trade/AnimationData.cpp +++ b/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& 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 AnimationTrackData::track() const { + return Animation::TrackViewStorage{ + /* We're sure the views are correct, circumventing the asserts */ + Containers::StridedArrayView1D{{nullptr, ~std::size_t{}}, static_cast(_keysData), _size, _keysStride}, + Containers::StridedArrayView1D{{nullptr, ~std::size_t{}}, static_cast(_valuesData), _size, _valuesStride}, + _interpolation, _interpolator, _before, _after}; +} + AnimationData::AnimationData(Containers::Array&& data, Containers::Array&& 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&& data, std::initializer_list 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&& data, Containers::Array&& 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 keys{{nullptr, ~std::size_t{}}, static_cast(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& AnimationData::track(UnsignedInt id) const { +Animation::TrackViewStorage 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); - return _tracks[id]._view; + "Trade::AnimationData::track(): index" << id << "out of range for" << _tracks.size() << "tracks", _tracks[0].track()); + return _tracks[id].track(); } -const Animation::TrackViewStorage& AnimationData::mutableTrack(UnsignedInt id) { +Animation::TrackViewStorage AnimationData::mutableTrack(UnsignedInt id) { + /* EW those casts! */ CORRADE_ASSERT(_dataFlags & DataFlag::Mutable, "Trade::AnimationData::mutableTrack(): the animation is not mutable", - reinterpret_cast&>(_tracks[id]._view)); + reinterpret_cast&>(static_cast&>(_tracks[0].track()))); CORRADE_ASSERT(id < _tracks.size(), "Trade::AnimationData::mutableTrack(): index" << id << "out of range for" << _tracks.size() << "tracks", - reinterpret_cast&>(_tracks[0]._view)); - return reinterpret_cast&>(_tracks[id]._view); + reinterpret_cast&>(static_cast&>(_tracks[0].track()))); + return reinterpret_cast&>(static_cast&>(_tracks[id].track())); } Containers::Array AnimationData::release() { diff --git a/src/Magnum/Trade/AnimationData.h b/src/Magnum/Trade/AnimationData.h index 816fec790..6590ed075 100644 --- a/src/Magnum/Trade/AnimationData.h +++ b/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& 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& view) noexcept; /** @overload * @@ -409,16 +412,23 @@ class AnimationTrackData { * @brief Type-erased @ref Animation::TrackView instance * @m_since_latest */ - Animation::TrackViewStorage track() const { return _view; } + Animation::TrackViewStorage 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 _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& track(UnsignedInt id) const; + Animation::TrackViewStorage 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& mutableTrack(UnsignedInt id); + Animation::TrackViewStorage 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> const Animation::TrackView& track(UnsignedInt id) const; + template> Animation::TrackView track(UnsignedInt id) const; /** * @brief Mutable track data @@ -717,7 +727,7 @@ class MAGNUM_TRADE_EXPORT AnimationData { * animation is mutable. * @see @ref dataFlags() */ - template> const Animation::TrackView& mutableTrack(UnsignedInt id); + template> Animation::TrackView mutableTrack(UnsignedInt id); /** * @brief Release data storage @@ -812,15 +822,15 @@ namespace Implementation { template inline AnimationTrackData::AnimationTrackData(AnimationTrackTarget targetName, UnsignedLong target, const Animation::TrackView& view) noexcept: AnimationTrackData{Implementation::animationTypeFor(), Implementation::animationTypeFor(), targetName, target, view} {} -template const Animation::TrackView& AnimationData::track(UnsignedInt id) const { - const Animation::TrackViewStorage& storage = track(id); +template Animation::TrackView AnimationData::track(UnsignedInt id) const { + const Animation::TrackViewStorage storage = track(id); CORRADE_ASSERT(Implementation::animationTypeFor() == _tracks[id]._type, "Trade::AnimationData::track(): improper type requested for" << _tracks[id]._type, (static_cast&>(storage))); CORRADE_ASSERT(Implementation::animationTypeFor() == _tracks[id]._resultType, "Trade::AnimationData::track(): improper result type requested for" << _tracks[id]._resultType, (static_cast&>(storage))); return static_cast&>(storage); } -template const Animation::TrackView& AnimationData::mutableTrack(UnsignedInt id) { - const Animation::TrackViewStorage& storage = mutableTrack(id); +template Animation::TrackView AnimationData::mutableTrack(UnsignedInt id) { + const Animation::TrackViewStorage storage = mutableTrack(id); CORRADE_ASSERT(Implementation::animationTypeFor() == _tracks[id]._type, "Trade::AnimationData::mutableTrack(): improper type requested for" << _tracks[id]._type, (static_cast&>(storage))); CORRADE_ASSERT(Implementation::animationTypeFor() == _tracks[id]._resultType, "Trade::AnimationData::mutableTrack(): improper result type requested for" << _tracks[id]._resultType, (static_cast&>(storage))); return static_cast&>(storage); diff --git a/src/Magnum/Trade/Test/AnimationDataTest.cpp b/src/Magnum/Trade/Test/AnimationDataTest.cpp index 78ffeb23b..ecbb5638d 100644 --- a/src/Magnum/Trade/Test/AnimationDataTest.cpp +++ b/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{ + {nullptr, 0xffffffffu}, + {nullptr, 0xffffffffu}, + Animation::Interpolation::Constant + }}; + + std::ostringstream out; + Error redirectError{&out}; + AnimationTrackData{AnimationTrackTarget::Rotation3D, 16, Animation::TrackView{ + {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{ + Containers::StridedArrayView1D{Containers::arrayCast(toomuch), 2, 32767}, + Containers::StridedArrayView1D{Containers::arrayCast(toomuch), 2, 32768}.flipped<0>(), + Animation::Interpolation::Constant + }}; + AnimationTrackData{AnimationTrackTarget::Scaling2D, 1, Animation::TrackView{ + Containers::StridedArrayView1D{Containers::arrayCast(toomuch), 2, 32768}.flipped<0>(), + Containers::StridedArrayView1D{Containers::arrayCast(toomuch), 2, 32767}, + Animation::Interpolation::Constant + }}; + + std::ostringstream out; + Error redirectError{&out}; + AnimationTrackData{AnimationTrackTarget::Scaling2D, 1, Animation::TrackView{ + Containers::StridedArrayView1D{Containers::arrayCast(toomuch), 2, 32768}, + Containers::StridedArrayView1D{Containers::arrayCast(toomuch), 2, 32767}, + Animation::Interpolation::Constant + }}; + AnimationTrackData{AnimationTrackTarget::Scaling2D, 1, Animation::TrackView{ + Containers::StridedArrayView1D{Containers::arrayCast(toomuch), 2, 32769}.flipped<0>(), + Containers::StridedArrayView1D{Containers::arrayCast(toomuch), 2, 32767}, + Animation::Interpolation::Constant + }}; + AnimationTrackData{AnimationTrackTarget::Scaling2D, 1, Animation::TrackView{ + Containers::StridedArrayView1D{Containers::arrayCast(toomuch), 2, 32767}, + Containers::StridedArrayView1D{Containers::arrayCast(toomuch), 2, 32768}, + Animation::Interpolation::Constant + }}; + AnimationTrackData{AnimationTrackTarget::Scaling2D, 1, Animation::TrackView{ + Containers::StridedArrayView1D{Containers::arrayCast(toomuch), 2, 32767}, + Containers::StridedArrayView1D{Containers::arrayCast(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;