Improve Unicode support in the console #1379

dgelessus · 2023-04-16T17:11:05Z

This implements full Unicode support in the console engine internals (fixes #479). The in-game console itself only has limited Unicode support for now:

A few string fields are still limited to 8-bit Latin-1 characters, namely the input line and display buffer.
plDebugText, which the console uses for text rendering, still only supports ASCII characters (not even Latin-1). Non-ASCII characters are not visible in the input and output.

Non-ASCII characters can be typed into the input, and even though they are invisible, they will be passed through correctly to Python and/or the console engine (unlike before). Cut/copy/paste also work correctly with non-ASCII characters.

So for example you can type or paste this command to bind the Ü key (semicolon on US keyboards):

Keyboard.BindConsoleCmd Ü Graphics.Show.Physicals

And you can run this in the Python console and get the correct output:

>>> print(ascii("Ü"))
'\xdc'

The previous implementation used very "C-style" string management (not in a good way). I've reimplemented most of it using ST::string, which should make the code easier to follow. The diff is quite messy though - apologies in advance to whoever decides to review this. You might want to skip over pfConsoleCommands.cpp and the other command implementations at first - they mostly contain boring char * -> ST::string changes.

Obviously I didn't test every single console command - only a few common ones and those where Unicode is obviously relevant.

zrax · 2023-04-16T18:34:58Z

Sources/Plasma/CoreLib/hsStream.cpp

@@ -230,7 +230,7 @@ bool hsStream::AtEnd()

 bool hsStream::IsTokenSeparator(char c)
 {
-    return (isspace(c) || c==',' || c=='=');
+    return (isspace(static_cast<unsigned char>(c)) || c==',' || c=='=');


?? ctype functions (isspace, isalnum, etc) take an int, which both a signed and unsigned implementation of char should automatically and safely promote to... Why all the casts?

That's the problem. char is signed and gets sign-extended to int, but the ctype functions are only defined for unsigned char values and EOF. See also e. g. https://nullprogram.com/blog/2023/02/11/#character-classification-and-mapping

MSVCRT has a debug assertion ensuring that you don't pass any other negative values. That started failing once I typed non-ASCII characters into the console, so I went ahead and fixed the issue for all ctype calls (there weren't all that many).

Ah, that make sense... Although perhaps it makes more sense to test for that specific case, since characters outside the 0-127 range will always be UTF-8 multibyte sequence bytes when coming from ST::strings.

Not sure what you mean exactly... something like c < 0x80 && isspace(c)? I don't think that's necessary if c is already char-sized. We don't use locales, so all the is functions will return false for all bytes outside the ASCII range.

(I did use c < 0x80 checks where c is a larger type like wchar_t or uint32_t, because just a cast isn't sufficient there.)

zrax · 2023-04-16T18:42:59Z

Sources/Plasma/FeatureLib/pfConsole/pfConsoleCommands.cpp

 {
    if (name.empty())
    {
-        if (statusStr)
-            *statusStr = "Object name is nil";
+        statusStr = "Object name is nil";


These would be a good place to use ST_LITERAL or the _st literal suffix

Ah yep. I used it more consistently elsewhere, but got lazy when I saw the 6000-line file 🙃

Okay, added ST_LITERAL in the relevant places. I didn't adjust every single PrintString call though - that would be quite a bit of work and not really worth it. (This is the dev console - I don't think we need to micro-optimize performance and memory usage that much.)

Hoikas · 2023-04-30T18:40:01Z

Sources/Plasma/FeatureLib/pfConsole/pfConsole.cpp


-    memset( fHistory, 0, sizeof( fHistory ) );
+    for (size_t i = 0; i < kNumHistoryTypes; i++) {
+        fHistory[i] = {};


If we don't mind requiring string_theory >= 3.4....

Suggested change

fHistory[i] = {};

fHistory[i].clear();

Yeah, Plasma already uses ST::string::clear() elsewhere, despite the CMakeLists.txt only saying string_theory 2.0 - that should probably be updated...

Oh wait, I remember now - fHistory[i] is actually a struct. This line initializes/clears all fields in the struct, not just the ST::string, so clear() doesn't work here.

Sources/Plasma/FeatureLib/pfConsole/pfConsole.cpp

Hoikas · 2023-04-30T18:46:34Z

Sources/Plasma/FeatureLib/pfConsoleCore/pfConsoleCmd.h

+        // Supposedly it's *possible* to use a non-trivial class in a union,
+        // but it seems complex and hard to do correctly,
+        // so let's just put this in its own field.


A lot of this code could probably be replaced with std::variant, but that's a very different PR.

Sources/Plasma/FeatureLib/pfConsole/pfConsoleCommands.cpp

Sources/Plasma/FeatureLib/pfConsoleCore/pfConsoleCmd.cpp

Hoikas · 2023-04-30T19:59:25Z

Sources/Plasma/FeatureLib/pfConsoleCore/pfConsoleEngine.cpp

+    ST::string name = cmd->GetName() + ' ';
+    pfConsoleCmdGroup* group = cmd->GetParent();
+    while (group != nullptr && group != pfConsoleCmdGroup::GetBaseGroup()) {
+        name = group->GetName() + '.' + name;
+        group = group->GetParent();
+    }


This should probably use ST::string_stream to avoid extra allocations.

This isn't easily implemented with ST::string_stream, because that only allows appending to the end, but the string here is built from back to front. I suppose the strings could be collected into a std::vector and then joined at the end, to avoid at least some intermediate allocations.

Hoikas

This LGTM, but I'd like to ensure that all of @zrax's items are cleared up before proceeding.

Out-of-range values, such as non-ASCII *signed* chars, cause undefined behavior according to the standard (and an assertion failure in the case of MSVCRT).

This is enough to make it not throw exceptions instantly when running a console command with non-ASCII characters, but some things probably still break with non-ASCII characters (and the console still can't actually render them).

This should fix any remaining encoding mess-ups on the output end. Not that it really matters, because the console still only displays ASCII...

This allows typing non-ASCII characters into the console and gets them through unharmed to Python (not just Plasma console commands), although they are still invisible in both the input and output.

Multiple places in the code already use ST::string::clear(), which was added in string_theory 3.4.

Co-authored-by: Adam Johnson <AdamJohnso@gmail.com>

dgelessus · 2023-07-01T14:08:09Z

Rebased to fix a conflict.

I think I had addressed/fixed all the things from the reviews - let me know if I missed anything.

Hoikas · 2023-07-08T18:59:20Z

Barring any objections, I intend to merge this tonight.

Hoikas · 2023-07-10T23:35:53Z

A couple of days late, but here we go...

zrax reviewed Apr 16, 2023

View reviewed changes

Hoikas reviewed Apr 30, 2023

View reviewed changes

Hoikas approved these changes May 7, 2023

View reviewed changes

dgelessus mentioned this pull request Jun 3, 2023

Prepare supporting additional languages #1397

Merged

dgelessus and others added 22 commits July 1, 2023 13:53

Replace last uses of hsExpander with std::vector

f1dce93

Remove unused PF_LOCAL_CONSOLE_CMD macro and localOnly flag

4cc5895

Remove unused pfConsole::RunCommandAsync

336529a

Ensure that <cctype> function arguments are in unsigned char range

647f449

Out-of-range values, such as non-ASCII *signed* chars, cause undefined behavior according to the standard (and an assertion failure in the case of MSVCRT).

Don't return references from pfConsoleCmdParam conversion methods

ef1f5b2

Make pfConsoleCmdParam string values consistently const

de6451a

Remove a few obsolete console commands that do nothing

c79d8dc

Migrate pfConsoleCmdParam to ST::string

2c56b9b

Migrate pfConsoleContext to ST::string

373609e

Migrate some of pfConsoleEngine's API to ST::string

881d05e

This is enough to make it not throw exceptions instantly when running a console command with non-ASCII characters, but some things probably still break with non-ASCII characters (and the console still can't actually render them).

Migrate console printing to ST::string

4828e78

This should fix any remaining encoding mess-ups on the output end. Not that it really matters, because the console still only displays ASCII...

Inline pfConsolePrintF now that PrintString natively uses ST::string

f592d46

Migrate console line execution handling to ST::string

31c43b9

This allows typing non-ASCII characters into the console and gets them through unharmed to Python (not just Plasma console commands), although they are still invisible in both the input and output.

Migrate console help tooltip to ST::string

3f61a62

Migrate console command search/completion to ST::string

9ac3caf

Migrate pfConsoleCmd and pfConsoleCmdGroup to ST::string

489cbfa

Refactor pfConsole.fWorkingLine conversion from/to ST::string

a970d45

Replace deprecated ST::string method call

6751555

Use ST_LITERAL in a few more places

e6aa497

Declare string_theory 3.4 requirement

0c2a659

Multiple places in the code already use ST::string::clear(), which was added in string_theory 3.4.

Apply suggestions from code review

72bace7

Co-authored-by: Adam Johnson <AdamJohnso@gmail.com>

Slightly optimize allocations in pfConsoleEngine::FindNestedPartialCmd

f6e6687

dgelessus force-pushed the pfconsole_unicode branch from dc49f67 to f6e6687 Compare July 1, 2023 13:57

Hoikas merged commit bc3b31b into H-uru:master Jul 10, 2023
14 checks passed

dgelessus deleted the pfconsole_unicode branch November 29, 2023 21:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Unicode support in the console #1379

Improve Unicode support in the console #1379

dgelessus commented Apr 16, 2023 •

edited

Loading

zrax Apr 16, 2023

dgelessus Apr 16, 2023

zrax Apr 17, 2023

dgelessus Apr 19, 2023

zrax Apr 16, 2023

dgelessus Apr 16, 2023

dgelessus Apr 19, 2023

Hoikas Apr 30, 2023

dgelessus Apr 30, 2023 •

edited

Loading

dgelessus Apr 30, 2023

Hoikas Apr 30, 2023

Hoikas Apr 30, 2023

dgelessus Apr 30, 2023

Hoikas left a comment

dgelessus commented Jul 1, 2023

Hoikas commented Jul 8, 2023

Hoikas commented Jul 10, 2023

Improve Unicode support in the console #1379

Improve Unicode support in the console #1379

Conversation

dgelessus commented Apr 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgelessus Apr 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hoikas left a comment

Choose a reason for hiding this comment

dgelessus commented Jul 1, 2023

Hoikas commented Jul 8, 2023

Hoikas commented Jul 10, 2023

dgelessus commented Apr 16, 2023 •

edited

Loading

dgelessus Apr 30, 2023 •

edited

Loading