fix(debug): Allocate enough terrain debug icons based on the map dimensions in W3DDebugIcons()#2231
Conversation
Greptile Overview
|
| Filename | Overview |
|---|---|
| Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDebugIcons.cpp | Implemented dynamic allocation based on map size, fixed delete to delete[], added allocation assertion |
| GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDebugIcons.cpp | Implemented dynamic allocation based on map size, fixed delete to delete[], added allocation assertion |
Sequence Diagram
sequenceDiagram
participant TV as W3DTerrainVisual
participant HM as LogicHeightMap
participant DI as W3DDebugIcons
participant Arr as DebugIcon Array (static)
TV->>HM: getXExtent()
HM-->>TV: mapWidth
TV->>HM: getYExtent()
HM-->>TV: mapHeight
TV->>DI: NEW W3DDebugIcons(mapWidth, mapHeight)
activate DI
DI->>DI: m_maxDebugIcons = mapWidth * mapHeight
DI->>DI: allocateIconsArray()
DI->>Arr: NEW DebugIcon[m_maxDebugIcons]
Arr-->>DI: allocated array
DI->>DI: m_numDebugIcons = 0
deactivate DI
TV->>DI: Release_Ref() (belongs to scene)
Generals/Code/GameEngineDevice/Include/W3DDevice/GameClient/W3DDebugIcons.h
Outdated
Show resolved
Hide resolved
1f397d1 to
3aae899
Compare
|
Updated to size the number of debug icons based on the size of the map |
Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDebugIcons.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDebugIcons.cpp
Outdated
Show resolved
Hide resolved
Generals/Code/GameEngineDevice/Include/W3DDevice/GameClient/W3DDebugIcons.h
Show resolved
Hide resolved
Generals/Code/GameEngineDevice/Include/W3DDevice/GameClient/W3DDebugIcons.h
Show resolved
Hide resolved
3aae899 to
a93d8a5
Compare
|
Tweaked the implementation to remove some of the variables that were redundant |
Additional Comments (2)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! Prompt To Fix With AIThis is a comment left during a code review.
Path: Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDebugIcons.cpp
Line: 105:105
Comment:
Should reset `m_maxDebugIcons = 0` for consistency with other static members
```suggestion
m_maxDebugIcons = 0;
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! Prompt To Fix With AIThis is a comment left during a code review.
Path: GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDebugIcons.cpp
Line: 105:105
Comment:
Should reset `m_maxDebugIcons = 0` for consistency with other static members
```suggestion
m_maxDebugIcons = 0;
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise. |
GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDebugIcons.cpp
Show resolved
Hide resolved
GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDebugIcons.cpp
Outdated
Show resolved
Hide resolved
a93d8a5 to
01cdaad
Compare
Additional Comments (2)
Prompt To Fix With AIThis is a comment left during a code review.
Path: Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDebugIcons.cpp
Line: 100:104
Comment:
[P0] Mismatched delete for array allocation
`m_debugIcons` is allocated with `NEW DebugIcon[m_maxDebugIcons]` in `allocateIconsArray()` but freed with `delete m_debugIcons` in the destructor. If `NEW` maps to normal `new`, this should be `delete[]` to avoid undefined behavior (bad destructor call / heap corruption depending on allocator).
How can I resolve this? If you propose a fix, please make it concise.
Same issue as Generals variant: Prompt To Fix With AIThis is a comment left during a code review.
Path: GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDebugIcons.cpp
Line: 100:104
Comment:
[P0] Mismatched delete for array allocation
Same issue as Generals variant: `m_debugIcons` is allocated with `NEW DebugIcon[m_maxDebugIcons]` but freed with `delete m_debugIcons`. If `NEW` maps to normal `new`, this must be `delete[]` to avoid UB / heap corruption.
How can I resolve this? If you propose a fix, please make it concise. |
01cdaad to
3d2bace
Compare
|
Updated based on review comments |
|
Greptile is complaining about a copy constructor:
|
I think it's hallucinating since that variable is static anyway |
Edit: Updated the PR to now allocate based on the map dimensions.
A simple PR that helps with debugging pathfinding and other systems that draw overlay tiles on the map.
In some instances the defaulT 100k tiles is not enought, resulting in areas of the map missing debug icons.
The following example is an extreme case, but i have seen some complex maps where parts of the map are missing debug icons on the terrain before any buildings are constructed.
Before:


After:

