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.
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.
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.
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.
This is the first builtin array attribute, with one of the objectives
being an ability to support an arbitrary count of per-vertex weights in
a single contiguous attribute without the complexity of having to go
through several four-component attributes.
On the shader side it still needs to get cut into at most four
components per attribute, but there's no reason for such limitation to
get propagated here as well.
Co-authored-by: Vladimír Vondruš <mosra@centrum.cz>
Finally got an idea how to provide various options store these
efficiently, so it's implemented now. Five different storage variants
times four different type sizes.
These APIs are mostly just for debugging purposes, not widely used, so
it doesn't make sense to have them as constexpr in the header. (Plus the
returned void view is useless in a constexpr context anyway.)
This header size is getting out of hand, so every stripped bit counts.
Also now that they're no longer constexpr, I can go back to using
regular assertions. The reinterpret_cast<> wasn't needed either.
I'm not sure why this restriction was there as nothing was preventing
them from being used. The attribute is only accessible through the
typeless attribute(), which gives back
`Containers::StridedArrayView2D<const char>` with second dimension size
being set to the full stride. And there it doesn't matter if the format
is an array or not.
This will be useful for joint IDs and weights, for example doing crazy
things like packing the IDs into an array of 8 4-bit numbers, saving
half the memory compared to the smallest builtin representation using
UnsignedByte[8].
A lot of pain went into creating owned mesh data even though it was
completely unnecessary in retrospect. Originally I thought "let's not
use any advanced feature" but the verbosity is just not worth it.
In the recent-ish SceneData tests I went with non-owned data already,
and it was much simpler. It's a learning process even for using my own
APIs, huh.
Otherwise it's really, REALLY hard to discover which data are missing in
the output. Especially for files with 1700 meshes, 800 materials, 3600
textures and such.
Vladimír Vondruš