Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unicode font rendering #2904

Merged
merged 4 commits into from
Sep 24, 2021
Merged

Unicode font rendering #2904

merged 4 commits into from
Sep 24, 2021

Conversation

AJenbo
Copy link
Member

@AJenbo AJenbo commented Sep 20, 2021

This adds full support for the first 1279 Unicode symbols:

  • Latin Extended-A
  • Latin Extended-B
  • IPA Extensions
  • Spacing Modifier Letters
  • Combining Diacritical Marks
  • Greek and Coptic
  • Cyrillic

Which is needed for the following existing translations:

  • Russian
  • Bulgarian
  • Polish

image

@AJenbo AJenbo force-pushed the text branch 2 times, most recently from a3b8931 to a2d27f1 Compare September 21, 2021 05:07
@AJenbo AJenbo marked this pull request as ready for review September 21, 2021 05:07
@NikoVP
Copy link
Contributor

NikoVP commented Sep 21, 2021

The correct list of blocks is:

  • Basic Latin
  • Latin-1 Supplement
  • Latin Extended-A
  • Latin Extended-B
  • Greek and Coptic
  • Cyrillic

Which allows display of most languages based on Latin, Cyrillic and Greek

@AJenbo
Copy link
Member Author

AJenbo commented Sep 21, 2021

Basic Latin and Latin-1 Supplement was done in the previous update to the code 😉 so it's not added in this change.

@@ -101,7 +101,7 @@ std::vector<std::string> HelpTextLines;
void InitHelp()
{
HelpFlag = false;
char tempString[512];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There might be other character arrays that need to be doubled in size to accommodate the larger characters. I'd search for strcpy and strcat. Some of them, like tempstr seem large enough as is; others I'm not sure about.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to do that in a later update, and maybe search them out by testing (ASAN reports them at runtime)

Copy link
Member

@StephenCWills StephenCWills left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were a couple more of those, apparently.

Source/engine/render/text_render.cpp Show resolved Hide resolved
Source/engine/render/text_render.cpp Show resolved Hide resolved

char path[32];
sprintf(path, "fonts\\%i-00.pcx", FontSizes[size]);
sprintf(path, "fonts\\%i-%02d.pcx", FontSizes[size], row);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoadFileInMem() triggers a fatal error if I type a character that's not supported.

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, plan is to add the rest, but maybe we should simply skip any such missing symbols 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AJenbo I would vote for writing a error character. A questions mark or a box or something.
The program shouldn't stop executing or simply print nothing.
That could also help to diagnostic future errors.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that might be safe to do considering that UTF-8 is self synchronizing. But I would go for ? as that is more canonical and simpler to add to the code.

Copy link
Member

@StephenCWills StephenCWills left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you said backspace would be easy, and because it's an easy issue for the player to stumble on, I think that should be fixed before merging. Everything else that was mentioned can be handled in future PRs so I'd consider those optional.

EDIT: I should have also mentioned that I didn't find anything new in my code review except my comment about the in-game chat backspace.

Copy link
Member

@StephenCWills StephenCWills left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just barely noticed this typo as I was finishing my review (Uft8 vs Utf8). Apart from this, I think it's ready for merge.

Source/utils/utf8.h Outdated Show resolved Hide resolved
Source/control.cpp Outdated Show resolved Hide resolved
Source/DiabloUI/diabloui.cpp Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants