From 270c21f263344c6155f89df1a6e8ecc045b04f84 Mon Sep 17 00:00:00 2001 From: Erik Date: Fri, 29 May 2026 12:16:11 +0200 Subject: [PATCH] =?UTF-8?q?refactor(render):=20Phase=20A8.F=20=E2=80=94=20?= =?UTF-8?q?Task=204=20review=20follow-up=20(honest=20cap=20comment,=20cycl?= =?UTF-8?q?e=20guard=20test,=20file=20fixpoint=20fast-follow)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 4.7 --- docs/ISSUES.md | 68 +++++++++++++++++++ .../Rendering/PortalVisibilityBuilder.cs | 14 +++- .../Rendering/PortalVisibilityBuilderTests.cs | 17 +++++ 3 files changed, 97 insertions(+), 2 deletions(-) diff --git a/docs/ISSUES.md b/docs/ISSUES.md index 8122839..ecd9b8a 100644 --- a/docs/ISSUES.md +++ b/docs/ISSUES.md @@ -48,6 +48,74 @@ Copy this block when adding a new issue: --- +## #102 — A8.F PortalVisibilityBuilder — port retail update_count fixpoint (replace MaxReprocessPerCell cap) + +**Status:** OPEN +**Severity:** MEDIUM +**Filed:** 2026-05-29 +**Component:** rendering, visibility, EnvCell portal traversal + +**Description:** A8.F Task 4 shipped a bounded-BFS port of retail's +`PView::ConstructView` → `ClipPortals` → `AddViewToPortals` in +[`src/AcDream.App/Rendering/PortalVisibilityBuilder.cs`](../src/AcDream.App/Rendering/PortalVisibilityBuilder.cs). +Code review found NO correctness bugs (the cellar-flap fix works and the +BFS terminates), but two scaling issues that bite only on CYCLIC / +high-fan-in portal graphs (dungeons, network hubs), NOT on the cottage +cellar (a 2-3 cell chain) which is the current M1.5 goal: + +- **I-1 — the cap is load-bearing, not a safety net.** `MaxReprocessPerCell = 4` + is the *actual* termination mechanism for cyclic graphs. The + `if (nview.Polygons.Count > before)` re-enqueue-on-growth guard is a + near-no-op because `CellView.Add` (PortalView.cs) appends + unconditionally and never dedupes, so a cell almost always "grows" and + is re-enqueued — convergence relies entirely on the count hitting 4. + A cell reachable through **>4 contributing portals under-counts** + (drops legitimately-visible contributions). +- **I-2 — duplicate polygons accumulate on cyclic/multi-path graphs.** + Measured on a synthetic 4-room ring: 34 `OutsideView` polygons and + 216-poly `CellView`s where retail converges to a small fixed set. + Correctness survives (overlapping stencil marks are idempotent) but + it's per-frame cost feeding the stencil pipeline. + +**Root cause / status:** We approximate retail's monotone-fixpoint +convergence with a fixed re-process cap. Retail instead converges via an +`update_count` / `set_view(...,i)` slice watermark — each cell records a +timestamp/watermark of how much of its view has been propagated, so a +re-visit only re-propagates the *new* slice and the graph reaches a true +fixpoint with no duplicate accumulation and no arbitrary cap. + +Retail anchors (`docs/research/named-retail/acclient_2013_pseudo_c.txt`): +- `AddToCell` 433050 — `esi[0x11]` update-count/slice watermark on the cell +- `InitCell` — per-cell timestamp init +- `AddViewToPortals` 433446 — change-detection that drives the fixpoint + +**Related M-4 stub:** the neighbour-side `OtherPortalClip` +(decomp:433524) is also not yet ported — the builder clips the portal +opening against the *current* cell's view but does NOT additionally clip +against the *neighbour's* matching portal polygon. Its absence can only +ever OVER-include (mark cells/regions visible that retail would cull), +never under-include, so it's deferred. There is a `TODO(A8.F)` marker at +the relevant site in `PortalVisibilityBuilder.cs`. + +**Files:** +- `src/AcDream.App/Rendering/PortalVisibilityBuilder.cs` — replace the + `MaxReprocessPerCell` cap + re-enqueue-on-growth guard with a + per-cell slice watermark; honest-limitation comment lives at the + `MaxReprocessPerCell` declaration. +- `src/AcDream.App/Rendering/PortalView.cs` — `CellView.Add` currently + never dedupes; the fixpoint port either dedupes here or tracks a + propagated-slice index per cell. + +**Acceptance:** On a cyclic/hub portal graph (synthetic 4-room ring + +the Town Network dungeon hub), `OutsideView` / `CellView` polygon counts +converge to a small fixed set (no duplicate accumulation), every cell +reachable through any number of contributing portals is included, and +the BFS still terminates. Existing cottage-cellar tests stay green. +**MUST land before A8.F is relied on for dungeons** (dungeons are +currently blocked on #95 regardless). + +--- + ## #87 — Drop WB fork patch by switching to PrepareEnvCellGeomMeshDataAsync **Status:** OPEN diff --git a/src/AcDream.App/Rendering/PortalVisibilityBuilder.cs b/src/AcDream.App/Rendering/PortalVisibilityBuilder.cs index 9a75c04..fbc68a7 100644 --- a/src/AcDream.App/Rendering/PortalVisibilityBuilder.cs +++ b/src/AcDream.App/Rendering/PortalVisibilityBuilder.cs @@ -28,8 +28,16 @@ public sealed class PortalVisibilityFrame public static class PortalVisibilityBuilder { - // Bound on neighbour re-processing (retail uses update_count timestamps). Portal graphs are - // small; this guards cycles while allowing multi-portal unions into the same neighbour. + // Per-cell re-processing bound. NOTE: this cap is the actual termination mechanism for + // cyclic portal graphs, NOT merely a safety net — the re-enqueue-on-growth guard below is + // a near-no-op because CellView.Add never dedupes, so a cell almost always "grows". Retail + // instead converges via an update_count / set_view(...,i) slice watermark (decomp: AddToCell + // 433050 esi[0x11], InitCell timestamp, AddViewToPortals 433446). Consequences vs retail: + // (a) a cell reachable through >4 contributing portals under-counts; (b) duplicate polygons + // accumulate on cyclic/multi-path graphs (correctness survives — stencil marks are idempotent + // — but it is per-frame cost). Both bite only on dungeon-scale cyclic/hub graphs; a cottage + // cellar is a short chain where each cell is visited once. The faithful fixpoint port is filed + // as a fast-follow (docs/ISSUES.md) before A8.F is relied on for dungeons. private const int MaxReprocessPerCell = 4; private const float PortalSideEpsilon = 0.01f; // matches CellVisibility.PointInCellEpsilon @@ -47,6 +55,8 @@ public static class PortalVisibilityBuilder var frame = new PortalVisibilityFrame(); if (cameraCell == null) return frame; + // Interior portals never cross landblocks (same invariant as CellVisibility.GetVisibleCells); + // building-boundary crossings are handled separately via the buildingMembership escape hatch. uint lbMask = cameraCell.CellId & 0xFFFF0000u; frame.CellViews[cameraCell.CellId] = CellView.FullScreen(); diff --git a/tests/AcDream.App.Tests/Rendering/PortalVisibilityBuilderTests.cs b/tests/AcDream.App.Tests/Rendering/PortalVisibilityBuilderTests.cs index e51d48b..5994b80 100644 --- a/tests/AcDream.App.Tests/Rendering/PortalVisibilityBuilderTests.cs +++ b/tests/AcDream.App.Tests/Rendering/PortalVisibilityBuilderTests.cs @@ -104,6 +104,23 @@ public class PortalVisibilityBuilderTests for (int i = 0; i < p.Length; i++) { var a = p[i]; var b = p[(i + 1) % p.Length]; area2 += a.X * b.Y - b.X * a.Y; } Assert.True(area2 > 0f, "clipped OutsideView region should be CCW after winding normalization"); } + + [Fact] + public void Builder_CyclicGraph_TerminatesWithBoundedPolys() + { + // A <-> B cycle; B also has an exit window. Must terminate and not blow up. + var a = Cell(0x0001, new CellPortalInfo(0x0002, 0, 0)); + a.PortalPolygons.Add(Quad(0f, 0f, 0.5f, 0.5f, -3f)); + var b = Cell(0x0002, new CellPortalInfo(0x0001, 0, 0), new CellPortalInfo(0xFFFF, 1, 0)); + b.PortalPolygons.Add(Quad(0f, 0f, 0.5f, 0.5f, -2f)); // back to A + b.PortalPolygons.Add(Quad(0f, 0f, 1.0f, 1.0f, -6f)); // exit window + var all = new Dictionary { [0x0001] = a, [0x0002] = b }; + + var frame = Build(a, all); // must return (no infinite loop) + Assert.False(frame.OutsideView.IsEmpty); + Assert.True(frame.OutsideView.Polygons.Count < 256, + $"OutsideView poly count {frame.OutsideView.Polygons.Count} — termination/dedup regression guard"); + } } internal static class PortalFrameTestHelper