client: runtime-spawned actors bind the animated roster setup (T-pose fix, #1601)#1616
Conversation
… fix, #1601) Runtime-spawned player actors (the rogues 'Gauge' in the shop sandbox and 'Sable' in the dwing sandbox) rendered a sideways T-pose while the baked PC 'Aldric' animated on the same client code. Root cause: a runtime actor is resolved by its in-game NAME (slugified), so a rogue's name matches no registry asset or alias and falls to defaults.character = template_human = Assets/painterly/models/hero.fbx. hero.fbx is the one character model flagged humanoid:false / needs_remodel (avatar isHuman=false) with no resolvable idle clip: it fails the humanoid-controller gate AND PlayIdleGraph gets a null clip, and its non-skinned import takes the -90 pitch stand-up -> a frozen, sideways T-pose. 'Aldric' works because 'aldric' aliases to the Humanoid 'fighter'. Fix (make a T-pose impossible at the character floor, reusing an existing animated asset rather than a new code branch): - data/asset-registry/registry.json: re-point template_human off hero.fbx onto patron_commoner -- the generic rigged humanoid the tavern/camp cast already spawn correctly (meshy 9-clip idle in anim_idle.fbx). Every character miss (and __any__) now animates via the same per-frame idle-graph fallback. - CombatSurfaceClient.cs: the in-code character floor (registry-absent path) now returns patron_commoner + its idle moveset instead of hero.fbx, so even a registry-less build floors to an animated humanoid. - asset_registry.py / AssetRegistry.cs: the hardcoded floors in both twins moved off hero.fbx for parity. - viewer/tests/test_asset_registry.py: +5 anti-T-pose invariant tests (the Gauge /Sable repros, arbitrary character names, the __any__ and in-code floors, and a default/alias-target + model_ref completeness lint).
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 45 seconds 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 (6)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
evaOS review status: completedPR: #1616 - client: runtime-spawned actors bind the animated roster setup (T-pose fix, #1601) evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #1616 Review URL: #1616 (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: #1616 - client: runtime-spawned actors bind the animated roster setup (T-pose fix, #1601)
Head: d36c8584da61829188314e65de93819ca8b47e03 into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 1/5 (~14 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
extensions/renderers/unity/scripts/AssetRegistry.cs |
modified | +8/-3 | Unity/gameplay state | Moderate: validated P3 finding |
extensions/renderers/unity/scripts/CombatSurfaceClient.cs |
modified | +16/-8 | Unity/gameplay state | Moderate: validated P2 finding |
Review Signal
Validated inline findings: 2 (P0: 0, P1: 0, P2: 1, P3: 1).
Dropped findings before posting: 1. High-severity findings: 0.
Risk Taxonomy
- Release regression: 1
- Unity scene/prefab: 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: #1601, #1423.
Suggested labels: unity.
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.
| } | ||
| } | ||
| return new[] { fbxDef, albDef, "" }; | ||
| return new[] { fbxDef, albDef, animDef }; |
There was a problem hiding this comment.
P2: #1601 T-pose fix not applied to its documented mirror paint_combat_v1.cs::resolveAsset, which still floors characters to the clipless hero.fbx
ResolveAsset's new comment (lines 1252-1262) says it 'Mirrors paint_combat_v1.cs resolveAsset' and frames the patron_commoner character floor as the correct #1601 behavior. But the original mirror at extensions/renderers/unity/scripts/paint_combat_v1.cs:330-346 was NOT updated in this PR: it still sets fbxDef = ... : "Assets/painterly/models/hero.fbx", albDef = ... : "Assets/painterly/models/hero_albedo.png", and returns empty anim_ref for the character floor (lines 331-333, 346) — exactly the sideways-T-pose root cause #1601 fixes in the two edited files. paint_combat_v1.cs is still a live canonical path: M15SpendGateProbe.cs:46 calls 'paint_combat_v1.cs SaveScene the canonical scene after every render', and BuildMacOSPlayer.cs:31 describes the build as consuming '/combat-surface via paint_combat_v1.cs engine-driven actor placement'. So any runtime-spawned rogue whose slug matches no asset/alias and renders through paint_combat_v1 (or a future regression that re-points CombatSurfaceClient back at its stated mirror) will still T-pose. The fix is incomplete: either apply the identical patron_commoner floor + anim_idle.fbx anim_ref to paint_combat_v1.cs::resolveAsset, or add a comment there explicitly documenting the intentional divergence and that paint_combat_v1 is superseded by CombatSurfaceClient for runtime-spawned actors.
Category: Release regression
Why this matters: The PR's stated root-cause fix (kill the T-pose at the character floor) is only applied to 2 of the 3 resolveAsset copies. The third is the documented mirror and is still live, so the bug class survives in that path and the three implementations now silently diverge, which defeats the 'kill it at the floor rather than adding a code branch' design stated in the registry's template_human gen_recipe.
There was a problem hiding this comment.
Adopted — paint_combat_v1.cs::resolveAsset now carries the identical patron_commoner floor (model+albedo+anim) with the same template-vs-real anim_ref split as CombatSurfaceClient; all three twins agree and the only remaining hero.fbx mentions anywhere are comments.
🤖 Addressed by Claude Code
| animRef = "Assets/painterly/models/hero@moveset.fbx", | ||
| modelRef = "Assets/chars_v2/patron_commoner/rigged.fbx", | ||
| albedoRef = "Assets/chars_v2/patron_commoner/albedo.jpg", | ||
| animRef = "Assets/chars_v2/patron_commoner/anim_idle.fbx", |
There was a problem hiding this comment.
P3: No C# test pins the Unity in-code floor away from hero.fbx (Python twin has one)
The Python twin viewer/asset_registry.py has test_in_code_floor_is_animatable (viewer/tests/test_asset_registry.py:187-192) that asserts _HARDCODED_FLOOR is NOT hero.fbx, and _assert_animatable_character covers the anim_ref. The two C# twins edited here — AssetRegistry.HardcodedFloor and CombatSurfaceClient.ResolveAsset — have no equivalent test (no C# test references HardcodedFloor/ResolveAsset/patron_commoner/T-pose; only PathingTestDriver.cs exists under the unity test path). #1601 is a visual regression fix with no committed Unity editor/play-mode or fixture evidence in the diff. A future refactor could revert either C# floor back to hero.fbx with nothing failing. Recommend an edit-mode test asserting HardcodedFloor('character').modelRef != hero.fbx and that animRef is non-empty for the character kind, mirroring the Python guard.
Category: Unity scene/prefab
Why this matters: Runtime-animation behavior changed with no automated regression guard on the Unity side; the only protection is a test on the Python twin, which does not exercise the C# resolve path the renderer actually calls.
There was a problem hiding this comment.
Adopted in the honest form available: the twins have no C# edit-mode rig in this repo, so the invariant is pinned as a source-level lint in viewer/tests/test_asset_registry.py — comments-stripped CombatSurfaceClient.cs / paint_combat_v1.cs / AssetRegistry.cs must contain NO hero.fbx code literal (a reappearance = a floor regression). Runs in the same CI as the Python-twin guards; a real Play-Mode smoke rides the next box build per the PR's box recipe.
🤖 Addressed by Claude Code
… + source-lint guard (evaOS P2/P3) paint_combat_v1.cs::resolveAsset (the documented mirror, still live via M15SpendGateProbe + BuildMacOSPlayer) still floored characters to the clipless hero.fbx. Same floor + template-vs-real anim_ref split as CombatSurfaceClient (a REAL asset row with no anim_ref keeps mesh-as-poseClip; a template fall-through takes the animated floor's idle). The Unity twins have no C# test rig, so the anti-T-pose invariant is pinned at source level: comments-stripped .cs must not mention hero.fbx in code (17/17).
evaOS review status: completedPR: #1616 - client: runtime-spawned actors bind the animated roster setup (T-pose fix, #1601) evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #1616 Review URL: #1616 (review) |
There was a problem hiding this comment.
Walkthrough
PR: #1616 - client: runtime-spawned actors bind the animated roster setup (T-pose fix, #1601)
Head: b15719de7664208cd570a1571dc5ab9366cd9959 into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 1/5 (~16 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
extensions/renderers/unity/scripts/AssetRegistry.cs |
modified | +8/-3 | Unity/gameplay state | Moderate: runtime path |
extensions/renderers/unity/scripts/CombatSurfaceClient.cs |
modified | +16/-8 | Unity/gameplay state | Moderate: validated P3 finding |
extensions/renderers/unity/scripts/paint_combat_v1.cs |
modified | +14/-5 | Unity/gameplay state | Moderate: validated P3 finding |
Review Signal
Validated inline findings: 2 (P0: 0, P1: 0, P2: 0, P3: 2).
Dropped findings before posting: 0. High-severity findings: 0.
Risk Taxonomy
- Unity scene/prefab: 2
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: #1601, #1423.
Suggested labels: unity.
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.
| // silently diverge — evaOS #1616 P2 caught this copy still flooring to hero.fbx). | ||
| string fbxDef=kind=="monster"?"Assets/chars_v2/goblin/goblin.fbx":"Assets/chars_v2/patron_commoner/rigged.fbx"; | ||
| string albDef=kind=="monster"?"Assets/chars_v2/goblin/albedo.png":"Assets/chars_v2/patron_commoner/albedo.jpg"; | ||
| string animDef=kind=="monster"?"":"Assets/chars_v2/patron_commoner/anim_idle.fbx"; |
There was a problem hiding this comment.
P3: Floor paths duplicated across 3 files with no shared constant or test guard
The patron_commoner floor (model/albedo/anim refs) is now copy-pasted in three places that the comments explicitly say 'must not silently diverge': AssetRegistry.cs:80-82, CombatSurfaceClient.cs:1267-1268, and paint_combat_v1.cs:335-337. The PR's own comment at paint_combat_v1.cs:334 acknowledges 'evaOS #1616 P2 caught this copy still flooring to hero.fbx' — i.e. this exact divergence was caught by human review, not by any automated check. There is still no source-lint/test asserting the three floors (and the Python twin viewer/asset_registry.py:47-49, plus registry.json's template_human/patron_commoner rows) stay byte-identical. A fourth silent divergence on the next floor change is likely. Consider a single shared constant or a qa fixture that asserts all four sources resolve to the same model/albedo/anim triple.
Category: Unity scene/prefab
Why this matters: A future floor repoint that touches only some copies reintroduces the T-pose regression this PR exists to fix, and review is the only current safety net.
| } | ||
| } | ||
| return new[] { fbxDef, albDef, "" }; | ||
| return new[] { fbxDef, albDef, animDef }; |
There was a problem hiding this comment.
P3: No editor/play-mode or fixture evidence in diff for the T-pose fix
This PR changes runtime actor-spawn behavior (the in-code character floor across both the live client SpawnActor path at CombatSurfaceClient.cs:1430-1494 and the capture paint path at paint_combat_v1.cs:330-355), which is exactly the runtime-behavior class the repo profile asks for Unity editor/play-mode/fixture smoke evidence on. The diff contains comments describing the measured T-pose/sideways symptom but no committed test, fixture, or captured evidence proving a runtime-spawned rogue (e.g. 'Gauge'/'Sable' named in the registry.json template_human gen_recipe) now renders an animated idle on the patron_commoner floor. The behavioral reasoning is sound on inspection (rest mode: anim_idle.fbx sampled at paint_combat_v1.cs:388,212; live client: ResolveIdleClip finds the moveset idle via _animOf at CombatSurfaceClient.cs:1439,1494), but it is unverified by any committed artifact. Add a focused smoke/fixture or link build-SHA capture evidence.
Category: Unity scene/prefab
Why this matters: The fix is correct-by-reasoning but the T-pose class has repeatedly regressed (#1408, this PR's own P2 catch); without committed proof the next refactor can reintroduce it unnoticed.
![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: b15719de76
ℹ️ 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".
| literal means a floor regressed to the clipless non-humanoid model.""" | ||
| import re | ||
| root = Path(__file__).resolve().parents[2] / "extensions" / "renderers" / "unity" / "scripts" | ||
| for name in ("CombatSurfaceClient.cs", "paint_combat_v1.cs", "AssetRegistry.cs"): |
There was a problem hiding this comment.
This new source guard skips paint_combat_replay_v1.cs, but that action-replay renderer still has the same inline resolver and still returns Assets/painterly/models/hero.fbx when /home/unity/worldos-unity/registry.json is missing or unreadable (paint_combat_replay_v1.cs:227-243). In registry-absent replay/evidence runs, unknown character names such as Gauge/Sable can therefore still hit the clipless hero floor even though this patch updates the other registry-absent floors. Please include/update the replay file as well so the anti-T-pose fix covers the replay lane.
Useful? React with 👍 / 👎.
1 participant
Fixes the runtime-spawn T-pose (#1601). Runtime-spawned player actors — 'Gauge' the rogue (shop sandbox) and 'Sable' the rogue (dwing sandbox) — rendered a sideways T-pose (frozen, gliding) while the baked PC 'Aldric' animated on the same client code.
Root cause
A runtime actor is resolved by its in-game NAME (slugified):
SpawnActor(t.id, t.name, ...)→ResolveAsset(Slugify(name), "character"). A rogue's name (gauge/sable) matches no registry asset or alias, so it falls through todefaults.character=template_human=Assets/painterly/models/hero.fbx.hero.fbxis the one character model flaggedhumanoid:false/needs_remodel(avatarisHuman=false) with no resolvable idle clip:CombatSurfaceClient.cs:1476), andPlayIdleGraphgets anullclip (ResolveIdleClip→CombatSurfaceClient.cs:1876finds nothing, and the donor-idle retarget needsisHuman), and-90pitch stand-up (CombatSurfaceClient.cs:1441) → frozen, lying sideways.'Aldric' works because
aldricaliases to the Humanoidfighter.Fix — a T-pose is impossible at the character floor
Reuses an existing animated asset (
patron_commoner, the generic rigged humanoid the tavern/camp cast already spawn correctly) instead of adding a code branch — the same idiom as the #1423 wren/camp-scout alias fix.data/asset-registry/registry.json— re-pointtemplate_humanoffhero.fbxontopatron_commoner(meshy 9-clip idle inanim_idle.fbx). Every character miss (and__any__) now animates via the per-frame idle-graph fallback every Generic-import character already uses.asset_idstaystemplate_human(no resolver-semantics change).CombatSurfaceClient.cs— the in-code character floor (registry-absent path) now returnspatron_commoner+ its idle moveset instead ofhero.fbx, so even a registry-less build floors to an animated humanoid.asset_registry.py/AssetRegistry.cs— the hardcoded floors in both twins moved offhero.fbxfor parity.Tests
viewer/tests/test_asset_registry.py: 16 pass (11 existing + 5 new anti-T-pose invariants — the Gauge/Sable repros, arbitrary character names, the__any__and in-code floors, and a default/alias-target +model_refcompleteness lint). Run:python3 -m pytest viewer/tests/test_asset_registry.py -q -p no:xdist.Box-side follow-up (no new assets required)
patron_commoner'srigged.fbx/albedo.jpg/anim_idle.fbxare already registry-referenced, so they are already baked into theworldos_actorsbundle — nothing to generate. To ship the fix the box operator must:data/asset-registry/registry.json→ the Unity project rootregistry.json(the fileBuildMacOSPlayer.EnsurePackagedreads,BuildMacOSPlayer.cs:63,68).BuildMacOSPlayer.PackageOnly()(EnsurePackaged) to re-copyregistry.jsonintoStreamingAssets/and rebuild the recompiled client, then rebuild the player.Runtime visual verification (rogue stands + idles + walks, no T-pose) to happen upstream in the sandbox.
Do not merge — orchestrator review.