Yes, I plan to merge audio import into Trade::AbstractImporter, but
until that's done, it feels silly to have to pay for the std::string
cruft given that it's present in just two arguments.
All that work to reduce header dependencies and yet here it's full of
them, somehow.
Well, given the fallout in various GL tests now I wonder if I should
just chuck the rest along with the state allocation as a PIMPL.
"These seemed like a good idea at the time." But that was in 2013 and
while that might have been acceptable back then, it's not anymore, it's
all just a complex abstraction that prevents any reasonable resource
reuse and forces a dedicated draw call for each use.
Just do this directly from the corresponding Primitives instead.
Deprecating those allows me to eventually deprecate the (also
overengineered) ResourceManager class, at which point Magnum should be
finally free from the worst design decisions from 15 years ago.
Unfortunately several examples still rely on it (while it only makes
them more complex, not better), have to wait until those are cleaned up
first.
This was done back in October 2024 for all plugins in the magnum-plugins
repository, here it was blocked by usage in (now deprecated) MagnumFont.
In comparison the AnyImageImporter etc. plugins *are* meant to be
constructed directly by plugins that depend on them, so for those
nothing changes.
With StbTrueTypeFont no longer being embarrassingly slow due to a stupid
error, the MagnumFont plugin doesn't really provide any advantage
anymore. Originally I thought I'd update it to not be forcibly relying
on a TGA format and maybe add kerning support, but the image + text
format is extremely inefficient compared to basically anything else and
so I don't see a point in supporting it further. Marking it as
deprecated should also hint to people that using this thing is not a
good idea.
The MagnumFontConverter plugin was also the only user of the
AbstractFontConverter APIs, which right now are the last that didn't go
through a STL cleanup process. I plan to make a converter plugin for
font "minification" using HarfBuzz subsetting functionality, and not
having to support anything else that relies on the outdated APIs will
make the font converter API updates a bit easier.
Finally, the MagnumFont plugin was also the last one that directly
instantiated the TgaImporter plugin, instead of going through
AnyImageConverter. Marking it deprecated allows me to deprecate direct
instatiation in the remaining plugins as well.
The previous set of commits is from 2022 and I had them stashed in a
branch because I was trying to use those as an opportunity to tune SIMD
String APIs. Unfortunately, shortly after, other things got a priority
and I never got back to these.
The previous code was exception-ridden horrible junior implementation
dating back to early 2010s, so even if the code from 2022 might not be
giving the best possible performance, it's still a vast improvement, so
there's no reason to not use it.
Because MeshTools::removeDuplicates() uses a STL hashmap, it's quite
bad. So bad that on the Rungholt scene it takes 4 seconds out of 7.3 for
the whole import process.
Now I'm able to measure -- with the Rungholt scene from McGuire
archives, it takes it a whooping 7.3 seconds, measured with
magnum-sceneconverter --info-meshes --profile rungholt.obj
Compared to that, AssimpImporter in the default configuration takes 10.3
seconds, but with
magnum-sceneconverter --info-meshes --profile -I AssimpImporter \
-i postprocess/JoinIdenticalVertices=false,postprocess/SortByPType=false \
rungholt.obj
it takes just 4.3 seconds. And has the same vertex data size in total,
so that should probably be the default. I got some work to do, then.
And expand tests with corner cases that I previously forgot about. Can't
really provide any benchmarking info, since the Rungholt scene from
McGuire archives uses quads and negative indices.
I thought I went over all these several years ago already, but
apparently not or maybe back then not all websites were HTTPS-ready. Now
they mostly are, except for maybe one or two.
Is critically needed for the GltfImporter now, as the per-data importer
state is not useful on its own anymore and needs the Utility::Json
instance as well. And doing the same for AnyImageImporter as well for
feature parity.
And then std::tie() into mutable variables that are easy to get out of
sync. Just don't. No.
Interesting is that STL vector's emplace_back() doesn't accept the
arguments directly. Not sure why, but I'm going to drop the std::vector
next anyway.
Not file contents yet because the file can start with a XML preamble,
seven megabytes of comments and only then there's <svg. I'm not in a
mood for that today.
Same approach as done e.g. in the Ui library -- taking advantage of the
base class already allocating an internal state struct, deriving from it
and putting the state there instead of either having it as a class
member (at the cost of extra header dependency) or as a separate state
struct (at the cost of extra allocation). The only downside is a virtual
destructor in the state struct, but compared to the alternatives that's
completely fine.
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.
So the users don't accidentally create an instance with the processed
format / size being different only to get greeted by a nasty bounds or
format assertion inside flushImage() / doSetImage().
Now it's only allowed to be done by a subclass (such as
DistanceFieldGlyphCacheGL, or, ahem, MagnumFont), and doSetImage()
additionally has an assert to notify the implementer that a subclass
needs to provide its own override.
This also feels kind of messy, honestly. It's as if the
DistanceFieldGlyphCache should never have been a subclass, maybe?
Now, instead of accessing the texture directly, it uses the public
setProcessedImage() API. For which it needs to subclass the cache in
order to advertise ImageProcessing, which means the class can no longer
have the implementations private.
I don't really like this, ideally none of this would be needed and
MagnumFont would implement fillGlyphCache() instead. Not sure how to do
that yet tho as there's still the issue with DistanceFieldGlyphCacheGL
being RGBA, so this is a half-assed solution until a better one is
found... or the plugin gets ditched completely.
Like with TextureTools::DistanceField, to make room for non-GL
implementations. Old names are deprecated aliases, same for headers. The
only remaining bit in the Text library is the Renderer class, but for
that one I first need to invent a non-shitty API so I can just deprecate
the old thing as a whole and create something reasonable from scratch in
RendererGL.
The internal GL texture format (especially the R8 vs Luminance mess on
ES2) is now considered an implementation detail and shouldn't affect
common use in any way.
The format is now required always, in order to prepare for use cases
where colored glyphs are a thing as well. Additionally, to match the
recent change in AbstractGlyphCache, the processed format is specified
separately, allowing the input and processed formats to be decoupled.
Which ultimately fixes the regression on ES2 and WebGL 1 where it was no
longer possible to call font.fillyGlyphCache() on a
DistanceFieldGlyphCache.
Also, as there's now a generic format on input, another ES2-specific
issue is now fixed as well, in particular a case where a GL error
would be emitted on drivers with EXT_texture_storage because an unsized
format is passed to setStorage(). This was a problem since a long time
ago, but I ignored it because it didn't affect WebGL 1 and all drivers
that exposed EXT_texture_storage exposed EXT_texture_rg, effectively
circumventing this issue. Or so I think, at least.
The constructors taking either a GL::TextureFormat or no format at all
are now deprecated aliases to the new functionality.
This is going to get called from fillGlyphCache() that takes a string,
and thus should be better than one virtual call per character. The
single-character variant still stays, as it's a nice convenience API.
Eventually glyphSize() will get a similar treatment as well.
Took me quite a while to realize what was going on, but in retrospect
it's obvious -- the rasterizer just *rounds* the sub-pixel-positioned
glyph quads to nearest pixels. Which then can cause the neighboring
glyph data to leak to it (because the texture is then sampled not
directly on the edge pixel, but slightly outside of it), or it can also
cut away the edge, when it gets rounded in the other direction.
This was a problem with the original -- laughably inefficient -- atlas
packer as well, but because that packer had excessive padding around
everything, only the second edge-cutting artifact manifested, and that
one is rather subtle unless you know what to look for.
This means the packing is now slightly worse than before and sizes that
previously worked may no longer fit anymore. But since the new atlas
packer is relatively new (well, from September, time sure flies
different here), and the improvement compared to the original packer is
still quite massive, I don't think this is a problem.
Vladimír Vondruš