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

Implemented drawing of CJK characters with ImGui screens #72579

Merged
merged 2 commits into from
Mar 29, 2024

Conversation

katemonster33
Copy link
Contributor

@katemonster33 katemonster33 commented Mar 24, 2024

Summary

Bugfixes "Implemented drawing of non-English characters in ImGui screens"

Purpose of change

Fixes: #72162

Describe the solution

Implemented a font "fallback" system in ImGui to allow CDDA to interrupt ImGui's drawing flow to draw glyphs using CDDA's Font::OutputChar method.

Describe alternatives you've considered

There are open PRs in the ImGui repo that implement dynamic font atlases which could potentially solve this problem, but they seem far from being viable enough to be a better solution than this.

Testing

Changed the language to Chinese, opened up the keybindings screen, and verified that the screen displays properly

Additional context

The 'x' and 'y' position that ImGui tries to draw at doesn't quite work for Cataclysm. I needed to modify those by magic numbers to make the font align properly. I'm still not sure why it's needed.

@github-actions github-actions bot added Info / User Interface Game - player communication, menus, etc. [C++] Changes (can be) made in C++. Previously named `Code` <Bugfix> This is a fix for a bug (or closes open issue) labels Mar 24, 2024
src/sdl_font.cpp Outdated Show resolved Hide resolved
@github-actions github-actions bot added json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Mar 24, 2024
@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Mar 24, 2024
@katemonster33
Copy link
Contributor Author

katemonster33 commented Mar 25, 2024

Just noticed something... On text entered into an input field, the cursor position doesn't match the space between glyphs

Edit: with CJK characters I mean

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Mar 25, 2024
@katemonster33
Copy link
Contributor Author

Just noticed something... On text entered into an input field, the cursor position doesn't match the space between glyphs

Edit: with CJK characters I mean

this should be good now

@ZeroInternalReflection
Copy link
Contributor

I distinctly recall this PR having a note requesting Curses testing, but I'm apparently confusing that with a different PR. Regardless, I'd call the impact on the Curses build "unfixed, but a step in the right direction".
In the current state of things, ImGui windows will print out a bunch of garbage instead of the appropriate characters (Simplified Chinese pictured):
Curses--Before

After this PR, any unhandled glyphs instead print out as blank space:
Curses--After

Notably, the blank space appears to be the appropriate size for the unprinted text, so it looks like the character width calculations are working. Contrast the Chinese above with Russian below:
Curses--After--Russian

@katemonster33
Copy link
Contributor Author

katemonster33 commented Mar 25, 2024

I distinctly recall this PR having a note requesting Curses testing, but I'm apparently confusing that with a different PR. Regardless, I'd call the impact on the Curses build "unfixed, but a step in the right direction". In the current state of things, ImGui windows will print out a bunch of garbage instead of the appropriate characters [...]

This is a good catch. The fault for the original issue likely lies in the ImTui backend. I definitely wouldn't expect this PR to fix it unfortunately, since this has everything to do with graphical textures that curses builds know nothing about.

I would go ahead and make a separate issue for that, we will need to track that as a blocker for moving the UI completely to ImGui.

src/cata_imgui.cpp Outdated Show resolved Hide resolved
@github-actions github-actions bot removed astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Mar 26, 2024
src/cata_imgui.cpp Outdated Show resolved Hide resolved
@github-actions github-actions bot added astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions and removed BasicBuildPassed This PR builds correctly, label assigned by github actions labels Mar 26, 2024
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Mar 26, 2024
@Maleclypse Maleclypse merged commit cd3792c into CleverRaven:master Mar 29, 2024
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Info / User Interface Game - player communication, menus, etc. json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Imgui y/n query popup fails to display translated string on Windows
3 participants