diff --git a/docs/ISSUES.md b/docs/ISSUES.md index 7c18f3a..e0546c2 100644 --- a/docs/ISSUES.md +++ b/docs/ISSUES.md @@ -46,9 +46,112 @@ Copy this block when adding a new issue: # Active issues -## #48 — A few specific scenery trees hover above terrain (per-GfxObj Z misplacement) +## #49 — Scenery (X, Y) placement drifts from retail at some landblocks **Status:** OPEN +**Severity:** MEDIUM (visible misplacement; species-specific or per-cell, not a global offset) +**Filed:** 2026-05-06 +**Component:** scenery placement / `SceneryGenerator` + +**Description:** While verifying the `#48` Z fix at Holtburg +landblock `0xA9B30001`, the user spotted a scenery tree placed at +the **wrong (X, Y)** in acdream relative to retail at the same +character coords. Specifically: a large tree that retail places far +across the road on the right (east) side appears in acdream on the +left (west) side, near a chess board / picnic-bench area. Side-by- +side screenshot pair captured 2026-05-06. + +This is **not** a Z bug — every tree in the same screenshot has its +trunk meeting the visible terrain (the `#48` `SampleTerrainZ` fix is +working). It's also **not** the LandBlockInfo Stab path — the chess +board / bench themselves are correctly placed, so the landblock +origin and `lbOffset` math are right. + +**Hypotheses (need cdb retail trace to disambiguate):** + +1. The displacement-noise math in `SceneryGenerator` differs from + retail's `chunk_005A0000` LCG by a constant or a sign flip. Audit + `eeee4c5` claimed "all MATCH" against the decomp, but a runtime + trace would prove or disprove. +2. Coordinate-system handedness: cell-local `(lx, ly)` in our path + may map to retail's `(ly, lx)` somewhere, rotating tree XY 90° + around the cell's NW corner. +3. The `obj.Align != 0` path in retail (`FUN_005a6f60`, aligns the + object to the landcell polygon's normal) may use a different + reference point than ours, drifting placement on sloped cells. +4. Slope filter could reject a cell retail accepts (or vice versa), + pushing trees into adjacent cells. +5. Region-table / `SceneInfo` lookup might select a different + scenery list for the cell type. + +**Investigation plan (gold-standard, per `project_retail_debugger.md`):** + +1. Run the existing `ACDREAM_DUMP_SCENERY_Z=1` diagnostic to capture + acdream's full per-spawn (gfx, world XY, scale, partT) for + landblock `0xA9B3FFFF`. +2. Attach cdb to a live retail client at the same Holtburg spot + (`tools/pdb-extract/check_exe_pdb.py` confirms PDB pairs with + v11.4186). Set a breakpoint on `CLandBlock::get_land_scenes` (or + the inner `chunk_005A0000` placement function); capture every + `(gfxObjId, worldX, worldY, scale, heading)` retail emits for + the same landblock. +3. Diff the two tables. The spawn that's offset will be obvious; + the offset pattern (one tree, all trees, one species, constant + delta, etc.) determines which hypothesis above is correct. + +**Files:** + +- [`src/AcDream.Core/World/SceneryGenerator.cs`](src/AcDream.Core/World/SceneryGenerator.cs) — placement math (LCG noise, displacement, rotation, scale, slope filter) +- `acclient!CLandBlock::get_land_scenes` (`docs/research/named-retail/acclient_2013_pseudo_c.txt`) — retail entry point +- `chunk_005A0000.c` — referenced retail source per `SceneryGenerator.cs` comments +- [`docs/research/named-retail/symbols.json`](docs/research/named-retail/symbols.json) — for cdb breakpoints + +**Acceptance:** Side-by-side outdoor screenshot pair (acdream vs +retail, same character coords, same time of day) shows scenery +positions matching at multiple landblocks. The cdb trace + diagnostic +diff documents quantitative agreement (zero offset within float +precision) on at least one landblock end-to-end. + +**Out of scope here (kept under `#48`):** Z floating. That's fixed. + +--- + +## #48 — [DONE 2026-05-06 · (this commit)] A few specific scenery trees hover above terrain (per-GfxObj Z misplacement) + +**Resolution:** Hypothesis 2 (physics-sampler vs bilinear-fallback Z +mismatch). The bilinear fallback in `GameWindow.SampleTerrainZ` had +its two diagonal arms swapped — used the SEtoNW triangle test on +SWtoNE cells and vice versa. Every scenery hydration in our +diagnostic ran through the bilinear path (`source=bilinear` in all +`[scenery-z]` log lines) because physics hadn't yet built a +`TerrainSurface` for the streaming-in landblock — so on sloped +cells, scenery sat at a different Z than the visible terrain mesh +by up to ~1.5 m. The bug was latent since `ff325ab` (2026-04-17) +which upgraded the fallback from naive 4-corner bilinear to +triangle-aware barycentric, but with the diagonal-pair tests +swapped. `TerrainSurface.SampleZ` (used by the physics path / player +Z) was always correct, so player feet stayed flush — the two paths +just disagreed and only scenery noticed. + +Fix: extracted the canonical triangle-pick math into +`TerrainSurface.InterpolateZInTriangle` (private static); added +`TerrainSurface.SampleZFromHeightmap` (public static) that reads +heights directly from the landblock byte array using the same +canonical math; redirected `GameWindow.SampleTerrainZ` to delegate +to it. New conformance test +`SampleZFromHeightmap_AgreesWithInstance_AcrossWholeLandblock` pins +both sampler paths together at 1500 sample points across both +diagonals, so future drift gets caught. User visually confirmed +2026-05-06. + +The diagnostic dump (`ACDREAM_DUMP_SCENERY_Z=1`, +`GameWindow.cs:4661`) is kept committed — it's gated by env var, +zero cost when off, and is the right starting point for `#49` +(scenery X/Y placement) too. + +Pseudocode: [`docs/research/2026-05-06-issue-48-fix-pseudocode.md`](docs/research/2026-05-06-issue-48-fix-pseudocode.md). + +**Status:** DONE **Severity:** LOW (cosmetic; ~3 trees per landblock, easy to ignore but obvious once spotted) **Filed:** 2026-05-06 **Component:** rendering / scenery placement / terrain Z sampling diff --git a/docs/research/2026-05-06-issue-48-fix-pseudocode.md b/docs/research/2026-05-06-issue-48-fix-pseudocode.md new file mode 100644 index 0000000..c613711 --- /dev/null +++ b/docs/research/2026-05-06-issue-48-fix-pseudocode.md @@ -0,0 +1,86 @@ +# Issue #48 fix — bilinear fallback triangle-pair test was swapped + +## Diagnosis (from the diagnostic dump) + +ACDREAM_DUMP_SCENERY_Z=1 in landblock 0xA9B3 around the user's spawn +showed: + +- Every scenery line had `source=bilinear` (physics engine had not + registered the landblock at hydration time — typical streaming race). +- The trees flagged as "floating" had multi-part setups whose lowest + vertex extended into the ground per our calc (`partWorldZMin = + groundZ - 3.825`) — i.e., per the diagnostic the tree should sit + flush. The retail client at the same world coords does sit flush. +- Player Z at the same world coords (~91 m) came from the physics + sampler (`TerrainSurface.SampleZ`), which is correct. + +That meant the bilinear fallback in `GameWindow.SampleTerrainZ` was +producing a different ground Z than the visible terrain mesh and the +player physics path on sloped cells, so trees were placed at a +ground-Z that didn't match the terrain the player was walking on. + +## Root cause + +Two terrain Z samplers exist: + +1. `AcDream.Core.Physics.TerrainSurface.SampleZ` (instance) — used by the + physics engine for player + entity ground-snap. Triangle-aware, + matches the visible terrain mesh. +2. `AcDream.App.Rendering.GameWindow.SampleTerrainZ` (private bilinear + fallback) — used by scenery hydration when physics has not yet built + a `TerrainSurface` for a streaming-in landblock. + +Both choose a per-cell **diagonal** with the AC2D `FSplitNESW` formula +(constants `0x0CCAC033`, `0x421BE3BD`, `0x6C1AC587`, `0x519B8F25`). +Both then choose one of the cell's two **triangles** based on the +fractional position within the cell. **The fallback's two arms were +swapped** relative to the chosen diagonal: + +| Diagonal | Correct dividing test | Correct triangles | Bilinear-fallback test (BUGGY) | +|---|---|---|---| +| `SWtoNE` (BL→TR, line y=x) | `tx > ty` | {BL,BR,TR} below / {BL,TR,TL} above | `s + t <= 1` (wrong — that's the SEtoNW test) | +| `SEtoNW` (BR→TL, line x+y=1) | `tx + ty <= 1` | {BL,BR,TL} below / {BR,TR,TL} above | `s >= t` (wrong — that's the SWtoNE test) | + +On sloped cells the wrong triangle's plane gives a Z that disagrees +with the rendered terrain by up to ~1.5 m. Flat cells happen to mask +the bug because all four corners share one Z. + +## Fix + +1. Extract the correct triangle-picker math from `TerrainSurface.SampleZ` + (instance) into a new public static method + `TerrainSurface.SampleZFromHeightmap(byte[] heights, float[] heightTable, + uint landblockX, uint landblockY, float localX, float localY)`. + Same algorithm, but reads the four corner heights directly from the + landblock's raw heightmap byte array instead of the pre-resolved + instance cache. One source of truth for the triangle math. +2. Replace `GameWindow.SampleTerrainZ` body with a call to that static. +3. Conformance test in `tests/AcDream.Core.Tests/Physics/TerrainSurfaceTests.cs` + exercising a sloped heightmap on both diagonals, asserting that the + new static and the existing instance method return the same Z at the + same `(localX, localY)` (especially at points near the cell diagonal + where the previous bug manifested). + +## Why this is the right fix + +- Retail (`docs/research/named-retail/acclient_2013_pseudo_c.txt`) places + scenery via `CLandBlock::get_land_scenes` (0x00530460) → + `Plane::set_height(plane, &pos)` (0x0052f050), which projects the + position onto the terrain triangle's plane. The split direction comes + from the same `FSplitNESW` formula. Our `TerrainSurface.SampleZ` is a + faithful port of that algorithm and is already used by physics; the + bilinear fallback should be identical. +- The bug is purely in the fallback path. Player Z is unaffected. +- No retail behavior changes; only acdream consistency. +- "Don't break" constraints from the handoff are satisfied: + - Player Z snap untouched (different code path). + - Species that already render flush still do (their Z was correct + on cells where bilinear and physics agreed; now it's correct + everywhere). + +## Pre-existing bugs out of scope + +The user reports separate (X, Y) misplacement at other locations. +That's a different bug — likely in `SceneryGenerator`'s placement +math or one of the terrain-mesh / region tables — and outside the +scope of this fix. File as a follow-up issue. diff --git a/src/AcDream.App/Rendering/GameWindow.cs b/src/AcDream.App/Rendering/GameWindow.cs index 612a9ae..b00345d 100644 --- a/src/AcDream.App/Rendering/GameWindow.cs +++ b/src/AcDream.App/Rendering/GameWindow.cs @@ -2532,62 +2532,28 @@ public sealed class GameWindow : IDisposable } /// - /// Bilinear sample of the landblock heightmap at (x, y) in landblock-local - /// world units. Matches the x-major indexing convention of LandblockMesh. + /// Triangle-aware terrain Z sample directly from a landblock's raw + /// heightmap. Used as the bilinear fallback in scenery hydration when + /// physics hasn't built a TerrainSurface for the landblock yet + /// (streaming race). Delegates to + /// + /// so this fallback and the player-physics path stay in lock-step on + /// sloped cells. + /// + /// + /// Issue #48: the previous in-place implementation here had its two + /// diagonal arms swapped (SWtoNE cells used the SEtoNW triangle test + /// and vice versa), so scenery on hilly terrain sat at a different Z + /// than the visible terrain mesh — a multi-meter offset in some + /// cells, the user-reported "floating trees" symptom. + /// /// - private float SampleTerrainZ(DatReaderWriter.DBObjs.LandBlock block, float[] heightTable, float worldX, float worldY) + private static float SampleTerrainZ(DatReaderWriter.DBObjs.LandBlock block, float[] heightTable, float localX, float localY) { - // Exact port of WorldBuilder TerrainUtils.GetHeight (line 59-108). - // Barycentric interpolation over the cell's triangle pair, respecting - // the cell's split direction (SWtoNE vs SEtoNW). - const float CellSize = 24f; - - uint cellX = (uint)(worldX / CellSize); - uint cellY = (uint)(worldY / CellSize); - if (cellX >= 8) cellX = 7; - if (cellY >= 8) cellY = 7; - uint landblockX = (block.Id >> 24) & 0xFFu; uint landblockY = (block.Id >> 16) & 0xFFu; - var splitDirection = AcDream.Core.Terrain.TerrainBlending.CalculateSplitDirection( - landblockX, cellX, landblockY, cellY); - - // 4 cell corners (heightmap x-major: Height[x*9 + y]) - float h0 = heightTable[block.Height[cellX * 9 + cellY]]; // BL - float h1 = heightTable[block.Height[(cellX + 1) * 9 + cellY]]; // BR - float h2 = heightTable[block.Height[(cellX + 1) * 9 + (cellY + 1)]]; // TR - float h3 = heightTable[block.Height[cellX * 9 + (cellY + 1)]]; // TL - - float lx = worldX - cellX * CellSize; - float ly = worldY - cellY * CellSize; - float s = lx / CellSize; - float t = ly / CellSize; - - if (splitDirection == AcDream.Core.Terrain.CellSplitDirection.SWtoNE) - { - if (s + t <= 1f) - { - return h0 * (1f - s - t) + h1 * s + h3 * t; - } - else - { - float u = s + t - 1f; - float v = 1f - s; - float w = 1f - u - v; - return h1 * w + h2 * u + h3 * v; - } - } - else // SEtoNW - { - if (s >= t) - { - return h0 * (1f - s) + h1 * (s - t) + h2 * t; - } - else - { - return h0 * (1f - t) + h2 * s + h3 * (t - s); - } - } + return AcDream.Core.Physics.TerrainSurface.SampleZFromHeightmap( + block.Height, heightTable, landblockX, landblockY, localX, localY); } private void OnLiveEntityDeleted(AcDream.Core.Net.Messages.DeleteObject.Parsed delete) @@ -4674,6 +4640,11 @@ public sealed class GameWindow : IDisposable } if (float.IsPositiveInfinity(zMin)) { zMin = 0f; zMax = 0f; } + // Per-part transform offset inside the setup (post-spawn-scale). + // For setup spawns this is Setup.PlacementFrames[Default].Frames[i] * + // spawn.Scale. For single-GfxObj spawns it's identity * spawn.Scale. + var partT = mr.PartTransform.Translation; + bool hasDD = dgfx.Flags.HasFlag(DatReaderWriter.Enums.GfxObjFlags.HasDIDDegrade); string ddInfo = string.Empty; if (hasDD && dgfx.DIDDegrade != 0) @@ -4695,12 +4666,20 @@ public sealed class GameWindow : IDisposable } } + // partWorldZMin = the lowest vertex of this part in world space. + // = finalZ (setup origin in world Z) + partT.Z (part offset) + zMin (mesh-local lowest vertex) + // If everything is right and the lowest part of the tree should + // touch the ground, we expect partWorldZMin <= groundZ for at + // least one part of a multi-part setup. + float partWorldZMin = finalZ + partT.Z + zMin; + Console.WriteLine( $"[scenery-z] lb=0x{lb.LandblockId:X8} root=0x{spawn.ObjectId:X8} gfx=0x{mr.GfxObjId:X8}" + $" source={source}" + $" world=({worldPx:F2},{worldPy:F2}) localXY=({localX:F2},{localY:F2})" + $" groundZ={groundZ:F3} BaseLoc.Z={spawn.LocalPosition.Z:F3} finalZ={finalZ:F3}" + - $" zRange=[{zMin:F3}..{zMax:F3}] zSpan={zMax - zMin:F3}" + + $" partT=({partT.X:F2},{partT.Y:F2},{partT.Z:F3}) spawnScale={spawn.Scale:F3}" + + $" zRange=[{zMin:F3}..{zMax:F3}] partWorldZMin={partWorldZMin:F3} delta={partWorldZMin - groundZ:F3}" + $" hasDIDDegrade={hasDD}{ddInfo}"); } } diff --git a/src/AcDream.Core/Physics/TerrainSurface.cs b/src/AcDream.Core/Physics/TerrainSurface.cs index caa5493..fe51188 100644 --- a/src/AcDream.Core/Physics/TerrainSurface.cs +++ b/src/AcDream.Core/Physics/TerrainSurface.cs @@ -143,23 +143,89 @@ public sealed class TerrainSurface // and ACE's LandblockStruct.ConstructPolygons. bool splitSWtoNE = IsSplitSWtoNE(_landblockX, (uint)cx, _landblockY, (uint)cy); + return InterpolateZInTriangle(hBL, hBR, hTR, hTL, tx, ty, splitSWtoNE); + } + + /// + /// Sample terrain Z directly from a landblock's raw heightmap. Same + /// algorithm as (instance), but reads the four + /// corner heights through heightTable[heights[x*9+y]] on the fly + /// instead of from the pre-resolved instance cache. Use this when a + /// hasn't been built yet for a landblock — + /// e.g. scenery hydration during streaming, before physics has registered + /// the landblock. Both paths produce the same Z, so scenery sits flush + /// with the visible terrain mesh and with the player physics path. + /// + /// + /// Issue #48 root cause: the previous bilinear fallback in + /// GameWindow.SampleTerrainZ had its two diagonal arms swapped + /// (used the SEtoNW triangle test for SWtoNE cells and vice versa), + /// so on sloped cells scenery sat at a different Z than the visible + /// terrain by up to ~1.5 m. Routing the fallback through this static + /// helper guarantees both samplers stay in lock-step. + /// + /// + public static float SampleZFromHeightmap( + byte[] heights, float[] heightTable, + uint landblockX, uint landblockY, + float localX, float localY) + { + ArgumentNullException.ThrowIfNull(heights); + ArgumentNullException.ThrowIfNull(heightTable); + if (heights.Length < 81) + throw new ArgumentException("heights must have 81 entries", nameof(heights)); + if (heightTable.Length < 256) + throw new ArgumentException("heightTable must have 256 entries", nameof(heightTable)); + + float fx = Math.Clamp(localX / CellSize, 0f, CellsPerSide - 0.001f); + float fy = Math.Clamp(localY / CellSize, 0f, CellsPerSide - 0.001f); + int cx = (int)fx; + int cy = (int)fy; + cx = Math.Clamp(cx, 0, CellsPerSide - 1); + cy = Math.Clamp(cy, 0, CellsPerSide - 1); + + float tx = fx - cx; + float ty = fy - cy; + + // x-major heightmap indexing matches TerrainSurface's pre-resolution + // (heights[x * 9 + y]) and ACE LandblockStruct. + float hBL = heightTable[heights[cx * HeightmapSide + cy ]]; + float hBR = heightTable[heights[(cx+1) * HeightmapSide + cy ]]; + float hTR = heightTable[heights[(cx+1) * HeightmapSide + (cy+1)]]; + float hTL = heightTable[heights[cx * HeightmapSide + (cy+1)]]; + + bool splitSWtoNE = IsSplitSWtoNE(landblockX, (uint)cx, landblockY, (uint)cy); + return InterpolateZInTriangle(hBL, hBR, hTR, hTL, tx, ty, splitSWtoNE); + } + + /// + /// Pick the cell's triangle for the chosen diagonal and barycentric- + /// interpolate Z. Single source of truth shared by both + /// (instance, pre-resolved cache) and + /// (static, raw heightmap). + /// Triangle layout matches ACE LandblockStruct.ConstructPolygons: + /// SWtoNE cells split BL→TR (line y=x), SEtoNW cells split BR→TL + /// (line x+y=1). + /// + private static float InterpolateZInTriangle( + float hBL, float hBR, float hTR, float hTL, + float tx, float ty, bool splitSWtoNE) + { if (splitSWtoNE) { // Diagonal BL(0,0) → TR(1,1) — line y = x. // Triangles: {BL,BR,TR} below (tx > ty), {BL,TR,TL} above. if (tx > ty) - return hBL + (hBR - hBL) * tx + (hTR - hBR) * ty; // BL+BR+TR triangle - else - return hBL + (hTR - hTL) * tx + (hTL - hBL) * ty; // BL+TR+TL triangle + return hBL + (hBR - hBL) * tx + (hTR - hBR) * ty; // BL+BR+TR + return hBL + (hTR - hTL) * tx + (hTL - hBL) * ty; // BL+TR+TL } else { // Diagonal BR(1,0) → TL(0,1) — line x + y = 1. // Triangles: {BL,BR,TL} below (tx+ty <= 1), {BR,TR,TL} above. if (tx + ty <= 1f) - return hBL + (hBR - hBL) * tx + (hTL - hBL) * ty; // BL+BR+TL triangle - else - return hTR + (hTL - hTR) * (1f - tx) + (hBR - hTR) * (1f - ty); // BR+TR+TL triangle + return hBL + (hBR - hBL) * tx + (hTL - hBL) * ty; // BL+BR+TL + return hTR + (hTL - hTR) * (1f - tx) + (hBR - hTR) * (1f - ty); // BR+TR+TL } } diff --git a/tests/AcDream.Core.Tests/Physics/TerrainSurfaceTests.cs b/tests/AcDream.Core.Tests/Physics/TerrainSurfaceTests.cs index 7ceda8f..c26b214 100644 --- a/tests/AcDream.Core.Tests/Physics/TerrainSurfaceTests.cs +++ b/tests/AcDream.Core.Tests/Physics/TerrainSurfaceTests.cs @@ -67,6 +67,64 @@ public class TerrainSurfaceTests Assert.Equal(42f, surface.SampleZ(300f, 300f)); } + [Fact] + public void SampleZFromHeightmap_AgreesWithInstance_AcrossWholeLandblock() + { + // Issue #48 conformance: the static SampleZFromHeightmap (bilinear + // fallback used at scenery hydration before physics registers a + // landblock) must produce the same Z as the instance SampleZ + // (player physics path) at every (x, y). The previous fallback in + // GameWindow had its diagonal arms swapped — this test pins both + // paths to one source of truth. + // + // Heightmap with distinct per-(x,y) values so every triangle plane + // is genuinely different from the others; flat / planar heightmaps + // would mask a triangle-pick bug because all four corners would + // give the same interpolated Z. + var heights = new byte[81]; + for (int x = 0; x < 9; x++) + for (int y = 0; y < 9; y++) + heights[x * 9 + y] = (byte)((x * 17 + y * 13) % 256); + + var hTable = LinearHeightTable(); + + // Pick a landblock where IsSplitSWtoNE(...) returns BOTH true and + // false across the 64 cells — Holtburg coords (0xA9, 0xB3) work. + const uint lbX = 0xA9, lbY = 0xB3; + var instance = new TerrainSurface(heights, hTable, lbX, lbY); + + // Sample on a fine grid (~1500 points) covering all 64 cells and + // crossing every cell's diagonal boundary. A triangle-pick bug + // would show up as a >0.5 m Z mismatch on the diagonal-spanning + // cells (the corner heights vary by ~10 bytes = 10 Z each cell). + for (float lx = 0.5f; lx < 192f; lx += 5f) + for (float ly = 0.5f; ly < 192f; ly += 5f) + { + float instanceZ = instance.SampleZ(lx, ly); + float staticZ = TerrainSurface.SampleZFromHeightmap( + heights, hTable, lbX, lbY, lx, ly); + Assert.True( + Math.Abs(instanceZ - staticZ) < 0.0001f, + $"Z mismatch at ({lx:F1},{ly:F1}) lb=(0x{lbX:X},0x{lbY:X}): instance={instanceZ:F4} static={staticZ:F4}"); + } + } + + [Fact] + public void SampleZFromHeightmap_RejectsBadInputs() + { + var goodHeights = new byte[81]; + var goodTable = LinearHeightTable(); + + Assert.Throws(() => + TerrainSurface.SampleZFromHeightmap(null!, goodTable, 0, 0, 0f, 0f)); + Assert.Throws(() => + TerrainSurface.SampleZFromHeightmap(goodHeights, null!, 0, 0, 0f, 0f)); + Assert.Throws(() => + TerrainSurface.SampleZFromHeightmap(new byte[80], goodTable, 0, 0, 0f, 0f)); + Assert.Throws(() => + TerrainSurface.SampleZFromHeightmap(goodHeights, new float[255], 0, 0, 0f, 0f)); + } + [Fact] public void SampleSurfacePolygon_ReturnsContainingTriangleVertices() {