Repository navigation
Conversation
…cliff state of maps up to version 7 Merging the Height Map code gave Generals the cliff state rows of Zero Hour, which have (width+7)/8 bytes. Retail Generals keeps them in rows of (width+1)/8 bytes, which are too short for the last cells of a row when the map width modulo 8 is 2 to 6. These cells share their bit with the first cells of the next row. Since the merge they read as no cliff instead, so the pathfinder classifies them differently and retail Generals replays on such maps mismatch. A retail compatible Generals now restores the shared values after loading. For version 7 maps the last cells of a row take the stored bits of the first cells of the next row. For older maps, whose flags are computed from the heights column by column, the first cells of the next row take the flags of the last cells, which retail computes last. Without retail compatibility, Generals and Zero Hour compute the flags of these cells from the heights for version 7 maps, because the file has no bits for them. Steep slopes in the last columns of such maps are no longer passable for ground units. A retail compatible Zero Hour and all version 8 maps keep the cliff state as before.
xezon
added
Bug
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes.
 |
| Layer / File(s) | Summary |
|---|---|
Reconstruct missing row-tail flags Core/GameEngineDevice/Source/W3DDevice/GameClient/WorldHeightMap.cpp |
Retail-compatible Generals builds copy shared cliff bits between rows for maps through blend version 7. Non-retail-compatible builds compute version-7 tail flags from terrain heights. The version-7 byte-width calculation is unchanged. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix
Possibly related issues
TheSuperHackers/GeneralsGameCode#3610reports a replay mismatch related to a height-map merge. This change restores retail cliff-state handling for height-map blend data.TheSuperHackers/GeneralsGameCode#3333covers retail-compatible pathfinding and compatibility separation. This change updates cliff values consumed by the pathfinder, but does not implement that separation.
Possibly related PRs
- TheSuperHackers/GeneralsGameCode#3368: This PR changes WorldBuilder serialization of version-7 cliff-state rows. This change restores the runtime interpretation of those rows.
Suggested reviewers: bobtista
Merge Risk: 
216eb
-
The parser change is consistent with the documented retail compatibility behavior and introduces no identified merge-blocking risk.
Pre-merge checks: 
-
Check name Status Explanation Title check The title names the main change: restoring Generals retail compatibility of the cliff state for maps up to version 7 in WorldHeightMap. The raw summary and objectives both describe the ParseBlendTileD… Description check The description states the problem (cliff-state row width mismatch from the Height Map merge in #1991), the fix in WorldHeightMap::ParseBlendTileData, and the testing done. It relates directly to the …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. Full details: Title check
-
Explanation
The title names the main change: restoring Generals retail compatibility of the cliff state for maps up to version 7 in WorldHeightMap. The raw summary and objectives both describe the ParseBlendTileData adjustment for this purpose. The title is specific and accurate, and it follows the conventional commit style used in the repository.
Full details: Description check
-
Explanation
The description states the problem (cliff-state row width mismatch from the Height Map merge in
#1991), the fix in WorldHeightMap::ParseBlendTileData, and the testing done. It relates directly to the changeset and the objectives, so it passes this lenient check.
-
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.
 [Medium impact] The PR appears safe to merge; no actionable defects were found.
|
| if (info->version==K_BLEND_TILE_VERSION_7) { | ||
| Int byteWidth = (m_width+1)/8; // previous incorrect length that got used to save the file. jba. [4/3/2003] | ||
| // previous incorrect length that got used to save the file. jba. [4/3/2003] | ||
| Int byteWidth = (m_width+1)/8; | ||
| UnsignedByte *data = new UnsignedByte[m_height*byteWidth]; | ||
| file.readArrayOfBytes((char*)data, m_height*byteWidth); | ||
| for (j=0; j<m_height; j++) { | ||
| for (i=0; i<byteWidth; i++) { | ||
| m_cellCliffState[j*m_flipStateWidth + i] = data[j*byteWidth + i]; | ||
| } | ||
| } | ||
| delete[] data; | ||
| } else { |
There was a problem hiding this comment.
yeah, it would be better to put the fix in this block like so
if (info->version >= K_BLEND_TILE_VERSION_7) {
#if RTS_GENERALS && RETAILCOMPATIBLE_CRC
file.readArrayOfBytes((char*)m_cellCliffState, m_height*m_flipStateWidth);
#else
if (info->version==K_BLEND_TILE_VERSION_7) {
Int byteWidth = (m_width+1)/8; // previous incorrect length that got used to save the file. jba. [4/3/2003]
UnsignedByte *data = new UnsignedByte[m_height*byteWidth];
file.readArrayOfBytes((char*)data, m_height*byteWidth);
for (j=0; j<m_height; j++) {
for (i=0; i<byteWidth; i++) {
m_cellCliffState[j*m_flipStateWidth + i] = data[j*byteWidth + i];
}
}
delete[] data;
} else {
file.readArrayOfBytes((char*)m_cellCliffState, m_height*m_flipStateWidth);
}
#endif
} else {
initCliffFlagsFromHeights();
}This will likely fully correct the issue.
There was a problem hiding this comment.
This also needs numBytesX back at (m_width+1)/8 for version 7 and below, otherwise the read goes past the v7 row data and misaligns everything after it.
There was a problem hiding this comment.
This will likely fully correct the issue.
But Generals runtime works with fixed Zero Hour array, not the old too short Generals array.
|
I've updated the issue with a second replay and custom map that's not fixed by this change. |
216ebe7 to
d78c2af
Compare
Ok thanks. Fixed in #3644 |
This change fixes a Generals retail mismatch caused by the Height Map merge in #1991 (330ad22).
Problem
The cliff state of a map is a bit array with one bit per terrain cell. The pathfinder reads it to decide which cells are cliffs. Retail Generals keeps it in rows of
(width+1)/8bytes. That is too short for the last cells of a row whenwidth % 8is 2 to 6, so those cells share their bit with the first cells of the next row. The merge gave Generals Zero Hour's rows of(width+7)/8bytes, so those cells read as no cliff. The pathfinder then classifies them differently from retail, and retail Generals replays on such maps mismatch. One example is a 1v1v1 replay on Grand Canyon (350 × 350), which mismatches in frame 35830.Change
WorldHeightMap::ParseBlendTileDataadjusts the cliff state once after loading.getCliffStateandsetCliffStateare unchanged.RTS_GENERALS && RETAIL_COMPATIBLE_CRC) restores the retail values of the shared cells for maps up to version 7:RETAIL_COMPATIBLE_CRC(both games): for version 7 maps, the last cells of a row are computed from the heights, because the file has no bits for them.Testing
-headless -replay: all 6 Generals 1.08 replays of GeneralsReplays plus the Grand Canyon replay pass, and all 10 Zero Hour 1.04 replays pass. Without the fix the Grand Canyon replay mismatches in frame 35830.maps.bigand on the two Generals test maps. That includes the version 6 maps CHI08 and Cliff. With the transform disabled, the model finds 99 to 248 wrong cells per map.RTS_BUILD_OPTION_RETAIL_COMPATIBLE_GAME=OFF. On all version 7 maps in the retailmaps.bigthe recomputed cells lie in the map border, outside the playable area. Maps with a very small border, such as Grand Canyon, gain these cliffs, and the computed values match the retail ones on all of its 1,745 shared cells.AI Use
Issue identified and fixed by Claude Opus 5.5