perf(scene): Create light list iterators on the stack at points of use

[Mauller]

This PR helps with a performance issue caused by a non block allocated, heap based, memory allocation that occurs multiple times every frame.

BaseHeightMapRenderObjClass::getStaticDiffuse() was calling RTS3DScene::createLightsIterator() and destroyLightsIterator(RefRenderObjListIterator * it) to retrieve the LightList from the current RTS3DScene.

Since the RefRenderObjListIterator is not properly poolified in the W3D or GameMemory managers, it was causing the game memory manager to constantly acquire, zero, then release system memory.

In this tweak i added an extra function in RTS3DScene to retrieve a pointer to the LightList instead.
This then allowed a stack created RefRenderObjListIterator to be created within BaseHeightMapRenderObjClass::getStaticDiffuse() instead.

This then resulted in a 20-40% performance improvement at the start of the game depending on map.

EDIT:

I have updated this to also remove the create and destroy iterator functions. I then upated all current call sites to use a stack created iterator where necessary or removed using the iterator if it was unused.

[greptile-apps]

Greptile Summary

This PR eliminates per-frame heap allocations in BaseHeightMapRenderObjClass::getStaticDiffuse() and W3DView::updateTerrain() by replacing heap-allocated RefRenderObjListIterator (via createLightsIterator/destroyLightsIterator) with stack-allocated iterators obtained through the new getLightList() accessor. The approach is sound: LightList is a member variable of the parent scene class, always valid for the scene's lifetime, and the same stack-allocation pattern is already used elsewhere in W3DScene.cpp. In W3DTerrainVisual::load(), the iterator argument was a dead parameter in initHeightData() (never read in the function body), so passing nullptr is a safe no-op.

Confidence Score: 5/5

This PR is safe to merge — the optimization is correct, well-scoped, and consistent with existing patterns in the codebase.

No P0 or P1 findings. The key correctness concerns (null safety of getLightList, dead iterator parameter in initHeightData, stack lifetime of the iterator) were all verified against the implementation and are sound.

No files require special attention.

Important Files Changed

Filename	Overview
Core/GameEngineDevice/Source/W3DDevice/GameClient/BaseHeightMap.cpp	Core hot-path change: stack-allocates RefRenderObjListIterator using getLightList(); getLightList() always returns a valid pointer, pattern is safe.
Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DView.cpp	updateTerrain() migrated to stack-allocated iterator; m_3DScene null-safety was unchanged from before.
Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DTerrainVisual.cpp	initHeightData() now receives nullptr for the lights iterator; confirmed the parameter pLightsIteratork is never read inside the function body, so this is safe.
Generals/Code/GameEngineDevice/Include/W3DDevice/GameClient/W3DScene.h	Adds getLightList() accessor and removes createLightsIterator/destroyLightsIterator declarations; clean API change.
Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DScene.cpp	Removes createLightsIterator/destroyLightsIterator implementations; the TODO comment '// poolify' is resolved by the new approach.
GeneralsMD/Code/GameEngineDevice/Include/W3DDevice/GameClient/W3DScene.h	Mirror of the Generals header change — identical getLightList() addition and iterator method removal for the Zero Hour codebase.
GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DScene.cpp	Mirror of the Generals .cpp change — removes createLightsIterator/destroyLightsIterator from the Zero Hour build.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant BaseHeightMap
    participant RTS3DScene
    participant LightList

    Note over Caller,LightList: Before (heap allocation each frame)
    Caller->>BaseHeightMap: getStaticDiffuse()
    BaseHeightMap->>RTS3DScene: createLightsIterator()
    RTS3DScene->>LightList: NEW RefRenderObjListIterator(&LightList)
    RTS3DScene-->>BaseHeightMap: it (heap ptr)
    BaseHeightMap->>BaseHeightMap: doTheLight(..., it, ...)
    BaseHeightMap->>RTS3DScene: destroyLightsIterator(it)
    RTS3DScene->>LightList: delete it

    Note over Caller,LightList: After (stack allocation)
    Caller->>BaseHeightMap: getStaticDiffuse()
    BaseHeightMap->>RTS3DScene: getLightList()
    RTS3DScene-->>BaseHeightMap: &LightList (raw ptr)
    BaseHeightMap->>BaseHeightMap: RefRenderObjListIterator it(lightlist) [stack]
    BaseHeightMap->>BaseHeightMap: doTheLight(..., &it, ...)
    Note over BaseHeightMap: it destroyed automatically on scope exit

Loading

_{Reviews (2): Last reviewed commit: "perf(heighmap): Create light list iterat..." | Re-trigger Greptile}

[Mauller]

Flame graph showing before and after comparisons when loading Han Dynasty V3

Before:

After:

[Mauller]

Some ingame with overlay fps for comparisson.

Before:

After:

[xezon]

On my wip height map change I removed the entire thing.

If I recall correctly it is entirely unused.

[Mauller]

On my wip height map change I removed the entire thing.

If I recall correctly it is entirely unused.

Are you certain?

Need to check if its just unused in a lot of maps but used when point and directional lights are on a map.

[xezon]

Not 100% certain, but at the time I reworked height map I found no evidence that it is used for anything but is passed into many functions.

[Mauller]

Not 100% certain, but at the time I reworked height map I found no evidence that it is used for anything but is passed into many functions.

I think i remember some people wanting to add point and directional lights into the game again, so might be good to leave in the code for now but add this optimisation to get rid of the performance issue with it.

[Mauller]

Updated to remove the create and destroy iterator functions and all affected call sites.

[Skyaero42]

I like it