This is a breaking change that changes the signature, sorry -- if you
were using concatenate() for mesh concatenation before, enjoy the new
less strange signature, if you were using it for making the mesh owned
before (which was a strange and not very well thought out use case),
please use the recently added owned() instead. I thought about adding an
overload for backwards compatibility, but it would need to allocate to
work. This way with the breakage it's ensured you actually change to the
right API.
This also cleans up a lot of ugly code in the internals and resolves one
XFAIL in removeDuplicates().
The array size is always last, defaulting to 0. This makes it consistent
with the offset-only constructor and removes two unnecessary overloads.
It's a breaking change, but I don't think array attributes have many
users yet -- and better to do this now than later. In any case, sorry
about breaking your code.
The old one is deprecated, and will be removed in a future release.
Unfortunately, to avoid deprecation warnings, all use of NoInit in the
Math library temporarily have to be Magnum::NoInit This will be cleaned
up when the deprecated alias is removed.
I also tried storing only a 32-bit index and have the base pointer stored
in ArrayEqual/ArrayHash also, but that didn't really improve amything
much (probably because the allocated items are 8-byte aligned anyway) and
only made the code a lot less clear.
One less executable to build, and we need to test more variants. The
original measured thing ("when to remove duplicates") is no longer really
relevant.
Interestingly enough the fuzzy variant isn't that much slower.
Something fishy going on in there, caused by the algorithm overwriting
the key values (and the map relying on them being immutable).
Interestingly enough the fuzzy variants works on GCC's libstdc++ even
though the key data get changed every entry, fails only on libc++.
Basically using the same idea as with the discrete version -- having the
second dimension dynamic, together with restricting the implementation to
just Float and Double.
According to the SubdivideRemoveDuplicatesBenchmark, this makes the
implementation slightly slower. I presume this is due to how minmax and
offsets are calculated which is quite cache-inefficient as it goes over
the same memory block multiple times. Added a TODO for later.
I spotted a potential bug -- and it clearly *is* a bug. The test doesn't
reduce the data size in the last dimension, leaving a duplicate (and
unused) item at the end.
I need a variant of removeDuplicatesFuzzyInPlace() that doesn't allocate
the output array but instead puts the data into a pre-existing location.
The discrete / non-fuzzy variant had this already, but this one not.
Magnum/MeshTools/RemoveDuplicates.h:373:30: Requested here: Implicit
conversion from 'std::size_t' (aka 'unsigned long') to 'float' changes
value from 18446744073709551615 to 18446744073709551616
[-Wimplicit-int-float-conversion].
Not that it would really matter at this scale, but thanks anyway.
A breaking change, sorry, but I don't want to add yet another layer of
backwards compatibility on APIs that are in master for just a month or
so. This is a test for how many people actually use these APIs -- if
nobody complains, great!
Conflating the fuzzy operation with the discrete one wasn't a good idea,
as people could be unintentionally using the (slower) fuzzy variant on
data that could be easily deduplicated using the discrete variant. One
such case is in the icosphere primitive, and I'm going to look at that
right now.
While 27f6cc309d made it easier to create
references to attribute-less meshes by avoiding a branch on
attributeCount() (and then using a constructor with explicit vertex
count), code still had to branch on isIndexed() because indices() could
be called only on indexed meshes. This change allows code like
Trade::MeshData reference{data.primitive(),
{}, data.indexData(), Trade::MeshIndexData{data.indices()},
{}, data.vertexData(), Trade::meshAttributeDataNonOwningArray(data.attributeData()),
data.vertexCount()};
which works correctly for all cases and doesn't introduce extra branches
and code paths that would need to be tested. While definitely better, I
might still give up at some point and introduce some
Trade::MeshData::nonOwningImmutableReference() helper for this.
Vladimír Vondruš