client: gate actor spawns on bundle-ready + render-coverage rebind guard (#1666) + cert full-figure assert#1679
Conversation
…ard (#1666) + cert full-figure assert The last demo-path render defect (#1666 g5 residual): in the crypt fixture three monsters share one alias/prefab ('Goblin Warrior 1/2/3'); two render full-figure, the FIRST-spawned instance renders as a ~0.2m red accessory fragment (body renderers never initialized), always the first spawn. The raw-bind height guards (#1666/#1676) PASS because the prefab's bind bounds are full height — a render-coverage case. Race hypothesis REFUTED at the load level: Bundle() is a synchronous AssetBundle.LoadFromFile memoized on _bundleTried (CombatSurfaceClient.cs:1285), and LoadAsset<T> is synchronous b.LoadAsset<T> (:1299) — no LoadFromFileAsync/LoadAssetAsync anywhere. All three goblins spawn back-to-back in the SAME ApplySurf frame (:861-880). There is no async load to race. The residual is a first-instance artifact: the first SpawnActor of a frame is also the first-ever bundle/controller/skinned-mesh access; the 2nd/3rd of the SAME prefab, no longer first, render full. Fix (two layers, root-cause-agnostic): - PrewarmActorPipeline() in Start(): fold the one-time bundle + humanoid-controller + donor-idle loads AND a hidden throwaway skinned-mesh upload of each template out of the first spawn, so no real actor is ever the first skinned/bundle access of its frame. - RenderCoverageGuardCo: after an actor settles (3 frames), measure its RENDERED (BakeMesh) figure height; if it covers < RenderCoverageFloor (0.6) x its scale-target, re-instantiate ONCE (a fresh bind is no longer first-in-frame) and log '[CSC] #1666 render-coverage rebind'. One attempt per id (_coverageRebound) — no loop. player_cert: add cast_renders_full_figure (Primitive C). For each foe token, project a figure box and measure the vertical extent of foe-body pixels (figure_body_extent_frac); a fragment token turns the suite RED. Calibrated red-first against crypt_g5.png: fragment body extent 0.08-0.17 vs full goblins 0.47-0.82, floor 0.35. qa/test_player_cert.py adds synthetic + real-frame calibration units.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| foreach (var fbx in new[] { TemplateMonsterFbx, TemplateCharFbx }) | ||
| { | ||
| var prefab = LoadAsset<GameObject>(fbx); | ||
| if (prefab == null) continue; | ||
| var go = (GameObject)Object.Instantiate(prefab); | ||
| go.name = "__prewarm"; go.transform.position = new Vector3(0f, -10000f, 0f); go.SetActive(false); | ||
| foreach (var smr in go.GetComponentsInChildren<SkinnedMeshRenderer>(true)) | ||
| if (smr != null && smr.sharedMesh != null) { var bk = new Mesh(); smr.BakeMesh(bk); Object.DestroyImmediate(bk); } | ||
| Object.DestroyImmediate(go); | ||
| } |
| } | ||
| Debug.Log("[CSC] #1666 actor pipeline pre-warmed"); | ||
| } | ||
| catch (System.Exception e) { Debug.LogWarning("[CSC] #1666 prewarm: " + e.Message); } |
evaOS review status: completedPR: #1679 - client: gate actor spawns on bundle-ready + render-coverage rebind guard (#1666) + cert full-figure assert evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #1679 Review URL: #1679 (review) |
![evaos-code-review-bot[bot]](https://avatars.githubusercontent.com/in/4184532?s=60&v=4){kind=small}
There was a problem hiding this comment.
Walkthrough
PR: #1679 - client: gate actor spawns on bundle-ready + render-coverage rebind guard (#1666) + cert full-figure assert
Head: c0da673513e211db00a8b9c88515f9e7c80f8af0 into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 2/5 (~26 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
extensions/renderers/unity/scripts/CombatSurfaceClient.cs |
modified | +78/-1 | Unity/gameplay state | Moderate: validated P3 finding |
qa/player_cert.py |
modified | +142/-0 | Changed file | Moderate: validated P3 finding |
qa/test_player_cert.py |
modified | +69/-0 | Test coverage | Low |
Review Signal
Validated inline findings: 3 (P0: 0, P1: 0, P2: 0, P3: 3).
Dropped findings before posting: 0. High-severity findings: 0.
Risk Taxonomy
- Proof gap: 2
- Runtime correctness: 1
Validation and Proof
1 required validation/proof recommendation(s) selected from changed files.
- required: Unity editor or Play Mode smoke - Unity asset/script/project files changed. Proof: Unity editor smoke; Play Mode log; scene/prefab screenshot or recording.
Proof status: missing - 1 required validation/proof recommendation(s) missing from PR metadata.
Profile validation hints: Prefer correctness, persistence, CI, release, and regression findings over style-only feedback.
Profile proof expectations: Look for Unity editor, play-mode, fixture, or focused smoke evidence when runtime behavior changes.
Related Context
Related issues/PRs: #1666, #1676.
Suggested labels: unity, tests.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions:
Assets/**- Prioritize scene, prefab, save-state, asset-reference, and gameplay regressions. - Path instructions:
ProjectSettings/**- Treat build, platform, input, graphics, and release behavior changes as high risk. - Label suggestions: unity, gameplay, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
| x0, y0, x1, y1 = region | ||
| x0, y0 = max(0, int(x0)), max(0, int(min(y0, y1))) | ||
| x1, y1 = min(w, int(x1)), min(h, int(max(y0, y1))) | ||
| if y1 - y0 < 2 or x1 - x0 < 1: |
There was a problem hiding this comment.
P3: figure_body_extent_frac measures row-SPAN, not row-FILL — a scattered body-pixel distribution inflates the score
figure_body_extent_frac returns (rows.max()-rows.min()+1)/(y1-y0) — the vertical SPAN of any foe-body rows, not the fraction of rows that actually contain body pixels. Two disconnected body clusters (e.g. a lit headpiece + lit boots with an unlit/occluded torso gap) would still score high even though no contiguous body figure is rendered. For the documented failure mode (fragment ≈ 0 body pixels) this is fine and correctly RED-first, but the metric is more permissive than the docstring ('a full figure fills most of its projected box (~0.5-0.8)') implies. Consider also requiring a minimum body-pixel row-density (e.g. fraction of rows in [rows.min,rows.max] that .any()), or document explicitly that this is a span/extents metric, so a future partially-occluded-but-healthy foe can't pass on a scattered few pixels.
Category: Proof gap
Why this matters: The assert is the regression gate for the g5 first-spawn fragment bug. A span-only metric could let a different future partial-render regression (scattered pixels, occluded torso) read GREEN, weakening the gate the PR is built to enforce.
| "its OWN body depth). A ghost-cyan overlay in the open turns this RED — the occluder-scoping " | ||
| "stencil fix keeps it GREEN."), | ||
| Assertion( | ||
| id="cast_renders_full_figure", scope="live_party", needs=("engine", "player"), |
There was a problem hiding this comment.
P3: No live/CI evidence that probe_cast_figures returns GREEN on a healthy built board
The new live assertion cast_renders_full_figure (scope=live_party) is registered, but the only real-PNG test (test_cast_figure_calibration_real_crypt_g5) asserts RED against the buggy crypt_g5 frame and pytest.skips when LEXAR is absent (so CI never exercises it). There is no fixture/test proving the live probe wiring — ortho fallback to 11.0, cell_to_world + world_to_window_px projection of FIGURE_FEET/HEAD_WORLD_Y, and the BODY_HALF_WIDTH_PX box — yields a GREEN verdict against a correctly-rendering built app with foes present. If the projection constants (FIGURE_HEAD_WORLD_Y=2.4, FIGURE_FEET_WORLD_Y=0.15, ortho fallback) are off for some room, the box could miss the figure and the suite would ERROR (safe) rather than silently GREEN — but that ERROR path itself is unverified. A focused editor/play-mode smoke or a synthetic full-figure projected-frame test for the live projection (not just the pure extent fn) would close the gap.
Category: Proof gap
Why this matters: Per repo guidance, runtime-behavior assertions need focused Unity/editor/play smoke evidence. Without it, the gate could ship mis-calibrated and either always-ERROR (no signal) or, worst case, miss a real fragment in a room whose projection differs from crypt_g5.
| @@ -1761,9 +1808,38 @@ Transform SpawnActor(string id, string tokName, string team, int cx, int cy) | |||
| // graph is running steadily (a bounds-min re-snap), unless a move/down has since taken over. Cells carry | |||
| // no elevation (CellToWorld is flat FloorY), so this is a footing re-measure, not a dais-height lookup. | |||
| StartCoroutine(ResnapGroundedCo(id, cx, cy)); | |||
There was a problem hiding this comment.
P3: Render-coverage re-bind does not stop the first spawn's in-flight ResnapGroundedCo (benign double re-ground)
When RenderCoverageGuardCo fires (3 frames after the first spawn) it calls SpawnActor(id,...,armCoverageGuard:false), which destroys and rebuilds Actor_ and starts a SECOND ResnapGroundedCo(id,...). The FIRST spawn's ResnapGroundedCo (started at line 1810, 4-frame wait) is not stopped; if it hasn't fired yet it will also run, and FindActor(id) resolves to the NEW actor (same name) so it re-grounds the already-rebuilt actor a second time. This is idempotent (GroundSnap to the same cell) and harmless in practice, but it is an orphaned coroutine the re-bind path does not account for. Unlike Despawn (which StopCoroutine's the glide and KillIdleGraph/KillWalkGraph), the guard's re-spawn relies on the new SpawnActor's internal KillIdleGraph(id) for the idle graph but leaves the prior ResnapGroundedCo running. Worth a one-line note or an explicit stop for clarity; not a correctness defect.
Category: Runtime correctness
Why this matters: The re-bind path is the guaranteed catch-all for the residual fragment bug. Any un-accounted coroutine/state on the re-bind path is a future-regression surface if ResnapGroundedCo or SpawnActor's settling logic ever gains side effects (e.g. a downed/revive interaction). Documenting or stopping it keeps the one-attempt invariant clean.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0da673513
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| rec["room"] = room | ||
| mask = W.walkmask_from_surface(surf) | ||
| cols, rows = mask["cols"], mask["rows"] | ||
| foes = [t for t in _actor_tokens(surf) if t["team"] == "foe"] |
There was a problem hiding this comment.
When player_cert is run after a foe has reached 0 HP but before the engine removes it from the combat surface, the viewer still emits that token with health: "down", and the Unity client intentionally leaves it prone instead of despawning it. This new filter sends every foe token into the vertical full-figure check, so a correctly downed/prone goblin will have a low body extent and make cast_renders_full_figure report RED even though the render state is expected; skip or separately handle downed/0-HP foes before applying this standing-figure threshold.
Useful? React with 👍 / 👎.
| import numpy as np # noqa: PLC0415 | ||
| a = np.asarray(img, dtype=int) | ||
| r, g, b = a[..., 0], a[..., 1], a[..., 2] | ||
| return (g > BODY_G_FLOOR) & (g >= r - BODY_GR_SLACK) & (g > b + BODY_GB_MARGIN) |
There was a problem hiding this comment.
Because cast_renders_full_figure is registered as a standing check for every foe token, this goblin-specific color mask will report zero body extent for any living full-height foe whose material is not bright yellow-green, such as a red/brown/gray monster or a later registry asset. In those runs the cert turns RED even though the figure is present; the assertion needs a palette-independent silhouette/extent signal or should be scoped to the calibrated goblin fixture only.
Useful? React with 👍 / 👎.
| ortho = 11.0 | ||
| rec["window"] = [w, h] | ||
| cpx = W.cell_px(ortho, h) | ||
| shot = W._capture_shot(qa, out, "cast_figures") |
There was a problem hiding this comment.
When this assertion is run directly with --only cast_renders_full_figure or immediately after qa_sandbox.py up, the QA /debug listener can be reachable before the Unity PollLoop has fetched the first surface and spawned the foe actors. This captures a frame against engine foe tokens with no actor pixels yet, producing zero extents and a RED verdict for a build that would settle correctly a few frames later; wait for player-side actor readiness or at least a settle interval before taking this /shot.
Useful? React with 👍 / 👎.
1 participant
Fixes the last open demo-path render defect — #1666's g5 per-instance manifestation.
Race: REFUTED (at the load level), with the real per-instance mechanism identified
The hypothesis was that the first monster spawn races an async actor-bundle load. Code trace refutes the async part:
Bundle()is a synchronousAssetBundle.LoadFromFile, memoized on_bundleTried(CombatSurfaceClient.cs:1285).LoadAsset<T>is synchronousb.LoadAsset<T>(:1299). NoLoadFromFileAsync/LoadAssetAsyncexists anywhere in the file.ApplySurfframe (:861-880) — no coroutine yield between them, nothing async to race.The real per-instance divergence: the first
SpawnActorof a frame is also the first-ever access that synchronously loads the bundle + humanoid controller + donor clip and is the first freshly-instantiated skinned mesh of the frame. The 2nd/3rd instances of the same prefab, no longer first, render full. This is exactly the render-coverage case both prior lanes flagged: the raw-prefab bind-height guards (#1666/#1676) PASS because the prefab's ownsharedMesh.boundsis full height, yet the first instance draws only a ~0.2m accessory fragment.Fix (two layers, root-cause-agnostic)
PrewarmActorPipeline()inStart()— folds the one-time bundle + controller + donor loads AND a hidden throwaway skinned-mesh upload of each template out of the first spawn, so no real actor is ever the first skinned/bundle access of its frame. All guarded; absent bundle/assets → graceful skip, byte-identical to a no-bundle build. (This is the "gate on bundle-ready" intent, adapted since the load is synchronous — a queue has nothing async to wait on.)RenderCoverageGuardCo— after an actor settles (3 frames), measures its RENDERED (BakeMesh) figure height; if it covers< RenderCoverageFloor (0.6)× its scale-target height, re-instantiates the actor once (a fresh bind is no longer first-in-frame) and logs[CSC] #1666 render-coverage rebind. One attempt per id (_coverageRebound, cleared onDespawn) — a still-degenerate re-bind can never loop. Catches ANY future partial-render bind, not just the race.Cert (
qa/player_cert.py):cast_renders_full_figure(Primitive C)For each foe token, projects a figure box and measures the vertical extent of foe-body pixels (
figure_body_extent_frac); a fragment-rendered token turns the suite RED. A global color mask is unusable in the brazier-lit crypt (warm stone/flames read warm), so extent is only measured inside per-token projected boxes. Calibrated red-first againstcrypt_g5.png: fragment body extent 0.08–0.17 vs full goblins 0.47–0.82, floor 0.35 (in the gap).qa/test_player_cert.pyadds synthetic + real-frame calibration units (the real-frame test asserts fragment→RED, both full goblins→GREEN).Tests
uv run pytest qa/test_player_cert.py -q→ 36 passed (incl. the 4 new figure tests, real crypt_g5.png calibration exercised).C# is not locally compilable — changes are surgical, mirror the file's idioms (guarded lazy-load, coroutine settle-then-act like
ResnapGroundedCo), and are line-cited above.Box re-verify recipe
[CSC] #1666 actor pipeline pre-warmed; a[CSC] #1666 render-coverage rebindline means the backstop fired (still a PASS — the actor is re-bound full).python3 qa/player_cert.py --live --run <sandbox>→cast_renders_full_figureGREEN (RED if any foe still renders a fragment).Never touched: owner surfaces (:8766), :8866/:8972 (no sandbox brought up; pixel calibration used the saved frames).