Skip to content

perf(light): seed the sky from the heightmap instead of every open cell - #41

Merged
TheMeinerLP merged 1 commit into
mainfrom
perf/sky-heightmap
Aug 2, 2026
Merged

perf(light): seed the sky from the heightmap instead of every open cell#41
TheMeinerLP merged 1 commit into
mainfrom
perf/sky-heightmap

Conversation

@TheMeinerLP

Copy link
Copy Markdown
Contributor

From Project Status → Open → Smaller items:

A full sky propagation still queues every open cell of a column rather than seeding from a
heightmap. Measured at 79.3 → 55.1 µs in the rebuild, with byte identity verified over 240 worlds.

seedSky queued every position that sees the sky — up to 98 304 entries for a 24-section chunk, each
popped again and expanded six ways only to find that all six neighbours are already at the full
level.

Lighting a position and queueing it are two different things, and this is where the difference is
largest. A position whose four horizontal neighbours are open at the same height can raise nobody:
above and below it the column is at the full level or blocked, and so is every neighbour. It still
has to be lit — the output and the border exchange read the levels — but it has no business in the
queue.

The walk down each column already finds where the sky stops, so it now keeps that. The positions that
need queueing then fall out as a range rather than a per-position test: a neighbouring column is
dark below its own sky bottom, so a column queues from its own bottom up to the deepest bottom among
its four neighbours. Above that line every neighbour is open. On a fully open chunk nothing is
queued at all.

The heightmap is one int per column, so it does not grow with the height of the chunk and is
allocated once with the propagator.

The part worth reading: it took three attempts to build a test that could fail

The first check of this change was that all 212 tests passed. They also passed with the queued range
deliberately shortened by one position — the plainest way to get this wrong. So the suite proved
nothing about it, and the change had no coverage at all.

attempt terrain shortened range caught?
the existing suite flat, fully open or fully closed no — on flat terrain the range is empty and the whole derivation is a no-op
varied ground height per column stone rising from y = 0 no — see below
varied ceiling height, stone air above and below a solid roof no — see below
varied ceiling height, slab a roof that stops the sky and passes light sideways yes, 2 of 6 red

The second and third failures have the same cause and it is not obvious. The position where a
column's sky stops is the blocker, and a blocker that occludes every face takes no light from the
side either. So the topmost position of the queued range has nowhere to give its light, and getting
that edge wrong is invisible against solid stone no matter how varied the terrain is.

A slab stops the sky from above and accepts light from its four sides. That is the only shape that
separates a correct range from one position short of it — and it is exactly the case
SectionOpacity stores occlusion per face for.

SkyHeightmapSeedTest (new, 6 tests) compares against the algorithm this commit replaces, written
out in the test rather than called, so the oracle shares no code with what it checks. Four
configurations use stone and two use slabs; only the slab ones catch the off-by-one, and the class
javadoc says so, because the next person to add a case here will otherwise reach for stone.

./gradlew build is green.

What I expect the measurement to say, so it can be wrong

Running the same pair of classes through the Benchmark workflow, three forks, main against this
branch, with falcoReadStates and minestomFull as untouched controls in the same job.

Two predictions, stated before the numbers arrive:

  1. propagateSky improves at every height, and propagate — block light, which does not go
    through seedSky — does not move. That second half is a control with an expected answer of zero.
  2. The 4-section regression from perf(light): three changes to the propagation loop from item 2 of Open #36 is probably not fixed by this. I attributed it to the
    flat column plus seedSky, and only the queueing goes away here; the walk down each column
    still reads the flat array. If the regression survives, that attribution was half right and the
    walk is the remaining half.

seedSky queued every position that sees the sky. On a chunk of 24
sections that is up to 98 304 entries, each popped again and expanded six
ways to establish that all six neighbours are already at the full level.

Lighting a position and queueing it are two different things, and this is
where the difference is largest. A position whose four horizontal
neighbours are open at the same height can raise nobody: above and below
it the column is at the full level or blocked, and so is every
neighbour. It still has to be lit, because the output and the border
exchange read the levels, but it has no business in the queue.

The walk down each column already finds where the sky stops, so it now
keeps that per column. The positions that need queueing then fall out as
a range rather than a test: a neighbouring column is dark below its own
sky bottom, so a column has to queue from its own bottom up to the
deepest bottom among its four neighbours. Above that line every
neighbour is open. On an open chunk nothing is queued at all.

The heightmap is one int per column, so it does not grow with the height
of the chunk and is allocated once with the propagator.

ChunkLightState#skyTopOf computes the same heightmap a second time, after
the propagation. Handing this one over instead is a separate change and
is not made here.
@TheMeinerLP
TheMeinerLP requested a review from a team as a code owner August 2, 2026 20:52
@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Test results

  201 files    201 suites   4m 5s ⏱️
  683 tests   683 ✅ 0 💤 0 ❌
2 058 runs  2 058 ✅ 0 💤 0 ❌

Results for commit 153d31b.

@TheMeinerLP
TheMeinerLP merged commit 272cb0b into main Aug 2, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant