Repository navigation
Conversation
xezon
added
Minor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughHLodDefClass now copies additional models and retains proxy references from an HLodClass. Saving includes LOD, aggregate, and proxy arrays. Tests compare saved HLOD data with serialized fixtures. ChangesHLOD Definition Serialization
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Description check |  |
The description clearly explains the missing aggregate and proxy save data, the serialization changes, initialization behavior, and added tests. |
| Title check | |
The title clearly and concisely identifies the main change: preserving aggregate and proxy arrays in HLOD definitions. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
- 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.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
 [Medium impact] The PR appears safe to merge; no new actionable issue was found.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Propagate aggregate chunk-finalization failures. · hlod.cpp:553-560
Generals/Code/Libraries/Source/WWVegas/WW3D2/hlod.cpp:553-560
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPropagate aggregate chunk-finalization failures.
When
Aggregates.ModelCount > 0,End_Chunk()can report that the aggregate header rewrite failed.Save_Aggregate_Arrayignores that result and returnsWW3D_ERROR_OK, soHLodDefClass::Savecan report success for an invalid aggregate chunk. Check all three operations, and callEnd_Chunk()after every attempted outer chunk, including a failedBegin_Chunk().Suggested fix
WW3DErrorType HLodDefClass::Save_Aggregate_Array(ChunkSaveClass & csave) { if (Aggregates.ModelCount > 0) { - csave.Begin_Chunk(W3D_CHUNK_HLOD_AGGREGATE_ARRAY); - Aggregates.Save_W3D(csave); - csave.End_Chunk(); + bool success = csave.Begin_Chunk(W3D_CHUNK_HLOD_AGGREGATE_ARRAY) == TRUE; + if (success) { + success = Aggregates.Save_W3D(csave); + } + success = (csave.End_Chunk() == TRUE) && success; + if (!success) { + return WW3D_ERROR_SAVE_FAILED; + } } return WW3D_ERROR_OK; }
🟡 Minor · Propagate aggregate write failures. · hlod.cpp:553-560
GeneralsMD/Code/Libraries/Source/WWVegas/WW3D2/hlod.cpp:553-560
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPropagate aggregate write failures.
When
Aggregates.ModelCount > 0,Save_Aggregate_ArrayignoresBegin_ChunkandAggregates.Save_W3D. A failed write can therefore produce incomplete aggregate output whileHLodDefClass::SavereportsWW3D_ERROR_OK. Preserve the unconditionalEnd_Chunk()call and returnWW3D_ERROR_SAVE_FAILEDfor any failed operation.Suggested fix
WW3DErrorType HLodDefClass::Save_Aggregate_Array(ChunkSaveClass & csave) { if (Aggregates.ModelCount > 0) { - csave.Begin_Chunk(W3D_CHUNK_HLOD_AGGREGATE_ARRAY); - Aggregates.Save_W3D(csave); - csave.End_Chunk(); + bool success = csave.Begin_Chunk(W3D_CHUNK_HLOD_AGGREGATE_ARRAY) == TRUE; + if (success) { + success = Aggregates.Save_W3D(csave); + } + success &= csave.End_Chunk(); + if (!success) { + return WW3D_ERROR_SAVE_FAILED; + } } return WW3D_ERROR_OK; }
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
da9c77c1-abbc-4826-98d5-9dcb71b52f32
📒 Files selected for processing: 6
Generals/Code/Libraries/Source/WWVegas/WW3D2/hlod.cppGenerals/Code/Libraries/Source/WWVegas/WW3D2/hlod.hGeneralsMD/Code/Libraries/Source/WWVegas/WW3D2/hlod.cppGeneralsMD/Code/Libraries/Source/WWVegas/WW3D2/hlod.hTests/Google/Core/CMakeLists.txtTests/Google/Core/Libraries/WWVegas/WW3D2/hlod_test.cpp
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.
HLodDefClass::Load_W3D reads the aggregate and proxy arrays of a W3D_CHUNK_HLOD, but Save wrote neither, so W3DView's Export LOD dropped them. Save_Aggregate_Array was never called, and if it had been, SubObjectArrayClass::Save_W3D would have nested the aggregates in a W3D_CHUNK_HLOD_LOD_ARRAY where Load_W3D cannot read them. There was no writer for proxies at all. Initialize, which W3DView uses after Record Screen Area and Include NULL LOD, copied only the LOD arrays. SubObjectArrayClass::Save_W3D now writes only the array contents, as its Load_W3D reads them and as max2w3d writes them. Save_Lod_Array wraps each LOD array in its chunk, and Save writes the aggregate array and the new proxy array after the LOD arrays, in max2w3d's order. Initialize copies the additional models as aggregates and shares the proxy array, and Free now also releases the aggregates. No HLod in the retail archives of either game has an aggregate or proxy array. A Google Test round-trips an HLod chunk with both arrays and one with only a LOD array, and saves a definition built from an HLod with an additional model.
835bf71 to
45cf94b
Compare
1 participant
This change fixes missing save data in
HLodDefClass::Save.It is not a practical issue for the game and existing tools, only for future W3D writers that would go through this code.
The new code also matches what the max2w3d plugin does, so Claude claims.
AI summary
HLodDefClass::Load_W3Dreads the aggregate and proxy arrays of aW3D_CHUNK_HLOD, butSave wrote neither, so W3DView's Export LOD dropped them.
Save_Aggregate_Arraywas nevercalled, and if it had been,
SubObjectArrayClass::Save_W3Dwould have nested theaggregates in a
W3D_CHUNK_HLOD_LOD_ARRAYwhereLoad_W3Dcannot read them. There was nowriter for proxies at all. Initialize, which W3DView uses after Record Screen Area and
Include NULL LOD, copied only the LOD arrays.
SubObjectArrayClass::Save_W3Dnow writes only the array contents, as its Load_W3D readsthem and as max2w3d writes them. Save_Lod_Array wraps each LOD array in its chunk, and
Save writes the aggregate array and the new proxy array after the LOD arrays, in
max2w3d's order. Initialize copies the additional models as aggregates and shares the
proxy array, and Free now also releases the aggregates. No HLod in the retail archives
of either game has an aggregate or proxy array.
A Google Test round-trips an HLod chunk with both arrays and one with only a LOD array,
and saves a definition built from an HLod with an additional model.
AI Use
This bug was found and fixed by Claude Opus 5.5. Tweaked a bit by hand.