Refreshed full Unicode support patch

[samhocevar]

I reviewed, refreshed and slightly improved PR #2541 for issue #2538. It works really well for me and I can at last use emoji in my ImGui software!

Could @cloudwu and I help some more with getting these changes integrated? Right now the most inelegant part is the #include <windows.h> before ImFileOpen(), but that could be fixed by moving that function to the bottom of imgui.cpp, near the other platform-specific implementations for e.g. clipboard handling.

[cloudwu]

I have reviewed improvements , it's ok to me. A different opinion is that I don't think making ImWchar32 unsigned is necessary.

For the ImFileOpen part. I suggest to add a new function fopen_utf8 at the bottom of imgui.cpp, and let ImFileOpen() call it.

[samhocevar]

If ImWchar16 is unsigned but ImWchar32 is signed, some code will trigger compiler warnings about signed vs. unsigned comparisons depending on char size.

[ocornut]

Thank you @samhocevar and @cloudwu again for your work on this.

Three things I can think of:

(1) Afailk as per comments in #2541 there is a problem as MultiByteToWideChar couldn't be found on one of the CI machine. To be honest I find the supposed conclusion (that the function is only available with #define UNICODE ) midly suspicious but I haven't had time to inspect this more.

(2) There was some other confusion (maybe on my side) about how the Surrogate logic should or does work.

(
(3) An issue tangential to this (which could/should be solved in a completely different PR) is our font system relies on 2 linearly indexed arrays (ImFont::IndexLookup, ImFont::IndexAdvanceX) which makes those large codepoints quite unefficient memory wise. For the worse case of using codepoint 0x110000 we'd be wasting 8 megabits per font which is way more than the average imgui context including bell and whistle. The problem is functions CalcTextSizeA() are quite performance bottleneck-ey for large UI, so if someone is interested in exploring a solution (e.g. two layers array, or hash map) I would like this solution that would include numbers and analysis as to how the change might affect performances.
)

Thanks again and sorry this has been dangling.

[ocornut]

PS: Curious @samhocevar how you are dealing with emojis, are you using a font with monochrome emojis? or uploading colorful bitmaps yourself?

[samhocevar]

1. I tested the build with every combination of Win32/x64, VS2015/VS2017/VS2019, Unicode/MultiByte, so I too find the diagnostic strange. I cannot seem to access that old build log so if you could provide me with a build error log or the Appveyor configuration, I will happily have a look.

2. You can see the surrogates as UTF-8 for 16-bit characters. Large codepoints need to be encoded as two 16-bit characters. The previous patch relied on incoming surrogates pairs being always valid (true if they come from the Windows keyboard driver), which was not error-prone (for instance, the invalid sequence D800 1234 DC00 would be interpreted as if it were 1234 D800 DC00); my approach in the above patch is to send U+FFFD whenever an invalid surrogate is received (resulting in FFFD 1234 FFFD in my example).

3. Ack. For now this is not too much a concern because the currently allocated blocks are 0x10000-0x1FFFF (containing fancy letters and emoji, doubling the range size but not more than that), and 0x20000-0x2FFFF (containing CJK ideographs, and only accounting for an additional 50% increase in size). Anyone using that second range would probably also be using the gigantic CJK plane, which has tens of thousands of glyphs, and thus not be surprised to use a bit more memory.

As for emojis, I use black and white for now. Rendering them in colour would mean implementing the GSUB (ligatures), COLR (colour layers) and CPAL (palettes) TTF tables in stb_truetype and I have the grosse flemme™.

[ocornut]

I cannot seem to access that old build log so if you could provide me with a build error log or the Appveyor configuration, I will happily have a look.

Apologies it is my fault, I deleted the Appveyor account a little ago (had an issue, wanted to recreate the account the next day, forgot about it). If you force-push your PR it should run on Appveyor again and we can see how it goes.

2/3: understood, thanks.

Feedback:

(0)
Rename io.Surrogate to ioInputQueueSurrogate ?

(1)
There's already a <Windows.h> include block near the end of the file (in // [SECTION] PLATFORM DEPENDENT HELPERS) perhaps the whole thing could be moved at the top. But existing section honors a IMGUI_DISABLE_WIN32_FUNCTIONS define and the PR adding a little more dependency in there (stringapiset.h Kernel32.lib), I haven't thought of possible side-effect for e.g. Mingw or people attempting to bypass usage of CRT....

Depending on next Appveyor build and answer for those above, possible workaround: ImTextCountCharsFromUtf8() behave exactly the same for ImWchar16 and ImWchar32, so we could keep using it, and instead of relying on ::MultiByteToWideChar() we could duplicate ImTextStrFromUtf8 into ImTextStr16FromUtf8 and ImTextStr32FromUtf8 + add a redirecting define, then we can keep using ImTextStr16FromUtf8() for _wfopen(). Ditto with ImTextStrToUtf8 solely for GetClipboardTextFn_DefaultImpl()... we'd be adding two extras trivial duplicate functions, ~40 lines.

(2)
We could add a
#define IM_UNICODE_MAX_CODEPOINT (sizeof(ImWchar) == 2 ? 0x10000 : 0x110000)
and use that in a few places?
0x10FFFF/0x110000 are used in a few places in the code where it feels we could use the 16-bit version if compiled as such?

(3)
I have a WIP testing/automation framework (currently private but happy to give you two access if interested), can you suggest of interesting tests to add in terms of Unicode handling?

Thanks again!

[samhocevar]

0. / 2. Thanks for the feedback. I renamed Surrogate and implemented IM_UNICODE_MAX_CODEPOINT (I wanted to do that before but was unsure how to name it).

1. Looks like the Windows build works fine with the current code (both ImWchar16 and ImWchar32). Also I believe relying on kernel32 is harmless. Any userland piece of code will use it, even if it does not use the CRT (otherwise it won’t be able to open files, create threads, or even allocate memory). The patch does not require stringapiset.h, though.

3. What kind of tests are we talking about? A series of calls to AddInputCharacter / AddInputCharacterUTF16 followed by a check for the values in InputQueueCharacters?

[ocornut]

1. (Required tangent, worthy of its own topic) My remaining concern is we are not honoring IMGUI_DISABLE_WIN32_FUNCTIONS any more. While I'm not particularly obsessed with this feature, it feels like while we are there the barrier to honor it is fairly low. But I now also realize it is currently (even before the PR) very ill-defined, as we are still using _wfopen() and I'm not sure where to draw the line, the initial intent was to support MinGW and CRT-slim projects and there are a few uses of it (e.g. f388216 and tbh I don't remember how useful that particular third section of the #if is). If we want to use MultiByteToWideChar we should probably clarify this mess. If we don't need to use it, we can hide and defer solving that mess for later.

2. There are still various uses of 0x10FFFF/0x110000 in code and comments which look like they could be replaced?

3. Tests: Yes I suppose that would be useful, if anything it will help conveying what this surrogate thing is about for people who are not well acquainted with it.

[samhocevar]

Hi, long time no see! Here are a my answers:

1. Yes, I would argue it’s the use of _wfopen that does not honour IMGUI_DISABLE_WIN32_FUNCTIONS, as it is a Microsoft extension to the CRT and is the one that makes it necessary to use MultiByteToWideChar (which is part of kernel32.dll, probably the most low-level userland library you can find on any version of Windows).
2. I believe the remaining occurrences refer more to “the largest Unicode value in the Unicode standard” as opposed to “the largest Unicode value supported by the Dear ImGui API”; it’s really a matter of interpretation but I also believe that the Dear ImGui internals should properly handle the Unicode standard even if client applications choose to deal with a restricted subset of it.
3. How could I get started? Do you use a specific unit test framework?

[ocornut]

(1-A) Made a small chance to not use _wfopen() on master if IMGUI_DISABLE_WIN32_FUNCTIONS is set. You may get a minor conflict there.

(1-B) still annoyed with the build failure we got in #2541 (comment) (which we cannot repro and logs are lost because I had to recreate the Appveyor setup). Even though it is strange I expect we'll stumble on this again. I also believe that merging this is likely the fastest way we will be able to repro and fix the issue so I am ok to move forward with that plan and wait for reactions.

(2) Seems right, thank you!
(EDIT) I think the one in imgui_demo.cpp could use IM_UNICODE_MAX_CODEPOINT ? If anything that would mitigate (5).
(EDIT) And ImFontGlyphRangesBuilder::BuildRanges() also should use IM_UNICODE_MAX_CODEPOINT to match the buffer it creates in Clear().

(3) I sent you an invite to the imgui_tests framework. I think you'll figure how to use it yourself, but if you don't have the time/energy I would appreciate if either of you can formulate a few tests involving surrogate pairs or encoding edge cases and I'll try to implement them. (EDIT) Clarification: currently you'll probable need to compile and run the framework with both imconfig.h, we'll later be able to add both variants in the CI system if we want.

(4) In ImTextStrFromUtf8() the < 0x10000 test was removed when compiled with ImWchar=2 looks wrong? The char should be ignored or replaced with 0xFFFD ?

(5) The bruteforce-y loop in imgui_demo.cpp (for "Display all glyphs of the font" looks a little troubling going from 0x10000 to 0x10FFFF. I will try it rewrite it ahead of this PR so it doesn't need to do a brute-force iteration.
Possible solutions: (5-A) make Glyphs[] sorted by codepoint so we can fast skip to the next chunk of 256 points. That's doable but will bite us back when we try to make the atlas more dynamic. (5-B) Store a next index in each Glyphs[] ? (5-C) Very very arbitrarily store a bitfield in ImFont e.g. (IM_UNICODE_MAX_CODEPOINT/4096) bits to mark used pages, aka 2 bytes for ImWchar=2, 34 bytes for ImWchar=2. Last solution seems very adhoc and arbitrary but simplest and most efficient.

(EDIT) (6) ImFontGlyphRangesBuilder could probably be improved in a manner similar to (5) so it's less costly with ImWchar=2 but I'm not so worried about that.

[ocornut]

I have pushed a small branch with 2 commits
https://github.com/ocornut/imgui/tree/features/unicode_bits

Commit 8ff5801 implements a few of the minor changes, in particular the assert change to ImFontAtlas::AddCustomRectRegular() is technically an API breaking change. Because this is marked a Beta API and is so rarely used, I just made it break so the few users of this api can react sooner.
Also implemented a few other minor changes which I think are correct, might incur small amount of breakage in your rebase (didn't push to master to avoid pressure). Ideally if you can rebase with this as the first commit then I can merge this ASAP regardless of state of PR.

Commit c758a53 implement the idea I suggested in (5) above so the demo doesn't need to peek at one million characters. I tried to write the code to avoid complexity and overload for casual readers. Might rework.

[ocornut]

Updated:

• I have rebased your branch into the features/unicode branch (https://github.com/ocornut/imgui/tree/features/unicode) (feel free to pull and repush in yours if needing further rework).
• Squashed cloudwu commits in 1 and and sam's into 1 to facilitate rebase (as a few things went back and forth and conflicted with other minor changes).
• At the bottom of the commit pile added 5d23715 which as I see it could be merged today in master.
• At the top of the commit added 9abcb7e as discussed above (now simplified a little).
• Hope I didn't break anything!

(PS: I also have fixed ImFileOpen to honor IMGUI_DISABLE_WIN32_FUNCTIONS now)

Also note we moved to Github Actions for CI (covering many more variants)so if you force-push the same code into your branch you'll get the results
https://github.com/ocornut/imgui/runs/311564434

[ocornut]

Now merged the first commit 51a02b3 in master so there are 3 commits left in features/unicode.

[ocornut]

This is now merged, thank you @cloudwu @samhocevar !

[ocornut]

I have pushed a small refactor of the Windows include mess in 2dcf8df, to reduce code compilation.

One of the functional change is that 775a0f1 tested for !defined(__CYGWIN__) && !defined(__GNUC__) before including windows.h, but later code in the file that existed before this PR did always include windows.h when _WIN32 was defined, so I restored that behavior.

[ocornut]

I made a small change to enable ImWchar32

Previously
#define ImWchar ImWchar32
now
#define IMGUI_USE_WCHAR32

Since we haven't released 1.76 yet and this PR is part of 1.76 it won't be documented in the "breaking api change".

(This is to facilitate work on the cimgui bindings.)