Similarly as done in Aug 2024 in Corrade. When these were a part of the
function signature, they ended up being encoded into the exported
symbol. There are still cases of StridedArrayView slice() having
enable_if in the signature, which amounts to about 18 kB symbols in all
libMagnum*-d.so libraries, but apart from that this is the state before:
$ strings libMagnum*-d.so | grep enable_if | grep -v slice | wc -c
29591
And this is after. All of those are coming from STL, thus from
old or deprecated APIs that still use std::vector, std::tuple and such,
and from the few std::sort() uses.
$ strings libMagnum*-d.so | grep enable_if | grep -v slice | wc -c
4103
In a non-deprecated build it's just this, which is a 10x reduction.
Can't really do much about these maybe exceút for implementing my own
swap() specializations (sigh?), but I think it's fine.
$ strings libMagnum*-d.so | grep enable_if | grep -v slice | wc -c
2904
I also made it consistently use
typename std::enable_if<..., int>::type = 0
instead of
class = typename std::enable_if<...>::type
because the former works correctly also in presence of overloads and
having it used consistently everywhere makes it easier to grep & change
later. All SFINAE is now also excluded from Doxygen output, because it
doesn't make much sense there. It's better to just explain the
restriction in words than with this nasty hack.
Less error-prone because of full type safety and much nicer overall. The
test code originates from a time before slice-to-a-member-function was a
thing and then it got just copied everywhere without much thought.
As the time goes, I have less and less patience trying to figure these
things out. It's likely my fault for using the not-well-tested DSA APIs
or something.
Because, searching for maxSamples() and finding it only for a
Renderbuffer, I got an impression that there isn't an equivalent for
textures. There is, so link to that in the docs.
For integer attributes, that is. This never worked and the MeshGLTest
even had an explicit test case for this, but it took me until now to
realize that this would be best caught at compile time. So now it is.
But since this functionality dates back to 2010, just removing the
values would likely break a lot of code (well, broken code, but still),
so the disallowed values are now deprecated, giving a compile time
warning when used, and are removed only on non-deprecated builds.
The wording of the ARB_compressed_texture_pixel_storage requirement is
slightly different in Texture and such, likely due to being updated there
but not here.
And of course, besides the already-existing workaround for NV where it
reports size in bits instead of bytes, Mesa reports completely broken
pixel sizes in some cases. For DX1 I get 2x2 (?!), for ASTC 10x10 I get
1x1, which is basically unfixable.
I wonder how at all these utilities were used internally with success for
so long. Or were they never used because in every case I ended up
supplying the properties manually?
IIRC this still failed when I did the last changes to these tests in
October 2024 in e9b07ef4ce, so it seems
that the driver got fixed since. There are still the broken cases in
Texture3D tho, updated in the previous commit.
Not sure what was happening here, but my suspicion is that I took what
NVidia did with BPTC when used in 3D textures as the correct thing, and
then tried to shape the test around that. Or something.
Basically, instead of the blocks being 4x4x1 as the format says, the test
overwrote the pixel storage parameters to be four times larger 4x4x4
blocks, and then went with that, trying to shape the other test variants
to match that as well. And failing really bad, because it failed on Mesa,
and the only case where where it matched the (wrong) expectation was on
NVidia and only when the custom (wrong) pixel storage was used.
In all other cases this is called outside of #ifndef CORRADE_NO_ASSERT,
so this is likely some yet-undiscovered copypaste error. OTOH it's such a
corner case of a corner case that I don't think anyone ever hits that
because it'd fail on just anything else anyway:
GL::CubeMapTexture texture;
// nothing done here, not even a format / size specified
texture.compressedSubImage(...);
This extra call originates in ae70a62644
(2015, again) and was used in all places where DSA functions were called
directly without feature-aware dispatch. Since then, some of the cases
were turned into feature-aware dispatches, which means the call isn't
needed in those anymore, so I removed it. And to clarify why is it needed
in the remaining cases I added extra comments.
The uncompressed variant preserves the buffer contents if passed an empty
view, which was done in 5ee461bdbe (2015!)
but no corresponding change was ever done for the compressed variant. So
let's make both do the same, and actually test the code path, which
wasn't done until now either.
For compressed images the expected message was completely wrong, meaning
it'd fail in any case, and they were missing a SKIP on no-assert builds.
Then, for GL::[Compressed]BufferImage the assertions in setData() were
never tested at all.
They're not parsed since 6b22a11170
(2020), so there's no point in keeping those workarounds. They're only
kept in utility application sources as they're parsed for pages, and in
tweakable implementations where it's easier to just copypaste the whole
ifdef expression from the header every time instead of modifying it to
not include DOXYGEN_GENERATING_OUTPUT.
All other image classes do that, and thus code generally assumes that
querying it is an immediate operation, not a monster switch over
hundreds of values. Plus this prepares for the future internal
representation that is just sizes + strides instead of the
overcomplicated PixelStorage madness.
Compared to Corrade, the improvement in compile time is about a minute
cumulative across all cores, or about 8 seconds on an 8-core system (~2
minutes before, ~1:52 after). Not bad at all. And this is with a
deprecated build, the non-deprecated build is 1:48 -> 1:41.
The LDR/HDR detection from 8da46ef9dc
unfortunately made Emscripten apps crash on startup if --closure was
enabled in linker flags, unless the page was run with
?magnum-disable-extensions=GL_WEBGL_compressed_texture_astc
added to the URL. The fix basically forces me to make the code not rely
on undocumented Emscripten internals anymore, which is nice, however I
now have to duplicate it because of compiler silliness and the
comment:code ratio is not getting any better either.
Back in 2020 when I wrote this I didn't really expect the MeshData to be
directly used for much more than putting them on a GPU, mostly because
that used to be the primary use case with the old MeshData2D /
MeshData3D. So the documentation was focusing mainly on populating a GPU
mesh, and any docs for CPU-side access were added rather hastily.
As now the asset processing use case is much larger, the original docs no
longer made sense. Let's hope this is better.
New since 7ca7e5a62b, and no, I'm not going
to switch from enums to some static constexpr int. Unless this
changed in recent standards, it still means one can take an address of
it. Which shouldn't be possible for a constant as that could
unnecessarily pessimize its perf.
Neither of those are really critically important. Failure not being a
failure is fine (just don't make the shader broken in the first place),
multiple color output bindings should be in the shader source anyway.
This was quite a mess, the whole array name copypasted to each and every
access. I mean, yeah, originally I thought this would be *the* usage
pattern, but oh god this resulted in SO MANY copypaste errors.
Which ultimately means the two annoying NVidia failures in the
TextureGLTest related to pixel storage in compressed 3D images are no
longer failing as a result of cleanup.
Vladimír Vondruš