Browse Source

Text: taking a pointer in cache.findFont() is stupid, don't.

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.
pull/168/head
Vladimír Vondruš 3 years ago
parent
commit
d0b792791f
  1. 2
      doc/snippets/MagnumText.cpp
  2. 2
      src/Magnum/Text/AbstractFont.cpp
  3. 6
      src/Magnum/Text/AbstractGlyphCache.cpp
  4. 19
      src/Magnum/Text/AbstractGlyphCache.h
  5. 4
      src/Magnum/Text/Renderer.cpp
  6. 19
      src/Magnum/Text/Test/AbstractGlyphCacheTest.cpp
  7. 6
      src/MagnumPlugins/MagnumFont/Test/MagnumFontGLTest.cpp
  8. 2
      src/MagnumPlugins/MagnumFontConverter/MagnumFontConverter.cpp

2
doc/snippets/MagnumText.cpp

@ -237,7 +237,7 @@ Text::AbstractGlyphCache& cache = DOXYGEN_ELLIPSIS(cacheInstance);
Containers::ArrayView<const UnsignedInt> fontGlyphIds = DOXYGEN_ELLIPSIS({});
Containers::Optional<UnsignedInt> fontId = cache.findFont(font.get());
Containers::Optional<UnsignedInt> fontId = cache.findFont(*font);
DOXYGEN_ELLIPSIS()
for(std::size_t i = 0; i != fontGlyphIds.size(); ++i) {
Containers::Triple<Vector2i, Int, Range2Di> glyph =

2
src/Magnum/Text/AbstractFont.cpp

@ -324,7 +324,7 @@ Containers::Pointer<AbstractLayouter> AbstractFont::layout(const AbstractGlyphCa
"Text::AbstractFont::layout(): array glyph caches are not supported", {});
/* Find this font in the cache. This is kept as a runtime error however. */
Containers::Optional<UnsignedInt> fontId = cache.findFont(this);
Containers::Optional<UnsignedInt> fontId = cache.findFont(*this);
if(!fontId) {
Error{} << "Text::AbstractFont::layout(): font not found among" << cache.fontCount() << "fonts in passed glyph cache";
return {};

6
src/Magnum/Text/AbstractGlyphCache.cpp

@ -246,12 +246,10 @@ const AbstractFont* AbstractGlyphCache::fontPointer(const UnsignedInt fontId) co
return state.fonts[fontId].pointer;
}
Containers::Optional<UnsignedInt> AbstractGlyphCache::findFont(const AbstractFont* pointer) const {
CORRADE_ASSERT(pointer,
"Text::AbstractGlyphCache::findFont(): expected a non-null pointer", {});
Containers::Optional<UnsignedInt> AbstractGlyphCache::findFont(const AbstractFont& font) const {
const State& state = *_state;
for(UnsignedInt i = 0; i != state.fonts.size() - 1; ++i)
if(state.fonts[i].pointer == pointer) return i;
if(state.fonts[i].pointer == &font) return i;
return {};
}

19
src/Magnum/Text/AbstractGlyphCache.h

@ -457,9 +457,9 @@ class MAGNUM_TEXT_EXPORT AbstractGlyphCache {
* @brief Add a font
* @param glyphCount Upper bound on glyph IDs present in the font,
* or the value of @ref AbstractFont::glyphCount()
* @param pointer Optional unique font identifier for later
* lookup via @ref findFont(). Use @cpp nullptr @ce if not
* associated with any particular font instance.
* @param pointer Font instance for later lookup via
* @ref findFont(). Use @cpp nullptr @ce if not associated with
* any particular font instance.
* @m_since_latest
*
* Returns font ID that's subsequently used to identify the font in
@ -491,16 +491,15 @@ class MAGNUM_TEXT_EXPORT AbstractGlyphCache {
const AbstractFont* fontPointer(UnsignedInt fontId) const;
/**
* @brief Find a font ID for a unique font identifier
* @brief Find a font ID for a font instance
* @m_since_latest
*
* The @p pointer is expected to not be @cpp nullptr @ce, as there can
* be multiple fonts with no associated identifier. If no font is found
* for given identifier, returns @ref Containers::NullOpt. The lookup
* is done with an @f$ \mathcal{O}(n) @f$ complexity with @f$ n @f$
* being @ref fontCount().
* Returns a font ID if a pointer matching @p font was used in an
* earlier @ref addFont() call, @ref Containers::NullOpt otherwise. The
* lookup is done with an @f$ \mathcal{O}(n) @f$ complexity with
* @f$ n @f$ being @ref fontCount().
*/
Containers::Optional<UnsignedInt> findFont(const AbstractFont* pointer) const;
Containers::Optional<UnsignedInt> findFont(const AbstractFont& font) const;
#ifdef MAGNUM_BUILD_DEPRECATED
/**

4
src/Magnum/Text/Renderer.cpp

@ -140,7 +140,7 @@ Range2D renderGlyphQuadsInto(const AbstractFont& font, const Float size, const A
CORRADE_ASSERT(font.isOpened(),
"Text::renderGlyphQuadsInto(): no font opened", {});
const Containers::Optional<UnsignedInt> fontId = cache.findFont(&font);
const Containers::Optional<UnsignedInt> fontId = cache.findFont(font);
CORRADE_ASSERT(fontId,
"Text::renderGlyphQuadsInto(): font not found among" << cache.fontCount() << "fonts in passed glyph cache", {});
@ -303,7 +303,7 @@ std::tuple<std::vector<Vertex>, Range2D> renderVerticesInternal(AbstractFont& fo
/* Find this font in the cache and assert in the high-level API already to
avoid confusion */
CORRADE_ASSERT(cache.findFont(&font),
CORRADE_ASSERT(cache.findFont(font),
"Text::Renderer: font not found among" << cache.fontCount() << "fonts in passed glyph cache", {});
/* Output data, reserve memory as when the text would be ASCII-only. In

19
src/Magnum/Text/Test/AbstractGlyphCacheTest.cpp

@ -85,7 +85,6 @@ struct AbstractGlyphCacheTest: TestSuite::Tester {
void addFont();
void addFontDuplicatePointer();
void fontOutOfRange();
void findFontNullptr();
#ifdef MAGNUM_BUILD_DEPRECATED
void reserve();
@ -182,7 +181,6 @@ AbstractGlyphCacheTest::AbstractGlyphCacheTest() {
&AbstractGlyphCacheTest::addFont,
&AbstractGlyphCacheTest::addFontDuplicatePointer,
&AbstractGlyphCacheTest::fontOutOfRange,
&AbstractGlyphCacheTest::findFontNullptr,
#ifdef MAGNUM_BUILD_DEPRECATED
&AbstractGlyphCacheTest::reserve,
@ -603,21 +601,21 @@ void AbstractGlyphCacheTest::addFont() {
DummyGlyphCache cache{PixelFormat::R32F, {1024, 512}};
const AbstractFont* font = reinterpret_cast<const AbstractFont*>(0xdeadbeef);
CORRADE_COMPARE(cache.findFont(font), Containers::NullOpt);
CORRADE_COMPARE(cache.findFont(*font), Containers::NullOpt);
CORRADE_COMPARE(cache.addFont(35, nullptr), 0);
CORRADE_COMPARE(cache.fontCount(), 1);
CORRADE_COMPARE(cache.glyphCount(), 1);
CORRADE_COMPARE(cache.fontGlyphCount(0), 35);
CORRADE_COMPARE(cache.fontPointer(0), nullptr);
CORRADE_COMPARE(cache.findFont(font), Containers::NullOpt);
CORRADE_COMPARE(cache.findFont(*font), Containers::NullOpt);
CORRADE_COMPARE(cache.addFont(12, font), 1);
CORRADE_COMPARE(cache.fontCount(), 2);
CORRADE_COMPARE(cache.glyphCount(), 1);
CORRADE_COMPARE(cache.fontGlyphCount(1), 12);
CORRADE_COMPARE(cache.fontPointer(1), font);
CORRADE_COMPARE(cache.findFont(font), 1);
CORRADE_COMPARE(cache.findFont(*font), 1);
}
void AbstractGlyphCacheTest::addFontDuplicatePointer() {
@ -653,17 +651,6 @@ void AbstractGlyphCacheTest::fontOutOfRange() {
"Text::AbstractGlyphCache::fontPointer(): index 2 out of range for 2 fonts\n");
}
void AbstractGlyphCacheTest::findFontNullptr() {
CORRADE_SKIP_IF_NO_ASSERT();
DummyGlyphCache cache{PixelFormat::R32F, {1024, 512}};
std::ostringstream out;
Error redirectError{&out};
cache.findFont(nullptr);
CORRADE_COMPARE(out.str(), "Text::AbstractGlyphCache::findFont(): expected a non-null pointer\n");
}
#ifdef MAGNUM_BUILD_DEPRECATED
void AbstractGlyphCacheTest::reserve() {
DummyGlyphCache cache{PixelFormat::R8Unorm, {24, 20}, {1, 2}};

6
src/MagnumPlugins/MagnumFont/Test/MagnumFontGLTest.cpp

@ -88,7 +88,7 @@ void MagnumFontGLTest::createGlyphCache() {
/* The font should associate itself with the cache */
CORRADE_COMPARE(cache->fontCount(), 1);
CORRADE_COMPARE(cache->findFont(font.get()), 0);
CORRADE_COMPARE(cache->findFont(*font), 0);
/* Verify glyph contents */
CORRADE_COMPARE(cache->glyphCount(), 5);
@ -162,7 +162,7 @@ void MagnumFontGLTest::createGlyphCacheProcessedImage() {
/* The font should associate itself with the cache */
CORRADE_COMPARE(cache->fontCount(), 1);
CORRADE_COMPARE(cache->findFont(font.get()), 0);
CORRADE_COMPARE(cache->findFont(*font), 0);
/* Verify glyph contents */
CORRADE_COMPARE(cache->glyphCount(), 5);
@ -233,7 +233,7 @@ void MagnumFontGLTest::createGlyphCacheNoGlyphs() {
/* The font should associate itself with the cache even in this case */
CORRADE_COMPARE(cache->fontCount(), 1);
CORRADE_COMPARE(cache->findFont(font.get()), 0);
CORRADE_COMPARE(cache->findFont(*font), 0);
/* There's just the empty glyph added by the cache itself, nothing else */
CORRADE_COMPARE(cache->glyphCount(), 1);

2
src/MagnumPlugins/MagnumFontConverter/MagnumFontConverter.cpp

@ -63,7 +63,7 @@ std::vector<std::pair<std::string, Containers::Array<char>>> MagnumFontConverter
return {};
}
Containers::Optional<UnsignedInt> fontId = cache.findFont(&font);
Containers::Optional<UnsignedInt> fontId = cache.findFont(font);
#ifdef MAGNUM_BUILD_DEPRECATED
/* Make it work with the old-style glyph cache filling that adds exactly
one font into the cache and doesn't associate any pointer with it */

Loading…
Cancel
Save