Conversation
decentraland-bot
added
the
ext-contribution
🚦 CI StatusBuild skipped — no changes detected under Waiting for lint to start… Waiting for tests to start… Bare-metal benchmarks run automatically after each successful build; results land in this section. Add the On demand — comment |
|
Slack notification sent to #explorer-ext-contributions for external review. |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review: fix: enable camera controls and local item-only previews
STEP 1 — Context
This PR touches avatar-preview-renderer/, a standalone MonoBehaviour-based WebGL project (ECS rules do not apply). It adds Unity-side handlers for camera RPCs that the wearable-preview JS wrapper already sends (changeCameraPosition, panCamera, changeZoom), a Builder item-only preview mode (type=wearable), and an error-recovery path that replaces the old throw in the reload loop.
12 files changed: 5 source files, 1 README, 4 test files + meta, 2 folder meta.
STEP 2 — Root-cause check: PASS
The PR solves the right problems:
- Camera RPCs: The
wearable-previewwrapper already sendsSetCameraPosition,SetOffset, andSetZoommessages viaSendMessage(confirmed insrc/lib/unity/scene.ts). The Unity renderer lacked handlers — this PR adds them. - Builder item-only view: New feature for the Builder to preview a single local wearable definition in isolation.
- Error recovery: The old
throw;in theReload()catch block left_loading = truepermanently, deadlocking all futureInvokeReload()calls. The new code properly resets_loading = falseand allows recovery.
STEP 3 — Design & integration: PASS
No new lifecycle units are introduced. All new behavior extends existing owners:
- Orbit system →
PreviewCameraController(already owns camera manipulation: FOV zoom, pan, fit) - StopMotion →
DragRotator(already owns rotation state: velocities, auto-rotate, target) - Camera forwarding →
PreviewController(follows the existingPanforwarding pattern throughpreviewUIPresenter) - Item-only Builder view →
LoadForBuilder(extends the existing load path; wearable/avatar loaders already exist)
The dual camera mode (fit-based FOV zoom vs. orbit-based spherical coordinates) is clean: BeginOrbit() lazily initializes from the current camera state, orbit mode bypasses the FOV zoom and pan-offset systems, and ResetFraming clears orbit state on mode switch or reload. Pan correctly branches to move the orbit target when orbiting.
Visibility management (avatarLoader/wearableLoader/glowCatcher GameObjects) is now toggled in both ShowWearableView and Reload. Traced all paths — Marketplace, Builder with/without wearable, other modes — the final visibility state is consistent. The duplication is a minor maintenance concern but not a bug.
STEP 4 — Member audit
| Member | Consumers | Assessment |
|---|---|---|
DragRotator.StopMotion() |
PreviewController.StopAutomaticRotation (1) |
Distinct from ResetRotation — clears velocities/auto-rotate without resetting angles. Not merge material. |
PreviewController.ChangeCameraPosition(Vector3) |
JSBridge.SetCameraPosition, JSBridge.SetZoom (2) |
Forwarding + mode gate. Consistent pattern. |
PreviewController.SetCameraTarget(Vector3) |
JSBridge.SetOffset (1) |
Forwarding + mode gate. Parallels ChangeCameraPosition. |
PreviewCameraController.ShowWearable(bool, PreviewMode) |
PreviewController.ShowWearableView (1) |
Renamed from ShowMarketplaceWearable. Mode param needed for Builder/avatar framing selection. |
PreviewCameraController.ChangeCameraPosition(Vector3) |
PreviewController.ChangeCameraPosition (1) |
Public API for orbit delta. |
PreviewCameraController.SetCameraTarget(Vector3) |
PreviewController.SetCameraTarget (1) |
Public API for absolute orbit target. |
PreviewCameraController.BeginOrbit() / ApplyOrbit() |
Internal (3 call sites each) | Private, well-scoped. |
No single-use merge candidates. All members have appropriate scope.
STEP 5 — Line-level findings
See inline comments below.
[P2] Nested ternary readability — PreviewCameraController.cs:230. Correct operator precedence (C# ternary is right-associative), but the nested ternary is harder to parse at a glance. Parentheses make the intent explicit.
[P2] Defensive radius guard in BeginOrbit — PreviewCameraController.cs:263. If panSubjectDistance were ever set to 0 in the inspector, offset.magnitude would be 0, producing a NaN division on line 265. Practically unreachable (default is 7f and the value is serialized), but ChangeCameraPosition already guards with Mathf.Max(MIN_FRUSTUM_DEPTH, ...) — BeginOrbit should be consistent.
Additional observations (non-blocking)
-
Error recovery leaves
mainCamera.cullingMask = 0: After the catch-blockreturn, the camera renders nothing and the outline-updateenabledflags stay false. The PR description explicitly acknowledges the JS error overlay covers this, and recovery viaInvokeReload()restores everything. Strictly better than the base version (permanent deadlock). -
wearableRotator.EnableAutoRotatebehavioral change: Now explicitly set toconfig.Mode is PreviewMode.Marketplaceinstead of relying on thetruedefault fromAwake(). This is a correctness fix — without it, the wearable would auto-rotate in Builder mode, which conflicts with scripted camera controls. -
Switcher disabled in Builder mode:
EnableSwitchernow requiresconfig.Mode == PreviewMode.Marketplace, which is correct — the Builder item-only view has no meaningful avatar/wearable toggle since the avatar is hidden. -
#nullable enable annotations: Correct gradual-adoption approach.string.Equals(bodyShapeName, ...)is null-safe.EntityDefinition? itemAloneis properly null-checked before use. -
Test design: The reflection-based approach is deliberate — the test assembly avoids depending on
Assembly-CSharpand tests the exact web-bridge interface (method names as strings) that JS calls. The locale test (fr-FR) explicitly verifiesCultureInfo.InvariantCultureparsing. Both tests verify round-trip accuracy. -
Consumer impact:
ShowMarketplaceWearable→ShowWearablerename is internal to the renderer (confirmed: no external consumers). The new camera RPCs match the existingwearable-previewwrapper'sSendMessagecalls exactly (format, naming, semantics).
STEP 6 — Complexity: COMPLEX
Touches camera control system (new orbit mode with spherical coordinates), async error handling in the reload loop, visibility management across preview modes, and a new Builder preview path.
STEP 7 — QA: YES
Runtime code changes affect camera behavior, wearable/avatar visibility, and error recovery in the WebGL preview renderer.
STEP 8 — Non-blocking warnings
None. No Main scene modified.
Security review
Input validation: Camera parsing uses float.TryParse with CultureInfo.InvariantCulture, rejects NaN/Infinity, requires exactly 3 comma-separated values for vectors. Downstream clamping: Beta to [ε, π−ε], Radius to ≥ 0.01, Alpha wraps naturally. No gaps found.
Error exposure: OnError(e.Message) was pre-existing; new error messages are generic (no paths, stack traces, or internal state).
Mode guard: StopAutomaticRotation restricts camera RPCs to Builder/Marketplace modes.
Supply chain: No new dependencies, binaries, CI/workflow changes, or registry modifications.
DEPENDENCY_REVIEW: PASS
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: New orbit-based camera control system, async error recovery rework, and Builder item-only preview path across the avatar-preview-renderer.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by unknown (<@unknown>) via Slack
| } | ||
|
|
||
| public void ShowMarketplaceWearable(bool showWearable) | ||
| public void ShowWearable(bool showWearable, PreviewMode mode) |
There was a problem hiding this comment.
[P2] Readability: The nested ternary is correct (C# ternary is right-associative) but parentheses make the intent immediately obvious.
| public void ShowWearable(bool showWearable, PreviewMode mode) | |
| _active = showWearable ? _wearableFraming : (mode == PreviewMode.Builder ? _builderFraming : _avatarFraming); |
| var cameraTransform = _active.Camera.transform; | ||
| _active.OrbitTarget = cameraTransform.position + cameraTransform.forward * panSubjectDistance; | ||
| var offset = cameraTransform.position - _active.OrbitTarget; | ||
| _active.Radius = offset.magnitude; |
There was a problem hiding this comment.
[P2] Defensive guard: ChangeCameraPosition already floors Radius at MIN_FRUSTUM_DEPTH — BeginOrbit should be consistent. If panSubjectDistance were ever 0 in the inspector, offset.magnitude would be 0 and the division on line 265 would produce NaN.
| _active.Radius = offset.magnitude; | |
| _active.Radius = Mathf.Max(MIN_FRUSTUM_DEPTH, offset.magnitude); |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
STEP 1 — Context & Scope
Reviewed at base 2831b062 / head 28bcf4da. Loaded CLAUDE.md and docs/README.md. The diff touches the avatar-preview-renderer/ project — a standalone MonoBehaviour WebGL renderer. ECS rules do not apply (per CLAUDE.md).
Files changed: 7 (5 source, 1 test, 1 README). ~200 additions / 15 deletions.
STEP 2 — Root-cause check
Problem: The avatar-preview-renderer lacked scripted camera control from JS and Builder-mode item-only previews.
Approach: The diff adds orbit-camera methods callable from JS (SetCameraPosition, SetOffset, SetZoom), builder item-only rendering via type=wearable, and error recovery to release the loader lock after unsupported requests.
The changes address the cause directly — extending the preview API surface. The old throw in the catch block left _loading = true permanently, preventing all future reloads; the new recovery pattern is a strict improvement.
PASS.
STEP 3 — Design & Integration
Orbit camera system: BeginOrbit/ApplyOrbit on CameraFraming is well-placed in PreviewCameraController. It correctly disables auto-refit (HasSubject = false), snapshots FOV, branches pan between orbit and fit modes, and gets fully cleared by ResetFraming on mode/view switches. No new long-lived unit introduced — the orbit state lives as fields on the existing CameraFraming inner class.
Builder item-only view: Reuses the existing WearableLoader and ShowWearableView flow. No new lifecycle owner needed — the wearable is loaded/cleaned up within the existing LoadForBuilder → Reload cycle. The LoadForBuilder return type change (void → bool) cleanly signals whether a standalone item was produced.
Error recovery: The old throw from a coroutine left _loading = true forever. The new pattern reports the error to JS via OnError, releases the lock (_loading = false), and allows the next Reload() to proceed. Correct.
Mode coupling (P2): ShowWearable takes a PreviewMode parameter instead of reading a stored mode. StopAutomaticRotation reads PreviewConfiguration.Instance.Mode directly rather than relying on state the camera controller owns. Both work correctly today because all callers are consistent, but the camera controller cannot self-enforce its invariants. See inline comments.
Teardown trace: No new subscriptions, event hookups, connections, or buffers. StopMotion() clears velocity state — no teardown needed. Orbit state is reset by ResetFraming on mode/view changes. ✓
PASS — no lifecycle duplication, no per-frame reconciliation, no persistent state outside the existing CameraFraming class.
STEP 4 — Member Audit
| New member | Consumers | Assessment |
|---|---|---|
DragRotator.StopMotion() |
PreviewController.StopAutomaticRotation (1) |
Distinct from ResetRotation (zeroes angles + position). StopMotion freezes motion without resetting orientation — correct separation. |
PreviewCameraController.ChangeCameraPosition(Vector3) |
PreviewController (1) → JSBridge (2 paths) |
Three consumers via the delegation chain. Clean public API. |
PreviewCameraController.SetCameraTarget(Vector3) |
PreviewController (1) → JSBridge (1) |
Two consumers. Clean. |
PreviewCameraController.ShowWearable(bool, PreviewMode) |
PreviewController.ShowWearableView (1) |
Replaces old ShowMarketplaceWearable. Mode parameter is a P2 coupling concern (see below). |
PreviewController.StopAutomaticRotation() |
ChangeCameraPosition, SetCameraTarget (2) |
Private helper. Guards camera ops with mode check. |
JSBridge.ParseCameraVector/ParseCameraNumber |
SetCameraPosition, SetOffset, SetZoom (3+) |
Private helpers, well-scoped. |
CameraFraming.InitialLocalRotation |
ResetFraming (1) |
Needed to restore rotation after orbit — mirrors existing InitialLocalPosition. |
CameraFraming.Orbiting/OrbitTarget/Alpha/Beta/Radius |
BeginOrbit, ApplyOrbit, ChangeCameraPosition, SetCameraTarget, Pan, ResetFraming (6+) |
Orbit state on existing class — well-scoped. |
No member audit issues.
STEP 5 — Line-level Review
All findings are P2. See inline comments for details and suggestions.
Additional notes:
- The test assembly
AvatarPreview.Editor.Tests.asmdefuses the legacyoptionalUnityReferencesfield (pre-2019.2 pattern). Modern Unity (6000.x) recommends explicitreferencestoUnityEngine.TestRunner/UnityEditor.TestRunnerwithdefineConstraints: ["UNITY_INCLUDE_TESTS"]. The legacy format still works — low priority. - The test correctly validates the JS bridge's camera API contract via reflection (mirroring the
SendMessageinvocation path real callers use) and exercises locale-independent parsing (fr-FR). The method-name strings are the contract — a rename breaks the test the same way it breaks JS callers. #nullable enable annotations(withoutwarnings) is a valid stepping stone — marks nullable parameters without generating NRT warnings across the rest of the file.- The orbit math (spherical coordinates with ISO physics convention — Beta as polar angle from +Y) round-trips correctly.
SetZoomnegation chain is correct: positive zoom value → negative delta.z → radius decreases → camera closer → zooms in. string.Equals(bodyShapeName, ...)replacingbodyShapeName.Equals(...)is a correct null-safety improvement matching the newstring?annotation.LoadForBuildervalidation (base64Entities.Length != 1 || base64Emote != null) correctly rejects emotes, multi-entity, and zero-entity item-only requests.
STEP 6 — Complexity
COMPLEX — Adds orbit camera coordinate system with spherical math, modifies error recovery in the async reload loop, changes camera/input handling across JSBridge, PreviewCameraController, and PreviewController.
STEP 7 — QA Assessment
QA_REQUIRED: YES — Changes affect runtime camera rendering behavior, item visibility toggling, and error recovery flow.
STEP 8 — Non-blocking Warnings
No Main.unity scene modified. No warnings.
DEPENDENCY_REVIEW: PASS
Security review: No dependency, binary, or lockfile changes. No new native plugins. Test assembly correctly scoped to Editor-only (includePlatforms: ["Editor"]). No secrets or credentials. No automation or execution hook changes. Input validation rejects NaN/Infinity and uses CultureInfo.InvariantCulture.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Adds orbit camera system with spherical coordinates, modifies async error recovery, changes input handling across three files
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
| [UsedImplicitly] | ||
| public void SetCameraPosition(string value) => previewController.ChangeCameraPosition(ParseCameraVector(value)); | ||
|
|
||
| [UsedImplicitly] | ||
| public void SetOffset(string value) => previewController.SetCameraTarget(ParseCameraVector(value)); | ||
|
|
||
| [UsedImplicitly] | ||
| public void SetZoom(string value) => previewController.ChangeCameraPosition(new Vector3(0f, 0f, -ParseCameraNumber(value))); |
There was a problem hiding this comment.
[P2] Camera bridge methods let ArgumentException propagate unhandled through SendMessage, unlike SetSpringBonesParams (line 98) which catches and logs. The JS caller receives no OnError callback on bad input and cannot distinguish success from failure.
| [UsedImplicitly] | |
| public void SetCameraPosition(string value) => previewController.ChangeCameraPosition(ParseCameraVector(value)); | |
| [UsedImplicitly] | |
| public void SetOffset(string value) => previewController.SetCameraTarget(ParseCameraVector(value)); | |
| [UsedImplicitly] | |
| public void SetZoom(string value) => previewController.ChangeCameraPosition(new Vector3(0f, 0f, -ParseCameraNumber(value))); | |
| [UsedImplicitly] | |
| public void SetCameraPosition(string value) | |
| { | |
| try { previewController.ChangeCameraPosition(ParseCameraVector(value)); } | |
| catch (Exception e) { Debug.LogError($"[Camera] SetCameraPosition: {e.Message}"); } | |
| } | |
| [UsedImplicitly] | |
| public void SetOffset(string value) | |
| { | |
| try { previewController.SetCameraTarget(ParseCameraVector(value)); } | |
| catch (Exception e) { Debug.LogError($"[Camera] SetOffset: {e.Message}"); } | |
| } | |
| [UsedImplicitly] | |
| public void SetZoom(string value) | |
| { | |
| try { previewController.ChangeCameraPosition(new Vector3(0f, 0f, -ParseCameraNumber(value))); } | |
| catch (Exception e) { Debug.LogError($"[Camera] SetZoom: {e.Message}"); } | |
| } |
| var offset = cameraTransform.position - _active.OrbitTarget; | ||
| _active.Radius = offset.magnitude; | ||
| _active.Alpha = Mathf.Atan2(offset.z, offset.x); | ||
| _active.Beta = Mathf.Acos(Mathf.Clamp(offset.y / _active.Radius, -1f, 1f)); |
There was a problem hiding this comment.
[P2] BeginOrbit computes Beta via Acos but does not clamp the result to [POLE_EPSILON, PI - POLE_EPSILON], unlike ChangeCameraPosition (line 244) which does. If the camera is directly above/below the computed orbit target, Beta would be 0 or π, making LookRotation(-offset, Vector3.up) receive a vertical vector parallel to up. Unlikely with current scene layouts but easy to defend.
| _active.Beta = Mathf.Acos(Mathf.Clamp(offset.y / _active.Radius, -1f, 1f)); | |
| _active.Beta = Mathf.Clamp(Mathf.Acos(Mathf.Clamp(offset.y / _active.Radius, -1f, 1f)), POLE_EPSILON, Mathf.PI - POLE_EPSILON); |
| public void ShowWearable(bool showWearable, PreviewMode mode) | ||
| { | ||
| _active = showWearable ? _wearableFraming : _avatarFraming; | ||
| _active = showWearable ? _wearableFraming : mode == PreviewMode.Builder ? _builderFraming : _avatarFraming; |
There was a problem hiding this comment.
[P2] ShowWearable accepts a PreviewMode parameter that could theoretically diverge from the mode set via SetMode. Similarly, StopAutomaticRotation in PreviewController reads PreviewConfiguration.Instance.Mode rather than state the camera controller owns. Consider storing the mode in a _currentMode field (set in SetMode) and using it here — this lets the camera controller self-enforce its invariants and removes the parameter.
| public void ShowWearable(bool showWearable, PreviewMode mode) | |
| { | |
| _active = showWearable ? _wearableFraming : _avatarFraming; | |
| _active = showWearable ? _wearableFraming : mode == PreviewMode.Builder ? _builderFraming : _avatarFraming; | |
| public void ShowWearable(bool showWearable) | |
| { | |
| _active = showWearable ? _wearableFraming : _currentMode == PreviewMode.Builder ? _builderFraming : _avatarFraming; |
Companion change in SetMode:
private PreviewMode _currentMode;
public void SetMode(PreviewMode mode)
{
_currentMode = mode;
// ... existing body
}| if (_shouldReload) continue; | ||
|
|
||
| _loading = false; | ||
| previewUIPresenter.ShowLoader(false); | ||
| return; |
There was a problem hiding this comment.
[P2] The error exit resets _loading and hides the loader but leaves mainCamera.cullingMask = 0 (set at line 184), so the renderer shows a black screen until the next Reload(). Restoring the mask here lets the previous content remain visible rather than going entirely blank.
| if (_shouldReload) continue; | |
| _loading = false; | |
| previewUIPresenter.ShowLoader(false); | |
| return; | |
| if (_shouldReload) continue; | |
| _loading = false; | |
| mainCamera.cullingMask = -1; | |
| previewUIPresenter.ShowLoader(false); |
|
Avatar Preview Renderer — Vercel Preview is ready!
|
2 participants
Summary
Make the existing camera RPCs move the Unity preview and support
type=wearablefor a single local Builder upload. The isolated view hides the avatar, and an unsupported request releases the loader so another upload can recover.What could break
Camera commands stop automatic subject rotation. Check manual dragging and Marketplace avatar/item switching after camera changes.
How to test
type=wearable, thentype=avatar: expect an isolated item, then the worn item. An emote requested as item-only must report an error and allow the next valid upload.Validated a local Unity Web build with the validator's capture probe: repeatable camera views, reversible zoom, absolute pan, isolated wearable, paused emote samples and engine recovery after rejection. The current iframe wrapper retains a stale error overlay after recovery; that wrapper UI is a separate follow-up.