unify(map): Merge GameLogic map headers and implementations - #3189
unify(map): Merge GameLogic map headers and implementations#3189OmarAglan wants to merge 1 commit into
Conversation
PR Summary by QodoUnify map: merge GameLogic map implementations
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1. Retail parser reads v2 dictionaries
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
59b40a9 to
21a5be6
Compare
OmarAglan
changed the title
|
You missed the other instances of |
| #define K_TRIGGERS_VERSION_1 1 | ||
| #define K_TRIGGERS_VERSION_2 2 // Added m_isWaterArea | ||
| #define K_TRIGGERS_VERSION_3 3 // Added m_isRiver & m_riverStart | ||
| #define K_TRIGGERS_VERSION_4 4 // Added layer name. |
There was a problem hiding this comment.
this is added so the game builds, as its required by Polygon Trigger path.
|
/agentic_review |
ok, this is bad, well im working on it, maybe im still lacking, well get them all. |
|
Code review by qodo was updated up to the latest commit 21a5be6 |
21a5be6 to
82d9f62
Compare
i think i chnaged them all, hope so. |
|
You should try to minimize these guards, only add as needed. Consider when it may affect the CRC and when it doesn’t. For example deleting polygon triggers you would expect impacts CRC, but adding a function or class member or define generally doesn’t. |
One of the finer nuances here that determines if it affects the CRC is how the data is handled for the CRC. There are some instances where a whole object get's CRC'ed instead of its portion of the CRC being generated from the objects specific CRC function. But what you mentioned is true as long as the above is not a problem. |
well, yes that make sense, will look into it, but how can i test if my chnage will impact CRC, like replays or something, a general question? |
There is no standardized vgenerals replay testing right now but you could create a new replay in a map that has polygon triggers (or add them yourself, and make sure the trigger affects the game somehow) with AI's and then replay check them with |
82d9f62 to
7177417
Compare
|
updated and cleaned up the merge, it now has less guards, and now the behavior is:
ready for review |
|
/agentic_review |
| Int readDicts = 0; | ||
| if (info->version >= K_PLAYERS_NAMES_FOR_SCRIPTS_VERSION_2) { | ||
| readDicts = file.readInt(); | ||
| } |
There was a problem hiding this comment.
1. Retail parser reads v2 dictionaries 📎 Requirement gap ≡ Correctness
ParsePlayersDataChunk unconditionally enables the Zero Hour version-2 dictionary layout for retail-compatible Generals instead of retaining its legacy parser. Version-2 chunks will therefore consume an extra integer and dictionaries on a target whose original parsing behavior must remain unchanged.
Agent Prompt
## Issue description
Retail-compatible Generals now uses the Zero Hour version-2 player-data parser, changing required target-specific behavior.
## Issue Context
Keep the legacy parser for `RTS_GENERALS && RETAIL_COMPATIBLE_CRC`; enable the version-2 dictionary format only for Zero Hour and non-retail Generals.
## Fix Focus Areas
- Generals/Code/GameEngine/Source/GameLogic/Map/SidesList.cpp[432-448]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| }; | ||
|
|
||
| DEBUG_ASSERTCRASH(m_numTeams < 1024, ("hmm, seems like an awful lot of teams...")); | ||
| DEBUG_ASSERTCRASH(m_numTeams < 2048, ("%d teams have been allocated (so far). This seems excessive.", m_numTeams )); |
There was a problem hiding this comment.
2. Retail team limit changed 📎 Requirement gap ≡ Correctness
The Generals team-count assertion is raised from the legacy 1024 limit to Zero Hour's 2048 limit without a retail-compatibility guard. This changes required retail-compatible Generals behavior rather than limiting the improvement to Zero Hour and non-retail builds.
Agent Prompt
## Issue description
Retail-compatible Generals no longer retains its original 1024-team assertion.
## Issue Context
Select the 1024 assertion for `RTS_GENERALS && RETAIL_COMPATIBLE_CRC` and retain the 2048 assertion for Zero Hour and non-retail Generals.
## Fix Focus Areas
- Generals/Code/GameEngine/Source/GameLogic/Map/SidesList.cpp[1134-1134]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 7177417 |
3 participants
Merge with Rebase
This merges the Generals and Zero Hour implementations of
PolygonTrigger,SidesList, andTerrainLogictogether with their matching headers.Target conditions preserve retail compatibility while sharing isolated Zero Hour improvements that do not affect the Generals CRC path:
PolygonTriggerpath;MapReaderWriterInfootherwise remains target-specific.SidesList.halready matches between both games except for its product copyright line.Testing
git diff --check