Took me a while to realize that tying this to a certain hardcoded field
isn't a good idea. The new variant is useful also for example for
getting absolute light positions or just whatever else. Besides taking a
SceneField there's now also an overload taking a field ID to avoid
double lookups. The only behavioral difference compared to the old API
is that the field is now required to exist, instead of the API being a
silent no-op if not present.
Eventually these APIs may get further extended to take a BitArrayView of
objects for which to calculate the transforms, for example to take only
meshes that are a part of the hierarchy, or meshes that satisfy an
arbitrary other condition. Which will also resolve the remaining
concerns with the API. I'm still keeping it marked as experimental tho,
the usefulness isn't set in stone yet.
The old APIs are marked as deprecated and implemented using the new
ones.
So people new to the plugin stuff can quickly get to usage introduction
and code snippets. The plugins alone don't list anything like that and
it may be *very* confusing otherwise.
Ugh, that whole code looks like commited halfway through, without
even looking at it again. Comments making no sense, variables mutating
for no reason...
It dates back to 145e055b41 when the code
was written back in 2020. Probably just some debugging leftover, as the
array gets fully overwritten later.
The only vaguely related case where zero-initialization and/or garbage
memory would matter is e496241290, but
that one isn't the case for fuzzy comparison -- here each attribute gets
handled separately and any padding is skipped.
It's included in the MagnumGL library because it shouldn't be compiled
if MAGNUM_TARGET_GL is disabled. So, in other words, this was breaking
the GL-less build.
Among other things this makes it possible to use Utility::copy() instead
of a manual loop and grow arrays with std::realloc() instead of always
new'ing-copy-deleting because Containers::Pair is trivially copyable.
This fixes a "visibility attribute ignored" warning in
MultisampleTexture.cpp on newer GCC and makes the three behave
consistently without having to perform crazy ordering between function
definitions and explicit template instantiation.
It's horrible, yes, but I had to resort to the same approach in
StringView.cpp because in general every other variant was failing in
some way in some compilers. I hate everything about this.
This means I (and people making their own plugins) don't need to go and
update each and every plugin once the version in the interface string
gets bumped after a (silent) ABI break. Such as when new virtual
functions get added, as those often lead to strange crashes if the
plugins don't get rebuilt after.
The plugins will now use this macro, which means they'll
automatically embed an interface string that was present in the base
class header at build time. However, when the base class updates, the
previous string is still embedded in the plugin binary, which will then
fail to load -- this being automatic doesn't mean the original purpose
is lost. Subsequently rebuilding the plugins from source will make them
pick up the updated interface string again.
Integer / packed formats are the majority of uses for this utility, and
for them the additional complexity with NaN and infinity handling isn't
needed at all.
Running ShadersMeshVisualizerGLTest with this change in a Debug build
led to its runtime being reduced by about 35%, the total test time
excluding benchmarks went from 14.5 seconds to 11. Not bad.
The change won't make any difference in Release, but in Debug builds it
avoids calling into set() for every element. This function showed up
pretty high in profiler output for DebugTools::CompareImage, now it
doesn't anymore. In particular, for ShadersMeshVisualizerGLTest it
results in ~10% reduction in run time, which is quite significant.
The
(size - 1)/8 + 1
expression is the same as
(size - 1)/8 + 8/8
which is the same as the following in all cases except when size == 0,
for which it underflows:
(size - 1 + 8)/8 == (size + 7)/8
To avoid needless surprises, I'm changing to what's used everywhere
else, including the BitArray, and also reusing the already-calculated
value for the _data array size.
The article this API was originally based on assumes a scenario which
just *isn't* matching usual practices here, giving wrong results. Too
bad I didn't spend more time questioning the proof there and just
blindly assumed it's correct because everyone said so.
Won't be typing all that reasoning again in the commit message, see the
changelog entry and the comment in the test for details.
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.
This removes the remaining need for reinterpret_cast anything in this
class (it's moved to four places in TrackView instead), and the
constructor doesn't need to be templated anymore either.
It was there to allow creating const views, because the other one had
conflicting deduction for the V template parameter. The proper fix is to
use std::remove_const<V>::type instead.
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.
Not everything, and especially not several hundred megabytes of
animation track data. This also prepares it for recording names of
custom animation track targets that were added in the previous commits.
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.
Vladimír Vondruš