refactor(render): Phase A8.F — Task 4 review follow-up (honest cap comment, cycle guard test, file fixpoint fast-follow)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
Erik 2026-05-29 12:16:11 +02:00
parent 0ed462cb62
commit 270c21f263
3 changed files with 97 additions and 2 deletions

View file

@ -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