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 eligibility checks, shaped measurement and rasterization, bounded texture allocation, and a consistent legacy fallback; the two previously reported boundary failures are resolved. |
| Core/Libraries/Source/WWVegas/WWLib/Usp10Loader.cpp | Dynamically loads the required Uniscribe functions from the Windows system directory and fails safely when loading is unavailable. |
| Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplayString.cpp | Uses shaped full-string widths when available and invalidates cached text when complex rendering is toggled. |
| GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplayString.cpp | Mirrors the Generals display-string integration for Zero Hour. |
| Core/Libraries/Source/WWVegas/WWLib/CMakeLists.txt | Adds the Windows-only Uniscribe loader sources to the shared WWLib target. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
Text[Display string] --> Eligible{Supported complex single-line text?}
Eligible -- No --> Legacy[Legacy per-character renderer]
Eligible -- Yes --> Measure[Measure with Uniscribe]
Measure --> Supported{Dimensions supported?}
Supported -- No --> Legacy
Supported -- Yes --> Rasterize[Rasterize shaped run]
Rasterize --> Chunk[Copy raster chunks into sentence textures]
Chunk --> Draw[Draw sentence]
Legacy --> Draw
Reviews (5): 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
| if (!Font->Rasterize_Complex_Text(text, &raster, &text_width, &text_height) || | ||
| !Is_Complex_Text_Size_Supported(text_width, text_height)) |
There was a problem hiding this comment.
1. Unsupported runs rasterize twice 🐞 Bug ➹ Performance
Build_Complex_Sentence creates the complete GDI bitmap and A4R4G4B4 raster before checking whether its width exceeds WrapWidth. Every single-line complex string requiring wrapping therefore pays for a full-run shape, bitmap allocation, pixel conversion, and discard before Build_Sentence renders it again through the legacy wrapped path, with particularly high cost for long text.
Agent Prompt
## Issue description
Complex text is fully rasterized before checking whether its dimensions qualify for the complex single-line path. If its width reaches the configured wrapping width, that raster is discarded and the legacy path renders the text again.
## Issue Context
Use the lightweight Uniscribe extent query and `Is_Complex_Text_Size_Supported` before allocating/rasterizing the full run. Retain a post-rasterization dimension check for defensive consistency if needed.
## Fix Focus Areas
- Core/Libraries/Source/WWVegas/WW3D2/render2dsentence.cpp[680-692]
- Core/Libraries/Source/WWVegas/WW3D2/render2dsentence.cpp[642-645]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
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. |
5 participants
This change adds contextual shaping and bidirectional ordering for complex single-line UI text in
Render2DSentenceClass.The sentence renderer normally processes text one
WCHARat a time. This prevents Arabic letters from using their contextual forms and does not preserve the correct visual order of mixed Arabic and Latin runs. Complex strings are now measured and rendered as one run with Windows Uniscribe before the resulting pixels are copied into the existing A4R4G4B4 sentence textures.Plain Latin strings continue to use the existing per-character rendering path. Wrapped, multiline, hot-key parsed, and monospaced text remain unchanged and can be handled separately in later work.
The implementation uses the Windows
usp10library because the existing font path is based on GDIHFONTobjects. This keeps the change within the current renderer and avoids introducing a broader DirectWrite backend change.The main-menu test confirms correct contextual shaping, bidirectional ordering, digit ordering, centering, and clipping.
before
after
The temporary main-menu test strings and diagnostic code are not included in this pull request.
The change was validated with:
git diff --checkThe implementation was developed with AI assistance, then reviewed and simplified against the nearby renderer code. The final diff was manually reviewed, and the rendering behavior was manually tested in game.