first attempt at updating stb_truetype and stb_rect_pack #53

ronaaron · 2020-01-22T14:50:08Z

In response to the issue I reported, I forked the repo and pushed my changes to a branch.

The code does compile and it seems to run 'ok' on Linux, but it crashes on macOS because apparently the allocator stuff in the baker percolates around.

I don't understand all the ramifications so I'm asking for someone who knows the code to please assist.

ronaaron · 2020-02-06T10:10:47Z

Updated the ttf branch so the stb files are separate.

dumblob

Thanks for the PR. This is an important step.

In addition to my inline comments, could you also bump Nuklear version, generate new nuklear.h and include it in this PR?

I'll merge it after my comments got addressed 😉. @SairesArt please take a look at this PR as well.

src/paq.sh

src/nuklear_font.c

ronaaron · 2020-02-06T11:48:15Z

I've compiled on both macOS and Linux, I have no idea why this 'build failed'.

ronaaron · 2020-02-06T11:49:36Z

Dont know why the build failed for github, it works fine for me on macOS and Linux

dumblob · 2020-02-06T14:00:12Z

That's weird it compiled for you as the syntax {0} is not correct C99 (correctly it should be (struct ...){0} - i.e. with casting 😉). Try to use the same arguments as CI uses, i.e. -std=c99 -pedantic -O2.

dumblob · 2020-02-06T14:02:05Z

On the other hand, I'd still say we should stick with C89 for such core functionality (only demos and backends are allowed to use C99 or newer, but the core nuklear.h should stay for some years at C89 for compatibility reasons). Which means we can't use the (...){0} syntax at all.

dumblob

I'll then wait for @SairesArt until he looks at what stuff is common in reviews and what to look for when reviewing for this project 😉.

src/nuklear.h

src/paq.bat

ronaaron · 2020-02-06T17:08:25Z

OK, changed the comments to c89...

dumblob · 2020-02-06T17:55:04Z

@ronaaron I'm a bit reluctant to change the stb libs - I'd really like to have them 1:1. Would you rather change the python script?

ronaaron · 2020-02-07T09:05:12Z

Hey, I need this to work for my own project...

ronaaron · 2020-02-07T09:14:28Z

Different behavior from the previous version of nuklear.h:

I get SIGSEGV from stbtt_PackFontRangesGatherRects called by nk_font_atlas_bake from nk_glfw3_font_stash_end.

This happens if I have more than 3 or 4 fonts in the atlas. I assume it's due to the allocator, but ...

  15294	      for (j=0; j < ranges[i].num_chars; ++j) {
  15295	         int x0,y0,x1,y1; 
 -> 15296	         int codepoint = ranges[i].array_of_unicode_codepoints == NULL ? ranges[i].first_unicode_codepoint_in_range + j : ranges[i].array_of_unicode_codepoints[j];
  15297	         int glyph = stbtt_FindGlyphIndex(info, codepoint);

dumblob · 2020-02-07T09:19:17Z

@ronaaron what does Valgrind verbose output say to the SIGSEGV?

ronaaron · 2020-02-07T09:20:35Z

I'm running on macOS, no valgrind available. I tried using ASAN, but it doesn't trip.

dumblob · 2020-02-07T09:32:31Z

Well, yeah - the Nuklear comment in that function says it all:

NK_INTERN int
nk_tt_PackFontRangesGatherRects(struct nk_tt_pack_context *spc,
    struct nk_tt_fontinfo *info, struct nk_tt_pack_range *ranges,
    int num_ranges, struct nk_rp_rect *rects)
{
    /* rects array must be big enough to accommodate all characters in the given ranges */
...

Is your app open source? If so, could we get it somewhere and run it with Valgrind? If not, you still can quickly run a VM with any mainstream Linux distro 😉.

ronaaron · 2020-02-07T09:43:48Z

It's not open source, sorry. But here's relevant valgrind info:

==19314== Conditional jump or move depends on uninitialised value(s)
==19314== at 0x602742: stbtt_PackFontRangesGatherRects (nuklear.h:15296)
==19314== by 0x60BC4F: nk_font_bake_pack(nk_font_baker*, unsigned long*, int*, int*, nk_recti*, nk_font_config const*, int, nk_allocator*) (nuklear.h:16460)
==19314== by 0x60AA77: nk_font_atlas_bake (nuklear.h:17397)
==19314== by 0x63F757: nk_x11_font_stash_end() (nk_xlib_gl3.h:697)

==19314== Uninitialised value was created by a heap allocation
==19314== at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==19314== by 0x5EE7FA: nk_malloc(nk_handle, void*, unsigned long) (nuklear.h:8026)
==19314== by 0x60A8A4: nk_font_atlas_bake (nuklear.h:17382)
==19314== by 0x63F757: nk_x11_font_stash_end() (nk_xlib_gl3.h:697)

==19314== Use of uninitialised value of size 8
==19314== at 0x602783: stbtt_PackFontRangesGatherRects (nuklear.h:15296)
...

==19314== Invalid read of size 4
==19314== at 0x602783: stbtt_PackFontRangesGatherRects (nuklear.h:15296)
...

ronaaron · 2020-02-07T10:21:28Z

The problem goes away by increasing the amount of memory allocated in nk_font_baker_memory().
Specifically, I did this:
*temp += (nk_size)*glyph_count * 2 * sizeof(stbtt_packedchar);
instead of
*temp += (nk_size)*glyph_count * sizeof(stbtt_packedchar);

Of course that's not likely the correct solution, but it points in a direction.

ronaaron · 2020-02-07T10:37:02Z

It still works with that fix when I double or triple the number of fonts, so something along those lines is a good fix.

dumblob

We're almost there 😉.

dumblob · 2020-02-10T15:41:06Z

src/nuklear_font.c

-    *temp += (nk_size)total_range_count * sizeof(struct nk_tt_pack_range);
-    *temp += (nk_size)*glyph_count * sizeof(struct nk_tt_packedchar);
+    *temp = (nk_size)*glyph_count * sizeof(struct stbrp_rect);
+    *temp += (nk_size)*glyph_count * sizeof(stbtt_pack_range);


Overall I think we're getting close to merging. But this change from total_range_count to glyph_count is overly suspicious to merge it 😢.

I think stb_truetype started using some structs a bit differently in the newer versions. Maybe it's some off-by-one oversight or struct padding or whatever. I'd look at how the structs nk_rp_rect nk_tt_pack_range nk_tt_packedchar stbrp_rect stbtt_pack_range stbtt_packedchar nk_font_bake_data are being used everywhere and whether they're used everywhere with corresponding counts as I don't think stbtt_pack_range depends on glyph count, but rather truly on the calculated total_range_count.

Have you tried just adding temp += 1 or temp += 8 or similar (i.e. a very small fixed number)?

ronaaron · 2020-02-10T15:44:15Z

body p { margin-bottom: 0cm; margin-top: 0pt; } On 10/02/2020 17:41, dumblob wrote: @dumblob requested changes on this pull request. We're almost there 😉. In src/nuklear_font.c: @@ -2152,9 +137,9 @@ nk_font_baker_memory(nk_size *temp, int *glyph_count, *glyph_count += nk_range_glyph_count(i->range, range_count); } while ((i = i->n) != iter); } - *temp = (nk_size)*glyph_count * sizeof(struct nk_rp_rect); - *temp += (nk_size)total_range_count * sizeof(struct nk_tt_pack_range); - *temp += (nk_size)*glyph_count * sizeof(struct nk_tt_packedchar); + *temp = (nk_size)*glyph_count * sizeof(struct stbrp_rect); + *temp += (nk_size)*glyph_count * sizeof(stbtt_pack_range); Overall I think we're getting close to merging. But this change from to is overly suspicious to merge it 😢. I think stb_truetype started using some structs a bit differently in the newer versions. Maybe it's some off-by-one oversight or struct padding or whatever. I'd look at how the structs nk_rp_rect nk_tt_pack_range nk_tt_packedchar stbrp_rect stbtt_pack_range stbtt_packedchar nk_font_bake_data are being used everywhere and whether they're used everywhere with corresponding counts as I don't think stbtt_pack_range depends on glyph count, but rather truly on the calculated total_range_count. Have you tried just adding time += 1 or time += 8 or similar (i.e. a very small fixed number)? Yes, and it crashed. [ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "#53?email_source=notifications\u0026email_token=AECUVBSRIDLIS652OCP3U43RCFYSPA5CNFSM4KKHB4HKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCU4LVCI#pullrequestreview-356039305", "url": "#53?email_source=notifications\u0026email_token=AECUVBSRIDLIS652OCP3U43RCFYSPA5CNFSM4KKHB4HKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCU4LVCI#pullrequestreview-356039305", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ] Ron AARON | CTO Aaron High-Tech, Ltd | +1 516.373.0794 / +972 52.652.5543 | GnuPG Key: 91F92EB8

ronaaron · 2020-03-06T10:08:10Z

I fixed the actual problem with the memory. It needs to zero the allocated memory.

dumblob · 2020-03-06T10:47:19Z

src/nuklear_font.c

@@ -1179,6 +1179,7 @@ nk_font_atlas_bake(struct nk_font_atlas *atlas, int *width, int *height,
    tmp = atlas->temporary.alloc(atlas->temporary.userdata,0, tmp_size);
    NK_ASSERT(tmp);
    if (!tmp) goto failed;
+	memset(tmp,0,tmp_size);


Please tabs to spaces and then I'm merging. This is a leap forward for Nuklear. Thanks!

dumblob · 2020-03-06T10:58:53Z

Seems you're online - because of the importance of this change, feel free to also bump version in package.json. Tomorrow at the latest I'm merging this PR 😉. Thanks again for your persistence!

dumblob · 2020-03-06T11:22:47Z

Thanks @ronaaron for the tiresome work! It was totally worth it. Hope you'll become project member at some point - feel free to express your will any time 😉.

2asoft · 2020-09-04T17:35:48Z

src/nuklear_font.c

-
-        n = 1+nk_ttUSHORT(endPtsOfContours + numberOfContours*2-2);
-        m = n + 2*numberOfContours;  /* a loose bound on how many vertices we might need */
-        vertices = (struct nk_tt_vertex *)alloc->alloc(alloc->userdata, 0, (nk_size)m * sizeof(vertices[0]));


The adapted stb_truetype library in Nuklear used to use the nk_allocator that nk_font_baker (which comes from nk_font_atlas) uses for all its memory allocations.
Removing these customizations, and insisting on not introducing them, has forced the use of separate methods for memory management in stb_truetype:

#ifndef STBTT_malloc static nk_handle fictional_handle = {0}; #define STBTT_malloc(x,u) nk_malloc( fictional_handle, 0, x ) #define STBTT_free(x,u) nk_mfree( fictional_handle , x) #endif

nk_malloc and nk_mfree only exist when NK_INCLUDE_DEFAULT_ALLOCATOR is defined.

So the negative effect of this change is that the library no longer builds when NK_INCLUDE_DEFAULT_ALLOCATOR is not defined.

This presents a problem for libraries that wrap Nuklear and provide their own memory management, eg many bindings for other languages.

As an example, I am working on updating a binding from a Nuklear version before this PR to a Nuklear version after this PR.
It is preferred to not let Nuklear pull in stdlib for memory management, since the language in question (Rust, but the point of this comment is language-agnostic) can provide its own, and it makes no sense to have two separate ways of managing memory.
This was previously possible by creating a nk_allocator that points to custom alloc/free implementations that delegate to the language's implementations, and initializing a nk_font_atlas with this allocator; this allocator would be used for all font-related memory management in Nuklear, including in stb_truetype.

After this PR, stb_truetype no longer uses the nk_allocator of nk_font_atlas.
This means it must be provided with a separate implementation in the binding code.
This is not technically difficult (just implement a malloc and a free, and alias STBTT_malloc and STBTT_free to them), but is certainly cumbersome for the users of this library that actually prefer finer control over memory management (which, given the library's lightweight nature, is more than just those using bindings to other languages).
Additionally, the nk_font_baker allocator seems to no longer be used for anything.

Another subtle difference is that, once STBTT_malloc and STBTT_free are dealt with as above, it is no longer possible to use two nk_font_atlas with two different allocators at the same time in the same manner as before this change, since stb_truetype's memory management cannot switch between different allocators at run time (without manually implementing that, and introducing locking for multithreaded accesses if necessary, etc).
Using multiple nk_font_atlas in the same application simultaneously is not something that I have personally needed so far, and can't immediately think of a reason where this would be needed, but it is certainly worth noting.

@asoft thanks for coming here and describing in detail what this change caused. What would you suggest we implement/change?

Btw. we're open to PRs (especially those increasing interoperability without much cost), so feel free to make one and we might discuss the details there.

If allowing for finer control over mememory management is a goal of Nuklear, I think the current situation around memory management in stb_tt should be treated as a bug.
I'd like this bug to be fixed.

Options I see, in order of perceived implementation difficulty:

Add configuration to setup stb_tt library-wide to use a given allocator (via an alternative implementation of STBTT_malloc and STBTT_free).

Propagate nk_font_atlas's allocator to stb_tt by modifying Nuklear's copy of stb_tt, adding the allocator parameter in all methods where STBTT_malloc/STBTT_free are used

Propagate nk_font_atlas's allocator to stb_tt or STBTT_malloc/STBTT_free via some other means, without modifying the committed copy of stb_tt

Pros/cons that I see:

pros: simple to implement; simple to use
cons: introduces yet another place for memory management; has all drawbacks mentioned in initial comment.

pros: simple to implement; simple to use; does not change amount of various memory management entry points
cons: complicates stb_tt version updates

pros: simple to use; does not change amount of various memory management entry points
cons: likely complicated implementation, cannot be implemented immediately, need to research options;

I favor option 3, because it does not have the cons of options 1 and 2.
I'm playing locally with some options, don't have a good solution yet.

Hm, how about reversing the dependency and propagate stb_tt's STBTT_malloc/STBTT_free to nk_font_atlas or something along these lines?

I found a simple option 3 solution: #186.

first attempt at updating stb_truetype and stb_rect_pack

cc5ef11

ghost mentioned this pull request Feb 5, 2020

Chinese font rendering is blurring. #28

Open

dumblob mentioned this pull request Feb 6, 2020

Please update the version of stb_truetype.h and stb_rect_pack.h in nuklear.h #51

Closed

separated stb files

26cace7

ronaaron closed this Feb 6, 2020

ronaaron reopened this Feb 6, 2020

dumblob requested changes Feb 6, 2020

View reviewed changes

src/paq.sh Outdated Show resolved Hide resolved

src/nuklear_font.c Show resolved Hide resolved

src/nuklear_font.c Show resolved Hide resolved

updated version, added memory allocator

5a5048d

fixed c99 compile

f89b5d7

dumblob requested changes Feb 6, 2020

View reviewed changes

src/nuklear.h Outdated Show resolved Hide resolved

src/paq.bat Outdated Show resolved Hide resolved

ronaaron added 3 commits February 6, 2020 17:28

c89; crlf

d4a0f6e

but the demo is c99...

b26e05f

change comments in stb libs to c89

1f48b5c

modded build.py etc to convert comments in external files

cd7c7c5

fixed memory crash in baker

1126fd7

ifdef around my stb macro

1cc9fa9

dumblob requested changes Feb 10, 2020

View reviewed changes

dumblob added the help wanted Extra attention is needed label Feb 24, 2020

really fixed memory issue

3d84f6c

dumblob requested changes Mar 6, 2020

View reviewed changes

really fixed memory issue

ae6deb7

really fixed memory issue, bumped version, remove tabs

8513b63

dumblob merged commit 0eb6187 into Immediate-Mode-UI:master Mar 6, 2020

2asoft reviewed Sep 4, 2020

View reviewed changes

2asoft mentioned this pull request Sep 5, 2020

stb_truetype memory management through the nk_font_atlas allocator #186

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

first attempt at updating stb_truetype and stb_rect_pack #53

first attempt at updating stb_truetype and stb_rect_pack #53

ronaaron commented Jan 22, 2020

ronaaron commented Feb 6, 2020

dumblob left a comment

ronaaron commented Feb 6, 2020

ronaaron commented Feb 6, 2020

dumblob commented Feb 6, 2020

dumblob commented Feb 6, 2020 •

edited

Loading

dumblob left a comment

ronaaron commented Feb 6, 2020

dumblob commented Feb 6, 2020

ronaaron commented Feb 7, 2020

ronaaron commented Feb 7, 2020 •

edited

Loading

dumblob commented Feb 7, 2020

ronaaron commented Feb 7, 2020

dumblob commented Feb 7, 2020

ronaaron commented Feb 7, 2020

ronaaron commented Feb 7, 2020

ronaaron commented Feb 7, 2020

dumblob left a comment

dumblob Feb 10, 2020 •

edited

Loading

ronaaron commented Feb 10, 2020 via email

ronaaron commented Mar 6, 2020

dumblob Mar 6, 2020

dumblob commented Mar 6, 2020

dumblob commented Mar 6, 2020

2asoft Sep 4, 2020 •

edited

Loading

dumblob Sep 4, 2020

2asoft Sep 4, 2020

dumblob Sep 5, 2020

2asoft Sep 5, 2020

first attempt at updating stb_truetype and stb_rect_pack #53

first attempt at updating stb_truetype and stb_rect_pack #53

Conversation

ronaaron commented Jan 22, 2020

ronaaron commented Feb 6, 2020

dumblob left a comment

Choose a reason for hiding this comment

ronaaron commented Feb 6, 2020

ronaaron commented Feb 6, 2020

dumblob commented Feb 6, 2020

dumblob commented Feb 6, 2020 • edited Loading

dumblob left a comment

Choose a reason for hiding this comment

ronaaron commented Feb 6, 2020

dumblob commented Feb 6, 2020

ronaaron commented Feb 7, 2020

ronaaron commented Feb 7, 2020 • edited Loading

dumblob commented Feb 7, 2020

ronaaron commented Feb 7, 2020

dumblob commented Feb 7, 2020

ronaaron commented Feb 7, 2020

ronaaron commented Feb 7, 2020

ronaaron commented Feb 7, 2020

dumblob left a comment

Choose a reason for hiding this comment

dumblob Feb 10, 2020 • edited Loading

Choose a reason for hiding this comment

ronaaron commented Feb 10, 2020 via email

ronaaron commented Mar 6, 2020

dumblob Mar 6, 2020

Choose a reason for hiding this comment

dumblob commented Mar 6, 2020

dumblob commented Mar 6, 2020

2asoft Sep 4, 2020 • edited Loading

Choose a reason for hiding this comment

dumblob Sep 4, 2020

Choose a reason for hiding this comment

2asoft Sep 4, 2020

Choose a reason for hiding this comment

dumblob Sep 5, 2020

Choose a reason for hiding this comment

2asoft Sep 5, 2020

Choose a reason for hiding this comment

dumblob commented Feb 6, 2020 •

edited

Loading

ronaaron commented Feb 7, 2020 •

edited

Loading

dumblob Feb 10, 2020 •

edited

Loading

2asoft Sep 4, 2020 •

edited

Loading