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 | Implements complex-text eligibility, Uniscribe layout and rasterization, texture chunking, and legacy fallback behavior. |
| Core/Libraries/Source/WWVegas/WW3D2/render2dsentence.h | Extends font and sentence-renderer interfaces with complex-text measurement, rendering, and opt-out controls. |
| Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplayString.cpp | Integrates shaped extents and deferred sentence rebuilding. |
| GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplayString.cpp | Mirrors the Generals integration. |
| Core/GameEngine/Source/GameClient/GUI/GameWindowManager.cpp | Disables complex shaping for editable text until shaped caret metrics are supported. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Display string text changes] --> B[Measure formatted extents]
B --> C{Eligible complex single-line text?}
C -- No --> D[Legacy per-character metrics]
C -- Yes --> E[Uniscribe itemization and shaping]
E --> F[Cache shaped dimensions]
A --> G[Build sentence before drawing]
G --> H{Complex rasterization succeeds?}
H -- Yes --> I[Copy shaped raster into sentence textures]
H -- No --> J[Build legacy sentence]
J --> K[Replace shaped dimensions with legacy extent]
I --> L[Generate and draw polygons]
K --> L
D --> L
Reviews (16): 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 |
|
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. |
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".
80cebad to
60951d4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60951d4c23
ℹ️ 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".
a5a59b9 to
42b2337
Compare
42b2337 to
2220c79
Compare
4985fdf to
c986ded
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Rebased! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c986dedf67
ℹ️ 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".
| hotkey.translate(TheHotKeyManager->searchHotKey(getText())); | ||
| } | ||
|
|
||
| const Bool hasHotkey = !hotkey.isEmpty(); |
There was a problem hiding this comment.
When a hotkey-enabled complex label contains a literal ampersand followed by whitespace (for example, Arabic text containing &), this still sets hasHotkey: the current HotKeyManager::searchHotKey() returns the character after any & without validating it (Core/GameEngine/Source/GameClient/MessageStream/HotKey.cpp:211-218), while the sentence renderer consumes the marker only when the following character is greater than L' ' (render2dsentence.cpp:1150). ParseHotKey therefore remains true and Is_Single_Line_Complex_Text() rejects the whole label, leaving its Arabic text unshaped. This predicate mismatch is fresh evidence beyond the resolved no-marker case; derive hasHotkey using the renderer's accelerator-validity rules.
Useful? React with 👍 / 👎.
c986ded to
f881a12
Compare
|
This change adds a lot of new code for the text rendering. Is all of it absolutely required? And is there no code duplication, aka new code doing the same as nearby other code, in part? |
f881a12 to
614ddc6
Compare
| // TheSuperHackers @bugfix Omar Aglan 06/09/2026 Match the sentence renderer's hotkey marker validation. | ||
| for (const WideChar *marker = getText().str(); *marker; ++marker) { | ||
| if (*marker == L'&' && marker[1] > L' ') { | ||
| hotkey.concat(marker[1]); | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Hotkey extraction now happens only when setUseHotkey() is called. If the display string's text later changes while hotkeys remain enabled, the sentence is rebuilt using the new text but m_textRendererHotKey still renders the previously cached hotkey character. This can display an underlined character that no longer matches the current label. Recompute the hotkey whenever the text changes, as the previous draw-time lookup did. The mirrored GeneralsMD implementation has the same issue.
Knowledge Base Used: Game client runtime
Prompt To Fix With AI
This is a comment left during a code review.
Path: Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplayString.cpp
Line: 429-436
Comment:
**Hotkey Character Becomes Stale**
Hotkey extraction now happens only when `setUseHotkey()` is called. If the display string's text later changes while hotkeys remain enabled, the sentence is rebuilt using the new text but `m_textRendererHotKey` still renders the previously cached hotkey character. This can display an underlined character that no longer matches the current label. Recompute the hotkey whenever the text changes, as the previous draw-time lookup did. The mirrored GeneralsMD implementation has the same issue.
**Knowledge Base Used:** [Game client runtime](https://app.greptile.com/thesuperhackers/-/custom-context/knowledge-base/thesuperhackers/generalsgamecode/-/docs/game-client-runtime.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
right now, i can't simplify more than that! |
614ddc6 to
c333fd1
Compare
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 text. Eligible strings are now itemized as one paragraph, divided into visually ordered font runs only at character-cluster boundaries, and rasterized once before being copied across the existing A4R4G4B4 sentence textures.Plain Latin strings continue to use the existing per-character renderer. Multiline text, text requiring wrapping, text with an active hot-key marker, monospaced text, and editable text entries remain on the legacy path.
The required Uniscribe entry points 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.