Repository navigation
feat(text): Load extra string files next to the string table - #3460
tintinhamans wants to merge 4 commits into
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 15 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe text manager collects and merges entries from language-specific and neutral ChangesText file loading
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant GameTextManager
participant ArchiveFile
participant parseStringFile
participant parseCSF
GameTextManager->>ArchiveFile: Collect STR and CSF files
ArchiveFile-->>GameTextManager: Return directory file lists
loop Each ordered file
alt STR file
GameTextManager->>parseStringFile: Parse file into output records
parseStringFile-->>GameTextManager: Return parsed records
else CSF file
GameTextManager->>parseCSF: Parse file into output records
parseCSF-->>GameTextManager: Return parsed records
end
GameTextManager->>GameTextManager: Keep first record for each label
end
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change loads extra string files with validated parsing. Malformed CSF headers and oversized wave names are now rejected instead of causing bad allocations or buffer overflows. No concrete merge-blocking risk remains in the reviewed changes. Pre-merge checks |
 |
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Issue #255 requires reliable loading of plain .str files from .big archives. GameTextManager now discovers archive-backed .str files and parses them with bounded line and quote reads. The repo… |
| Out of Scope Changes check | |
The changes remain within the string-file loading scope. .csf layering, precedence handling, vector-based parser output, malformed-file checks, map-string parser reuse, and the archive directory tra… |
| Title check | |
The title clearly and concisely describes the main change: loading additional string files next to the primary string table. |
| Description check | |
The description is directly related to the changeset and explains file discovery, precedence, parser validation, fallback behavior, and testing. |
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
32e49e1c-a26d-4c7c-847c-55d9760cf9b9
📒 Files selected for processing (2)
Core/GameEngine/Source/Common/System/ArchiveFile.cppCore/GameEngine/Source/GameClient/GameText.cpp
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
7ea64ce to
a93f4a7
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟠 Major · Validate the CSF label count before allocation. · GameText.cpp:443
Core/GameEngine/Source/GameClient/GameText.cpp:443
🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate the CSF label count before allocation.
getCSFInfoaccepts a signednum_labelsafter checking only the header ID.mergeStringFilethen passes that value toNEW StringInfo[capacity]beforeparseCSFcan reject excess records. A negative or file-inconsistent count can request an invalid or excessive allocation and prevent text initialization.Reject negative counts and counts larger than the remaining file bytes divided by the minimum label-record size.
🐛 Suggested fix
if ( header.id == CSF_ID ) { - textCount = header.num_labels; + const Int remainingBytes = file->size() - file->position(); + const Int minLabelBytes = sizeof(Int) * 3; + + if ( remainingBytes < 0 || header.num_labels < 0 || + header.num_labels > remainingBytes / minLabelBytes ) + { + break; + } + + textCount = header.num_labels; if ( header.version >= 2 ) { language = (LanguageID) header.langid;
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
b595e7f8-c6f3-4828-a350-957039cab174
📒 Files selected for processing (1)
Core/GameEngine/Source/GameClient/GameText.cpp
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.
01f10f4 to
c79f7c4
Compare
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
99ffeae to
b754b90
Compare
b754b90 to
01eb4bd
Compare
tintinhamans
changed the title
01eb4bd to
bd77ebd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
bb49ef62-8a47-4902-a46f-615802606014
📒 Files selected for processing (1)
Core/GameEngine/Source/GameClient/GameText.cpp
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.
d03882c to
a835031
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
8daf08b2-bd6d-478d-91d3-dd2b5548bd16
📒 Files selected for processing (1)
Core/GameEngine/Source/GameClient/GameText.cpp
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.
a835031 to
224c143
Compare
224c143 to
b7b7332
Compare
| if ( file->read ( &num_strings, sizeof ( Int ) ) != (Int)(sizeof ( Int )) ) | ||
| { | ||
| goto quit; | ||
| } |
There was a problem hiding this comment.
parseCSF checks whether num_strings was read, but still accepts a negative value. The string loop then does nothing, and the file contributes a label with empty text. An extra CSF can therefore hide a working label from generals.csf instead of being skipped as malformed. Reject negative string counts before accepting the label.
| if ( file->read ( &num_strings, sizeof ( Int ) ) != (Int)(sizeof ( Int )) ) | |
| { | |
| goto quit; | |
| } | |
| if ( file->read ( &num_strings, sizeof ( Int ) ) != (Int)(sizeof ( Int )) || num_strings < 0 ) | |
| { | |
| goto quit; | |
| } |
1 participant
Merge after #3458. Closes #255
Right now the game reads one string file per language, so changing two labels means shipping a full
generals.csf.With this, every other
.strand.csfindata\<Language>\anddata\loads too, loose or in a big. A file only needs the labels it changes.Precedence is the rule modders already know from bigs: whatever sorts first wins. Language folder before
data\, files in name order,.strbefore.csfof the same name, and the game's own string file last so it only fills in what nothing else set.000_mymod.strbeatszz_other.strthe same way000_mymod.bigbeatszz_other.big.Retail 1.04 has nothing extra to load. Steam 1.05 has
Data\Patch.strinPatchData.big(the two Custom Mission labels) which the 1.05 exe reads and this code never did. That gets picked up now.data\<Language>\vsdata\.strbeats.csfof the same namegenerals.csfThe first two rows are just how the file system already works.
map.stris untouched, it stays a separate table that is only checked for labels the main one doesn't have.Three commits: the parsers first push into a vector instead of a pre-sized array, then the parsers get bounds, then the layering. The vector drops the second pass over every str file that only counted END lines, the 500 entry slack, and the separate map.str parser, which was a copy of the str parser with a language filter. Malformed files get dropped instead of corrupting memory. For a csf that is oversized labels or text, a header count the file can't hold, a truncated file, or one that stops short of its header count. For a str it is a line, string or speech name longer than the buffer, or a quote that never closes, which hangs startup on main. Every read is checked, so a bogus string count no longer spins. A
Generals.strthat fails to parse falls back togenerals.csf.Tested: INI CRC unchanged (
FEAAE3F3on main, with this, and with extra str files around). Same 11 labels out of LF, CRLF and mixed, loose and in a big, so #255's line ending concern doesn't apply. 22 scenarios in game, sheet below.