From 6b31924b0bc9e611019110f2ee9c9684ab9cf6d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Tue, 1 Oct 2024 14:09:01 +0200 Subject: [PATCH] Text: only allow creating processed GlyphCacheGL by subclasses. 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? --- src/Magnum/Text/CMakeLists.txt | 5 +- src/Magnum/Text/GlyphCacheGL.cpp | 3 ++ src/Magnum/Text/GlyphCacheGL.h | 46 ++++++++++-------- src/Magnum/Text/Test/CMakeLists.txt | 2 +- src/Magnum/Text/Test/GlyphCacheGLTest.cpp | 54 +++++++++++++-------- src/MagnumPlugins/MagnumFont/MagnumFont.cpp | 2 +- 6 files changed, 68 insertions(+), 44 deletions(-) diff --git a/src/Magnum/Text/CMakeLists.txt b/src/Magnum/Text/CMakeLists.txt index be415a30e..f903b0a26 100644 --- a/src/Magnum/Text/CMakeLists.txt +++ b/src/Magnum/Text/CMakeLists.txt @@ -62,10 +62,9 @@ set(MagnumText_PRIVATE_HEADERS Implementation/printFourCC.h) if(MAGNUM_TARGET_GL) - list(APPEND MagnumText_SRCS - GlyphCacheGL.cpp) list(APPEND MagnumText_GracefulAssert_SRCS - DistanceFieldGlyphCacheGL.cpp) + DistanceFieldGlyphCacheGL.cpp + GlyphCacheGL.cpp) list(APPEND MagnumText_HEADERS DistanceFieldGlyphCacheGL.h GlyphCacheGL.h) diff --git a/src/Magnum/Text/GlyphCacheGL.cpp b/src/Magnum/Text/GlyphCacheGL.cpp index 43aba4cd6..f53daa0b3 100644 --- a/src/Magnum/Text/GlyphCacheGL.cpp +++ b/src/Magnum/Text/GlyphCacheGL.cpp @@ -108,6 +108,9 @@ GlyphCacheGL::GlyphCacheGL(NoCreateT) noexcept: AbstractGlyphCache{NoCreate}, _t GlyphCacheFeatures GlyphCacheGL::doFeatures() const { return {}; } void GlyphCacheGL::doSetImage(const Vector2i& offset, const ImageView2D& image) { + CORRADE_ASSERT(format() == processedFormat() && size() == processedSize(), + "Text::GlyphCacheGL::flushImage(): subclass expected to provide a doSetImage() implementation to handle different processed format or size", ); + /* On ES2 without EXT_unpack_subimage and on WebGL 1 there's no possibility to upload just a slice of the input, upload the whole image instead by ignoring the PixelStorage properties of the input */ diff --git a/src/Magnum/Text/GlyphCacheGL.h b/src/Magnum/Text/GlyphCacheGL.h index 5cb64a2cd..fa656be91 100644 --- a/src/Magnum/Text/GlyphCacheGL.h +++ b/src/Magnum/Text/GlyphCacheGL.h @@ -102,26 +102,6 @@ class MAGNUM_TEXT_EXPORT GlyphCacheGL: public AbstractGlyphCache { */ explicit GlyphCacheGL(PixelFormat format, const Vector2i& size, const Vector2i& padding = Vector2i{1}); - /** - * @brief Construct with a specific processed format and size - * @param format Source image format - * @param size Source image size size in pixels - * @param processedFormat Processed image format - * @param processedSize Processed glyph cache texture size in - * pixels - * @param padding Padding around every glyph in pixels. See - * @ref Text-AbstractGlyphCache-padding for more information about - * the default. - * @m_since_latest - * - * The @p size and @p processedSize is expected to be non-zero. All - * glyphs are saved in @p format relative to @p size and with - * @p padding, although the actual glyph cache texture is in - * @p processedFormat and has @p processedSize. - * @see @ref AbstractGlyphCache(PixelFormat, const Vector2i&, const Vector2i&) - */ - explicit GlyphCacheGL(PixelFormat format, const Vector2i& size, PixelFormat processedFormat, const Vector2i& processedSize, const Vector2i& padding = Vector2i{1}); - #ifdef MAGNUM_BUILD_DEPRECATED /** * @brief Constructor @@ -178,6 +158,32 @@ class MAGNUM_TEXT_EXPORT GlyphCacheGL: public AbstractGlyphCache { /** @brief Cache texture */ GL::Texture2D& texture() { return _texture; } + protected: + /** + * @brief Construct with a specific processed format and size + * @param format Source image format + * @param size Source image size size in pixels + * @param processedFormat Processed image format + * @param processedSize Processed glyph cache texture size in + * pixels + * @param padding Padding around every glyph in pixels. See + * @ref Text-AbstractGlyphCache-padding for more information about + * the default. + * @m_since_latest + * + * The @p size and @p processedSize is expected to be non-zero. All + * glyphs are saved in @p format relative to @p size and with + * @p padding, although the actual glyph cache texture is in + * @p processedFormat and has @p processedSize. + * + * Meant to be only used by subclasses that advertise + * @ref GlyphCacheFeature::ImageProcessing and reimplement + * @ref doSetImage() to take the differences between @p format, @p size + * and @p processedFormat, @p processedSize into account. + * @see @ref AbstractGlyphCache(PixelFormat, const Vector2i&, const Vector2i&) + */ + explicit GlyphCacheGL(PixelFormat format, const Vector2i& size, PixelFormat processedFormat, const Vector2i& processedSize, const Vector2i& padding = Vector2i{1}); + #ifdef DOXYGEN_GENERATING_OUTPUT private: #else diff --git a/src/Magnum/Text/Test/CMakeLists.txt b/src/Magnum/Text/Test/CMakeLists.txt index 6214cb881..f921cd96f 100644 --- a/src/Magnum/Text/Test/CMakeLists.txt +++ b/src/Magnum/Text/Test/CMakeLists.txt @@ -116,7 +116,7 @@ if(MAGNUM_TARGET_GL) corrade_add_test(TextGlyphCacheGLTest GlyphCacheGLTest.cpp LIBRARIES - MagnumText + MagnumTextTestLib MagnumOpenGLTester MagnumDebugTools) corrade_add_test(TextRendererGLTest RendererGLTest.cpp diff --git a/src/Magnum/Text/Test/GlyphCacheGLTest.cpp b/src/Magnum/Text/Test/GlyphCacheGLTest.cpp index 4da6b24df..7ffadff32 100644 --- a/src/Magnum/Text/Test/GlyphCacheGLTest.cpp +++ b/src/Magnum/Text/Test/GlyphCacheGLTest.cpp @@ -23,8 +23,11 @@ DEALINGS IN THE SOFTWARE. */ +#include #include #include +#include /**< @todo remove once Debug is stream-free */ +#include #include #include "Magnum/Image.h" @@ -66,6 +69,8 @@ struct GlyphCacheGLTest: GL::OpenGLTester { void setImage(); void setImageFourChannel(); + + void flushImageSubclassProcessedFormatSize(); }; GlyphCacheGLTest::GlyphCacheGLTest() { @@ -84,7 +89,9 @@ GlyphCacheGLTest::GlyphCacheGLTest() { &GlyphCacheGLTest::constructMove, &GlyphCacheGLTest::setImage, - &GlyphCacheGLTest::setImageFourChannel}); + &GlyphCacheGLTest::setImageFourChannel, + + &GlyphCacheGLTest::flushImageSubclassProcessedFormatSize}); } void GlyphCacheGLTest::construct() { @@ -112,19 +119,12 @@ void GlyphCacheGLTest::constructNoPadding() { } void GlyphCacheGLTest::constructProcessed() { - struct: GlyphCacheGL { - using GlyphCacheGL::GlyphCacheGL; + struct Cache: GlyphCacheGL { + explicit Cache(PixelFormat format, const Vector2i& size, PixelFormat processedFormat, const Vector2i& processedSize, const Vector2i& padding): GlyphCacheGL{format, size, processedFormat, processedSize, padding} {} GlyphCacheFeatures doFeatures() const override { return GlyphCacheFeature::ImageProcessing; } - /* The symbols are private, we don't actually need them here, so just - override with an empty implementation */ - void doSetImage(const Vector2i&, const ImageView2D&) override {} - void doSetProcessedImage(const Vector2i&, const ImageView2D&) override {} - Image3D doProcessedImage() override { - CORRADE_INTERNAL_ASSERT_UNREACHABLE(); - } } cache{PixelFormat::R8Unorm, {1024, 2048}, PixelFormat::RGBA8Unorm, {128, 256}, {3, 2}}; MAGNUM_VERIFY_NO_GL_ERROR(); @@ -139,19 +139,12 @@ void GlyphCacheGLTest::constructProcessed() { } void GlyphCacheGLTest::constructProcessedNoPadding() { - struct: GlyphCacheGL { - using GlyphCacheGL::GlyphCacheGL; + struct Cache: GlyphCacheGL { + explicit Cache(PixelFormat format, const Vector2i& size, PixelFormat processedFormat, const Vector2i& processedSize): GlyphCacheGL{format, size, processedFormat, processedSize} {} GlyphCacheFeatures doFeatures() const override { return GlyphCacheFeature::ImageProcessing; } - /* The symbols are private, we don't actually need them here, so just - override with an empty implementation */ - void doSetImage(const Vector2i&, const ImageView2D&) override {} - void doSetProcessedImage(const Vector2i&, const ImageView2D&) override {} - Image3D doProcessedImage() override { - CORRADE_INTERNAL_ASSERT_UNREACHABLE(); - } } cache{PixelFormat::R8Unorm, {1024, 2048}, PixelFormat::RGBA8Unorm, {128, 256}}; MAGNUM_VERIFY_NO_GL_ERROR(); @@ -376,6 +369,29 @@ void GlyphCacheGLTest::setImageFourChannel() { DebugTools::CompareImage); } +void GlyphCacheGLTest::flushImageSubclassProcessedFormatSize() { + CORRADE_SKIP_IF_NO_ASSERT(); + + struct Cache: GlyphCacheGL { + explicit Cache(PixelFormat format, const Vector2i& size, PixelFormat processedFormat, const Vector2i& processedSize): GlyphCacheGL{format, size, processedFormat, processedSize} {} + + GlyphCacheFeatures doFeatures() const override { + return GlyphCacheFeature::ImageProcessing; + } + }; + Cache differentFormat{PixelFormat::R8Unorm, {32, 32}, PixelFormat::RGBA8Unorm, {32, 32}}; + Cache differentSize{PixelFormat::R8Unorm, {32, 32}, PixelFormat::R8Unorm, {16, 32}}; + + std::ostringstream out; + Error redirectError{&out}; + differentFormat.flushImage({{}, {32, 32}}); + differentSize.flushImage({{}, {32, 32}}); + CORRADE_COMPARE_AS(out.str(), + "Text::GlyphCacheGL::flushImage(): subclass expected to provide a doSetImage() implementation to handle different processed format or size\n" + "Text::GlyphCacheGL::flushImage(): subclass expected to provide a doSetImage() implementation to handle different processed format or size\n", + TestSuite::Compare::String); +} + }}}} CORRADE_TEST_MAIN(Magnum::Text::Test::GlyphCacheGLTest) diff --git a/src/MagnumPlugins/MagnumFont/MagnumFont.cpp b/src/MagnumPlugins/MagnumFont/MagnumFont.cpp index 0bd79629f..8f6f1f6a3 100644 --- a/src/MagnumPlugins/MagnumFont/MagnumFont.cpp +++ b/src/MagnumPlugins/MagnumFont/MagnumFont.cpp @@ -166,7 +166,7 @@ Containers::Pointer MagnumFont::doCreateGlyphCache() { /** @todo figure out a nicer way, and ideally how to do this with fillGlyphCache() instead */ struct Cache: GlyphCacheGL { - using GlyphCacheGL::GlyphCacheGL; + explicit Cache(PixelFormat format, const Vector2i& size, PixelFormat processedFormat, const Vector2i& processedSize, const Vector2i& padding): GlyphCacheGL{format, size, processedFormat, processedSize, padding} {} GlyphCacheFeatures doFeatures() const override { return GlyphCacheFeature::ImageProcessing;