fix(light): read the diagonal chunks of an area's ring - #24
Merged
Conversation
Open item 1 of Project Status ended on "nothing in the API says so yet", and that was the whole of it: the behaviour is understood and narrowed twice, but a caller reading the class could not learn it. The class javadoc says a single instance may be used by as many threads as one likes, and argues it from the service holding no state. That argument is sound and answers a different question than the one a caller has. Two calls share no state inside the service; they can still share the chunks they read, and calculateWithNeighbours reads nine of them. Two threads on overlapping neighbourhoods therefore have one reading what the other writes - not memory corruption, since every write holds the chunk's write lock, but a result committed from bytes that were already stale, which a player sees as a seam and which set() then makes permanent by clearing the update flag. Both the class and the method now say that, and both name the way out: ChunkLightScheduler never submits overlapping work, so the obligation belongs to whoever calls the service directly. That promise about the scheduler had no test, which is a poor thing to lean on in a javadoc. testAChunkUnderComputationIsNotSubmittedASecondTime pins it: an executor that holds its areas instead of running them keeps the first one in flight, a second tick submits nothing, and once the area finishes the chunk is submitted again. No production behaviour changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Open item 2 of Project Status. The ring was built from the face neighbours of every chunk in the area, so a chunk touching it only at a corner was never read. Its light does reach the area - through the chunk between the two - but that chunk is a ring chunk, and a ring chunk's state is computed from its own block states alone, so what it received from its diagonal neighbour is not in it. The source was simply absent from the result. The test measures it rather than describing it: a glowstone in the corner of the diagonal chunk, two blocks from the corner of the area, arrives as 0 today and as 13 with the four offsets added. Every other test in the file places its sources inside the area or on a face, which is why nothing caught this. The cost, counted rather than estimated: for a compact area it is the four corners of the bounding box regardless of size, so a 4x4 area reads 36 chunks instead of 32, a 1x16 line 54 instead of 50. A shape that runs diagonally pays more - a staircase of 16 chunks goes from 36 to 54. That is the trade Project Status names, and it is bounded in a way the wording there does not suggest: the ring does not grow with the area, it grows by its corners. No JMH run behind this. The counts are set arithmetic over the offsets, exact and reproducible, but they are chunk counts and not microseconds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Test results 195 files 195 suites 4m 20s ⏱️ Results for commit 8805178. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
Open item 2 of Project Status:
the ring of an area was built from the face neighbours of every chunk in it, so a chunk touching the
area only at a corner was never read.
Its light does reach the area — through the chunk between the two. But that chunk is a ring chunk,
and a ring chunk's state is computed from its own block states alone, so what it received from its
diagonal neighbour is not in it. The source was simply absent from the result.
Measured, not described
A glowstone in the corner of the diagonal chunk, two blocks from the corner of the area:
Not "dimmer than it should be" — absent. With the four offsets added it arrives as 13, which is
15 minus the two blocks it travelled.
Every other test in
ChunkLightAreaTestplaces its sources inside the area or on a face, which isexactly why this survived: the gap is invisible unless a test is written for it.
The cost, counted rather than estimated
Project Status names the trade — "the ring grows, and the ring is what every pass pays for" — and
that wording suggests the ring grows with the area. It does not. It grows by the corners of its
bounding box, which is a constant for any compact shape:
The staircase is the honest worst case: a shape that runs diagonally adds a corner per step.
fillcan produce one, since it walks face neighbours, but the shape a scheduler actually produces is a
cluster around a player, where the figure is the 12 % row.
No JMH run behind this. The counts are set arithmetic over the offsets — exact and reproducible,
but chunk counts rather than microseconds. If you want the µs before merging, say so and I will run
the light benchmarks with the configured forks and iterations; I did not want to publish a smoke-run
number in a project that keeps Measured Results
as strictly as this one does.
Why this was not simply "the obvious fix"
Project Status also records the consequence: this gap is why
calculateWithNeighbourswas notdeprecated in favour of
ChunkLightArea, because for a single chunk the older method — which readsthe full 3×3 — was the more accurate of the two. With this, the area reads the same 3×3 for a
one-chunk area, and that argument falls away. The two methods now differ in cost, not in what they
can see.
Types of changes
Both boxes on purpose: the light a caller gets changes — it gains a source it was missing — and the
work per pass changes with it.
Checklist
implementation and watched them fail for the right reason
test<What><Expectation>, and use plain JUnit assertions@NotNull; the package@NotNullByDefaultcovers it./gradlew buildis green locally — 673 tests, 0 failuresThe equivalence tests against Minestom's engine stay green, which is worth stating: the extra source
does not push the result away from what Minestom computes.
Documentation not touched. Project Status item 2 describes this as open and the wiki is yours to
sequence; the numbers above are ready to move there if you want them.
Measurements
No timing claim is made. The table above is a chunk count derived from the offsets, and the section
header exists so it is clear that this is arithmetic and not a benchmark.
🤖 Generated with Claude Code