chore: opti - reuse persistent native containers RenderSystem (landscape)

[NickKhalow]

Pull Request Description

What does this PR change?

RenderGroundSystem.RenderGroundInternal allocated and disposed a NativeArray<int> (per-mesh instance counts) and a NativeList<Matrix4x4> (instance transforms) with Allocator.TempJob every frame the ground was visible. This PR holds them as Allocator.Persistent state instead:

• Both containers live in a single nullable tuple field (instanceCounts, transforms), allocated on first use.
• On subsequent frames the counts array is zeroed and the list cleared instead of reallocating.
• Both are disposed once in OnDispose.

Why no reallocation on landscape change is needed: instanceCounts holds one slot per ground mesh kind — GroundMeshes is enum-indexed (GroundMeshPiece: Middle/Edge/Corner) with a private setter, so its length is fixed for the lifetime of the app and never depends on the loaded terrain. transforms is a NativeList and grows on demand. This is documented in a code comment at the allocation site.

Test Instructions

For testers: only the happy path needs testing — there are no edge cases reachable from user actions; the change is a frame-loop allocation optimization with identical rendering behavior.

Steps (standard run):

metaforge explorer run 9697

Expected result:

• Ground and grass render exactly as on dev in Genesis City (walk/teleport around a few parcels, look at terrain edges and corners).
• Switching to a World and back still shows correct ground.
• No new errors/warnings in logs (metaforge explorer logs tail --filter "LANDSCAPE").

Quality Checklist

• Changes have been tested locally
• Documentation has been updated (if required)
• Performance impact has been considered
• For SDK features: Test scene is included

Code Review Reference

Please review our Branch & PR Standards before submitting. It explains the automated review flow, QA/DEV approval requirements, and what each label does — especially useful for first-time contributors.

[github-actions]

🚦 CI Status

Build failed! Check the logs to see what went wrong.
If the error repeats please consider the clean-build tag.

Warnings not reduced: 13292 => 13296 — remove at least 5 warnings to merge.

Warnings/errors in files changed by this PR (4)

Assets/DCL/Landscape/Tests/PerformanceTests/GroundContainerReuse/RenderGroundContainerReusePerformanceTest.cs:196  AccessToDisposedClosure  Captured variable is disposed in the outer scope
Assets/DCL/Landscape/Tests/PerformanceTests/GroundContainerReuse/RenderGroundContainerReusePerformanceTest.cs:196  AccessToDisposedClosure  Captured variable is disposed in the outer scope
Assets/DCL/Landscape/Tests/PerformanceTests/GroundContainerReuse/RenderGroundContainerReusePerformanceTest.cs:197  AccessToDisposedClosure  Captured variable is disposed in the outer scope
Assets/DCL/Landscape/Tests/PerformanceTests/GroundContainerReuse/RenderGroundContainerReusePerformanceTest.cs:198  AccessToDisposedClosure  Captured variable is disposed in the outer scope

All Unity tests passed ✅

TESTS SUITE	Result	Passed	Failed	Skipped
EditMode	✅ Passed	24941	0	13
PlayMode	✅ Passed	236	0	36

[decentraland-bot]

STEP 2 — Root-cause check

PASS. The PR correctly addresses the root cause: per-frame Allocator.TempJob allocation and disposal churn of native containers in RenderGroundInternal. The fix promotes them to Allocator.Persistent fields allocated once and cleared per frame — a direct elimination of the allocation overhead, not a workaround.

STEP 3 — Design & integration

PASS. No new unit is introduced. The change modifies the existing RenderGroundSystem to hold persistent scratch buffers as a nullable tuple field, disposed in OnDispose. This matches the established pattern in the sibling LandscapeMiscCullingSystem (which holds nativeFrustumPlanes, cliffsBoundaries, waterBoundaries as Allocator.Persistent fields with disposal in OnDispose).

The containers are per-frame scratch buffers cleared each frame — they hold no entity/component state across frames, so CLAUDE.md §1 ("Systems must not hold persistent entity/component collections") is not violated.

TEARDOWN TRACE:

• nativeContainers.instanceCounts → .Dispose() in OnDispose (line 193) ✅
• nativeContainers.transforms → .Dispose() in OnDispose (line 194) ✅
• Pre-existing: landscape.TerrainLoaded += OnTerrainLoaded (line 50) has no -= in OnDispose — this is a pre-existing leak, not introduced by this PR.

STEP 4 — Member audit

nativeContainers (private field, added): Used by 2 consumers — RenderGroundInternal (read/write, lines 110–127) and OnDispose (read + dispose, lines 187–195). No redundancy, no single-use smell.

STEP 5 — Line-level findings

See inline comments below.

Security review: No security issues found. No secrets, credentials, user input handling, or auth changes. Internal rendering-system optimization only.

STEP 6 — Complexity

SIMPLE — 2 meaningful files (1 system, 1 new test), ~50 lines of meaningful production changes, straightforward allocation-to-persistent promotion within a single system's private method. No ECS infrastructure, query, component, or execution-order changes.

STEP 7 — QA assessment

YES — Runtime rendering code is modified (ground rendering hot path). Ground/grass rendering should be visually verified.

STEP 8 — Non-blocking warnings

⚠️ PR title uses opti: which is not a recognized semantic commit type. CI is already flagging this (semantic / title-matches-convention ❌). Consider perf: per conventional commits.

⚠️ Pre-existing: TerrainLoaded event subscriptions (lines 50, 57) are never unsubscribed in OnDispose. Not introduced by this PR but worth a follow-up fix.

⚠️ Missing trailing newline at end of RenderGroundContainerReusePerformanceTest.cs.

---

REVIEW_RESULT: FAIL ❌
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Allocation-to-persistent promotion within RenderGroundSystem's private rendering method, plus a new performance test file
QA_REQUIRED: YES

---

Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub

[decentraland-bot]

[P1] Reflection assertions will always fail. The field nativeContainers is typed (NativeArray<int>, NativeList<Matrix4x4>)? — i.e., Nullable<ValueTuple<…>>. No field in RenderGroundSystem has FieldType == typeof(NativeArray<int>) because the tuple wrapper changes the reflected type. Both assertions evaluate to false → both fail → this test provides no regression protection.

Suggested change

	Assert.That(fields.Any(f => f.FieldType == typeof(NativeArray<int>)), Is.True,
	"RenderGroundSystem must hold instanceCounts as a reused NativeArray<int> field " +
	"rather than a per-frame Allocator.TempJob local.");
	Assert.That(fields.Any(f => f.FieldType == typeof(NativeList<Matrix4x4>)), Is.True,
	"RenderGroundSystem must hold transforms as a reused NativeList<Matrix4x4> field " +
	"rather than a per-frame Allocator.TempJob local.");
	Assert.That(
	fields.Any(f => f.FieldType == typeof((NativeArray<int>, NativeList<Matrix4x4>)?)),
	Is.True,
	"RenderGroundSystem must hold instanceCounts and transforms as a reused " +
	"nullable tuple field (NativeArray<int>, NativeList<Matrix4x4>)? " +
	"rather than per-frame Allocator.TempJob locals.");

[decentraland-bot]

[P2] Null nativeContainers after disposal for double-dispose safety. After Dispose(), the field still holds a reference to the disposed containers. Setting it to null makes the null-check on line 189 a reliable guard against double-dispose and use-after-free if OnDispose is ever called twice or if RenderGroundInternal runs after disposal due to a lifecycle ordering bug.

Suggested change

	instanceCounts.Dispose();
	transforms.Dispose();
	}
	instanceCounts.Dispose();
	transforms.Dispose();
	nativeContainers = null;
	}