fix(gamefont): Fix and improve memory allocation strategies for game fonts - #3288
Open
xezon wants to merge 6 commits into
Open
fix(gamefont): Fix and improve memory allocation strategies for game fonts#3288xezon wants to merge 6 commits into
xezon wants to merge 6 commits into
Conversation
…s instead of the point size Create_GDI_Font sized its scratch DIB as a PointSize*2 square, a guess that happens to clear Arial's tmHeight by about a third but is not enforced anywhere. The copy loop in Store_GDI_Char is bounded by the extent GetTextExtentPoint32W reports, not by the bitmap, so a font whose tmHeight exceeds PointSize*2 or a glyph wider than PointSize*2 reads past GDIBitmapBits. Select the font and read its metrics before creating the bitmap, then size the bitmap from tmHeight and tmMaxCharWidth, and clamp the reported glyph extent to it. Both extents are sanity clamped so a malformed font renders clipped instead of allocating an absurd bitmap. For well formed fonts the clamp never engages, so glyph widths and text layout are unchanged. Sizing for the widest glyph the font reports costs some memory: Arial reports a tmMaxCharWidth of about 3.6 times the point size, so the bitmap is roughly 40% larger in area than the old square. There is one such bitmap per font. Also advance CurrPixelOffset by exactly what Update_Current_Buffer reserved and what Blit_Char reads back, and zero any rows GDI did not report, since the glyph blocks are not zero initialized. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… a full texel Store_GDI_Char composed each cached pixel as (v ? 0x0FFF : 0) | ((v >> 4) << 12) from the 8 bit GDI coverage v, so of the 16 stored bits only the alpha nibble and whether the coverage was non zero carried any information. Store v itself and let Blit_Char rebuild the texel. The reconstruction is exact, including the transparent white pixels that a coverage below one alpha step produces, so rendering is unchanged bit for bit. Blit_Char runs when a sentence is rebuilt rather than per frame, so the added work per pixel does not matter. A glyph block now holds the same number of glyphs in half the bytes, which is what keeps large point sizes affordable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… cell The block length was a fixed CHAR_BUFFER_LEN. Above roughly 120 point two glyphs no longer fit into one block, so every glyph got a block of its own, and the partly filled previous block was abandoned each time. Derive the block length from the widest glyph the font can produce: sixteen glyph cells, floored at the byte count that holds as many glyphs as the original block did, and capped so that a very large font does not allocate megabyte blocks. The space abandoned when a glyph does not fit is then at most one glyph at any point size, instead of growing with the font. The first blocks ramp up to a quarter and a half of that length, because a font whose working set is a handful of glyphs would otherwise pay for a whole block of them. For Arial the floor sets the block length up to roughly 19 point, and the cap from roughly 80 point. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rge fonts The texture size search only considered 64, 128 and 256 pixels. Once the character height reaches 256, around 170 point for Arial, no candidate can hold even one row of glyphs, so CurrTextureSize kept its initial value and the assert that the text fits the texture failed. Without the assert the character was blitted past the end of the locked surface. Derive the smallest usable size from the character height and from the widest glyph of the text still to be placed, and search up from there, bounded by the largest texture the device reports. The widest glyph the font can produce is not usable for this: Arial reports a tmMaxCharWidth of about 3.6 times the point size, which would push most fonts at every resolution into a larger texture. Reaching further up is a fix rather than an optimization, since the memory metric in the search rightly prefers small textures: every display string owns its own. When a string runs off the bottom of a texture, pass the text starting at the character that is placed first on the new texture. The character loops have already consumed that character when they allocate, so its width was not considered and the new texture could be too narrow for it. Its spacing is now counted in the search as well, which is the correct count. A font whose text already fit 256 pixels therefore picks the size it picked before, except that a texture opened partway through a string also counts the glyph that starts it. Skip a glyph that still does not fit instead of blitting it out of bounds, keeping the assert for debug builds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…change The asset manager keeps a permanent reference to every FontCharsClass it creates, one per font name, point size and bold flag, and only releases them in Free_Assets when the display shuts down. A resolution or font scale change requests a whole new set of point sizes without retiring the old ones, so glyph caches accumulate for the lifetime of the session. Add FontCharsClass::Free_Glyph_Cache, which drops the glyph blocks and the character arrays but keeps the object, its GDI font and its derived metrics. Every GameFont and fontData pointer held by a display string therefore stays valid, and a glyph that is wanted again is simply rasterized again. Call it for every font from W3DDisplay::reset, which runs on map load and on the way back to the shell, and after a successful mode change in W3DDisplay::setDisplayMode, which is where the previously scaled sizes become dead. Rebuilding is lazy and costs one glyph rasterization each, hidden inside transitions that already take seconds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
FontCharsBuffer stopped being a memory pool object in #3268, which dropped its W3DMPO_CODE, so no pool by that name is ever created and the entry in the pool size table can never be matched. Removing it changes nothing at runtime. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
xezon
added
Major
PR Summary by QodoFix and bound game font memory allocation
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Code Review by Qodo
1. Right-edge glyphs corrupt textures
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+1147
to
+1148
| const bool fits_texture = ((TextureOffset.I + char_spacing) < CurrTextureSize) && ((TextureOffset.J + char_height) < CurrTextureSize); | ||
| WWASSERT (fits_texture); |
There was a problem hiding this comment.
1. Right-edge glyphs corrupt textures 🐞 Bug ≡ Correctness
fits_texture checks char_spacing, while FontCharsClass::Blit_Char writes the larger data->Width column count. This occurs when packing places a glyph within the final overlap or overhang columns of a row, allowing the guarded blit to write beyond the locked surface in both sentence builders.
Agent Prompt
## Issue description
The sentence builders determine whether a glyph fits using character spacing, but the blitter writes the glyph's full width. A glyph near the right texture edge can therefore pass the new safety check and write beyond the locked surface.
## Fix Focus Areas
- Core/Libraries/Source/WWVegas/WW3D2/render2dsentence.cpp[894-960]
- Core/Libraries/Source/WWVegas/WW3D2/render2dsentence.cpp[1049-1156]
## Recommended Fix
Fetch the current glyph width and use `TextureOffset.I + char_width` for row-overflow detection and final fit validation in both centered and non-centered builders. Continue advancing `TextureOffset.I` and layout coordinates by `char_spacing` so overlap and text layout remain unchanged.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+655
to
+660
| const D3DCAPS8 &dx8caps = DX8Wrapper::Get_Current_Caps ()->Get_DX8_Caps (); | ||
| const int max_device_size = (int)min (dx8caps.MaxTextureWidth, dx8caps.MaxTextureHeight); | ||
| while (max_pow2 > TextureSizeMinPow2 && (1 << max_pow2) > max_device_size) { | ||
| max_pow2 --; | ||
| } | ||
| min_pow2 = min (min_pow2, max_pow2); |
There was a problem hiding this comment.
2. Large-font text vanishes on older cards 🐞 Bug ≡ Correctness
Allocate_New_Surface lowers min_pow2 to the device limit even when that size is smaller than the measured glyph extent. The subsequent fits_texture check then fails and suppresses Blit_Char while still advancing the layout, so large characters disappear on devices whose maximum texture is below the required glyph size.
Agent Prompt
Issue description
`Allocate_New_Surface` can select a texture smaller than the widest glyph after clamping to `MaxTextureWidth`/`MaxTextureHeight`; later packing recognizes that it does not fit and skips the glyph.
Fix Focus Areas
- Core/Libraries/Source/WWVegas/WW3D2/render2dsentence.cpp[643-660]
- Core/Libraries/Source/WWVegas/WW3D2/render2dsentence.cpp[1147-1158]
Recommended Fix
Do not treat a device-limited texture as sufficient for a glyph that exceeds it. Add an explicit oversized-glyph path that preserves visible output and consistent layout, such as choosing a supported fallback font/size before building the sentence or implementing clipped/tiled glyph rendering; do not silently skip the blit after advancing the cursor.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
| Filename | Overview |
|---|---|
| Core/Libraries/Source/WWVegas/WW3D2/render2dsentence.cpp | Implements dynamic scratch/cache/texture sizing and cache cleanup, but the new fit guard validates spacing rather than the full glyph blit width. |
| Core/Libraries/Source/WWVegas/WW3D2/render2dsentence.h | Changes glyph storage to byte coverage and adds metric-derived cache sizing state. |
| Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplay.cpp | Clears font glyph caches after successful mode changes and during display reset. |
| GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplay.cpp | Mirrors the display-lifecycle cache cleanup for Zero Hour. |
| Generals/Code/Libraries/Source/WWVegas/WW3D2/assetmgr.cpp | Adds iteration over retained font objects to discard their glyph caches. |
| GeneralsMD/Code/Libraries/Source/WWVegas/WW3D2/assetmgr.cpp | Mirrors the asset-manager glyph-cache cleanup for Zero Hour. |
| Core/GameEngine/Source/Common/System/GameMemoryInitPools_Generals.inl | Removes the obsolete FontCharsBuffer pool-size entry. |
| Core/GameEngine/Source/Common/System/GameMemoryInitPools_GeneralsMD.inl | Removes the corresponding obsolete pool-size entry for Zero Hour. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
Metrics[GDI font metrics] --> Scratch[Bounded scratch bitmap]
Scratch --> Rasterize[Rasterize glyph coverage]
Rasterize --> Cache[Byte-based glyph cache blocks]
Cache --> Sentence[Build sentence texture]
Device[Device texture limits] --> Sentence
Sentence --> Blit[Rebuild A4R4G4B4 texels and blit]
Reset[Map reset or display-mode change] --> Clear[Clear glyph caches]
Clear --> Rasterize
Prompt To Fix All With AI
### Issue 1
Core/Libraries/Source/WWVegas/WW3D2/render2dsentence.cpp:952
**Glyph fit check is narrow**
When a glyph is near the texture's right edge, this check can pass even though the glyph does not fit. It checks `char_spacing`, which excludes `PixelOverlap` and `CharOverhang`, while `Blit_Char` writes the full glyph width. The glyph can therefore write one or more columns beyond the texture row instead of being skipped. Validate the full glyph width here and in the equivalent non-centered path at line 1147.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "chore(gamememory): Remove the stale Font..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Check to ensure the text will fit on this texture | ||
| // | ||
| WWASSERT (((TextureOffset.I + char_spacing) < CurrTextureSize) && ((TextureOffset.J + char_height) < CurrTextureSize)); | ||
| const bool fits_texture = ((TextureOffset.I + char_spacing) < CurrTextureSize) && ((TextureOffset.J + char_height) < CurrTextureSize); |
There was a problem hiding this comment.
When a glyph is near the texture's right edge, this check can pass even though the glyph does not fit. It checks char_spacing, which excludes PixelOverlap and CharOverhang, while Blit_Char writes the full glyph width. The glyph can therefore write one or more columns beyond the texture row instead of being skipped. Validate the full glyph width here and in the equivalent non-centered path at line 1147.
Knowledge Base Used: WWVegas services
Prompt To Fix With AI
This is a comment left during a code review.
Path: Core/Libraries/Source/WWVegas/WW3D2/render2dsentence.cpp
Line: 952
Comment:
**Glyph fit check is narrow**
When a glyph is near the texture's right edge, this check can pass even though the glyph does not fit. It checks `char_spacing`, which excludes `PixelOverlap` and `CharOverhang`, while `Blit_Char` writes the full glyph width. The glyph can therefore write one or more columns beyond the texture row instead of being skipped. Validate the full glyph width here and in the equivalent non-centered path at line 1147.
**Knowledge Base Used:** [WWVegas services](https://app.greptile.com/thesuperhackers/-/custom-context/knowledge-base/thesuperhackers/generalsgamecode/-/docs/wwvegas-services.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merge with Rebase
This change applies further fixes and improvements for the memory allocations of game fonts. It solves inefficient allocations patterns with large font sizes and also reduces the memory footprint for the game runtime.
The changes were mainly implemented with Claude Opus 5 and went through human review and local AI review.
Summary
FontCharsClasscaches the rasterized GDI glyphs for all 2D text, andRender2DSentenceClasspacks those glyphs into textures. The sizes of the glyph buffers, the GDI scratch bitmap and the sentence textures were all fixed for the 8-20 pt fonts of an 800x600 game.Font sizes scale with the resolution. With the default
ClassicNoCeilingmethod and scaler 0.7, fonts are scaled by 1.98 at 1920x1080 and by 3.66 at 3840x2160, andFontLibrary::getFontallows up to 512 pt. At those sizes the fixed sizes fail in several ways:Store_GDI_Charis bounded by the extent GDI reports, not by the scratch bitmap it reads from, so a tall or wide glyph reads past the bitmap.This PR makes the font code correct and bounded across the whole 1-512 pt range. Rendered output stays the same, and the fonts used at 800x600 keep their existing texture sizes, apart from also counting the first glyph of a texture opened partway through a string. It is split into six commits that can be reviewed independently.
Changes
1. Size the GDI scratch bitmap from the font metrics
Problem.
Create_GDI_Fontcreated the scratch DIB as aPointSize * 2square before it knew anything about the font.Store_GDI_Charthen copied as many rows and columns asGetTextExtentPoint32Wreported. Nothing guaranteed that this extent fit inside the bitmap, so a font whosetmHeightexceedsPointSize * 2, or a glyph wider than that, read pastGDIBitmapBits.Change.
tmHeightrows, andtmMaxCharWidth + tmOverhang + PixelOverlap + 1columns, which covers the overlap column and the one-pixel shift applied to'W'.4 * PointSize + 8, so a font with absurd metrics renders clipped instead of allocating an absurd bitmap.Blit_Charalways readsCharHeightrows, and the glyph blocks are not zero-initialized.CurrPixelOffsetnow advances bycx * CharHeight. That is exactly whatUpdate_Current_Bufferreserves and whatBlit_Charreads back; the old code addedPixelOverlapa second time.For well-formed fonts the clamps never engage, so glyph widths and text layout are unchanged.
Arial reports a
tmMaxCharWidthof about 3.6 times the point size, so the scratch bitmap is roughly 40% larger in area than the old square (46x18 instead of 24x24 at 12 pt). There is one such bitmap per font object; at 3840x2160 it costs a few tens of KB per font, which is the price of a copy that is safe for any glyph.2. Cache one byte of coverage per glyph pixel
Problem. Each cached pixel was a 16-bit A4R4G4B4 word, composed as
(v ? 0x0FFF : 0) | ((v >> 4) << 12)from the 8-bit GDI coveragev. Only the alpha nibble and whethervis zero carried any information.Change. The glyph buffers store
vitself, andBlit_Charrebuilds the texel with the same formula. The rebuild is exact, including the transparent white texels produced by coverage below one alpha step, so the textures contain the same bytes as before.Blit_Charruns when a sentence is rebuilt, not every frame. Every glyph now takes half the memory.3. Size the glyph cache blocks from the font's own glyph cell
Problem. Blocks had a fixed size of 32768
uint16. Small fonts filled them well, but above roughly 120 pt two glyphs no longer fit into one block, so each glyph gets a block of its own and the rest of the previous block is abandoned. From about 126 pt the widest glyphs exceed a block entirely and, since #3268, get an exactly sized block. Large fonts therefore make one heap allocation per glyph.Change. A block is sized from the worst-case glyph cell of its font,
GlyphBitmapWidth * CharHeight:The block size balances three costs:
MemoryPoolSingleBlockheader plus aGlobalAlloceach4. Let the sentence texture grow beyond 256 pixels for large fonts
Problem.
Allocate_New_Surfaceonly considered 64, 128 and 256 px textures. Once the character height reaches 256 px (around 170 pt for Arial), none of them holds a row of glyphs.CurrTextureSizethen kept its initial value of 256 and the assert that the text fits the texture failed. In a release build the glyph was blitted past the end of the locked surface.Change.
tmMaxCharWidthof about 3.6 times the point size, which would move most fonts at every resolution into a larger texture.MaxTextureWidth/MaxTextureHeight.W3DGameWindow::winSetTextcan do, if the new text has a wider glyph.Textures stay square, because
Build_TexturesandDraw_Sentencerely on that.5. Discard cached glyphs on map load and on resolution change
Problem.
WW3DAssetManagerkeeps a permanent reference to everyFontCharsClass, one per font name, point size and bold flag, and releases them only inFree_Assetswhen the display shuts down. A resolution or font scale change requests a whole new set of point sizes without retiring the old ones, so glyph caches accumulate for the whole session. Each UI font can also create up to four font objects: regular, bold for hotkeys, and the same-size Unicode alternate of each.Change.
FontCharsClass::Free_Glyph_Cachefrees the glyph blocks and character arrays but keeps the object, its GDI font and its metrics. EveryGameFontandfontDatapointer held by display strings stays valid, and a glyph that is needed again is simply rasterized again.WW3DAssetManager::Free_All_FontChars_Glyph_Cachesapplies this to every font.W3DDisplay::reset, which runs on map load and on the return to the shell, and after a successful mode change inW3DDisplay::setDisplayMode, the point where the previously scaled sizes become dead.Rebuilding costs one glyph rasterization per glyph that is used again, during transitions that already take seconds.
6. Remove the stale
FontCharsBuffermemory pool entryFontCharsBufferstopped being a memory pool object in #3268, so no pool by that name is ever created and its entry in the pool size tables can never match. Removing it changes nothing at runtime.Memory
The numbers below replay the old and the new allocation logic, as written, against real GDI measurements: Arial at 96 DPI, measured with
GetTextMetricsandGetTextExtentPoint32W. Point sizes use the defaultClassicNoCeilingscaling with scaler 0.7.The absolute amounts are modest; the main value of this PR is correctness and bounded behaviour at large sizes. The glyph cache still shrinks at every resolution.
A typical set of UI fonts
This is an illustrative working set, not a capture from the game. Glyphs are cached on demand, so only the glyphs a font actually draws count.
Each font also creates a same-size Unicode alternate that draws no glyphs for English text but still owns a scratch bitmap; those bitmaps are included.
At 3840x2160 the glyph blocks also drop from 38 allocations to 13. The game uses more fonts and sizes than this set, so real totals scale up accordingly.
Large fonts
A 30-glyph caption in bold, glyph blocks only:
From about 120 pt the old code allocates one block per glyph.
Changing resolution
Switching 1920x1080 → 2560x1440 → 3840x2160 in the options menu without loading a map:
Testing
vc6presets and a modern MSVC preset for Generals and Zero HourmainResolutionFontAdjustmentinoptions.ini, or the script action that sets a font size): glyphs above 170 pt render, or are skipped, without memory corruption