feat(text): Shape complex single line UI text - #3231
Conversation
PR Summary by QodoShape complex single-line UI text with Uniscribe
AI Description
Diagram
High-Level Assessment
Files changed (12)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
|
| Filename | Overview |
|---|---|
| Core/Libraries/Source/WWVegas/WW3D2/render2dsentence.cpp | Adds complex-text eligibility, measurement, rasterization, bounded surface allocation, and clean legacy fallback; the previously reported oversized-height failures are no longer reachable. |
| Core/Libraries/Source/WWVegas/WW3D2/render2dsentence.h | Extends font and sentence-renderer interfaces with complex-text operations and per-renderer enablement state. |
| Core/Libraries/Source/WWVegas/WWLib/Usp10Loader.cpp | Loads and validates the required Uniscribe functions at runtime, returning failure when the DLL or an export is unavailable. |
| Core/Libraries/Source/WWVegas/WWLib/Usp10Loader.h | Defines the dynamically loaded Uniscribe API contract and constants used by the shaped-text path. |
| Core/GameEngine/Source/GameClient/GUI/GameWindowManager.cpp | Explicitly keeps editable text-entry display strings on the legacy renderer. |
| Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplayString.cpp | Integrates shaped width measurement and complex-text enablement into the Generals display-string implementation. |
| GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplayString.cpp | Mirrors the display-string integration for Zero Hour. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
Text[Display string] --> Eligible{Eligible complex single-line text?}
Eligible -- No --> Legacy[Legacy per-character renderer]
Eligible -- Yes --> Available{Uniscribe available and dimensions supported?}
Available -- No --> Legacy
Available -- Yes --> Shape[Measure and rasterize shaped run]
Shape --> Split[Split raster across sentence textures]
Split --> Draw[Draw sentence chunks]
Legacy --> Draw
Reviews (7): Last reviewed commit: "feat(text): Shape complex single-line UI..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a852d41fbb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
I can't tell from the text and images what the problems were and how this fixes it.
Can you give before and afters of each of these individually? |
will provide examples of it as soon as possible |
the Arabic text as for now! before
after
|
f88c715 to
d25f054
Compare
d25f054 to
eff4156
Compare
|
draft to fix the vc6 issue |
| uint16 *raster = nullptr; | ||
| int text_width = 0; | ||
| int text_height = 0; | ||
| if (!Font->Rasterize_Complex_Text(text, &raster, &text_width, &text_height) || |
There was a problem hiding this comment.
Get_Complex_Text_Extents and Rasterize_Complex_Text each run ScriptStringAnalyse. If they disagree, rendering falls back to the old path even though layout may have already used the shaped size. Can we get the size and raster from the same analysis?
There was a problem hiding this comment.
it now fixed!
Build_Sentence() no longer measures the shaped text with Get_Complex_Text_Extents() and then performs a second analysis for rasterization and compares the two results.
It now checks only whether the string is eligible for complex rendering. Build_Complex_Sentence() rasterizes the text once and uses the width and height returned by that same analysis for texture admission and chunking. This removes the disagreement fallback described in the review.
|
|
||
| if ( font ) | ||
| { | ||
| if ( charPos == -1 ) |
There was a problem hiding this comment.
This sends every full string through Get_Text_Extents, not just complex text. It also changes multiline width from the sum of all lines to the widest line. Is that intended?
There was a problem hiding this comment.
it now fixed!
For a complete string, getWidth() now asks specifically whether complex-text extents are available. If the string is not eligible for shaping—including plain Latin, multiline text, partial charPos measurements, or strings with complex rendering disabled—it falls through to the original per-character width loop unchanged.
Therefore ordinary Latin strings retain the legacy path, and multiline width retains the previous behavior of summing the widths of its lines.
|
We can handle VC6 in a small prerequisite PR by runtime-loading |
eff4156 to
74c1e27
Compare
|
Code review by qodo was updated up to the latest commit 74c1e27 |
yes im working on it! |
|
In what shape is the arabic text you tested with? As far as I am aware the old translations had the words reversed to accomodate the game implementation. Can arabic text now be supplied normally? |
i used noraml text arabic, i didnt reverse the text, this fixes the hack that is to reverse the arabic text! |
|
needs rebase and conflict fix, working on it. |
74c1e27 to
a845dfe
Compare
a845dfe to
80cebad
Compare
|
Rebased and fixed and addressed all the changes required |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80cebad841
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return false; | ||
| } | ||
|
|
||
| HFONT old_font = (HFONT)::SelectObject(text_dc, GDIFont); |
There was a problem hiding this comment.
When a locale sets UnicodeFontName, W3DFontLibrary::loadFontData() stores that face in FontCharsClass::AlternateUnicodeFont, and the legacy path routes every non-ASCII glyph through it. This new rasterizer instead selects the primary GDIFont and relies solely on Windows fallback, so complex text can use an unintended system face or show missing glyphs even though the configured Unicode font supports them. Use the configured alternate font, or explicitly include it in the shaping fallback strategy.
Useful? React with 👍 / 👎.
| if (!ComplexTextEnabled || Font == nullptr || text == nullptr || text[0] == 0 || wcschr(text, L'\n') != nullptr || | ||
| ParseHotKey || MonoSpaced) |
There was a problem hiding this comment.
When a window has WIN_STATUS_HOTKEY_TEXT but its localized string contains no usable accelerator marker, W3DStaticText.cpp still calls setUseHotkey(TRUE), which sets ParseHotKey; W3DDisplayString::draw() later clears only m_useHotKey when searchHotKey() returns empty and never clears the renderer flag. Consequently this condition permanently rejects the complex-text path for such Arabic or other complex-script labels even though there is no hotkey to parse. Gate this exclusion on an actual marker, or clear ParseHotKey and rebuild when no hotkey is found.
Useful? React with 👍 / 👎.
| Vector2 complexExtents; | ||
| if ( charPos == -1 && m_textRenderer.Get_Complex_Text_Extents( m_textString.str(), &complexExtents ) ) | ||
| return (Int)complexExtents.X; |
There was a problem hiding this comment.
For every full-width query on complex text, this calls Get_Complex_Text_Extents(), which performs a fresh ScriptStringAnalyse even though computeExtents() already calculated and cached the same width in m_size. This occurs in frame-time paths such as InGameUI::drawPlayerInfoList(), which calls getWidth() for every displayed value and calls it twice for each player name on every draw, so Arabic names repeatedly run Uniscribe analysis despite unchanged text. Cache whether the current extent is shaped and return the cached width for these queries.
Useful? React with 👍 / 👎.
5 participants
Adds contextual shaping and bidirectional ordering for complex single-line UI text in
Render2DSentenceClass.The existing sentence renderer processes text one
WCHARat a time, which prevents Arabic letters from using their contextual forms and breaks the visual order of mixed Arabic and Latin runs. Eligible strings are now measured and rasterized as one Uniscribe run before being copied into the existing A4R4G4B4 sentence textures.Plain Latin strings continue to use the existing per-character renderer. Multiline text, text requiring wrapping, hot-key parsed text, monospaced text, and editable text entries remain on the legacy path.
The required Uniscribe functions are loaded at runtime through #3241. If Uniscribe is unavailable, rendering falls back to the legacy path.
Before
After
The change was validated with:
git diff --checkThe implementation was developed with AI assistance, then manually reviewed and simplified against the nearby renderer and runtime-loader code.