feat(devtools): add an internal presentation workbench - #150
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ifBars
changed the title
Greptile SummaryThis PR adds an internal developer-only presentation workbench: a console-driven, in-game tool for tuning first-person viewmodel, third-person avatar, and native icon captures without rebuilding a mod. It is kept entirely off the public API surface (verified by the exported-types test) and cleans up all session state on close or scene unload.
Confidence Score: 4/5Safe to merge for the workbench feature itself; one ordering footgun in the new builder methods could silently misconfigure avatar equippables for any mod that calls WithAvatarHeldTransform and WithAvatarHeldVisual as separate steps in the wrong order. The workbench infrastructure is well-structured, internally contained, and the state-restore logic on close is thorough. WithAvatarHeldVisual(provider) unconditionally resets _avatarHeldTransform to null, so a builder chain of .WithAvatarHeldTransform(t).WithAvatarHeldVisual(p) silently discards the configured avatar transform and falls back to the first-person held transform instead — no error is raised and the avatar equippable is built with the wrong pose. The remaining finding is a dead assignment in ScheduleIconCapture that has no runtime impact. Files Needing Attention: S1API/Products/ProductPresentationProfileBuilder.cs — the WithAvatarHeldVisual(Func) overload needs review of the _avatarHeldTransform reset.
|
| Filename | Overview |
|---|---|
| S1API/Products/ProductPresentationProfileBuilder.cs | Adds WithAvatarHeldVisual and WithAvatarHeldTransform builder methods; the no-transform overload of WithAvatarHeldVisual unconditionally resets _avatarHeldTransform, creating a silent order-dependency when the two methods are called independently. |
| S1API/Internal/Rendering/PresentationWorkbenchRuntime.cs | Core 791-line session lifecycle for the workbench: state save/restore, first-person/avatar/icon preview switching, debounced icon capture, and clipboard export. ScheduleIconCapture has a dead assignment in the immediate path but is otherwise correct. |
| S1API/Internal/Rendering/PresentationWorkbenchView.cs | Procedural Unity UGUI construction of the workbench overlay; all event bindings are cleaned up in Dispose, UI layout uses normalized anchor coords, and input parsing uses InvariantCulture. No issues found. |
| S1API/Products/ProductPresentationProfile.cs | Adds AvatarHeldVisualProvider / AvatarHeldTransform properties and TryGetAvatarHeldVisualProvider / TryGetAvatarHeldTransform helpers with correct held-context fallback semantics. |
| S1API/Internal/Products/CustomProductPresentationRuntime.cs | Threads avatar-source resolution through the new ResolveAvatarHeldSource helper and replaces ApplyVisualTransform(Held) with TryGetAvatarHeldTransform; backwards-compatible with existing products that set no avatar-specific fields. |
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
S1API/Products/ProductPresentationProfileBuilder.cs:127-134
**`WithAvatarHeldVisual(provider)` silently erases a previously-set avatar transform**
Calling `.WithAvatarHeldVisual(provider)` unconditionally sets `_avatarHeldTransform = null`, so a builder chain of `.WithAvatarHeldTransform(t).WithAvatarHeldVisual(p)` silently drops `t` with no error or warning. `TryGetAvatarHeldTransform` then falls through to the Held first-person transform, producing incorrect third-person positioning without any indication that the configured transform was lost. The single-argument overload of `WithAvatarHeldVisual` should leave `_avatarHeldTransform` unchanged so that the two methods are truly independent and order-insensitive. The reset should only happen inside `WithAvatarHeldVisual(provider, transform)` to clear a previously-set standalone avatar transform when both are replaced together.
### Issue 2 of 2
S1API/Internal/Rendering/PresentationWorkbenchRuntime.cs:519-533
The `immediate = true` branch of `ScheduleIconCapture` overwrites `_iconRefreshAt` immediately after the ternary sets it to `Time.unscaledTime`, making that first assignment dead code. The ternary should only compute the deferred timestamp for the `false` path.
```suggestion
private void ScheduleIconCapture(bool immediate)
{
if (immediate)
{
_iconRefreshAt = -1f;
CaptureIcon();
}
else
{
_iconRefreshAt = Time.unscaledTime + IconDebounceSeconds;
_view.SetStatus("Icon recapture scheduled.");
}
}
```
Reviews (1): Last reviewed commit: "fix(rendering): preserve avatar preview ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
S1API/Internal/Rendering/ItemPresentationWorkbenchAdapter.cs (2)
96-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse whitespace-aware fallback for
AnimationTrigger, matchingProductPresentationWorkbenchAdapter.
?? "RightArm_Hold_ClosedHand"only coversnull; the product adapter usesstring.IsNullOrWhiteSpace. An empty trigger authored on the equippable will silently propagate here.♻️ Suggested change
- string animationTrigger = - avatarEquippable?.AnimationTrigger ?? - "RightArm_Hold_ClosedHand"; + string animationTrigger = + string.IsNullOrWhiteSpace(avatarEquippable?.AnimationTrigger) + ? "RightArm_Hold_ClosedHand" + : avatarEquippable!.AnimationTrigger;Also applies to: 141-142
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@S1API/Internal/Rendering/ItemPresentationWorkbenchAdapter.cs` around lines 96 - 98, Update the AnimationTrigger fallback in ItemPresentationWorkbenchAdapter to use string.IsNullOrWhiteSpace, matching ProductPresentationWorkbenchAdapter, so null, empty, and whitespace-only authored triggers resolve to "RightArm_Hold_ClosedHand" at both referenced locations.
46-54: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThrowing from a
TryCreate…method breaks the try-pattern contract.
PresentationWorkbenchResolver.TryResolvechains the threeTryCreate*calls; an exception here aborts auto-resolution instead of falling through to the avatar adapter (it only surfaces as a logged error inPresentationWorkbenchRuntime.Open). Returningfalseis more consistent, since the icon context is optional.♻️ Suggested change
- GameObject iconVisual = - firstPersonVisual ?? - avatar!.Provider() ?? - throw new InvalidOperationException( - $"Avatar visual for item '{itemId}' disappeared during discovery."); + GameObject? iconVisual = + firstPersonVisual ?? avatar!.Provider(); + if (iconVisual == null) + return false;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@S1API/Internal/Rendering/ItemPresentationWorkbenchAdapter.cs` around lines 46 - 54, Update the TryCreate path in ItemPresentationWorkbenchAdapter so missing avatar visuals return false instead of throwing InvalidOperationException. Preserve the existing fallback selection and iconProvider behavior when a visual is available, allowing PresentationWorkbenchResolver.TryResolve to continue to the next adapter when no icon context exists.S1API.Tests/Rendering/PresentationWorkbenchTests.cs (1)
45-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider dropping the
MONOMELONgate on the exporter test.
PresentationWorkbenchExporteroutput is backend-agnostic, so this invariant-culture assertion is worth running in every configuration rather than only the Mono one.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@S1API.Tests/Rendering/PresentationWorkbenchTests.cs` around lines 45 - 88, Remove the MONOMELON conditional compilation directives surrounding ExportUsesExistingApisAndInvariantCulture so the exporter test runs in every configuration. Keep the test body and its invariant-culture assertions unchanged.S1API/Internal/Rendering/PresentationWorkbenchView.cs (1)
186-192: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilent no-op on unparsable camera fill.
Unlike the vector rows, an invalid fill value gives the user no feedback. Add a
SetStatusin the else branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@S1API/Internal/Rendering/PresentationWorkbenchView.cs` around lines 186 - 192, Update the Bind callback for _cameraFillField to add an else branch when TryParse fails, calling SetStatus with an appropriate invalid camera-fill message. Preserve the existing updateCameraFill(parsed) behavior for successfully parsed values.S1API/Internal/Rendering/PresentationWorkbenchRuntime.cs (1)
360-362: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
cullingMask = -1fallback renders every layer, including UI/viewmodel.When the
Playerlayer is missing, the preview camera draws everything into the RenderTexture. Prefer a narrow fallback (e.g. default layer only) or skip the avatar camera entirely.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@S1API/Internal/Rendering/PresentationWorkbenchRuntime.cs` around lines 360 - 362, Update the avatar camera culling-mask assignment near _avatarCamera so a missing Player layer does not fall back to -1, which renders all layers. Use a narrow fallback such as the Default layer mask, or skip configuring the avatar camera when LayerMask.NameToLayer("Player") returns an invalid index; preserve the Player-layer mask when it exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@S1API/Internal/Products/CustomProductPresentationRuntime.cs`:
- Around line 940-973: The ResolveAvatarHeldSource method should treat
AvatarHeldVisualProvider failures as optional override failures: log null or
thrown-provider errors, then return the existing heldSource instead of throwing.
Preserve successful provider results and caching, and update both cached-null
and invocation-exception paths consistently.
In `@S1API/Internal/Rendering/PresentationWorkbenchView.cs`:
- Line 469: Update the font lookup in the relevant label-rendering code of
PresentationWorkbenchView to request Unity 2022.3’s built-in "LegacyRuntime.ttf"
instead of "Arial.ttf", preserving the existing dynamic label behavior.
---
Nitpick comments:
In `@S1API.Tests/Rendering/PresentationWorkbenchTests.cs`:
- Around line 45-88: Remove the MONOMELON conditional compilation directives
surrounding ExportUsesExistingApisAndInvariantCulture so the exporter test runs
in every configuration. Keep the test body and its invariant-culture assertions
unchanged.
In `@S1API/Internal/Rendering/ItemPresentationWorkbenchAdapter.cs`:
- Around line 96-98: Update the AnimationTrigger fallback in
ItemPresentationWorkbenchAdapter to use string.IsNullOrWhiteSpace, matching
ProductPresentationWorkbenchAdapter, so null, empty, and whitespace-only
authored triggers resolve to "RightArm_Hold_ClosedHand" at both referenced
locations.
- Around line 46-54: Update the TryCreate path in
ItemPresentationWorkbenchAdapter so missing avatar visuals return false instead
of throwing InvalidOperationException. Preserve the existing fallback selection
and iconProvider behavior when a visual is available, allowing
PresentationWorkbenchResolver.TryResolve to continue to the next adapter when no
icon context exists.
In `@S1API/Internal/Rendering/PresentationWorkbenchRuntime.cs`:
- Around line 360-362: Update the avatar camera culling-mask assignment near
_avatarCamera so a missing Player layer does not fall back to -1, which renders
all layers. Use a narrow fallback such as the Default layer mask, or skip
configuring the avatar camera when LayerMask.NameToLayer("Player") returns an
invalid index; preserve the Player-layer mask when it exists.
In `@S1API/Internal/Rendering/PresentationWorkbenchView.cs`:
- Around line 186-192: Update the Bind callback for _cameraFillField to add an
else branch when TryParse fails, calling SetStatus with an appropriate invalid
camera-fill message. Preserve the existing updateCameraFill(parsed) behavior for
successfully parsed values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 68d4a1ed-bca2-4561-9bf1-ba6b7508e7e5
📒 Files selected for processing (21)
S1API.Tests/Products/ProductApiCompatibilityTests.csS1API.Tests/Products/ProductPresentationProfileApiCompileFixture.csS1API.Tests/Products/ProductPresentationProfileBuilderTests.csS1API.Tests/Rendering/PresentationWorkbenchTests.csS1API/Internal/Console/PresentationWorkbenchCommand.csS1API/Internal/Products/CustomProductPresentationRuntime.csS1API/Internal/Products/ProductPresentationWorkbenchAdapter.csS1API/Internal/Rendering/ItemPresentationWorkbenchAdapter.csS1API/Internal/Rendering/PresentationWorkbenchExporter.csS1API/Internal/Rendering/PresentationWorkbenchModel.csS1API/Internal/Rendering/PresentationWorkbenchResolver.csS1API/Internal/Rendering/PresentationWorkbenchRuntime.csS1API/Internal/Rendering/PresentationWorkbenchView.csS1API/Internal/Rendering/PresentationWorkbenchVisualResolver.csS1API/Products/ProductPresentationProfile.csS1API/Products/ProductPresentationProfileBuilder.csS1API/Products/ProductPresentationProfileRegistry.csS1API/S1API.csS1API/docs/generic-custom-products.mdS1API/docs/presentation-workbench.mdS1API/docs/toc.yml
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Review follow-up for
Validation:
|
1 participant
Summary
IconFactoryafter render-ready frames instead of holding the native rig's transient textureDeveloper workflow
No workbench registration code is added to consuming mods.
Context semantics
PlayerInventory.EquipContaineron theViewmodellayer without equipping or changing an inventory slot.AvatarEquippableprefabs retain their scaffold/alignment point, use the game's alignment math, honor trigger-vs-bool animation metadata, and support drag orbit plus wheel zoom.IconFactory.GenerateIconSprite, waits for render-ready frames, retries bounded captures, and presents a durable copied texture. Product profiles retain their exact generated-icon source/settings; ordinary items use their discoverable equippable visual.Copy actions emit existing
ProductPresentationTransform, local transform assignments, orIconFactory.GenerateIconSprite(...)code. No workbench-specific type appears in copied code or the exported assembly.Validation
D:\SteamLibrary\steamapps\common\Schedule I_alternate: passMods1913EE47C3944649283AE6427521912C30903BF3ACEAD5AC57F4BEE54D2708B7Closes #148