I wanted to just ignore these, but the RT builds for some reason have
warnings-as-error enabled by default and, UGH, I just want this thing to
finally pass the CI.
Not only the implementation is a template hell full of its own problems,
the internals are also just wrapping trivial bits of STL without adding
anything of value on top, so ultimately any code that actually used this
thing ended up being slower to build, more verbose, harder to reason
about and with thousands of tiny little inefficiencies all over the
place. So let's just finally say no to this idea from 2012.
"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.
I'm going to deprecate the (awfully inefficient)
DebugTools::ForceRenderer and ObjectRenderer, and need at least
something else to direct the users to. This should suffice.
The NoInit constructor no longer results in an Array with a custom
deleter which would prevent the resulting MeshData from being used in
plugins, and all contents are subsequently overwritten, so there's no
reason to not use it.
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.
Basically the same logic as with Trade::AbstractImporter::openMemory(),
the doOpenData() signature is (yet again) changed to accept a pair of a
r-value Array reference and a DataFlags describing whether the memory is
owned or not. Compared to Trade, I don't anticipate any in-place
mutation of open fonts, so there's no DataFlags::Mutable.
There's also no DataFlags::Global because data of the opened font aren't
exposed further anywhere (so far at least, but I can imagine SVG data
eventually possibly passed through to importers, which then could be a
use case for this flag).
This is the final breaking change to the AbstractFont doOpenData() API.
Adapting of all other code to these changes will be done in a subsequent
commit, so right now a non-deprecated build still fails.
This was apparently never tested, so accidentally omitting the close()
call during a refactor wouldn't trigger any test failure until
accidental discovery months later. 100% coverage and yet.
This will eventually make it possible to create always-open font
implementations, as in those doProperties() can be called on-demand
without requiring a dummy implementation of doOpenFile() / doOpenData()
and doClose() that is there only to fill the Properties struct.
Thanks to the doOpenFile() / doOpenData() signature changing in the
previous commit I can do this in a backwards-compatible way, keeping the
original signature as a deprecated API. Changing just the return value
would be a hard breaking change.
As with previous commit, only AbstractFont tests themselves are adapted
to the changes, not the rest yet, so a non-deprecated build will now
fail.
The AbstractFont::openFile() and openData() now get an additional
argument specifying a font index to allow picking a concrete font index
for example in a TTC collection.
This index has to be specified upfront with no possibility to change it
afterwards, because that's how both FreeType and stb_truetype work. Thus
there are also new fileFontCount() and dataFontCount() APIs taking a
filename / data argument, instead of this being a fontCount() query on
an opened file. The implication from this is that -- unlike basically
all other APIs in Trade and elsewhere -- specifying an out-of-bounds
font index isn't an assertion (i.e., a programmer error) but rather a
graceful failure, because requireing the user to first call
fileFontCount() and then openFile() would mean having to open and
parse the same file twice, which is undesirable overhead.
While this isn't breaking for end users, as it's just a new optional
argument to openFile() and openData(), it's a breaking change for plugin
interfaces. The old doOpenFile() and doOpenData() interfaces are still
there to help transitioning existing plugins, but are marked as
deprecated. Delegating to those turned out to be quite involved, so
there are many new tests verifying this compatibility code path.
The plugin interface string version is bumped but in the next commits
I'm making use of the doOpenData() / doOpenFile() API breakage to do
more changes. Those won't result in further plugin interface changes, so
this set of commits should be treated as a single indivisible change to
the plugin interface. For this reason, uses of AbstractFont outside of
AbstractFontTest (including the MagnumFont plugin) aren't adapted yet
-- they pass tests as before, but compile only with deprecated APIs
enabled.
Co-authored-by: Andrew Snyder <asnyder@minitab.com>
They're now uncovered, which only makes it harder to spot missing
coverage in non-deprecated code. And testing is simply a matter of
copypasting the existing tests.
It's done like this in all other code, not sure why not here. Likely
because debug output wasn't tested at all before and only got added
later, thus at the end? So make it consistent.
In some cases the plugin manager constructor was not tested at all,
leading to an uncovered line. For importers also verify that calling
close() on a non-opened instance doesn't call into doClose().
Again, the Audio importer isn't touched at all, as that interface is
scheduled for mergin into Trade.
So far, even though code coverage says it's all fine, things like data
conversion failures were not tested at all, or only by checking the
return value, but without verifying that the implementation got actually
called, and without checking that no superfluous message was printed by
accident.
The AbstractFontConverter interface is tested for failures as well even
though most of it is scheduled for rework (STL removal, etc.), as
otherwise it'd be likely that I forget to test the failure cases when
simply porting the old test code to the new APIs. On the other hand, the
Audio importer interface is not touched as it's scheduled for merging
into Trade.
Glyph cache implementations in the Text library pick features based on
those, and while I'm pretty sure I tested both codepaths at some point,
not having them automatically covered makes them prone to code rot.
This was already done in Magnum Extras, but because here the code
coverage is not 99.99% I never actually saw these. Until now, where both
Text::AbstractShaper and Text::AbstractGlyphCache report destructor
*declaration* as uncovered. Destructor definition is covered, obviously,
so reporting this as uncovered is wrong.
Code from 2012 still wants to fight back. Turns out the user
implementation of doOpenFile() was never tested, only the fallback paths
to doOpenData(). Heh.
Vladimír Vondruš