fix: the column key omitted the params — my perf fix broke the overhang
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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.
|
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.
|
||||||
|
|
||||||
|
---
|
||||||
|
|||||||
@@ -470,6 +470,19 @@ namespace
|
|||||||
// would falsely hit the pristine slot at (0,0). PrepareChunk upgrades this to the shared
|
// 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.
|
// strate key; without it we simply cache per instance. Degrade, never lie.
|
||||||
ColumnKey = InstanceId;
|
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
|
/** 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 A = (uint32)FMath::RoundToInt(Ctx.StrateBottomWorldZ);
|
||||||
const uint32 B = Ctx.LayoutVersion;
|
const uint32 B = Ctx.LayoutVersion;
|
||||||
const uint32 C = Ctx.Seed;
|
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)
|
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é »
|
if (ColumnKey == 0) { ColumnKey = 1; } // 0 = « jamais préparé »
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -692,7 +724,8 @@ namespace
|
|||||||
TArray<TUniquePtr<IVoxelHeightOp>> PerBiomeStructural; // pente d'overhang par biome
|
TArray<TUniquePtr<IVoxelHeightOp>> PerBiomeStructural; // pente d'overhang par biome
|
||||||
|
|
||||||
uint64 InstanceId = 0; // unique, jamais recyclée — le repli quand PrepareChunk n'a pas eu lieu
|
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
|
||||||
};
|
};
|
||||||
|
|
||||||
//=========================================================================
|
//=========================================================================
|
||||||
|
|||||||
Reference in New Issue
Block a user