Skip to content

Commit aa1b6b5

Browse files
soloturnnaalitclaude
committed
feat(world): merge chunk light after ready, not as a pipeline stage
Light merging was the last stage of ChunkProcessingPipeline, so a chunk could not become ready until all 26 neighbours needed for its merge existed too. One slow - or never-requested - neighbour stalled the chunk indefinitely, which is why c645e81 had to add a 10s idle timeout that force-skips the stage. Now a chunk goes ready as soon as its own generation finishes, and LateLightMerger merges it afterwards, incrementally, as neighbours arrive. A chunk at the edge of the loaded world simply stays unmerged - visible, with imperfect edge lighting - instead of never becoming ready. When a neighbour does load later, that neighbour's own merge propagates across the shared face and corrects it. Light merging was the only multi-chunk stage: the sole production uses of ChunkTaskProvider.createMulti were the three "Light merging" stages removed here. The pipeline's dependency machinery (blockedPositions, skipBlockedStages, MultiplyRequirementChunkTask) is therefore now dead, but is left in place - removing it here would double the review surface and ChunkProcessingPipelineTest still covers the API. Follow-up. Two things worth calling out: - LightMerger marked nothing dirty. That was harmless while merging ran before the chunk was ready, since its first mesh was still to come and picked the result up for free. Now the chunk is already visible, so without marking, merged light never reaches a mesh - ChunkMeshWorker only re-meshes chunks that are isReady() && isDirty(). LocalChunkView now marks the chunk it writes to and any chunk within one block of that write, as AbstractFullWorldView already does for runtime block changes. Marking the whole 27-chunk neighbourhood from the merger instead was the obvious alternative and is wrong: a chunk sits in 27 neighbourhoods, so it gets re-meshed over and over during a world load, and most of those merges never touch its light. That exhausted the 768MB heap in mesh generation within minutes of starting a world - found by running the game, not by any test here. The one-block expansion matters and is not symmetry for its own sake: a chunk mesh samples its neighbours light to shade the faces along a shared boundary, and propagation does not necessarily write into that neighbour as well - if its side of the boundary is opaque no light is written there at all, yet its mesh is what shades the solid face. Interior writes still touch only their own chunk. - Merging now touches live chunks rather than chunks only the pipeline can see, and deflated light storage can reallocate on write. It runs on the main thread from each provider's update() under a time budget, matching how runtime light propagation for block changes already works (WorldProviderCoreImpl.processPropagation). The design is @naalit's, from #4879: let a chunk go ready on its own and merge light incrementally afterwards. That PR could not be rebased - it targets an unmerged branch predating the engine package restructure - so this is a fresh implementation of the same idea, with no code carried over. It differs in two ways: #4879 merged off-thread, and it omitted the dirty-marking above, so its merged light would never have reached a mesh. Covered by two tests, both verified to fail against the pipeline-stage merge rather than merely pass against this one: - LateLightMergerMteTest asks for relevance on a single chunk, and separately on an unpadded 3x3x3 region, then bounds readiness at 8s - above generation time, below the ~10s the old idle-skip needed. Against the old code both time out. The second case also shows ChunkRegionFuture.REQUIRED_CHUNK_MARGIN is no longer structurally needed for readiness; f324128 records that padding chunks near a region edge could not complete their own stages before, which is the same root cause. The margin is left in place - other tests share it. - LateLightMergerTest drives the merger over a plain map and covers the requeue in mergeAt: queue a position, drop a neighbour before the queue drains, and the position must return to needsMerging rather than be lost. Deleting that one line makes this test fail. Co-Authored-By: naalit <sam@dirkback.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 338d7dd commit aa1b6b5

6 files changed

Lines changed: 414 additions & 20 deletions

File tree

‎engine-tests/src/test/java/org/terasology/engine/integrationenvironment/LateLightMergerMteTest.java‎

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
// Copyright 2026 The Terasology Foundation
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
package org.terasology.engine.integrationenvironment;
5+
6+
import org.joml.Vector3f;
7+
import org.joml.Vector3fc;
8+
import org.joml.Vector3i;
9+
import org.junit.jupiter.api.Test;
10+
import org.terasology.engine.entitySystem.entity.EntityManager;
11+
import org.terasology.engine.entitySystem.entity.EntityRef;
12+
import org.terasology.engine.entitySystem.entity.internal.EntityScope;
13+
import org.terasology.engine.integrationenvironment.jupiter.IntegrationEnvironment;
14+
import org.terasology.engine.logic.location.LocationComponent;
15+
import org.terasology.engine.network.NetworkMode;
16+
import org.terasology.engine.world.block.BlockRegionc;
17+
import org.terasology.engine.world.chunks.ChunkProvider;
18+
import org.terasology.engine.world.chunks.Chunks;
19+
import org.terasology.engine.world.chunks.localChunkProvider.RelevanceSystem;
20+
import org.terasology.unittest.worlds.DummyWorldGenerator;
21+
22+
import java.util.stream.StreamSupport;
23+
24+
/**
25+
* Pins down the central behaviour change of {@code LateLightMerger}: a chunk becomes ready as soon
26+
* as its own generation finishes, not only once its whole 3x3x3 neighbourhood does too.
27+
* <p>
28+
* Under the pipeline stage this replaced, a chunk whose neighbours were never requested could only
29+
* become ready via {@code ChunkProcessingPipeline}'s idle-skip timeout - two consecutive 5s-idle
30+
* polls, so ~10s at best (see {@code POLL_INTERVAL_MS} / {@code IDLE_POLLS_BEFORE_SKIP} there). Both
31+
* tests below bound completion well under that, on relevance requests deliberately built so the
32+
* requested chunk(s)' own neighbours are never themselves requested. That makes them fail loudly
33+
* (timeout) rather than merely slowly against the old, pipeline-stage merge - see the task/PR notes
34+
* for the actual before/after timings observed when checking that.
35+
* <p>
36+
* Deliberately not asserted here: that a requested chunk's neighbours stay unloaded. They usually do,
37+
* but {@code RelevanceSystem.addRelevanceEntity}'s own {@code .sorted()} pass over a
38+
* {@code BlockRegion}'s iterator can request one extra, wrong position - a pre-existing aliasing bug
39+
* (the iterator hands out a reused, mutable {@code Vector3i} that a later {@code hasNext()} call can
40+
* mutate out from under a caller that buffers rather than immediately consumes it), unrelated to
41+
* light merging. {@code RelevanceSystem.updateRelevance()}'s follow-up pass - which uses the
42+
* defensive-copying {@code ChunkRelevanceRegion.getNeededChunks()} instead - still requests the
43+
* correct position(s) a tick later, so it doesn't affect these tests' timing, but it does mean a
44+
* "neighbours were never loaded" assertion is not reliable and was left out rather than pinned to
45+
* today's incidental behaviour of an unrelated bug.
46+
*
47+
* @see org.terasology.engine.world.chunks.LateLightMerger
48+
*/
49+
@IntegrationEnvironment(networkMode = NetworkMode.LISTEN_SERVER)
50+
class LateLightMergerMteTest {
51+
52+
/**
53+
* Comfortably above what one dummy-world chunk takes to generate, comfortably below the ~10s
54+
* ChunkProcessingPipeline idle-skip the old pipeline-stage merge needed whenever a chunk's
55+
* neighbours were never requested.
56+
*/
57+
private static final long READY_TIMEOUT_MS = 8000;
58+
59+
@Test
60+
void chunkBecomesReadyWithoutNeighbourhoodLoaded(EntityManager entityManager, RelevanceSystem relevanceSystem,
61+
MainLoop mainLoop, ChunkProvider chunkProvider) {
62+
// Far from spawn (a fixed (0,0,0) for DummyWorldGenerator) and from the other test below, so
63+
// nothing else ever requests this position or its neighbours.
64+
Vector3fc center = new Vector3f(200_000, DummyWorldGenerator.SURFACE_HEIGHT, 200_000);
65+
Vector3i chunkPos = Chunks.toChunkPos(center, new Vector3i());
66+
67+
EntityRef entity = entityManager.create(new LocationComponent(center));
68+
entity.setScope(EntityScope.GLOBAL);
69+
// distance (1,1,1) requests relevance for exactly this one chunk - unlike
70+
// ChunkRegionFuture, no margin, so its neighbours are never deliberately requested.
71+
relevanceSystem.addRelevanceEntity(entity, new Vector3i(1, 1, 1), null);
72+
73+
mainLoop.awaitUntil(READY_TIMEOUT_MS, "an isolated chunk (no neighbours requested) to become ready",
74+
() -> chunkProvider.isChunkReady(chunkPos));
75+
}
76+
77+
@Test
78+
void relevanceRegionBecomesFullyReadyWithoutMargin(EntityManager entityManager, RelevanceSystem relevanceSystem,
79+
MainLoop mainLoop, ChunkProvider chunkProvider) {
80+
// ChunkRegionFuture.REQUIRED_CHUNK_MARGIN pads every relevance request by one extra shell of
81+
// chunks, specifically so the requested region's own outer shell has its neighbourhood
82+
// requested too (see its FIXME comment). Going straight to RelevanceSystem instead of through
83+
// ChunkRegionFuture, with no padding at all, tests whether that padding is still needed now
84+
// that readiness no longer waits on the neighbourhood.
85+
Vector3fc center = new Vector3f(300_000, DummyWorldGenerator.SURFACE_HEIGHT, 300_000);
86+
87+
EntityRef entity = entityManager.create(new LocationComponent(center));
88+
entity.setScope(EntityScope.GLOBAL);
89+
BlockRegionc region = relevanceSystem.addRelevanceEntity(entity, new Vector3i(3, 3, 3), null);
90+
91+
mainLoop.awaitUntil(READY_TIMEOUT_MS, "every chunk in an unpadded 3x3x3 relevance region to become ready",
92+
() -> StreamSupport.stream(region.spliterator(), false).allMatch(chunkProvider::isChunkReady));
93+
}
94+
}

‎engine-tests/src/test/java/org/terasology/engine/world/chunks/LateLightMergerTest.java‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
// Copyright 2026 The Terasology Foundation
2+
// SPDX-License-Identifier: Apache-2.0
3+
package org.terasology.engine.world.chunks;
4+
5+
import com.google.common.collect.Maps;
6+
import org.joml.Vector3i;
7+
import org.joml.Vector3ic;
8+
import org.junit.jupiter.api.BeforeEach;
9+
import org.junit.jupiter.api.Tag;
10+
import org.junit.jupiter.api.Test;
11+
import org.terasology.engine.TerasologyTestingEnvironment;
12+
import org.terasology.engine.registry.CoreRegistry;
13+
import org.terasology.engine.world.block.internal.BlockManagerImpl;
14+
import org.terasology.engine.world.block.tiles.NullWorldAtlas;
15+
import org.terasology.engine.world.chunks.blockdata.ExtraBlockDataManager;
16+
import org.terasology.engine.world.chunks.internal.ChunkImpl;
17+
import org.terasology.engine.world.propagation.light.LightMerger;
18+
import org.terasology.gestalt.assets.management.AssetManager;
19+
20+
import java.util.Map;
21+
22+
import static com.google.common.truth.Truth.assertThat;
23+
24+
/**
25+
* {@link LateLightMerger} is driven directly here rather than through an {@code IntegrationEnvironment}
26+
* - its constructor takes a plain {@code Map<Vector3ic, Chunk>}, so the bookkeeping can be tested
27+
* without a running engine. Real {@link ChunkImpl} chunks (rather than a bare stub) are used because
28+
* {@link LightMerger#merge} does genuine light propagation and needs real block/light storage - see
29+
* {@code BetweenChunkPropagationTest} for the same pattern.
30+
*/
31+
@Tag("TteTest")
32+
class LateLightMergerTest extends TerasologyTestingEnvironment {
33+
34+
private BlockManagerImpl blockManager;
35+
private ExtraBlockDataManager extraDataManager;
36+
37+
@BeforeEach
38+
@Override
39+
public void setup() throws Exception {
40+
super.setup();
41+
blockManager = new BlockManagerImpl(new NullWorldAtlas(), CoreRegistry.get(AssetManager.class), true);
42+
extraDataManager = new ExtraBlockDataManager();
43+
}
44+
45+
private Chunk createChunkAt(Vector3ic pos) {
46+
return new ChunkImpl(new Vector3i(pos), blockManager, extraDataManager);
47+
}
48+
49+
/**
50+
* Covers a bug found and fixed in review, with no prior coverage: {@link LateLightMerger#mergeAt}
51+
* re-checks the neighbourhood at merge time, not just at queue time in {@link
52+
* LateLightMerger#chunkReady}. A position that loses a neighbour in between must go back into
53+
* {@code needsMerging} rather than being dropped - it is queued from neither bookkeeping set
54+
* otherwise, and only a chunk becoming ready ever re-queues anything, so it would stay unmerged
55+
* forever even once the neighbour comes back.
56+
*/
57+
@Test
58+
void positionRequeuedWhenNeighbourGoesMissingBeforeMergeRuns() {
59+
Vector3ic center = new Vector3i(0, 0, 0);
60+
Vector3ic missingNeighbour = new Vector3i(1, 0, 0);
61+
62+
Map<Vector3ic, Chunk> chunkCache = Maps.newHashMap();
63+
for (Vector3ic pos : LightMerger.requiredChunks(center)) {
64+
chunkCache.put(new Vector3i(pos), createChunkAt(pos));
65+
}
66+
67+
// The merge only writes - and so only dirties - where light actually moves, so an entirely
68+
// uniform neighbourhood would merge to no observable effect at all. Light the face of the
69+
// +X neighbour that abuts the center chunk, giving the merge something to propagate inwards.
70+
Chunk litNeighbour = chunkCache.get(missingNeighbour);
71+
for (int y = 0; y < 4; y++) {
72+
for (int z = 0; z < 4; z++) {
73+
litNeighbour.setLight(0, y, z, (byte) 15);
74+
}
75+
}
76+
// ChunkImpl starts dirty (it still needs its first mesh); clear that so isDirty() below is a
77+
// clean signal for "the merge wrote here", not construction noise.
78+
chunkCache.values().forEach(chunk -> chunk.setDirty(false));
79+
80+
LateLightMerger merger = new LateLightMerger(chunkCache);
81+
82+
// Full neighbourhood already present, so this discovers it and queues center for merging.
83+
merger.chunkReady(center);
84+
85+
// A neighbour unloads before the merge actually runs - checkForUnload() runs every tick in
86+
// both providers, ahead of processPending().
87+
Chunk removedNeighbour = chunkCache.remove(missingNeighbour);
88+
merger.chunkUnloaded(missingNeighbour);
89+
90+
merger.processPending();
91+
92+
// mergeAt() must have found the hole and backed off rather than merging with it.
93+
assertThat(chunkCache.get(center).isDirty()).isFalse();
94+
95+
// The neighbour reloads. Nothing but a chunkReady() call ever re-discovers a completed
96+
// neighbourhood - if mergeAt() had dropped center instead of requeuing it, this would never
97+
// recover it and the assertions below would fail.
98+
chunkCache.put(new Vector3i(missingNeighbour), removedNeighbour);
99+
merger.chunkReady(missingNeighbour);
100+
merger.processPending();
101+
102+
// The seeded light has now propagated into the center chunk, which is both the proof that
103+
// mergeAt() ran for it and the reason ChunkMeshWorker will re-mesh it.
104+
assertThat(chunkCache.get(center).isDirty()).isTrue();
105+
}
106+
}

‎engine/src/main/java/org/terasology/engine/world/chunks/LateLightMerger.java‎

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,152 @@
1+
// Copyright 2026 The Terasology Foundation
2+
// SPDX-License-Identifier: Apache-2.0
3+
package org.terasology.engine.world.chunks;
4+
5+
import com.google.common.collect.Sets;
6+
import org.joml.Vector3i;
7+
import org.joml.Vector3ic;
8+
import org.slf4j.Logger;
9+
import org.slf4j.LoggerFactory;
10+
import org.terasology.engine.world.propagation.light.LightMerger;
11+
12+
import java.util.ArrayDeque;
13+
import java.util.Deque;
14+
import java.util.List;
15+
import java.util.Map;
16+
import java.util.Set;
17+
18+
/**
19+
* Merges light for chunks after they are already visible, instead of before.
20+
* <p>
21+
* {@link LightMerger} used to run as the last stage of {@code ChunkProcessingPipeline}, which meant a
22+
* chunk could not become ready until all 26 neighbours needed for its merge existed too - one slow or
23+
* never-requested neighbour stalled the chunk indefinitely. Now a chunk goes ready as soon as its own
24+
* generation finishes, and this class merges it with its neighbours afterwards, incrementally, as those
25+
* neighbours arrive. A chunk at the edge of the loaded world simply stays unmerged (visible, with
26+
* imperfect edge lighting) instead of never becoming ready at all.
27+
* <p>
28+
* Shared by {@code LocalChunkProvider} and {@code RemoteChunkProvider}, which otherwise need
29+
* byte-for-byte the same bookkeeping over their own {@code chunkCache}. {@link #chunkReady} and {@link
30+
* #processPending} are meant to be called only from the owning provider's {@code update()} - always the
31+
* same thread, so no internal synchronization here. That matters beyond tidiness: merging now touches
32+
* chunks that are live, not chunks only the pipeline can see. The renderer may be tessellating them on
33+
* another thread, and deflated light storage can reallocate on write, so a concurrent merge would not be
34+
* merely a stale-value glitch. Running on the thread that already owns {@code chunkCache} sidesteps
35+
* that, mirroring how runtime light propagation for block changes ({@code
36+
* WorldProviderCoreImpl.processPropagation()}) also runs on the main thread rather than in parallel.
37+
*/
38+
public final class LateLightMerger {
39+
/**
40+
* Per-tick budget for {@link #processPending}, in the spirit of the ready-chunk drain loop it runs
41+
* alongside. Merging a position re-propagates light across a full 3x3x3 of chunks, which is not
42+
* free, and a single newly-ready chunk can complete several neighbourhoods at once (see {@link
43+
* #chunkReady}), so this is drained gradually rather than all at once.
44+
*/
45+
private static final int PROCESSING_DEADLINE_MS = 24;
46+
private static final Logger logger = LoggerFactory.getLogger(LateLightMerger.class);
47+
48+
private final Map<Vector3ic, Chunk> chunkCache;
49+
/** Ready positions still waiting on part of their own 27-chunk neighbourhood. */
50+
private final Set<Vector3ic> needsMerging = Sets.newHashSet();
51+
/**
52+
* Positions whose neighbourhood is complete, awaiting the actual merge. Kept separate from {@link
53+
* #needsMerging} so discovering a mergeable position (cheap) is decoupled from performing the merge
54+
* (not cheap) - see {@link #processPending}.
55+
*/
56+
private final Deque<Vector3ic> readyToMerge = new ArrayDeque<>();
57+
58+
public LateLightMerger(Map<Vector3ic, Chunk> chunkCache) {
59+
this.chunkCache = chunkCache;
60+
}
61+
62+
/**
63+
* Record that {@code chunkPos} is ready and already in the chunk cache, and queue a merge for any
64+
* position this completes the neighbourhood of.
65+
* <p>
66+
* The scan below covers {@code chunkPos}'s own 3x3x3 neighbourhood, not just {@code chunkPos}
67+
* itself - a newly-ready chunk can just as easily complete one of its neighbours' neighbourhoods as
68+
* its own.
69+
*/
70+
public void chunkReady(Vector3ic chunkPos) {
71+
needsMerging.add(new Vector3i(chunkPos));
72+
for (Vector3ic candidate : LightMerger.requiredChunks(chunkPos)) {
73+
if (needsMerging.contains(candidate) && hasFullNeighbourhood(candidate)) {
74+
needsMerging.remove(candidate);
75+
readyToMerge.add(candidate);
76+
}
77+
}
78+
}
79+
80+
private boolean hasFullNeighbourhood(Vector3ic pos) {
81+
for (Vector3ic neighbour : LightMerger.requiredChunks(pos)) {
82+
if (!chunkCache.containsKey(neighbour)) {
83+
return false;
84+
}
85+
}
86+
return true;
87+
}
88+
89+
/**
90+
* Drain queued merges until {@link #PROCESSING_DEADLINE_MS} of wall-clock time has been spent.
91+
*/
92+
public void processPending() {
93+
long processingStartTime = System.currentTimeMillis();
94+
Vector3ic pos;
95+
while ((pos = readyToMerge.poll()) != null) {
96+
mergeAt(pos);
97+
long totalProcessingTime = System.currentTimeMillis() - processingStartTime;
98+
if (!readyToMerge.isEmpty() && totalProcessingTime > PROCESSING_DEADLINE_MS) {
99+
// Debug, not warn, unlike the ready-chunk drain this sits beside: there, overrunning
100+
// the budget means cheap per-chunk work took implausibly long and something is wrong.
101+
// Here a backlog is the designed steady state - merging is expensive and world
102+
// generation queues it faster than a frame can absorb - so warning would fire every
103+
// tick for the whole of a normal world load.
104+
logger.debug("Light merging hit its budget this tick ({}/{}ms). {} positions remain.",
105+
totalProcessingTime, PROCESSING_DEADLINE_MS, readyToMerge.size());
106+
break;
107+
}
108+
}
109+
}
110+
111+
/**
112+
* {@link #chunkReady} only checks the neighbourhood is complete at queue time; a relevance change
113+
* can unload a neighbour before this runs. Re-checking against {@code chunkCache} here, rather than
114+
* trusting the queue, avoids merging with a hole in the neighbourhood.
115+
* <p>
116+
* A position that loses a neighbour that way goes back into {@link #needsMerging} rather than being
117+
* dropped. It is queued from neither set otherwise, and only a chunk becoming ready re-queues
118+
* anything - so simply returning would leave it permanently unmerged even once the neighbour
119+
* reloads, showing as a lighting seam that never heals. The window is not narrow: {@code
120+
* checkForUnload()} runs every tick ahead of {@link #processPending}, and the budget below routinely
121+
* leaves positions queued across several ticks.
122+
*/
123+
private void mergeAt(Vector3ic pos) {
124+
List<Vector3ic> neighbourhood = LightMerger.requiredChunks(pos);
125+
Chunk[] chunks = new Chunk[neighbourhood.size()];
126+
for (int i = 0; i < chunks.length; i++) {
127+
chunks[i] = chunkCache.get(neighbourhood.get(i));
128+
if (chunks[i] == null) {
129+
needsMerging.add(pos);
130+
return;
131+
}
132+
}
133+
// Chunks whose light this actually moves are marked dirty by LocalChunkView as it writes, so
134+
// ChunkMeshWorker re-meshes them (it only re-emits chunks that are isReady() && isDirty()).
135+
// Deliberately not marking the whole neighbourhood here instead: a chunk belongs to 27 of
136+
// them, so that re-meshes each chunk many times over during a world load - enough to exhaust
137+
// the heap in mesh generation - and most of those merges never touch its light at all.
138+
LightMerger.merge(chunks);
139+
}
140+
141+
/** Drop any bookkeeping for {@code pos}. Call on unload, or edge positions leak forever. */
142+
public void chunkUnloaded(Vector3ic pos) {
143+
needsMerging.remove(pos);
144+
readyToMerge.remove(pos);
145+
}
146+
147+
/** Discard all bookkeeping, e.g. when the world is purged or the provider restarts. */
148+
public void clear() {
149+
needsMerging.clear();
150+
readyToMerge.clear();
151+
}
152+
}

0 commit comments

Comments
 (0)