TTF support#2232
Conversation
Integrates FreeType for font rasterization and HarfBuzz for complex text shaping, including support for right-to-left (RTL) languages like Arabic. This new system overrides the legacy BitFont rendering for certain text elements, enabling the use of TrueType/OpenType fonts and configurable sizing via `UIMD.INI`. External dependencies (FreeType, HarfBuzz, Fribidi, etc.) are managed through vcpkg. Project settings (`Phobos.props`) are updated to include vcpkg paths and switch to the Cdecl calling convention.
Replaces the FreeType and HarfBuzz-based text renderer with a GDI-based implementation. This change simplifies external dependencies by removing the `vcpkg` submodule and all direct references to its installed libraries in `Phobos.props`. The new system leverages Windows' native GDI for font rasterization and text layout, while retaining support for configurable fonts via `UIMD.INI`. The `MessageListClass_Draw_TTF` hook, which previously used the FreeType renderer, is also removed.
No hasArabic — GDI handles RTL automatically No DT_RTLREADING — not needed, Windows detects text direction No font caching — single font, loaded once No GDIFont struct — just an HFONT ~50 lines for the core rendering logic Color conversion simplified: (c & 0x1F) * 8 instead of * 255 / 31
-TTF not applying to igname msgs
|
Stop rewrite my code! |
That's not how it works, broski. In order to contribute your changes you've made a derivative of Phobos' GPL licensed code, modified it and published it. Your contribution is under GPL license and was distributed as a pull request. You can't just tell people to stop modifying it. The only thing I agree with is that you should be added to credits list for it (which you should've done in the first place). @Gedo6922 please add him and yourself to CREDITS.md. |
sure thing I already mentioned that it's Continuation of his work and will be added to the credit anyway |
-Adjusted pause menu
-Useless changes removed
-Known issues:
* Text Box for deployed MCV is smaller than the text "Power = XX Drain =XX"
* Multiplayer and game msg chat yet to support ttf
Excuse me??? Kerb, you ruined my project that adds Arabic RTL support to the CnCNet client. I also worked on TTF font rendering in-game and either through HarfBuzz or my own implementation Why did you change your mind about my project? Why did you turn against it? Why are you unwilling to let me improve the code? |
|
@G-LimeJuice We are not obliged to accept your code. We are not obliged to accept your code without edits as well. None of this is implied, written in any policies or licenses. I haven't "ruined" anything, it's you who ragequitted upon receiving criticism from the folks. Our only goal is for the code to be optimal, producing correct results AND maintainable. Throwing a tantrum in response is childish.
This is bullshit, it's you who quit without any explanation and closed your PR(s) upon receiving criticism, and now you're saying this. Maybe don't lie? If you don't want to understand and accept how civilized software development is done, then don't make a headache for everyone else. You are free to collaborate with us, but this isn't collaboration. |
|
Nightly build for this pull request:
This comment is automatic and is meant to allow guests to get latest nightly builds for this pull request without registering. It is updated on every successful build. |
- Optimize `DrawText` by reusing the DIB bitmap for reduced allocation overhead. - Extend GDI rendering to `BitText_Print` and `BitFont_Blit` via new hooks. - Improve text dimension calculation and single-character drawing logic. - Remove `vcpkg` from `.gitignore` as it's no longer a dependency for the text renderer.
- Refactor internal implementation for improved clarity, including variable naming and detailed comments. - Correct font creation by switching from `CreateFontW` to `CreateFontA` for reliable font name parsing from `UIMD.INI`. - Improve drawing process with explicit DIB background copying and 16-bit word alignment for stability. - Refine surface locking coordinates and drawing boundary calculations.
📝 WalkthroughWalkthroughThis PR introduces a new TextRenderer module that provides Win32 GDI-based TrueType font rendering for the Phobos mod. The implementation includes lazy font initialization from configuration, text drawing with DIB backbuffer composition, text measurement, and four binary hooks that integrate the renderer into the game engine's existing font and text operations. ChangesTextRenderer Module & Engine Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Phobos.vcxproj`:
- Line 33: Move the TextRenderer.h entry out of the compiled-files ItemGroup and
into the header-files ItemGroup in Phobos.vcxproj: locate the line `<ClInclude
Include="src\TextRenderer\TextRenderer.h" />` currently placed with
`<ClCompile>` entries and cut/paste it under the `<ItemGroup>` that contains
other `<ClInclude>` entries so header files are grouped with header ItemGroup,
preserving XML ordering and project formatting.
In `@src/TextRenderer/Hooks.cpp`:
- Around line 46-53: In BitText_Print (Hooks.cpp) the caller's width (W,
populated by GET_STACK) is ignored and a hardcoded 2000 is passed to
TextRenderer::DrawText; update the TextRenderer::DrawText invocation to pass W
instead of 2000 so wrapping/clipping honors the original narrow print region
(ensure the argument order remains pFont, pSurface, pWideString, X, Y, W, H, 0).
- Around line 49-50: The guard using TextRenderer::IsInitialized() must not
short-circuit lazy initialization; instead either make IsInitialized() perform
the lazy init (call LoadFontOnce()) or remove the early return and call DrawText
(or GetTextDimension where appropriate) directly so the renderer initializes on
first use; update the BitText_Print and BitFont_Blit hook sites to invoke
DrawText/GetTextDimension (or call LoadFontOnce() before checking
IsInitialized()) so the TTF renderer switches on the first call rather than
being permanently bypassed.
- Around line 68-74: The code mutates the shared font color (pFont->Color) when
nColor != -1 and never restores it, which taints later draws; fix by saving the
original color from pFont before changing it, set pFont->Color to the new color
only for the duration of the draw (TextRenderer::DrawText / any subsequent
BitFont_Blit calls), and always restore the saved original color after the draw
completes (including on all execution paths). Locate the change around
pFont->Color, the DrawText call in TextRenderer::DrawText and any BitFont_Blit
usage, and ensure restoration happens even if GetTextDimension or DrawText
returns early.
In `@src/TextRenderer/TextRenderer.cpp`:
- Around line 67-77: The code currently computes drawingWidth/drawingHeight from
raw posX/posY but only clamps the lock origin (lockX/lockY), causing negative
posX/posY to leave unclipped left/top content and oversized temp bitmaps; before
creating the temporary HBITMAP and before calling DrawTextW, adjust the visible
box by subtracting the clamped offset (clampedX = max(0,posX), clampedY =
max(0,posY)), reduce drawingWidth/drawingHeight by (clampedX - posX) and
(clampedY - posY) respectively so the temp bitmap only covers visible pixels,
and shift the GDI RECT/DrawTextW origin by (posX - clampedX, posY - clampedY)
(i.e. offset the drawing origin into the temp bitmap) so DrawTextW writes into
the correct location while Lock still uses lockX/lockY.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b55e1deb-ae4d-45e8-a7ef-da25733e467a
📒 Files selected for processing (4)
Phobos.vcxprojsrc/TextRenderer/Hooks.cppsrc/TextRenderer/TextRenderer.cppsrc/TextRenderer/TextRenderer.h
| <ClCompile Include="src\Phobos.INI.cpp" /> | ||
| <ClCompile Include="src\Phobos.Save.cpp" /> | ||
| <ClCompile Include="src\TextRenderer\TextRenderer.cpp" /> | ||
| <ClInclude Include="src\TextRenderer\TextRenderer.h" /> |
There was a problem hiding this comment.
Move TextRenderer.h to the header ItemGroup.
<ClInclude Include="src\TextRenderer\TextRenderer.h" /> is currently under the compiled-files ItemGroup; place it under the header-files ItemGroup to match project conventions.
Suggested fix
- <ClInclude Include="src\TextRenderer\TextRenderer.h" /> <!-- src/ -->
<ClInclude Include="src\Phobos.COM.h" />
<ClInclude Include="src\Phobos.CRT.h" />
<ClInclude Include="src\Phobos.h" />
<ClInclude Include="src\Phobos.version.h" />
+ <ClInclude Include="src\TextRenderer\TextRenderer.h" />As per coding guidelines: "Phobos.vcxproj: Add new .cpp files to the appropriate <ClCompile> ItemGroup and .h files to <ClInclude> ItemGroup."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Phobos.vcxproj` at line 33, Move the TextRenderer.h entry out of the
compiled-files ItemGroup and into the header-files ItemGroup in Phobos.vcxproj:
locate the line `<ClInclude Include="src\TextRenderer\TextRenderer.h" />`
currently placed with `<ClCompile>` entries and cut/paste it under the
`<ItemGroup>` that contains other `<ClInclude>` entries so header files are
grouped with header ItemGroup, preserving XML ordering and project formatting.
| GET_STACK(int, W, 0x18); | ||
| GET_STACK(int, H, 0x1C); | ||
|
|
||
| if (!TextRenderer::IsInitialized()) | ||
| return 0; | ||
|
|
||
| if (TextRenderer::DrawText(pFont, pSurface, pWideString, X, Y, 2000, H, 0)) | ||
| return 0x434BDE; |
There was a problem hiding this comment.
Use the caller's width in BitText_Print.
Line 52 ignores W and hardcodes 2000, which changes the original wrapping/clipping contract for every narrow print region. Text that previously fit a bounded box can now spill across adjacent UI.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/TextRenderer/Hooks.cpp` around lines 46 - 53, In BitText_Print
(Hooks.cpp) the caller's width (W, populated by GET_STACK) is ignored and a
hardcoded 2000 is passed to TextRenderer::DrawText; update the
TextRenderer::DrawText invocation to pass W instead of 2000 so wrapping/clipping
honors the original narrow print region (ensure the argument order remains
pFont, pSurface, pWideString, X, Y, W, H, 0).
| if (!TextRenderer::IsInitialized()) | ||
| return 0; |
There was a problem hiding this comment.
Don't gate lazy initialization behind IsInitialized().
IsInitialized() only reports whether the handles already exist; it does not call LoadFontOnce(). On the first BitText_Print / BitFont_Blit call, these guards force the original path even when TTF is enabled, and if no earlier DrawText/GetTextDimension hook runs first these two hooks never switch over to the renderer at all. Call into DrawText directly here or make IsInitialized() perform the lazy init.
Also applies to: 65-66
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/TextRenderer/Hooks.cpp` around lines 49 - 50, The guard using
TextRenderer::IsInitialized() must not short-circuit lazy initialization;
instead either make IsInitialized() perform the lazy init (call LoadFontOnce())
or remove the early return and call DrawText (or GetTextDimension where
appropriate) directly so the renderer initializes on first use; update the
BitText_Print and BitFont_Blit hook sites to invoke DrawText/GetTextDimension
(or call LoadFontOnce() before checking IsInitialized()) so the TTF renderer
switches on the first call rather than being permanently bypassed.
| wchar_t pText[2] = { wch, L'\0' }; | ||
| if (nColor != -1) pFont->Color = (WORD)nColor; | ||
|
|
||
| if (TextRenderer::DrawText(pFont, DSurface::Composite, pText, X, Y, 0, 0, 0)) | ||
| { | ||
| int charWidth = 0; | ||
| TextRenderer::GetTextDimension(pFont, pText, &charWidth, nullptr, 0); |
There was a problem hiding this comment.
Restore BitFont::Color after BitFont_Blit.
Line 69 mutates shared font state and never puts it back. One colored glyph render will taint later draws that reuse the same BitFont.
Proposed fix
wchar_t pText[2] = { wch, L'\0' };
- if (nColor != -1) pFont->Color = (WORD)nColor;
+ const auto originalColor = pFont->Color;
+ if (nColor != -1)
+ {
+ pFont->Color = static_cast<WORD>(nColor);
+ }
if (TextRenderer::DrawText(pFont, DSurface::Composite, pText, X, Y, 0, 0, 0))
{
int charWidth = 0;
TextRenderer::GetTextDimension(pFont, pText, &charWidth, nullptr, 0);
+ pFont->Color = originalColor;
R->EAX(X + charWidth + 1);
return 0x434155;
}
+ pFont->Color = originalColor;
return 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| wchar_t pText[2] = { wch, L'\0' }; | |
| if (nColor != -1) pFont->Color = (WORD)nColor; | |
| if (TextRenderer::DrawText(pFont, DSurface::Composite, pText, X, Y, 0, 0, 0)) | |
| { | |
| int charWidth = 0; | |
| TextRenderer::GetTextDimension(pFont, pText, &charWidth, nullptr, 0); | |
| wchar_t pText[2] = { wch, L'\0' }; | |
| const auto originalColor = pFont->Color; | |
| if (nColor != -1) | |
| { | |
| pFont->Color = static_cast<WORD>(nColor); | |
| } | |
| if (TextRenderer::DrawText(pFont, DSurface::Composite, pText, X, Y, 0, 0, 0)) | |
| { | |
| int charWidth = 0; | |
| TextRenderer::GetTextDimension(pFont, pText, &charWidth, nullptr, 0); | |
| pFont->Color = originalColor; | |
| R->EAX(X + charWidth + 1); | |
| return 0x434155; | |
| } | |
| pFont->Color = originalColor; | |
| return 0; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/TextRenderer/Hooks.cpp` around lines 68 - 74, The code mutates the shared
font color (pFont->Color) when nColor != -1 and never restores it, which taints
later draws; fix by saving the original color from pFont before changing it, set
pFont->Color to the new color only for the duration of the draw
(TextRenderer::DrawText / any subsequent BitFont_Blit calls), and always restore
the saved original color after the draw completes (including on all execution
paths). Locate the change around pFont->Color, the DrawText call in
TextRenderer::DrawText and any BitFont_Blit usage, and ensure restoration
happens even if GetTextDimension or DrawText returns early.
| int drawingWidth = boxWidth > 0 ? boxWidth : surfaceWidth - posX; | ||
| int drawingHeight = boxHeight > 0 ? boxHeight : surfaceHeight - posY; | ||
| drawingWidth = (drawingWidth + 1) & ~1; // Force 16-bit word alignment step boundaries | ||
|
|
||
| if (drawingWidth <= 0 || drawingHeight <= 0 || posX >= surfaceWidth || posY >= surfaceHeight) | ||
| return false; | ||
|
|
||
| // Obtain a direct memory address mapping relative to the target sub-coordinates lock location | ||
| int lockX = std::max(0, posX); | ||
| int lockY = std::max(0, posY); | ||
| void* surfaceBuffer = directDrawSurface->Lock(lockX, lockY); |
There was a problem hiding this comment.
Clip off-screen origins before building the temp bitmap.
Lines 67-77 size the draw box from the raw posX/posY, then lock from max(0, posX/Y). When either coordinate is negative, the hidden left/top portion is no longer clipped — DrawTextW still starts at (0,0) in the temp bitmap — so partially off-screen text is rendered in the wrong place and the scratch bitmap can be oversized by -posX/-posY. Shrink the visible box after clamping and offset the GDI rect by the clipped amount before drawing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/TextRenderer/TextRenderer.cpp` around lines 67 - 77, The code currently
computes drawingWidth/drawingHeight from raw posX/posY but only clamps the lock
origin (lockX/lockY), causing negative posX/posY to leave unclipped left/top
content and oversized temp bitmaps; before creating the temporary HBITMAP and
before calling DrawTextW, adjust the visible box by subtracting the clamped
offset (clampedX = max(0,posX), clampedY = max(0,posY)), reduce
drawingWidth/drawingHeight by (clampedX - posX) and (clampedY - posY)
respectively so the temp bitmap only covers visible pixels, and shift the GDI
RECT/DrawTextW origin by (posX - clampedX, posY - clampedY) (i.e. offset the
drawing origin into the temp bitmap) so DrawTextW writes into the correct
location while Lock still uses lockX/lockY.
TaranDahl
left a comment
TaranDahl
left a comment
There was a problem hiding this comment.
I don't see any issues, but I do not understand the font-related things.
|
@Gedo6922 Please explicit address each AI review comment. Meanwhile, please describe how to use TTF font in this PR and what known limitation a tester needs to know |
5 participants
Continuation of TTF work of which the previous owner disappeared from existence in unknown circumstances
The old pr: #2178
changes:
-Hook by GDI.
Summary by CodeRabbit