fix: stale bulk-adopted snapshot overwrote freshly drawn voxels - #102
Merged
Conversation
A build's persisted edits are bulk-adopted at load and staged in `pending_snapshots` to pace the sync channel. `flush_dirty_chunks` drained the script's draws first and the staged blobs second, publishing each blob verbatim over `packed_chunks` and clearing the chunk's deltas — so when a chunk's draws and its staged blob landed in the same flush pass, the edits-only blob won and the drawing was lost. Nothing retried: the chunk published fewer voxels than the store held, and stayed that way until the script re-ran. Only chunks carrying persisted edits could be hit, and they collapsed to exactly their edit count. Release builds draw fast enough for both to be pending together, which is why it rarely showed in debug. Drain the staged snapshots first, so an adopted blob sits underneath anything drawn since, and publish it verbatim only while the chunk is untouched — otherwise re-encode from the cells, which already fold packed + deltas + pending, and drop the superseded pending entry. Ordering alone isn't enough: the worker breaks out of the iterator under channel pressure, so a staged blob can outlive a pass in which draws flushed. Also fixes unit bounds, which clipped terrain loading and meshing: - expand_bounds_to_chunk guarded on AABB.contains, which compares point.z against position.x in godot-nim, so expansions were silently dropped once a unit reached further along -x than -z. Grow unconditionally. - The two on_chunk_created fire sites had opposite count guards and a third path never reported at all. Route them through note_chunk. - Warn when page-out drops unflushed writes — cached_chunk's eviction refuses that trade, unload_chunk didn't guard it.
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
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.
Chunks of a build could render with most of their voxels missing. It survived reloads, was invisible to the eye in debug builds, and a script re-run "fixed" it — which is what made it so slippery.
Root cause
A build's persisted edits are bulk-adopted at load and staged in
pending_snapshotsto pace the sync channel.flush_dirty_chunksdrained the script's draws first and the staged blobs second, publishing each blob verbatim overpacked_chunksand clearing the chunk's deltas:So when a chunk's draws and its staged blob landed in the same flush pass, the edits-only blob won. Nothing retried — the chunk published fewer voxels than the store held and stayed that way until the script re-ran.
Evidence from the reproduction (tutorial-3 maze, local chunk
(-2,0,0)):2 is exactly that chunk's persisted-edit count. The three neighbouring chunks in the same region have zero persisted edits and were never affected.
That also explains the symptoms: release-only (release draws fast enough for both to be pending together; debug usually drains the snapshot in an earlier pass), load-order sensitive (shifts when the script runs relative to the flush cadence), and fixed by re-running the script (
pending_snapshotsis only staged at load).The rule this restores
flush_chunk_snapshotsatisfies that — it encodes the cells, which already fold packed + deltas + pending. A staged blob does not, at publish time.So: drain staged snapshots first, so an adopted blob sits underneath anything drawn since; publish verbatim only while the chunk is untouched, else re-encode from the cells and drop the superseded pending entry. Ordering alone isn't sufficient — the worker breaks out of the iterator under channel pressure, so a staged blob can outlive a pass in which draws flushed. The fresh-load fast path is unchanged: nothing is dirty then, so every chunk still takes the verbatim route.
Also: unit bounds
The terrain clips chunk loading and meshing to
Build.bounds, and bounds could come up short:expand_bounds_to_chunkguarded onAABB.contains, which comparespoint.zagainstposition.xin godot-nim (see fix: AABB.contains compared point.z against position.x godot-nim#1) — expansions were silently dropped once a unit reached further along -x than -z. Now unconditional and idempotent.on_chunk_createdfire sites had opposite count guards, and a third path never reported at all. All routed throughnote_chunk.unload_chunknow warns when page-out drops unflushed writes —cached_chunk's eviction explicitly refuses that trade;unload_chunkdidn't guard it.Verification
Deterministic harness: a pristine level fixture restored before every run (Enu rewrites
load_orderon load, which silently changed the input between trials otherwise), release build, cold load, scored as pixel diff against a known-good render.nim build,nim build -d:release, andnim test_allall pass.tests/unit/bounds_test.nimis new and registered intasks.nim(unit tests are listed explicitly, not discovered); its-z-chunk case fails on the old bounds code and passes on the new.Level data is deliberately not included in this PR.
🤖 Generated with Claude Code