Taking Animation::TrackViewStorage wasn't really a good idea, as it
wasn't solving anything -- in order to create it, there needs to be a
TrackView of a concrete type first anyway, and even then it required a
lot of additional verbose typing.
The new constructors take basically what TrackView takes, plus there's
additionally a constructor that takes a typeless value view together
with explicitly specified value and result types, allowing a truly
type-erased usage. On the other hand, the templated variants directly
deduce the types without any additional typing, making the construction
similarly straightforward to e.g. SceneFieldData.
In case of the type-erased constructors, if the interpolator function
isn't supplied explicitly, an implicit one is picked based on the value
type, result type and interpolation. Not all combinations make sense of
course, so this is a new assertion point, however compared to the
previous way where the interpolator was picked from a *typed* TrackView,
this doesn't add any new restriction -- what asserted there, asserts now
as well, and additionally you can't have e.g. a Quaternion track with a
boolean result. I may also be eventually adding assertions to check that
the target name matches the result type -- so e.g. a rotation isn't
specified as an integer and such. Compared to newer APIs like MeshData,
MaterialData or SceneData the AnimationData API has a significant lack
of sanity asserts like this.
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.
Given the recent issues with vertex data with size over 4 GB, I feel
this limit might get hit soon as well. So far GPUs don't support vertex
counts larger than 32 bits, so storing them in a 32-bit number matches
the limitation there. Also, a vertex is usually at least 6 bytes (for
3-component positions quantized into 16-bit ints), thus a mesh hitting
this limit would be 24 GB in size. Which fits only on the beefiest
contemporary GPUs.
However I imagine the limit might get raised eventually, for example to
support a use case of a huge sparse mesh where only sub-parts of it are
drawn, and the sub-parts have counts that fit into 32 bits.
Again, similarly to what's done for custom MeshAttribute and SceneField
values already. I'm bumping the importer interface version as adding new
virtual functions is a silent ABI breakage, but it's good to do in any
case as the AnimationTrackTarget enum was extended to 16 bits and the
values got shifted.
For consistency with what's already done for MeshAttribute and
SceneField. The ::Custom enum value is deprecated in favor of these, the
only actually breaking change is that the debug printer now subtracts
32768 for custom values (consistently with custom mesh attributes and
scene fields), while it printed the absolute enum value before.
The `Type` was suggesting it'd be some C++ type, definitely not values
like Scaling3D or Translation2D, resulting in a significant "brain
autocompletion error" every time I was using that type.
Unfortunately on AnimationData the trackTargetType() couldn't similarly
get renamed to trackTarget() as there's already trackTarget() that
contains the node ID the target points to, so it's trackTargetName()
instead. Renaming trackTarget() to trackTargetId() wasn't an option as
that would be inconsistent with everything else (TextureTools::image(),
MaterialAttribute::BaseColorTexture, SceneField::Mesh are all IDs but
they don't have an `Id` suffix); renaming to AnimationTrackTargetName
would keep it insanely long and wouldn't make it consistent either
(MeshAttribute, SceneFIeld, MaterialAttribute are all referred to as
"names" yet they don't have a `Name` suffix).
So it has 32k values for custom targets, instead of just 127. This
makes it consistent with MeshAttribute, which also provides 32k values,
while SceneField has a whole 31-bit range to make it possible to store
arbitrary ECS identifiers as well.
Since this is an ABI break, I'm also shifting the values by 1 to have
zero used for an invalid value, consistently with SceneField,
MeshAttribute etc.
Passes for SceneData but fails for MeshData due to 32-bit types used by
accident. The two also have a vastly different calculations in the range
checks, should unify that first.
Every time I push an innocent change it ends up breaking the
non-deprecated build, such as 34a91cf458.
Sigh, I really need to clean up the backwards compatibility ifdefs...
Such an unnecessary footgun -- I was already checking the other case,
having attribute data too short, but this I thought is fine because it's
not leading to any crash. Well it's leading to needless pain and
suffering, that's what it is doing!
And of course already found FIVE such bugs in just Magnum tests alone.
Uses StridedBitArrayView underneath. Waited for this container to exist,
because implementing this using bools and wasting 8x more memory wasn't
a good option. Plus, being able to address single bits opens a
possibility to describe individual bits in enum flags, whereas the only
other option would be to take the whole flag as an opaque type
containing "some bit values".
Apparently I forgot to actually test these -- in order to fit the string
data pointer without making SceneFieldData too large, it'đ stored as an
offset from fieldData. And that's something not expressible in a
constexpr context. Thus the only way to create a constexpr string field
is by using the offset-only constructor (which is now appropriately
tested).
This change at least allowed me to move the constructor to the cpp file,
saving on header size and using more lightweight assertions.
Somehow I forgot to cross-reference the SceneFieldData constructor and
mention which are usable for strings and which not, and that no builtin
fields are strings.
This got probably implemented long before the change in
c74b4c6b90. Or actually maybe not at all.
In any case, it'd cause an ambiguity with the 2D char view constructor
when the "updimensioning" StridedArrayView constructor gets introduced.
I spent some time wondering if as<PbrClearCoatMaterialData>() can be
used on a material that doesn't neccessarily have that layer. It can, so
hint that in the docs.
To match the order (and thus also verbose log) in which they're added
inside addImporterContents(), and subsequently also the order
magnum-sceneconverter processes them manually. There's no reason to have
it different and cause confusion.
Because, when it fails, it'd attempt to print them as strings,
inevitably leading to a crash or garbage in the terminal. Moreover, with
the upcoming fix that makes StringView *actually* convertible from an
ArrayView, it would become ambiguous.
Stone age APIs used here again, I can't fathom how I could live without
member slicing for so long. This also fixes an OOB access which trips up
the new ArrayView assertions -- accessing element 0 in the constructor
isn't a good thing to do if there's no data at all.
This still contained the original message which didn't say the index.
Times changed significantly since then, and the printed index really
proved to be useful.
Also fixing a OOB assertion in the test triggered by the new ArrayView
element access assertions -- the graceful assertion returns the first
element, so we have to have at least one there to return.
Vladimír Vondruš