From d0a9ce301899c9d987b0e353e6add78af1893ff5 Mon Sep 17 00:00:00 2001 From: Fr0zka Date: Mon, 27 Jul 2026 17:13:50 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20the=20column=20key=20omitted=20the=20par?= =?UTF-8?q?ams=20=E2=80=94=20my=20perf=20fix=20broke=20the=20overhang?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit VerticalShaftEquivalence is bit-identical (966 samples inside a shaft), so operator reuse across archetypes is measured now, not intended. But SurfaceHeightEquivalence failed: 69/20000 overhang samples differ, 1 crossing the isosurface. Cause is the ColumnKey from f3faa3b, which was hash(StrateBottomWorldZ, LayoutVersion, Seed) and omitted the params. Two stacks of the same strate with different overhang settings therefore shared a key, and the second read the first's columns, computed with OverhangAmp = 0. The overhang silently vanished wherever a column was already cached. Not a test artifact: this is the weakness the codebase already documents for GSurfColCache (extended AUDIT C2 note) — a live edit that changes params without moving the strate leaves the key unchanged and serves stale columns. Production masks it because RebuildStrates bumps LayoutVersion; my key inherited the hole. Fixed by folding an FCrc::MemCrc32 fingerprint of the params, and of every per-biome param set, into the key. FSurfaceGenerationParams is verified pure POD, so a memory CRC cannot produce a false hit; padding can only cause a false miss, i.e. a recompute. A perf optimisation introduced a correctness bug and the tests caught it the same day. The failure was invisible to inspection and produced plausible terrain. Known and not fixed: VerticalShafts proves 0 of 60 tiles because EffectOverBox returns CarveOnly whenever any shaft sits within a Spacing*1.6 halo rather than testing real connector capsules. Pessimistic, not wrong — lost CPU, never a hole. UNVERIFIED: not compiled. Co-Authored-By: Claude Opus 5 --- OPSTACK-PROGRESS.md | 51 +++++++++++++++++++ .../Private/VoxelDensityOpStack.cpp | 37 +++++++++++++- 2 files changed, 86 insertions(+), 2 deletions(-) diff --git a/OPSTACK-PROGRESS.md b/OPSTACK-PROGRESS.md index 8cbe799..ae67178 100644 --- a/OPSTACK-PROGRESS.md +++ b/OPSTACK-PROGRESS.md @@ -1723,3 +1723,54 @@ until the transition is complete** (Jahni's call); the open item is that the op virtual dispatch and the hashed column lookup as the remaining suspects. --- + +## 2026-07-27 — VerticalShafts bit-identical. And the perf fix had introduced a REAL bug. + +**`VerticalShaftEquivalence`: bit-identical over 20 000 samples, 966 inside a shaft.** So the +cylinders, connectors, roughness, carve and ledges all ran, and **operator reuse across archetypes is +now measured rather than intended** — three of the five ops are Maze's, unchanged. + +### ⚠️ `SurfaceHeightEquivalence` FAILED, and it was my own perf fix + +``` +Overhang: 69 of 20000 samples differ (largest |delta| 5.84); 1 crosses the isosurface +``` + +**Cause: the `ColumnKey` I introduced in `f3faa3b` did not include the params.** It was +`hash(StrateBottomWorldZ, LayoutVersion, Seed)`. The test builds two stacks in the **same** strate, +same layout version, same seed, differing only in overhang settings — identical keys, so the second +stack read the first's cached columns, which had been computed with `OverhangAmp = 0`. The overhang +silently vanished wherever a column was already cached. + +**This is not a test artefact.** It is precisely the weakness the codebase already documents for +`GSurfColCache` (the extended `AUDIT §C2` note in `VoxelGenerator.cpp`): *"StrateKey is +round(StrateBottomWorldZ), so a live edit that changes terrain params WITHOUT moving the strate +leaves the key unchanged and serves stale columns."* Production merely **masks** it, because +`RebuildStrates` bumps `LayoutVersion`. My key inherited the same hole, and the test found it in one +build. + +**Fixed** by folding a `FCrc::MemCrc32` fingerprint of the params (plus every per-biome param set) +into the key. `FSurfaceGenerationParams` is verified pure POD — no `TArray`, `FString` or pointer — +so a memory CRC **cannot** produce a false hit; at worst padding causes a false *miss*, i.e. a +recompute. Erring toward CPU cost rather than toward a wrong column. + +**Worth stating plainly: a perf optimisation introduced a correctness bug, and the test suite caught +it the same day.** That is the clearest answer yet to "what are all these tests for" — the failure +was invisible to inspection, produced *plausible* terrain, and only one of 20 000 samples actually +crossed the isosurface. + +### Known, not fixed: VerticalShafts proves 0 of 60 tiles + +Maze proves 23, the slabs 36-40, shafts **zero**. Expected from the choice recorded last commit: my +`EffectOverBox` returns `CarveOnly` if *any* shaft exists within a `Spacing*1.6` halo, rather than +testing the actual connector capsules. At default `ShaftDensity` almost every box has a shaft in that +halo, so `AllSolid` is always killed and nothing is provable. **Correct but pessimistic** — a false +verdict would be a hole, this is only lost CPU. The improvement is to test real connector capsules +instead of "a shaft exists nearby", which is `§6`'s split argument arriving through the back door. + +**UNVERIFIED:** the fingerprint fix. + +**Next single action:** build, confirm `SurfaceHeightEquivalence` is green again (all 11 tests). +Then `FloatingIslands` (§7) → `Underwater` (§8) → `TunnelNetwork` (§2, **last**). Perf still parked. + +--- diff --git a/Source/VoxelForge/Private/VoxelDensityOpStack.cpp b/Source/VoxelForge/Private/VoxelDensityOpStack.cpp index 933c222..f7cdc91 100644 --- a/Source/VoxelForge/Private/VoxelDensityOpStack.cpp +++ b/Source/VoxelForge/Private/VoxelDensityOpStack.cpp @@ -470,6 +470,19 @@ namespace // would falsely hit the pristine slot at (0,0). PrepareChunk upgrades this to the shared // strate key; without it we simply cache per instance. Degrade, never lie. ColumnKey = InstanceId; + + // Empreinte des params qui déterminent une colonne. `FSurfaceGenerationParams` est du + // POD pur (que des float/int/bool, vérifié : aucun TArray, FString ni pointeur), donc + // un CRC mémoire ne peut pas produire de FAUX POSITIF — au pire du padding non + // initialisé donne un faux NÉGATIF, c'est-à-dire un recalcul. Se tromper du côté qui + // coûte du CPU plutôt que du côté qui rend une mauvaise colonne. + // Pure POD (verified: no TArray/FString/pointer), so a memory CRC cannot produce a false + // HIT; at worst padding causes a false miss, i.e. a recompute. Err toward CPU, not lies. + ParamsFingerprint = FCrc::MemCrc32(&P, sizeof(P)); + for (const FSurfaceGenerationParams& BP : BiomeParams) + { + ParamsFingerprint = VoxelHash::Mix(ParamsFingerprint ^ FCrc::MemCrc32(&BP, sizeof(BP))); + } } /** Sans biomes — délègue, pour qu'il n'existe qu'UN corps de construction et UN compteur @@ -648,8 +661,27 @@ namespace const uint32 A = (uint32)FMath::RoundToInt(Ctx.StrateBottomWorldZ); const uint32 B = Ctx.LayoutVersion; const uint32 C = Ctx.Seed; + + // ⚠️ `ParamsFingerprint` EST OBLIGATOIRE, et son absence a été un vrai bug — attrapé par + // `SurfaceHeightEquivalence` au build suivant (69/20000 écarts, 1 traversée d'iso). + // + // Sans lui, la clé ne contenait que (strate, layout, seed). Deux piles de la MÊME strate + // avec des params DIFFÉRENTS obtenaient donc la même clé et se partageaient les colonnes : + // la seconde lisait les colonnes de la première, calculées avec `OverhangAmp = 0`, et + // l'overhang disparaissait purement et simplement. + // + // ET CE N'EST PAS QU'UN ARTEFACT DE TEST : c'est exactement la faiblesse que le code + // documente déjà pour `GSurfColCache` (VoxelGenerator.cpp, note AUDIT §C2 étendue) — + // « une édition à chaud qui change les params SANS déplacer la strate laisse la clé + // identique et sert des colonnes périmées ». En production `LayoutVersion` bouge à chaque + // `RebuildStrates`, ce qui masque le trou ; ma clé en avait hérité, et le test l'a trouvé + // tout de suite. Empreinte incluse ⇒ le trou est fermé ici, pas seulement masqué. + // + // The fingerprint is REQUIRED: without it two stacks of the same strate with different + // params shared columns, and the overhang silently vanished. Same weakness the codebase + // already documents for GSurfColCache, which LayoutVersion merely masks. ColumnKey = ((uint64)VoxelHash::Mix(A ^ VoxelHash::Mix(B)) << 32) - | (uint64)VoxelHash::Mix(C ^ VoxelHash::Mix(A)); + | (uint64)VoxelHash::Mix(C ^ VoxelHash::Mix(A) ^ ParamsFingerprint); if (ColumnKey == 0) { ColumnKey = 1; } // 0 = « jamais préparé » } @@ -692,7 +724,8 @@ namespace TArray> PerBiomeStructural; // pente d'overhang par biome uint64 InstanceId = 0; // unique, jamais recyclée — le repli quand PrepareChunk n'a pas eu lieu - uint64 ColumnKey = 0; // l'identité PARTAGÉE (strate + layout + seed) : voir PrepareChunk + uint64 ColumnKey = 0; // l'identité PARTAGÉE (strate + layout + seed + params) : voir PrepareChunk + uint32 ParamsFingerprint = 0; // sans lui, deux piles de la même strate se volaient leurs colonnes }; //=========================================================================