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.
Want to construct them without a GL context present, and Optional is too
wasteful. Also adding it to the AbstractGlyphCache base, where it skips
also allocating the internal PIMPL state, because it's not going to get
used for anything in a NoCreate'd instance anyway.
Especially given that nullptr causes an assert. All call sites basically
ended up passing a &font and all that extra annoyance just doesn't make
sense.
Given this API is still relatively recent, I'm not bothering with
backwards compatibility.
Which also allows the internals to be a bit simpler / potentially more
efficient, as the implementation can now access the glyph cache contents
directly, without a getter.
In basically all cases it's two independent operations so forcing them
to be done together doesn't really bring any potential efficiency
advantages. On the other hand, splitting them allows allows the caller
to better make use of available memory, as the new
renderGlyphQuadsInto() allows the input and output arrays to be aliased.
Bumping AbstractFont plugin interface versions as this is a breaking
change.
The awful original STL-heavy public API is kept the same for now, it's
just the internals being now implemented using brand new APIs that are
actually usable with multiple fonts, font sizes and runs with different
scripts/languages/directions. There's also preparation for configurable
vertical text layouting, although for now the functionality asserts that
horizontal text is used.
This also makes Renderer.h header available on non-GL builds, as the new
APIs don't rely on a class full of GL objects anymore. The class will
get eventually renamed and moved to a dedicated RendererGL.h header, but
for now this partial update has to suffice.
To be used as a placeholder argument in the reworked renderer APIs. No
intention to deal with the complexities of this right now, just taking
it to not have to deprecate & replace the API when this feature actually
gets implemented.
So it's possible to shape the text even before having all glyphs ready.
That's one reason, second reason is that this is a more common behavior
-- it usually doesn't make sense to make the text jump based on whether
it's "zaxaca", "KEKEKE" or "yqpyq".
The original alignment based on glyph bounds is now moved into dedicated
`*GlyphBounds` variants. Additionally the `*LeftGlyphBounds` were
changed to subtract the initial glyph offset as well, `*Integer` now
rounds only in the direction where it's needed because a division by 2
happened, and there's a set of `*Bottom*` values that somehow weren't
there before.
Still not good enough regression testing for the changes I need to make.
This was absolutely not verifying correct behavior of the other vertical
alignment values for multiline text.
Otherwise the assertion from TextureTools::DistanceField fires only
later during image upload, which can cause great confusion, not being
sure what's to blame, etc.
Such as is the case with fillGlyphCache(), which just maintains the
union of all glyph rectangles. Sorry for having that temporarily broken.
Also I don't really like the solution here, but was the best I could
come up with, after a bunch of failed attempts to try to do this
directly in the TextureTools::DistanceField.
Just cannot use gl_FragCoord in here. That's it, that's the fix. What's
however COMPLETELY unexpected is that this simple change made the
process significantly faster on my Intel GPU, from ~815 µs to 670! I
can't even pretend I understand what's going on here, but maybe doing
less math in the fragment shader when calculating the texture
coordinates (and thus possibly the driver having a better idea how to
prefetch or schedule?) is what made this faster? Or maybe it's due to
one uniform input less, and two interpolated values instead of four on
the way from the vertex shader?
Empty input was asserting originally, but such strict behavior doesn't
seem to be useful in practice as all application code would need to
explicitly make sure that shape() isn't called if the input or the input
range is empty. In particular, I hit this assert as soon as I rebuilt
and ran magnum-player, and the assertion didn't really uncover any bug
or make anything better by firing.
Treating zero glyph output as a failure also isn't good -- rendering
for example a string of whitespace may also result in zero glyphs on the
output, which isn't a failure. Plus it's currently unclear what should
actually be a failure -- neither FreeType nor HarfBuzz really have a
concept of the shaping operation failing, hb_shape() returns void -- so
special-casing glyphCount() being zero as a failure just doesn't make
sense.
This thus means the doScript(), doLanguage() and doDirection()
implementations are now called always, regardless of whether shape() was
called at all, or whether it produced any glyphs. This allows the shaper
for example to give back a detected script etc even if the actual
input range to shape was empty.
Replaces the previous, grossly inefficient AbstractLayouter which was
performing one virtual call per glyph (!). It's now also reusable,
meaning it doesn't need to be allocated anew for every new shaped text,
and it no longer requires each and every font plugin to implement the
same redundant glyph data fetching from the glyph cache, scaling etc. --
all that is meant to be done by the users of AbstractShaper, i.e.
Renderer. The independency on a glyph cache theorerically also means it
can be used for a completely different, non-texture-based way to render
text (such as direct path drawing directly on the GPU), although I won't
be exploring that path now.
It also exposes an interface for specifying script, language,
direction and typographic features. Such interface will be currently
only implemented in HarfBuzz, but that's the intent -- to provide a
flexible enough interface to support all possible use cases that a font
or a font plugin may support, instead of exposing a least common
denominator and then having no easy way to shape a text in a non-Latin
script or use a fancy OpenType feature the chosen font has.
The old public interface is preserved for backwards compatibility,
marked as deprecated, however the virtual APIs are not, as supporting
that would be too nasty. I don't think any user code ever implemented a
font plugin so this should be okay.
To ensure smooth transition with no regressions, the Renderer class and
MagnumFont tests still use the old API in this commit, and their test
pass the same way as they did before (except for two removed MagnumFont
test cases which tested errors that are now an assertion in the
deprecated layout() API and thus cannot be tested from the plugin
anymore). Porting them away from the deprecated API will be done in
separate commits.
Will be used for the upcoming new AbstractShaper API. It's based on the
ISO script codes, matching the HarfBuzz script enum, with the intention
of being passed to it as-is.
It was all just way too random and completely detached from real-world
cases. In particular, the mesh rectangle and the texture rectangle were
completely different, which just cannot happen, and was completely
disregarding presence of a glyph cache, making it impossible to adapt to
the upcoming AbstractLayouter / AbstractShaper rework.
It now also explicitly verifies behavior of various Alignment values
instead of just testing them at random, and there's also a new TODO
related to those.
And thus the DistanceFieldGlyphCache subclass as well. The
deinlined destructor wasn't really needed as the GL::Texture has its
destructor deinlined as well, so it wouldn't cause too much extra work
for the compiler to have it implicit.
Also, I suspect the destructor was just a leftover from when there was
no AbstractGlyphCache base.
The class now supports incremental filling, multiple fonts, texture
arrays, removes all reliance on STL containers and is finally properly
documented.
To avoid complete breakage of every use, as much as possible was kept as
deprecated APIs -- in particular the reserve() with the nasty
std::vectors, the insert() that assumes a 2D cache and a single font
and textureSize() that returns a 2D vector. Those behave the same as
before, but will assert if the cache is an array or contains more than
one font.
On the other hand, begin() / end() access with std::unordered_map iterators
(ew!) was removed as the internals simply aren't a hashmap anymore. The
image() that returned an Image2D is now used to fill the glyph cache
instead of querying its potentially processed contents, and returns a
MutableImageView3D. I considered keeping it and adding sourceImage()
instead, but such naming turned out to be too inconsistent. For querying
processed image data (such as with the distance field cache) there's a
new processedImage() query, guarded by new GlyphCacheFeature bits -- if
both ImageProcessing and ProcessedImageDownload is set, it can be used
to retrieve the processed image (so, similar as ImageDownload was
before), and if neither is set, the cache contents are queryable
directly through image(), without needing any special support from
the GPU API.
Existing code is updated only in the minimal way possible to ensure that
no serious breakage was introduced by reimplementing the deprecated APIs
on top of the new backend. Porting away from deprecated APIs will be
done in next commits. The GlyphCache and DistanceFieldGlyphCache have
their public API kept intact for now, as a similar rework will be needed
for them as well.
Additionally, the MagnumFont and MagnumFontConverter plugins aren't
compiling yet as they require substantial changes to deal with the new
glyph cache features. That is not the case with other plugins in the
magnum-plugins repository tho, for those the backwards compatibility
"just works". On the other hand, since layout of the AbstractGlyphChange
changed, I'm bumping the AbstractFont plugin interface version to
force-trigger a rebuild of dependent projects. Because I ran a stale
magnum-player binary, it worked without crashing or GL errors but just
didn't show ANY text whatsoever due to ABI differences, and I wasted
some precious minutes before realizing that a simple rebuild would fix
it.
Not yet the full extent of it including rotations and incremental
packing, as the existing interface is too awful, also patching over
zero-sized glyphs that the new packer doesn't like. Another round of
deprecations needs to happen first.
It won't contain just font metrics anymore. Also don't require the
struct to be zero-initialized if opening fails -- simply allow the
plugins to return garbage in that case and save the values only if
opening actually succeeded.
Strictly speaking this isn't an ABI change as the return value isn't
part of the function signature and the struct is still the same, so the
plugin interface version isn't bumped for this change.
All std::string arguments are now a StringView, what returned a
std::pair is now a Pair. STL compatibility headers are included on
deprecated builds to ease porting, as usual.
The only *really* breaking changes are in the internals, where an
ArrayView<const char32_t> is used instead of std::u32string, which is in
line with the change done in Utility::Unicode::utf32(); and a Triple is
returned instead of a std::tuple. Behaviorally nothing changed except
that fillGlyphCache() now asserts if the input string contains invalid
UTF-8 (which is also in line with the cahnge done in Utility::Unicode).
Not that C++ STL and exceptions would be anything to take inspiration
from, but there's std::out_of_range. Python IndexError is also specified
as "index out of range", not "bounds".
About time this got done. This also has an XFAIL for the case where a
distance field image is processed with an offset, have to fix the
underlying issue in TextureTools first.
Also added a range assertion for the distance field image setter to
match what the abstract base does, together with a corresponding getter.
Of course I used the wrong `CORRADE_FAIL_IF(false, ...)` in a few
places. Yet another confirmation it was too hard to use and a dedicated
macro is needed.
Vladimír Vondruš