From 263693f2b27731c070ca77b89826b807b52187a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Mon, 25 Mar 2024 18:50:53 +0100 Subject: [PATCH] Text: make glyph caches pad by one pixel by default to avoid artifacts. 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. --- src/Magnum/Text/AbstractGlyphCache.cpp | 6 +-- src/Magnum/Text/AbstractGlyphCache.h | 45 ++++++++++++++--- src/Magnum/Text/GlyphCache.h | 10 ++-- src/Magnum/Text/Test/AbstractFontTest.cpp | 3 +- .../Text/Test/AbstractGlyphCacheTest.cpp | 49 +++++++++++++------ src/Magnum/Text/Test/RendererGLTest.cpp | 3 +- src/Magnum/Text/Test/RendererTest.cpp | 11 +++-- .../Test/MagnumFontConverterTest.cpp | 3 +- 8 files changed, 93 insertions(+), 37 deletions(-) diff --git a/src/Magnum/Text/AbstractGlyphCache.cpp b/src/Magnum/Text/AbstractGlyphCache.cpp index 40ee07383..aeec8143f 100644 --- a/src/Magnum/Text/AbstractGlyphCache.cpp +++ b/src/Magnum/Text/AbstractGlyphCache.cpp @@ -132,16 +132,16 @@ AbstractGlyphCache::AbstractGlyphCache(const PixelFormat format, const Vector3i& arrayAppend(_state->fonts, InPlaceInit, 0u, nullptr); } -AbstractGlyphCache::AbstractGlyphCache(const PixelFormat format, const Vector3i& size): AbstractGlyphCache{format, size, {}} {} +AbstractGlyphCache::AbstractGlyphCache(const PixelFormat format, const Vector3i& size): AbstractGlyphCache{format, size, Vector2i{1}} {} AbstractGlyphCache::AbstractGlyphCache(const PixelFormat format, const Vector2i& size, const Vector2i& padding): AbstractGlyphCache{format, Vector3i{size, 1}, padding} {} -AbstractGlyphCache::AbstractGlyphCache(const PixelFormat format, const Vector2i& size): AbstractGlyphCache{format, size, {}} {} +AbstractGlyphCache::AbstractGlyphCache(const PixelFormat format, const Vector2i& size): AbstractGlyphCache{format, size, Vector2i{1}} {} #ifdef MAGNUM_BUILD_DEPRECATED AbstractGlyphCache::AbstractGlyphCache(const Vector2i& size, const Vector2i& padding): AbstractGlyphCache{PixelFormat::R8Unorm, size, padding} {} -AbstractGlyphCache::AbstractGlyphCache(const Vector2i& size): AbstractGlyphCache{PixelFormat::R8Unorm, size, {}} {} +AbstractGlyphCache::AbstractGlyphCache(const Vector2i& size): AbstractGlyphCache{PixelFormat::R8Unorm, size, Vector2i{1}} {} #endif AbstractGlyphCache::AbstractGlyphCache(NoCreateT) noexcept {} diff --git a/src/Magnum/Text/AbstractGlyphCache.h b/src/Magnum/Text/AbstractGlyphCache.h index fe629a7ff..4bc4945d8 100644 --- a/src/Magnum/Text/AbstractGlyphCache.h +++ b/src/Magnum/Text/AbstractGlyphCache.h @@ -245,6 +245,27 @@ font-specific glyph IDs that resolved to @cpp 0 @ce with @ref glyphId(), rasterize them to the cache in the next round, and then update the rendered text again. +@section Text-AbstractGlyphCache-padding Glyph padding + +Fonts commonly shape the glyphs to sub-pixel positions, the GPU rasterizer then +rounds the actual quads to nearest pixels, which then can lead to neighboring +pixels to leak into the rendered glyph or, conversely, the rendered glyph +having an edge cut. Because of that, by default the glyphs are padded with one +pixel on each side, which should prevent such artifacts even if rendering the +glyphs 2x supersampled. + +If you have a pixel-perfect font that gets always shaped to whole pixel +positions, you can set it back to zero in the constructor. Or for example if +you can ensure that at least lines get placed on whole pixel positions and the +line advance is whole pixels as well, you can se it to zero in one dimension at +least, assuming neither @ref TextureTools::AtlasLandfillFlag::RotatePortrait +nor @relativeref{TextureTools::AtlasLandfillFlag,RotateLandscape} is enabled in +@ref atlas(). + +On the other hand, if you're rendering more than 2x supersampled or for example +using the cache for generating a distance field, you may want to increase the +padding even further. + @section Text-AbstractGlyphCache-subclassing Subclassing A subclass needs to implement the @ref doFeatures() and @ref doSetImage() @@ -264,14 +285,16 @@ class MAGNUM_TEXT_EXPORT AbstractGlyphCache { * @brief Construct a 2D array glyph cache * @param format Source image format * @param size Source image size in pixels - * @param padding Padding around every glyph in pixels + * @param padding Padding around every glyph in pixelss. See + * @ref Text-AbstractGlyphCache-padding for more information about + * the default. * @m_since_latest * * The @p size is expected to be non-zero. * @see @ref AbstractGlyphCache(PixelFormat, const Vector2i&, const Vector2i&) */ #ifdef DOXYGEN_GENERATING_OUTPUT - explicit AbstractGlyphCache(PixelFormat format, const Vector3i& size, const Vector2i& padding = {}); + explicit AbstractGlyphCache(PixelFormat format, const Vector3i& size, const Vector2i& padding = Vector2i{1}); #else /* To not need to include Vector */ explicit AbstractGlyphCache(PixelFormat format, const Vector3i& size, const Vector2i& padding); @@ -282,7 +305,9 @@ class MAGNUM_TEXT_EXPORT AbstractGlyphCache { * @brief Construct a 2D glyph cache * @param format Source image format * @param size Source image size in pixels - * @param padding Padding around every glyph in pixels + * @param padding Padding around every glyph in pixels. See + * @ref Text-AbstractGlyphCache-padding for more information about + * the default. * @m_since_latest * * Equivalent to calling @@ -290,7 +315,7 @@ class MAGNUM_TEXT_EXPORT AbstractGlyphCache { * with depth set to @cpp 1 @ce. */ #ifdef DOXYGEN_GENERATING_OUTPUT - explicit AbstractGlyphCache(PixelFormat format, const Vector2i& size, const Vector2i& padding = {}); + explicit AbstractGlyphCache(PixelFormat format, const Vector2i& size, const Vector2i& padding = Vector2i{1}); #else /* To not need to include Vector */ explicit AbstractGlyphCache(PixelFormat format, const Vector2i& size, const Vector2i& padding); @@ -301,7 +326,9 @@ class MAGNUM_TEXT_EXPORT AbstractGlyphCache { /** * @brief Construct a 2D glyph cache * @param size Source image size in pixels - * @param padding Padding around every glyph in pixels + * @param padding Padding around every glyph in pixelss. See + * @ref Text-AbstractGlyphCache-padding for more information about + * the default. * * Calls @ref AbstractGlyphCache(PixelFormat, const Vector2i&, const Vector2i&) * with @p format set to @ref PixelFormat::R8Unorm. @@ -309,7 +336,7 @@ class MAGNUM_TEXT_EXPORT AbstractGlyphCache { * instead. */ #ifdef DOXYGEN_GENERATING_OUTPUT - explicit AbstractGlyphCache(const Vector2i& size, const Vector2i& padding = {}); + explicit AbstractGlyphCache(const Vector2i& size, const Vector2i& padding = Vector2i{1}); #else /* To not need to include Vector */ CORRADE_DEPRECATED("use AbstractGlyphCache(PixelFormat, const Vector2i&, const Vector2i&) instead") explicit AbstractGlyphCache(const Vector2i& size, const Vector2i& padding); @@ -385,7 +412,11 @@ class MAGNUM_TEXT_EXPORT AbstractGlyphCache { CORRADE_DEPRECATED("use size() instead") Vector2i textureSize() const; #endif - /** @brief Glyph padding */ + /** + * @brief Glyph padding + * + * @see @ref Text-AbstractGlyphCache-padding + */ Vector2i padding() const; /** diff --git a/src/Magnum/Text/GlyphCache.h b/src/Magnum/Text/GlyphCache.h index dfd6aabd4..b8db17cd9 100644 --- a/src/Magnum/Text/GlyphCache.h +++ b/src/Magnum/Text/GlyphCache.h @@ -80,9 +80,10 @@ class MAGNUM_TEXT_EXPORT GlyphCache: public AbstractGlyphCache { * @brief Constructor * * Same as calling the above with @p originalSize and @p size being set - * to the same value. + * to the same value. See @ref Text-AbstractGlyphCache-padding for more + * information about the default @p padding. */ - explicit GlyphCache(GL::TextureFormat internalFormat, const Vector2i& size, const Vector2i& padding = {}); + explicit GlyphCache(GL::TextureFormat internalFormat, const Vector2i& size, const Vector2i& padding = Vector2i{1}); /** * @brief Constructor @@ -103,9 +104,10 @@ class MAGNUM_TEXT_EXPORT GlyphCache: public AbstractGlyphCache { * @brief Constructor * * Same as calling the above with @p originalSize and @p size being set - * to the same value. + * to the same value. See @ref Text-AbstractGlyphCache-padding for more + * information about the default @p padding. */ - explicit GlyphCache(const Vector2i& size, const Vector2i& padding = {}); + explicit GlyphCache(const Vector2i& size, const Vector2i& padding = Vector2i{1}); /** * @brief Construct without creating the internal state and the OpenGL texture object diff --git a/src/Magnum/Text/Test/AbstractFontTest.cpp b/src/Magnum/Text/Test/AbstractFontTest.cpp index a01e216b7..b364a0ca2 100644 --- a/src/Magnum/Text/Test/AbstractFontTest.cpp +++ b/src/Magnum/Text/Test/AbstractFontTest.cpp @@ -1228,7 +1228,8 @@ void AbstractFontTest::layout() { font.openFile({}, {}); CORRADE_COMPARE(font.size(), 0.5f); - DummyGlyphCache cache{PixelFormat::R8Unorm, {10, 20}}; + /* Default padding is 1 to avoid artifacts, set that to 0 to simplify */ + DummyGlyphCache cache{PixelFormat::R8Unorm, {10, 20}, {}}; UnsignedInt fontId = cache.addFont(15, &font); diff --git a/src/Magnum/Text/Test/AbstractGlyphCacheTest.cpp b/src/Magnum/Text/Test/AbstractGlyphCacheTest.cpp index df0e7b2e7..8f0b40045 100644 --- a/src/Magnum/Text/Test/AbstractGlyphCacheTest.cpp +++ b/src/Magnum/Text/Test/AbstractGlyphCacheTest.cpp @@ -309,7 +309,8 @@ void AbstractGlyphCacheTest::constructNoPadding() { DummyGlyphCache cache{PixelFormat::R32F, {1024, 512, 3}}; CORRADE_COMPARE(cache.format(), PixelFormat::R32F); CORRADE_COMPARE(cache.size(), (Vector3i{1024, 512, 3})); - CORRADE_COMPARE(cache.padding(), Vector2i{}); + /* 1 by default to avoid artifacts */ + CORRADE_COMPARE(cache.padding(), Vector2i{1}); CORRADE_COMPARE(cache.fontCount(), 0); /* Invalid glyph is always present */ CORRADE_COMPARE(cache.glyphCount(), 1); @@ -317,7 +318,7 @@ void AbstractGlyphCacheTest::constructNoPadding() { CORRADE_COMPARE(cache.atlas().size(), (Vector3i{1024, 512, 3})); CORRADE_COMPARE(cache.atlas().filledSize(), (Vector3i{1024, 512, 0})); CORRADE_COMPARE(cache.atlas().flags(), TextureTools::AtlasLandfillFlag::WidestFirst); - CORRADE_COMPARE(cache.atlas().padding(), Vector2i{}); + CORRADE_COMPARE(cache.atlas().padding(), Vector2i{1}); CORRADE_COMPARE(cache.image().format(), PixelFormat::R32F); CORRADE_COMPARE(cache.image().size(), (Vector3i{1024, 512, 3})); @@ -356,14 +357,15 @@ void AbstractGlyphCacheTest::construct2DNoPadding() { DummyGlyphCache cache{PixelFormat::R32F, {1024, 512}}; CORRADE_COMPARE(cache.format(), PixelFormat::R32F); CORRADE_COMPARE(cache.size(), (Vector3i{1024, 512, 1})); - CORRADE_COMPARE(cache.padding(), Vector2i{}); + /* 1 by default to avoid artifacts */ + CORRADE_COMPARE(cache.padding(), Vector2i{1}); CORRADE_COMPARE(cache.fontCount(), 0); /* Invalid glyph is always present */ CORRADE_COMPARE(cache.glyphCount(), 1); CORRADE_COMPARE(cache.atlas().size(), (Vector3i{1024, 512, 1})); CORRADE_COMPARE(cache.atlas().filledSize(), (Vector3i{1024, 0, 1})); CORRADE_COMPARE(cache.atlas().flags(), TextureTools::AtlasLandfillFlag::WidestFirst); - CORRADE_COMPARE(cache.atlas().padding(), Vector2i{}); + CORRADE_COMPARE(cache.atlas().padding(), Vector2i{1}); /* The rest shouldn't be any different */ } @@ -416,13 +418,14 @@ void AbstractGlyphCacheTest::constructDeprecatedNoPadding() { CORRADE_IGNORE_DEPRECATED_PUSH CORRADE_COMPARE(cache.textureSize(), (Vector2i{1024, 512})); CORRADE_IGNORE_DEPRECATED_POP - CORRADE_COMPARE(cache.padding(), Vector2i{}); + /* 1 by default to avoid artifacts */ + CORRADE_COMPARE(cache.padding(), Vector2i{1}); CORRADE_COMPARE(cache.fontCount(), 0); CORRADE_COMPARE(cache.glyphCount(), 1); CORRADE_COMPARE(cache.atlas().size(), (Vector3i{1024, 512, 1})); CORRADE_COMPARE(cache.atlas().filledSize(), (Vector3i{1024, 0, 1})); CORRADE_COMPARE(cache.atlas().flags(), TextureTools::AtlasLandfillFlag::WidestFirst); - CORRADE_COMPARE(cache.atlas().padding(), Vector2i{}); + CORRADE_COMPARE(cache.atlas().padding(), Vector2i{1}); } #endif @@ -538,7 +541,8 @@ void AbstractGlyphCacheTest::setInvalidGlyph2D() { void AbstractGlyphCacheTest::setInvalidGlyphOutOfRange() { CORRADE_SKIP_IF_NO_ASSERT(); - DummyGlyphCache cache{PixelFormat::R32F, {1024, 512, 3}}; + /* Default padding is 1, test that it works for zero as well */ + DummyGlyphCache cache{PixelFormat::R32F, {1024, 512, 3}, {}}; std::ostringstream out; Error redirectError{&out}; @@ -804,7 +808,10 @@ void AbstractGlyphCacheTest::addGlyphIndexOutOfRange() { void AbstractGlyphCacheTest::addGlyphAlreadyAdded() { CORRADE_SKIP_IF_NO_ASSERT(); - DummyGlyphCache cache{PixelFormat::R32F, {1024, 512, 3}}; + /* Default padding of 1 makes it impossible to add a glyph at zero offset + as it's out of range. Don't want to bother with that here so resetting + it to 0. */ + DummyGlyphCache cache{PixelFormat::R32F, {1024, 512, 3}, {}}; cache.addFont(9); UnsignedInt fontId = cache.addFont(3); @@ -822,7 +829,8 @@ void AbstractGlyphCacheTest::addGlyphAlreadyAdded() { void AbstractGlyphCacheTest::addGlyphOutOfRange() { CORRADE_SKIP_IF_NO_ASSERT(); - DummyGlyphCache cache{PixelFormat::R32F, {1024, 512, 3}}; + /* Default padding is 1, test that it works for zero as well */ + DummyGlyphCache cache{PixelFormat::R32F, {1024, 512, 3}, {}}; UnsignedInt fontId = cache.addFont(9); @@ -889,7 +897,10 @@ void AbstractGlyphCacheTest::addGlyphOutOfRangePadded() { void AbstractGlyphCacheTest::addGlyphTooMany() { CORRADE_SKIP_IF_NO_ASSERT(); - DummyGlyphCache cache{PixelFormat::R32F, {1024, 512}}; + /* Default padding of 1 makes it impossible to add a glyph at zero offset + as it's out of range. Don't want to bother with that here so resetting + it to 0. */ + DummyGlyphCache cache{PixelFormat::R32F, {1024, 512}, {}}; /* Adding a font with over 65k potential glyphs is okay */ UnsignedInt fontId = cache.addFont(100000); @@ -1472,8 +1483,8 @@ void AbstractGlyphCacheTest::processedImageNotImplemented() { } void AbstractGlyphCacheTest::access() { - /* Padding tested well enough in addGlyph(), not using it here */ - DummyGlyphCache cache{PixelFormat::R32F, {1024, 512, 3}}; + /* Padding tested well enough in addGlyph(), resetting it back to 0 here */ + DummyGlyphCache cache{PixelFormat::R32F, {1024, 512, 3}, {}}; cache.setInvalidGlyph({5, 7}, 2, {{5, 10}, {15, 30}}); @@ -1540,8 +1551,8 @@ void AbstractGlyphCacheTest::access() { } void AbstractGlyphCacheTest::accessBatch() { - /* Padding tested well enough in addGlyph(), not using it here */ - DummyGlyphCache cache{PixelFormat::R32F, {1024, 512, 3}}; + /* Padding tested well enough in addGlyph(), resetting it back to 0 here */ + DummyGlyphCache cache{PixelFormat::R32F, {1024, 512, 3}, {}}; cache.setInvalidGlyph({5, 7}, 2, {{5, 10}, {15, 30}}); @@ -1594,7 +1605,10 @@ void AbstractGlyphCacheTest::accessInvalid() { /* Silly test name, but these all test debug asserts while accessBatchInvalid() tests non-debug asserts */ - DummyGlyphCache cache{PixelFormat::R32F, {1024, 512, 3}}; + /* Default padding of 1 makes it impossible to add a glyph at zero offset + as it's out of range. Don't want to bother with that here so resetting + it to 0. */ + DummyGlyphCache cache{PixelFormat::R32F, {1024, 512, 3}, {}}; cache.addFont(9); UnsignedInt fontId = cache.addFont(3); @@ -1628,7 +1642,10 @@ void AbstractGlyphCacheTest::accessInvalid() { void AbstractGlyphCacheTest::accessBatchInvalid() { CORRADE_SKIP_IF_NO_ASSERT(); - DummyGlyphCache cache{PixelFormat::R32F, {1024, 512, 3}}; + /* Default padding of 1 makes it impossible to add a glyph at zero offset + as it's out of range. Don't want to bother with that here so resetting + it to 0. */ + DummyGlyphCache cache{PixelFormat::R32F, {1024, 512, 3}, {}}; cache.addFont(9); UnsignedInt fontId = cache.addFont(3); diff --git a/src/Magnum/Text/Test/RendererGLTest.cpp b/src/Magnum/Text/Test/RendererGLTest.cpp index 9b416c626..4093bb34b 100644 --- a/src/Magnum/Text/Test/RendererGLTest.cpp +++ b/src/Magnum/Text/Test/RendererGLTest.cpp @@ -106,7 +106,8 @@ struct TestFont: AbstractFont { }; GlyphCache testGlyphCache(AbstractFont& font) { - GlyphCache cache{{20, 20}}; + /* Default padding is 1 to avoid artifacts, set that to 0 to simplify */ + GlyphCache cache{{20, 20}, {}}; /* Add one more font to verify the right one gets picked */ cache.addFont(96); diff --git a/src/Magnum/Text/Test/RendererTest.cpp b/src/Magnum/Text/Test/RendererTest.cpp index 76eedcd2a..348888ce3 100644 --- a/src/Magnum/Text/Test/RendererTest.cpp +++ b/src/Magnum/Text/Test/RendererTest.cpp @@ -361,7 +361,8 @@ struct DummyGlyphCache: AbstractGlyphCache { }; DummyGlyphCache testGlyphCache(AbstractFont& font) { - DummyGlyphCache cache{PixelFormat::R8Unorm, {20, 20}}; + /* Default padding is 1 to avoid artifacts, set that to 0 to simplify */ + DummyGlyphCache cache{PixelFormat::R8Unorm, {20, 20}, {}}; /* Add one more font to verify the right one gets picked */ cache.addFont(96); @@ -378,7 +379,8 @@ DummyGlyphCache testGlyphCache(AbstractFont& font) { } DummyGlyphCache testGlyphCacheArray(AbstractFont& font) { - DummyGlyphCache cache{PixelFormat::R8Unorm, {20, 20, 3}}; + /* Default padding is 1 to avoid artifacts, set that to 0 to simplify */ + DummyGlyphCache cache{PixelFormat::R8Unorm, {20, 20, 3}, {}}; /* Add one more font to verify the right one gets picked */ cache.addFont(96); @@ -1049,8 +1051,9 @@ void RendererTest::multiline() { } font; font.openFile({}, 0.5f); - /* Just a single glyph that scales to {1, 1} in the end */ - DummyGlyphCache cache{PixelFormat::R8Unorm, {20, 20}}; + /* Just a single glyph that scales to {1, 1} in the end. Default padding is + 1 which would prevent this, set it back to 0. */ + DummyGlyphCache cache{PixelFormat::R8Unorm, {20, 20}, {}}; UnsignedInt fontId = cache.addFont(1, &font); cache.addGlyph(fontId, 0, {}, {{}, {2, 2}}); diff --git a/src/MagnumPlugins/MagnumFontConverter/Test/MagnumFontConverterTest.cpp b/src/MagnumPlugins/MagnumFontConverter/Test/MagnumFontConverterTest.cpp index 413531ef7..c4cd90e2b 100644 --- a/src/MagnumPlugins/MagnumFontConverter/Test/MagnumFontConverterTest.cpp +++ b/src/MagnumPlugins/MagnumFontConverter/Test/MagnumFontConverterTest.cpp @@ -323,7 +323,8 @@ void MagnumFontConverterTest::exportFontEmptyCache() { GlyphCacheFeatures doFeatures() const override { return {}; } void doSetImage(const Vector2i&, const ImageView2D&) override {} - } cache{PixelFormat::R8Unorm, {8, 4}}; + /* Default padding is 1 to avoid artifacts, set that to 0 to simplify */ + } cache{PixelFormat::R8Unorm, {8, 4}, {}}; /* Associate the font with the cache. The case where it's not even that is tested in exportFontNotFoundInCache() below. */