Yet another broken behavior with compressed textures on NVidia unearthed
by the recent changes with compressed block properties being set almost
always.
And make resetState() aware of ARB_compressed_texture_pixel_storage so
when it's not supported, it doesn't cause the very next compressed image
upload or download to fire a GL error due to an unknown state being set.
While the ES spec seems to say that these are all ignored when uploading
a compressed image (and so resetting them shouldn't be needed), with a
WebGL 2 build Chrome is complaining that the pixel unpack parameters are
invalid if they're not explicitly reset to zero before the compressed
upload.
With the recent changes where compressed image block properties no
longer need to be queries from GL, we also no longer need to query
GL_TEXTURE_COMPRESSED_IMAGE_SIZE, and thus we don't need the NV-specific
workaround where the returned value was sometimes broken for cubemaps.
With this change, neither GL is queried for compressed block size
properties nor they're taken from CompressedPixelStorage anymore. For
image upload they're taken directly from the passed image and set to
GL's pixel pack state, for image download they're taken from known
GL::CompressedPixelFormat properties set to GL's pixel unpack
state and saved to the image.
Besides removing a bunch of checks from tests, there isn't anything new
to *add* there -- everything should work just as before, or
(assuming shitty drivers with broken format queries) better.
Also, in some cases the internal format queries were made into a
zero-initialized output variable to ensure consistent behavior with
broken drivers. That's now done in all cases, with a lengthy comment to
ensure this doesn't get "cleaned up" by accident.
Similarly like pixel size is stored in uncompressed images, block size
makes it possible to perform size checks on passed data, slice the images
and so on. It only took over a decade to get that done.
The block properties coming from CompressedPixelStorage are currently
expected to be either not set at all or exactly match what's stored in
the image for given format. The PixelStorage will get eventually
deprecated in favor of a simpler and more flexible representation, but
that's another big chunk of work so it's first done like this.
The GL library tests currently blow up on various assertions and it
isn't yet updated to make use of the known format properties instead of
querying them from GL. That'll be done in the following commits.
The tests now pass, but this damn GL pixel storage API is so convoluted
with so many redundant and mutually conflicting degrees of freedom that
it's impossible to be sure. Looking forward to when I can finally drop
that thing and calculate it on the fly from just size + stride, like it
should have been from the start.
It's used only there and only to supply a silly argument to a broken API.
Unfortunately back when adding this utility in 2015 I didn't document
what was it for, which initially made me think it's there for some
suspicious reason. Well, the reason is suspicious, but for an entirely
different reason.
This was quite nasty, a multi-day effort to trim this down and then
increasingly growing disappointment as I discovered it was affecting
basically any use of the API.
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.
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.
Following the spirit of extension-based functionality, the entrypoints
are available always but do something (i.e., call the actual WebGL API)
only if the extension is advertised. Which it is only on Emscripten
3.1.66+ because older versions don't have the corresponding entrypoints,
so there it's marked as disabled.
Additionally, EXT_polygon_offset_clamp is now also working on 3.1.66+,
but there's no wrapper for it yet.
This "type erased std::vector member" was done in the times before
growable arrays were a thing, and kind of made sense to go the extra way
to avoid a <vector> include in the header. Except that it made rather
unportable assumptions about std::vector size, which weren't correct for
example with _GLIBXX_ASSERTIONS set.
But what was *completely* unacceptable was that the vector was of one or
another type depending on the GL feature set present in the current
context. Apart from adding a lot of extra *nasty* logic to construction,
moves and destruction, this approach led to the mesh instance asking the
current context on destruction in order to know whether a destructor
should be called on std::vector<Buffer> or std::vector<AttributeLayout>.
Ugh.
Now it's a regular Array member (which isn't *that* heavy to need such
type-erased treatment, although it eventually could be), and thanks to
the AttributeLayout packing improvements in previous commits it's no
longer prohibitively wasteful to just abuse AttributeLayout instances to
store just owning Buffer instances alone -- doing so now wastes only 16
bytes per buffer, compared to 36 before. Given there's usually just one
or two vertex buffers per mesh (compared to attributes, which are
usually 4 or more), it should be fine.
The MeshGLTest::destructMovedOutInstance() test added few commits back
also no longer asserts on no GL context being present.
Originally GL::hasTextureFormat() returned false on ES2 for
PixelFormat::R8Unorm, RG8Unorm, RGB8Unorm and RGBA8Unorm because
glTexStorage() didn't work with the matching Luminance, LuminanceAlpha,
RGB and RGBA formats. But since the only ES2 platform is nowadays
basically just WebGL 1, which has neither EXT_texture_rg nor
EXT_texture_storage, this implicit failure made no sense and just made
the textureFormat() (and the new genericPixelFormat() API) useless
there.
Now it maps to them, and it's up to the caller to make sure
glTexStorage() doesn't get called with those, only glTexImage does.
Furthermore, if formats from EXT_texture_rg are used, the
genericPixelFormat() now also provides inverse mapping of them back to
the generic PixelFormat. Before it was basically *no* ES2 TextureFormat
that'd work with either of these, now it's all that have a (vaguely)
corresponding PixelFormat.
An ad-hoc solution was already done in DebugTools::screenshot(), now I
need it in another place. While not as fast as the O(1) mapping from
the generic format to the API-specific ones due to the potentially
linear lookup, it definitely could be useful in general.
Only noticed this now when adding inverse mapping. Sigh. OTOH, with the
inverse mapping in place this will no longer be possible to happen, as
it would cause a compile error due to a duplicate switch case.
Partially needed to avoid build breakages because Corrade itself
switched as well, partially because a cleanup is always good. Done
except for (STL-heavy) code that's deprecated or SceneGraph-related APIs
that are still quite full of STL as well.
The GL::Renderer::setClearDepth() and setDepthRange() APIs now use the
non-clamping NV entrypoints if available. The float overloads do that
too, to avoid differences in behavior depending on whether these
functions are called with a float or a double type.
Instead of having them wrapped in extra functions for
extension-dependent functionality. The only case that stays is the line
width range implementation, as the raw variant looked too ugly /
dangerous otherwise.
"Funny" how this is the only API where I can't use glCreateTextures().
Like, it would have been so easy to just stop teaching glGen*() and
all their quirks and "this ID exists but it's not an object until you
bind it somewhere actually" to people altogether, BUT NO! FFS.
And use static functions with an explicit "self" pointer instead. Those
have half the size (8 vs 16 bytes on 64bit x86), which in turn reduces
the state tracker memory use by about 750 bytes. On desktop GL with an
Intel GPU & Mesa this reduces the state tracker allocation size by almot
10%, from 8.3 kB to 7.6 kB. Not bad.
Apart from small memory savings, this also removes the need to include
the full class definiton from the State headers on MSVC (because
on that compiler the member function pointer size is different based on
whether the type definition is known or not, IMAGINE THAT BEING A
FEATURE AND NOT A BUG), leading to less header dependencies and better
incremental compile times there.
This was already done in some cases (and the Vk library used this from
the beginning), and as I'm about to add some more extension-dependent
functionality it felt like a good time to finish that change, finally.
In some cases the *Implementation() could even be dropped in favor of
pointing to the GL API directly (such as is already done for various
glUniform*() calls), that'd be another step -- this is good enough for
now.
And this, this change allows the growable Array to use malloc() instead
of new, and thus also realloc(), saving unnecessary reallocations if the
memory can be grown in-place. All because Containers::Pair is trivially
copyable while std::pair wasn't.
There isn't any good reason to use the STL anymore.
Hm, and here I used the "capability" of std::pair that allowed it to
store references. I don't even want to know what all was involved to
support that, Containers::Reference is much easier to reason about.
Apparently even `= {}` was broken for std::pair once, not to mention the
unnecessary extra overhead with this type not being trivially copyable.
Good riddance.
Same as in the previous commit, most cases are inputs so a StringStl.h
compatibility include will do, the only breaking change is
GL::Shader::sources() which now returns a StringIterable instead of a
std::vector<std::string> (ew).
Awesome about this whole thing is that The Shader API now allows
creating a shader from sources coming either from string view literals
or Utility::Resource completely without having to allocate any strings
internally, because all those can be just non-owning references wrapped
with String::nullTerminatedGlobalView(). The only parts which aren't
references are the #line markers, but (especially on 64bit) those can
easily fit into the 22-byte (or 10-byte on 32bit) SSO storage.
Also, various Shader constructors and assignment operators had to be
deinlined in order to avoid having to include the String header, which
would be needed for Array destruction during a move.
Co-authored-by: Hugo Amiard <hugo.amiard@wonderlandengine.com>
Most of these are just inputs, so a compatibility StringStl.h include
will do, the only exception is the callback for which there needs to
stay a deprecated overload (which is internally delegated from the
StringView one).
Also explicitly testing with non-null-terminated strings -- the APIs
take an explicit size so it shouldn't be a problem, but it's always good
to have this verified independently. Drivers are crap, you know.
One consequence of no longer using an impossible-to-forward-declare
std::string is that I had to deinline the DebugGroup constructor because
it no longer worked with just a forward-declared StringView.
Vladimír Vondruš