Refactor: Store BLF Glyphs in blender::Map #118528

Harley Acheson · 2024-02-20T18:53:06+01:00

Harley Acheson commented

2024-02-20 18:53:06 +01:00

Store the GlyphBLFs in a blender::Map rather than our current hash
table, which is an array of 257 ListBases.

Store the GlyphBLFs in a blender::Map rather than our current hash table, which is an array of 257 ListBases.

🚀 2

Harley Acheson added 1 commit 2024-02-20 18:53:15 +01:00

2bf35aa9a4 Refactor: Store BLF Glyphs in blender::Map

Store the GlyphBLFs in a blender::Map rather than our current hash
table, which is an array of 257 ListBases.

Harley Acheson added 1 commit 2024-02-20 18:54:35 +01:00

8e8cca8d10 Formatting issue

Harley Acheson added this to the User Interface project 2024-02-20 18:55:24 +01:00

Harley Acheson requested review from Hans Goudey 2024-02-20 18:55:32 +01:00

Harley Acheson commented

2024-02-20 18:59:12 +01:00

@HooglyBoogly

As mentioned this PR works fine, but the Map is of <int, GlyphBLF *>. Is that okay? I tried making the map store < int, std::unique_ptr<GlyphBLF>> instead and just got errors I couldn't get past.

@HooglyBoogly As mentioned this PR works fine, but the Map is of `<int, GlyphBLF *>`. Is that okay? I tried making the map store `< int, std::unique_ptr<GlyphBLF>>` instead and just got errors I couldn't get past.

Hans Goudey requested changes 2024-02-20 19:03:29 +01:00

Hans Goudey left a comment

Very nice! If you share the errors with unique_ptr I'll take a look, but doing that separately is okay too.

There's one more thing that's worth doing in this PR, to avoid the need for repeating g->c << 6 | subpixel everywhere: adding a struct containing the character and the subpixel value and giving it the necessary methods to be used as a key, as described here: https://developer.blender.org/docs/features/core/blenlib/containers/#hashing

Very nice! If you share the errors with `unique_ptr` I'll take a look, but doing that separately is okay too. There's one more thing that's worth doing in this PR, to avoid the need for repeating `g->c << 6 | subpixel` everywhere: adding a struct containing the character and the subpixel value and giving it the necessary methods to be used as a key, as described here: https://developer.blender.org/docs/features/core/blenlib/containers/#hashing

Harley Acheson commented

2024-02-20 19:24:03 +01:00

If you share the errors with unique_ptr I'll take a look, but doing that separately is okay too.

Yes, wasn't quite sure how best to show you. But with this PR as a base, since it works, I then try the following changes...

diff --git a/source/blender/blenfont/intern/blf_glyph.cc b/source/blender/blenfont/intern/blf_glyph.cc
index e35dccf7906..24325dd1784 100644
--- a/source/blender/blenfont/intern/blf_glyph.cc
+++ b/source/blender/blenfont/intern/blf_glyph.cc
@@ -144,9 +144,8 @@ void blf_glyph_cache_release(FontBLF *font)
 
 GlyphCacheBLF::~GlyphCacheBLF()
 {
-  for (auto item : this->glyphs.items()) {
-    MEM_delete(item.value);
-  }
+  this->glyphs.clear_and_shrink();
+
   if (this->texture) {
     GPU_texture_free(this->texture);
   }
@@ -170,7 +169,7 @@ static GlyphBLF *blf_glyph_cache_find_glyph(const GlyphCacheBLF *gc,
                                             uint charcode,
                                             uint8_t subpixel)
 {
-  return gc->glyphs.lookup_default(charcode << 6 | subpixel, nullptr);
+  return gc->glyphs.lookup(charcode << 6 | subpixel).get();
 }
 
 #ifdef BLF_GAMMA_CORRECT_GLYPHS
@@ -220,7 +219,7 @@ static GlyphBLF *blf_glyph_cache_add_glyph(FontBLF *font,
                                            FT_UInt glyph_index,
                                            uint8_t subpixel)
 {
-  GlyphBLF *g = MEM_new<GlyphBLF>(__func__);
+  std::unique_ptr<GlyphBLF> g = std::make_unique<GlyphBLF>(GlyphBLF{});
 
   g->c = charcode;
   g->idx = glyph_index;
@@ -336,7 +335,7 @@ static GlyphBLF *blf_glyph_cache_add_glyph(FontBLF *font,
 
   gc->glyphs.add(g->c << 6 | subpixel, g);
 
-  return g;
+  return g.get();
 }
 
 /** \} */
diff --git a/source/blender/blenfont/intern/blf_internal_types.hh b/source/blender/blenfont/intern/blf_internal_types.hh
index 2a0e5665d32..2bc45e518fa 100644
--- a/source/blender/blenfont/intern/blf_internal_types.hh
+++ b/source/blender/blenfont/intern/blf_internal_types.hh
@@ -133,7 +133,7 @@ struct GlyphCacheBLF {
   int fixed_width;
 
   /** The glyphs. */
-  blender::Map<int, GlyphBLF *> glyphs;
+  blender::Map<int, std::unique_ptr<GlyphBLF>> glyphs;
 
   /** Texture array, to draw the glyphs. */
   GPUTexture *texture;

But the compiler then complains about "'std::unique_ptr<GlyphBLF,std::default_delete>::unique_ptr(const std::unique_ptr<GlyphBLF,std::default_delete> &)': attempting to reference a deleted function" and points at ForwardKey at about line 190 in BLI_map_slots.hh.

> If you share the errors with `unique_ptr` I'll take a look, but doing that separately is okay too. Yes, wasn't quite sure how best to show you. But with this PR as a base, since it works, I then try the following changes... ``` diff --git a/source/blender/blenfont/intern/blf_glyph.cc b/source/blender/blenfont/intern/blf_glyph.cc index e35dccf7906..24325dd1784 100644 --- a/source/blender/blenfont/intern/blf_glyph.cc +++ b/source/blender/blenfont/intern/blf_glyph.cc @@ -144,9 +144,8 @@ void blf_glyph_cache_release(FontBLF *font) GlyphCacheBLF::~GlyphCacheBLF() { - for (auto item : this->glyphs.items()) { - MEM_delete(item.value); - } + this->glyphs.clear_and_shrink(); + if (this->texture) { GPU_texture_free(this->texture); } @@ -170,7 +169,7 @@ static GlyphBLF *blf_glyph_cache_find_glyph(const GlyphCacheBLF *gc, uint charcode, uint8_t subpixel) { - return gc->glyphs.lookup_default(charcode << 6 | subpixel, nullptr); + return gc->glyphs.lookup(charcode << 6 | subpixel).get(); } #ifdef BLF_GAMMA_CORRECT_GLYPHS @@ -220,7 +219,7 @@ static GlyphBLF *blf_glyph_cache_add_glyph(FontBLF *font, FT_UInt glyph_index, uint8_t subpixel) { - GlyphBLF *g = MEM_new<GlyphBLF>(__func__); + std::unique_ptr<GlyphBLF> g = std::make_unique<GlyphBLF>(GlyphBLF{}); g->c = charcode; g->idx = glyph_index; @@ -336,7 +335,7 @@ static GlyphBLF *blf_glyph_cache_add_glyph(FontBLF *font, gc->glyphs.add(g->c << 6 | subpixel, g); - return g; + return g.get(); } /** \} */ diff --git a/source/blender/blenfont/intern/blf_internal_types.hh b/source/blender/blenfont/intern/blf_internal_types.hh index 2a0e5665d32..2bc45e518fa 100644 --- a/source/blender/blenfont/intern/blf_internal_types.hh +++ b/source/blender/blenfont/intern/blf_internal_types.hh @@ -133,7 +133,7 @@ struct GlyphCacheBLF { int fixed_width; /** The glyphs. */ - blender::Map<int, GlyphBLF *> glyphs; + blender::Map<int, std::unique_ptr<GlyphBLF>> glyphs; /** Texture array, to draw the glyphs. */ GPUTexture *texture; ``` But the compiler then complains about "'std::unique_ptr<GlyphBLF,std::default_delete<GlyphBLF>>::unique_ptr(const std::unique_ptr<GlyphBLF,std::default_delete<GlyphBLF>> &)': attempting to reference a deleted function" and points at `ForwardKey` at about line 190 in BLI_map_slots.hh.

Hans Goudey commented

2024-02-20 19:37:03 +01:00

Generally looking at the full call stack in an error message is necessary to pinpoint where the problem is. This one means that unique_ptr has no copy constructor. So it has to be moved instead. The problem is with this line:

gc->glyphs.add(g->c << 6 | subpixel, g);

Passing g here requires a copy constructor. With std::move it will use the move constructor instead.
Keep in mind that moving from the original unique_ptr will empty the original, so you'll need to store the raw pointer from before in order to return it from the function.

Generally looking at the full call stack in an error message is necessary to pinpoint where the problem is. This one means that `unique_ptr` has no copy constructor. So it has to be moved instead. The problem is with this line: ```cpp gc->glyphs.add(g->c << 6 | subpixel, g); ``` Passing `g` here requires a copy constructor. With `std::move` it will use the move constructor instead. Keep in mind that moving from the original unique_ptr will empty the original, so you'll need to store the raw pointer from before in order to return it from the function.

Harley Acheson commented

2024-02-20 20:33:02 +01:00

Generally looking at the full call stack in an error message is necessary to pinpoint where the problem is.

Yes, not seeing anything in my IDE that helps show where this is. This is at compile time so I don't have a call stack. There are more error messages in my Output , but almost all chatter about what it is doing in BLI_memory_utils.hh.

This one means that unique_ptr has no copy constructor. So it has to be moved instead. The problem is with this line:

gc->glyphs.add(g->c << 6 | subpixel, g);
Passing g here requires a copy constructor. With std::move it will use the move constructor instead.
Keep in mind that moving from the original unique_ptr will empty the original, so you'll need to store the raw pointer from before in order to return it from the function.

If I change that to gc->glyphs.add(g->c << 6 | subpixel, std::move(g)); this error message doesn't change for me at all.

> Generally looking at the full call stack in an error message is necessary to pinpoint where the problem is. Yes, not seeing anything in my IDE that helps show where this is. This is at compile time so I don't have a call stack. There are more error messages in my Output , but almost all chatter about what it is doing in BLI_memory_utils.hh. This one means that `unique_ptr` has no copy constructor. So it has to be moved instead. The problem is with this line: > ```cpp > gc->glyphs.add(g->c << 6 | subpixel, g); > ``` > Passing `g` here requires a copy constructor. With `std::move` it will use the move constructor instead. > Keep in mind that moving from the original unique_ptr will empty the original, so you'll need to store the raw pointer from before in order to return it from the function. If I change that to `gc->glyphs.add(g->c << 6 | subpixel, std::move(g));` this error message doesn't change for me at all.

Ray molenkamp commented

2024-02-20 22:43:06 +01:00

static GlyphCacheBLF *blf_glyph_cache_new(FontBLF *font)
{
  std::unique_ptr<GlyphCacheBLF> gc = std::make_unique<GlyphCacheBLF>();

and

  int key = g->c << 6 | subpixel;
  gc->glyphs.add(key, std::move(g));

  return gc->glyphs.lookup(key).get();

did the trick for me

``` static GlyphCacheBLF *blf_glyph_cache_new(FontBLF *font) { std::unique_ptr<GlyphCacheBLF> gc = std::make_unique<GlyphCacheBLF>(); ``` and ``` int key = g->c << 6 | subpixel; gc->glyphs.add(key, std::move(g)); return gc->glyphs.lookup(key).get(); ``` did the trick for me

Harley Acheson added 1 commit 2024-02-21 00:52:17 +01:00

ec91b2da8b Storing unique_ptr

Harley Acheson added 1 commit 2024-02-21 00:56:44 +01:00

9888e85189 formatting changes

Harley Acheson commented

2024-02-21 01:02:52 +01:00

@HooglyBoogly

Ray's change here, seemingly disconnected from all this since for GlyphCacheBLF, was the biggest change to get rid of the template errors:

-  std::unique_ptr<GlyphCacheBLF> gc = std::make_unique<GlyphCacheBLF>(GlyphCacheBLF{});
+  std::unique_ptr<GlyphCacheBLF> gc = std::make_unique<GlyphCacheBLF>();

Another unexpected thing was that Map.lookup returns a reference to the value. But asserts if the item is not found in the cache. But Map.lookup_default, although allows a default value if not found, returns a COPY of the value and so needs that copy constructor.

But just used lookup_ptr_as instead, which allows a check for null and returns a reference.

@HooglyBoogly Ray's change here, seemingly disconnected from all this since for GlyphCacheBLF, was the biggest change to get rid of the template errors: ``` - std::unique_ptr<GlyphCacheBLF> gc = std::make_unique<GlyphCacheBLF>(GlyphCacheBLF{}); + std::unique_ptr<GlyphCacheBLF> gc = std::make_unique<GlyphCacheBLF>(); ``` Another unexpected thing was that `Map.lookup` returns a reference to the value. But asserts if the item is not found in the cache. But `Map.lookup_default`, although allows a default value if not found, returns a COPY of the value and so needs that copy constructor. But just used `lookup_ptr_as` instead, which allows a check for null and returns a reference.

Harley Acheson added 2 commits 2024-02-21 17:18:19 +01:00

8ab0931873 Merge branch 'main' of projects.blender.org:blender/blender into GlyphMap

88f5438810 Custom Hash Key

Harley Acheson added 1 commit 2024-02-21 17:19:09 +01:00

86fb6bf27c remove unused function

Harley Acheson added 1 commit 2024-02-21 17:23:45 +01:00

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details

170ba276dc whitespace changes

Harley Acheson commented

2024-02-21 17:24:24 +01:00

@blender-bot build

Hans Goudey approved these changes 2024-02-21 17:25:39 +01:00

source/blender/blenfont/intern/blf_internal_types.hh Outdated

						
				@ -119,0 +126,4 @@

				  }

				  uint64_t hash() const

				  {

				    return (charcode << 6 | subpixel);

Hans Goudey commented

2024-02-21 17:24:28 +01:00

get_default_hash(charcode, subpixel) might be more straightforward, unless there's still a reason to use this hash

`get_default_hash(charcode, subpixel)` might be more straightforward, unless there's still a reason to use this hash

Harley Acheson commented

2024-02-21 17:47:30 +01:00

My hesitancy is mostly about my own unknowns with get_default_hash

Unicode codepoints max out at U+10FFFF, so with a byte for subpixel I am well within 32-bit integer. But then I see that DefaultHash drops the lower four bits of each before multiplying them?

My hesitancy is mostly about my own unknowns with `get_default_hash` Unicode codepoints max out at U+10FFFF, so with a byte for subpixel I am well within 32-bit integer. But then I see that `DefaultHash` drops the lower four bits of each before multiplying them?

Hans Goudey commented

2024-02-21 17:51:45 +01:00

DefaultHash for all integer types is just the value unchanged. Then the two hashes are combined like this: h1 ^ (h2 * 19349669);

My reasoning was more like "just use the default unless we know some reason to do it differently"

`DefaultHash` for all integer types is just the value unchanged. Then the two hashes are combined like this: `h1 ^ (h2 * 19349669);` My reasoning was more like "just use the default unless we know some reason to do it differently"

Harley Acheson commented

2024-02-21 17:57:47 +01:00

I can see the multiples being combined like that. The only part that gives me pause is that each part first goes through a change that looks like it drops 4 bits.

Hans Goudey commented

2024-02-21 18:04:48 +01:00

Seems like you're looking at the DefaultHash specialization for pointers. For integers it's this:

#define TRIVIAL_DEFAULT_INT_HASH(TYPE) \
  template<> struct DefaultHash<TYPE> { \
    uint64_t operator()(TYPE value) const \
    { \
      return uint64_t(value); \
    } \
  }

...


TRIVIAL_DEFAULT_INT_HASH(uint32_t);

Seems like you're looking at the `DefaultHash` specialization for pointers. For integers it's this: ```cpp #define TRIVIAL_DEFAULT_INT_HASH(TYPE) \ template<> struct DefaultHash<TYPE> { \ uint64_t operator()(TYPE value) const \ { \ return uint64_t(value); \ } \ } ... TRIVIAL_DEFAULT_INT_HASH(uint32_t); ```

Harley Acheson commented

2024-02-21 18:13:24 +01:00

Yup, I sure was. LOL

Harley marked this conversation as resolved

Harley Acheson added 1 commit 2024-02-21 18:00:03 +01:00

f19eedd644 Use get_default_hash

Harley Acheson merged commit b02f0cf206 into main

2024-02-21 18:17:30 +01:00

Harley Acheson referenced this issue from a commit

2024-02-21 18:17:32 +01:00

Refactor: Store BLF Glyphs in blender::Map

Harley Acheson deleted branch GlyphMap

2024-02-21 18:17:33 +01:00

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Refactor: Store BLF Glyphs in blender::Map #118528