docs: Sol investigation -- refutes my boundary-tile hypothesis, corrects my false accusation
Sol High investigated with Luna xHigh sub-agents; two approaches built in isolated worktrees, main tree untouched. Two corrections to my own assertions: 1. "One un-ticked neighbouring strate kills boundary tiles" is WRONG. UsesOperatorStackForChunk depends only on ChunkCoord.Z, and ClassifyTile's earlier Z loop already required the predicate for every sampled cave Z within one slot -- so the later XYZ sweep is redundant, not over-strict. Removing it unlocks nothing; boundary tiles still fail the one-slot/type/params guards, correctly. 2. I falsely wrote that VF-03's fixture citation was fabricated. It exists (VoxelForgeTestFixture.h ~134/146, explicit about CP_UseOpStack contamination). I had read only the 30-line header and asserted a negative from a partial read. Header corrected. VF-03's core claim is now CONFIRMED: GetDensityAt keys thread_local CP_* by (ChunkCoord, LayoutVersion) with no world identity, and every manager's version starts equal. Also: the Cave Bail Not Op Stack counter I specced is ambiguous -- it fires before the different-slot attribution, so it cannot distinguish "flown slot un-ticked" from "boundary tile met an un-ticked slot first". My 80% reading was over-confident. Same defect I fixed for TilesSkippedAllSolid, reintroduced one layer down. Approach A recommended (explicit opt-in + six-box LRU) over B (code-enforced cutover): the code does not justify weakening ClassifyTile, and B would delete the flag, which is the only same-route A/B lever available to prove the refactor. Narrow A's warning to cave archetypes before adopting. Nothing built. Worktrees left in place for review. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -10,7 +10,8 @@
|
||||
> |---|---|
|
||||
> | **VF-05** (radius envelope in `BuildChunkCache`) | ✅ **CONFIRMED and FIXED** — `CODEX-TASK-006`. Real, and the worst of three instances of this class: it is `TunnelNetwork`, it is in code **both** density paths share, and it breaks window invariance (`ARCHITECTURE §8.4`). Genuinely good find. |
|
||||
> | **VF-02** (3-second shutdown timeout) | ✅ **premise confirmed** — `VoxelWorld.cpp:327` literally reads *"Timeout after 3 seconds to avoid hanging the editor."* Note `CLAUDE.md` states the invariant more strongly than the code implements it ("EndPlay blocks on `ActiveTaskCount → 0`" — it blocks *with a deadline*). Worth deciding deliberately. |
|
||||
> | **VF-03** (TLS caches omit the owning world) | ⚠️ **substance plausible, EVIDENCE OVERSTATED.** It claims *"the test fixture explicitly documents observed cross-world contamination."* It does not. `VoxelForgeTestFixture.h` documents that the `thread_local` caches exist and flags an unrelated `TSoftObjectPtr` risk. The underlying point (caches keyed on chunk/seed/layout but not on which generator owns them) may still hold — but it needs checking on its own merits, not on this citation. |
|
||||
> | **VF-03** (TLS caches omit the owning world) | ⛔ **MY EARLIER VERDICT HERE WAS WRONG — corrected 2026-08-16.** I wrote that its fixture citation was fabricated. **It is not.** `VoxelForgeTestFixture.h` lines ~134/146 explicitly document `CP_UseOpStack` contamination between worlds; I had read only the file's 30-line header comment and asserted a negative from a partial read. VF-03's core claim is **CONFIRMED**: `GetDensityAt` keys its `thread_local CP_*` state by `(ChunkCoord, LayoutVersion)` with **no generator/world identity**, and every manager's version starts at the same value — so a second world on the same worker can inherit the first's params, `CP_UseOpStack` and stack. The *breadth* of VF-03 (the `OC_*`/`BM_*`/passage/biome/diff caches) is still unproven and should be audited as one owner-identity task. Original note kept below for the record: ~~substance plausible, evidence overstated~~ |
|
||||
> | ~~VF-03 (superseded)~~ | ~~**substance plausible, EVIDENCE OVERSTATED.**~~ It claims *"the test fixture explicitly documents observed cross-world contamination."* It does not. `VoxelForgeTestFixture.h` documents that the `thread_local` caches exist and flags an unrelated `TSoftObjectPtr` risk. The underlying point (caches keyed on chunk/seed/layout but not on which generator owns them) may still hold — but it needs checking on its own merits, not on this citation. |
|
||||
> | **VF-01** (live rebuild races streaming workers) | ✅ **CONFIRMED — the most serious finding here.** `UVoxelStrateManager::Initialize` does `StrateLayout.Empty()` (:~36) **and** `Passages.Empty()` (:171) then `Passages.Add()`, i.e. it frees and reallocates both arrays. There is **no lock, no barrier, no drain** anywhere in that file. Worker-side readers of the same arrays: `AnyPassageNearBox` (:460, range-for over `Passages`), `EvaluateModifierSDF` (indexes `Passages[...]`), `FindSlotIndexForChunkZ` (iterates `StrateLayout`) — all reached from `GetDensityAt`/`ClassifyTile` on mesher workers. And `RegenerateAllChunks()` (which bumps the epoch) runs **after** `Initialize`, so previous-epoch workers are still live during the mutation. **This is the same class already fixed once in this codebase** — `DiffLayer.ChunkMods` got `ModsLock` after a carve-vs-stream access violation. Four call sites, incl. `OnObjectModifiedInEditor` (:309), which fires automatically when a strate asset is edited while the world streams. **NOT fixed — see the note below.** |
|
||||
> | **VF-10** (~74-field per-voxel params copy) | ✅ **confirmed real, but Sol missed the conclusion that matters.** The 74 fields are real and the copy is per near-surface sample. **However it is INHERITED from the original path — `GetDensityWithParams` does the same copy — so both paths pay it equally and it does NOT explain the op-stack perf regression.** The op stack actually *improved* it (memoised so eleven detail ops don't each repeat it), and the site says so in its own comment. Genuine future optimisation for both paths; **not** the answer to "why is the op path slower". |
|
||||
> | VF-04, VF-06, VF-07, VF-08, VF-09 | **NOT independently verified.** Read them as leads. |
|
||||
|
||||
Reference in New Issue
Block a user