Conversation
decentraland-bot
added
the
ext-contribution
Contributor
🚦 CI StatusWindows and Mac built successfully in Unity Cloud.
Warnings not reduced: 11910 => 11916 — remove at least 7 warnings to merge. Warnings/errors in files changed by this PR (56)…truncated; see the linked run for the full report. All Unity tests passed ✅
Tests time sums the test cases; Job time is the job's wall clock including checkout, licensing and asset import. Slowest tests
Full report: run summary · results + editor logs: editmode · playmode 🏁 Bare-metal benchmark finished — run #35027455890. Full reportPR #10112, run #35027455890 Overall: ✅ no significant changes Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Apple M1
Intel Core i5
On demand — comment |
github-actions
Bot
requested review from
NickKhalow and
alejandro-jimenez-dcl
September 15, 2026 19:40
Contributor
|
Slack notification sent to #explorer-ext-contributions for external review. |
decentraland-bot
left a comment
decentraland-bot
left a comment
Contributor
There was a problem hiding this comment.
Review — PR #10112: feat: vertical orbit in Loading and Backpack character preview
STEP 2 — Root-cause check: PASS
This PR adds a new feature (vertical orbit in character preview), not a fix. Root-cause analysis does not apply.
STEP 3 — Design & integration: PASS
The vertical orbit is added to CharacterPreviewAvatarContainer, which already owns horizontal rotation and FOV in its Update(). The new CameraPivot GameObject is inserted in the prefab hierarchy between the container root and the CameraTarget — the correct placement for a rigid orbit rig under Cinemachine FreeLook. The design reuses the same velocity/inertia/deceleration model as horizontal rotation, with Decelerate() extracted as a shared static helper.
Owner search: The character preview's camera lifecycle is fully owned by CharacterPreviewAvatarContainer (created via CharacterPreviewPlugin → pooled instantiation, disposed in CharacterPreviewController.Dispose()). The rotation and FOV logic already live in Update() on this MonoBehaviour. The new vertical orbit logic belongs here — no existing owner is bypassed, no new lifecycle is introduced.
Teardown trace: No new subscriptions, events, IDisposable fields, or CancellationTokenSource instances are added. cameraPivot is a serialized Transform owned by the prefab hierarchy, not by this class. ResetVerticalRotation() is called on Initialize() and ResetAvatarMovement(), matching the existing reset pattern. Clean.
STEP 4 — Member audit
| Member | Consumers | Verdict |
|---|---|---|
ResetVerticalRotation() |
BackpackCharacterPreviewController (3 sites), Initialize(), ResetAvatarMovement() |
Multiple consumers — justified as public |
VerticalRotationModifier, MaxVerticalAngle, VerticalAngularVelocity |
Written by CharacterPreviewCameraController, consumed by AvatarContainer |
Mirrors existing RotationModifier/RotationInertia/AngularVelocity pattern |
Accelerate() |
Horizontal + vertical velocity | Refactored from inline code, 2 consumers — good extraction |
Decelerate() |
Horizontal + vertical deceleration | New static, 2 consumers — good extraction |
cameraPivot, cameraFloorHeight |
Serialized config consumed by FloorPitchLimit() |
Serialized fields, not derived predicates |
STEP 5 — Line-level findings
See inline comments below.
Categories checked clean:
- R1 perf-alloc:
new Vector2is a struct (stack-allocated).Quaternion.Euler,Mathf.Acos/Atan2/Clampare all value-type.Decelerate()is static with value args. The hot path (Update → UpdateRotation → UpdateCameraPitch → FloorPitchLimit → ApplyCameraPitch) is allocation-free. ✅ - R2 LINQ: No LINQ introduced. ✅
- R3 class-in-struct: No class references stored in structs. ✅
- R4 ECS discipline: Not applicable — this is a MonoBehaviour, not an ECS system. ✅
- R5 Entity by-ref: Not applicable. ✅
- R6 Teardown pairs: No new subscriptions, events, pools, or CTS. ✅
- R7 Nullability: All new fields are non-nullable value types or serialized
Transformreferences (Unity Object lifetime, not C# nullable). ✅ - R8 Root cause: Feature addition, not a fix. ✅
- R9 Logging: No new logging added. ✅
- R10 Catch scope: No new try/catch. ✅
- R11 Async/CT: No new async code. ✅
- R12 Abstraction: No new interfaces or wrappers.
Accelerate/Decelerateare well-scoped utilities with 2 consumers each. ✅ - R13 Reuse: Horizontal rotation primitives reused (same velocity/inertia model, shared
Decelerate/Accelerate). ✅ - R14 Contract honesty: No correlated booleans or partially-initialized objects. ✅
- R15 Dead weight: No commented-out code or unused fields. ✅
- R16/R17 Naming:
cameraPitch,CameraPivot,FloorPitchLimit,ApplyCameraPitch— clear and precise. TheVerticalRotation*prefix family matches the existingRotation*pattern. ✅ - R19 File hygiene: One class per file maintained. No non-Unity files under
Assets/. ✅ - R20 Idiom cluster:
readonlynot applicable to the new auto-properties (they have setters by design — written by the camera controller, read by the container). No string comparisons or path operations. ✅ - R21 MVC: Not a view/controller change. ✅
- R24 Scope: All 11 files are directly related to the character preview vertical orbit feature. No unrelated changes. ✅
STEP 6 — Complexity
COMPLEX — Touches camera/input handling, Cinemachine FreeLook rig integration, prefab hierarchy restructuring, and 11 files with significant logic changes.
STEP 7 — QA assessment
QA_REQUIRED: YES — Runtime camera behavior change affecting Backpack and Loading Screen character previews on both horizontal and vertical axes.
STEP 8 — Non-blocking warnings
No warnings. Main scene not modified.
Merge gates
- R25 — Outstanding comments: No prior review threads. This review's findings are the first round.
- R26 — QA sign-off: Awaiting QA. The PR does not carry
no QA needed. QA should verify: (1) vertical orbit drag on Backpack and Loading Screen, (2) orbit resets on equip/unequip/outfit change, (3) floor limit prevents camera from going under the platform, (4) Passport and Credit Purchase previews do NOT have vertical orbit, (5) inertia stops at pitch limits, (6) test on both Windows and Mac.
STEP 9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches Cinemachine camera rig, prefab hierarchy, input handling, and cross-screen preview settings
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by unknown via Slack
| float floorAbovePivot = cameraFloorHeight - cameraPivot.localPosition.y; | ||
| float phase = Mathf.Atan2(restingOffset.z, restingOffset.y) * Mathf.Rad2Deg; | ||
|
|
||
| return (Mathf.Acos(Mathf.Clamp(floorAbovePivot / radius, -1f, 1f)) * Mathf.Rad2Deg) - phase; |
Contributor
There was a problem hiding this comment.
[P1] R7 Nullability / Correctness — NaN propagation from division by zero in FloorPitchLimit().
When restingOffset.y and restingOffset.z are both zero (camera sits exactly at the pivot), radius is 0f. The expression floorAbovePivot / radius produces NaN (0/0) or ±Infinity. Mathf.Clamp(NaN, -1, 1) returns NaN on Mono/IL2CPP, Mathf.Acos(NaN) returns NaN, and Mathf.Min(MaxVerticalAngle, NaN) returns NaN. Once cameraPitch absorbs NaN through the Mathf.Clamp chain in UpdateCameraPitch(), every subsequent frame stays NaN and the camera rotation is permanently broken.
While the Cinemachine orbits (radius 2.2–6) make radius ≈ 0 unlikely during normal operation, the guard is cheap and prevents a permanent NaN corruption if the camera ever passes through the pivot (e.g. during a zoom snap or a prefab misconfiguration).
Suggested change
| return (Mathf.Acos(Mathf.Clamp(floorAbovePivot / radius, -1f, 1f)) * Mathf.Rad2Deg) - phase; | |
| Vector3 restingOffset = cameraPivot.InverseTransformPoint(camera.transform.position); | |
| float radius = new Vector2(restingOffset.y, restingOffset.z).magnitude; | |
| if (radius < 1e-4f) | |
| return MaxVerticalAngle; | |
| float floorAbovePivot = cameraFloorHeight - cameraPivot.localPosition.y; | |
| float phase = Mathf.Atan2(restingOffset.z, restingOffset.y) * Mathf.Rad2Deg; | |
| return (Mathf.Acos(Mathf.Clamp(floorAbovePivot / radius, -1f, 1f)) * Mathf.Rad2Deg) - phase; |
Contributor
Author
There was a problem hiding this comment.
checked by Claude and deliberately not applied. I traced it rather than taking it: the divisor can't reach zero (the camera hangs ~6 units off the pivot by rig construction, and zoom here drives FOV, not distance); at zero it would be -1f / 0f = -Infinity, which the existing Mathf.Clamp(…, -1f, 1f) already folds into a 40° fallback — the identical result to the proposed guard; and the claimed "permanent NaN corruption" can't occur because Mathf.Clamp returns value untouched when max is NaN. So it would be an unreachable branch, which CLAUDE.md names as a smell in its own right.
Comment on lines
+250
to
+264
| /// <summary> | ||
| /// Swings the camera over and under the avatar, which stays upright on its platform. | ||
| /// The rig reads its target's rotation (LockToTargetNoRoll binding) and the target hangs off this | ||
| /// pivot, so camera and framing point rotate together: the elevation changes and the shot the | ||
| /// screen was tuned for survives. Pitching the target instead would orbit around the framing | ||
| /// point, which sits off the avatar's axis wherever the shot is angled. | ||
| /// </summary> | ||
| private void ApplyCameraPitch() | ||
| { | ||
| cameraPivot.localRotation = Quaternion.Euler(cameraPitch, 0f, 0f); | ||
|
|
||
| // Position and aim damping would leave the camera seconds behind the drag. The orbit is | ||
| // already smoothed by its own inertia, so the rig takes this rotation undamped and lands on | ||
| // the pose exactly. Damping stays in effect on every frame the orbit does not move. | ||
| freeLookCamera.PreviousStateIsValid = false; |
Contributor
There was a problem hiding this comment.
[P2] R23 — Comment narrates Cinemachine's external behavior.
The <summary> block describes what the Cinemachine rig does with the target's rotation ("The rig reads its target's rotation (LockToTargetNoRoll binding)…") and what would happen if a different approach were taken ("Pitching the target instead would orbit around the framing point…"). Per R23, a comment must state only what the annotated code itself does or guarantees — external behavior can change without this code changing, silently turning the comment into a lie.
The inline comment on PreviousStateIsValid is fine — it explains why this code sets that flag.
Suggested trim — keep the summary focused on what ApplyCameraPitch does:
Suggested change
| /// <summary> | |
| /// Swings the camera over and under the avatar, which stays upright on its platform. | |
| /// The rig reads its target's rotation (LockToTargetNoRoll binding) and the target hangs off this | |
| /// pivot, so camera and framing point rotate together: the elevation changes and the shot the | |
| /// screen was tuned for survives. Pitching the target instead would orbit around the framing | |
| /// point, which sits off the avatar's axis wherever the shot is angled. | |
| /// </summary> | |
| private void ApplyCameraPitch() | |
| { | |
| cameraPivot.localRotation = Quaternion.Euler(cameraPitch, 0f, 0f); | |
| // Position and aim damping would leave the camera seconds behind the drag. The orbit is | |
| // already smoothed by its own inertia, so the rig takes this rotation undamped and lands on | |
| // the pose exactly. Damping stays in effect on every frame the orbit does not move. | |
| freeLookCamera.PreviousStateIsValid = false; | |
| /// <summary> | |
| /// Applies the current pitch to the camera pivot, orbiting the camera vertically | |
| /// while preserving each screen's tuned shot. | |
| /// </summary> | |
| private void ApplyCameraPitch() | |
| { | |
| cameraPivot.localRotation = Quaternion.Euler(cameraPitch, 0f, 0f); | |
| // Position and aim damping would leave the camera seconds behind the drag. The orbit is | |
| // already smoothed by its own inertia, so the rig takes this rotation undamped and lands on | |
| // the pose exactly. Damping stays in effect on every frame the orbit does not move. | |
| freeLookCamera.PreviousStateIsValid = false; | |
| } |
Merged
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Left-drag now orbits the preview camera vertically as well as spinning the
avatar. The rig turns rigidly about a new CameraPivot on the avatar's axis, so
the camera and the framing point rotate together and each screen's tuned shot
survives. Pitching the Cinemachine target instead would orbit around the
framing point, which sits off the avatar's axis wherever the shot is angled.
The descent limit is derived rather than fixed: the camera's height traces
radius * cos(pitch + phase) about the pivot, solved for cameraFloorHeight so it
sets down at the avatar's feet instead of shooting from under the platform.
Upward travel is capped by maxVerticalAngle at 40 degrees, and the inertia is
dropped at either limit so the camera stops dead rather than drifting into it.
Position and aim damping would trail the drag by seconds, so the rig takes the
orbit undamped and keeps its damping for panning. The backpack levels the
elevation on equip, unequip and outfit changes; yaw and zoom are left alone.
Enabled on the backpack and the login screen. The passport and credit purchase
previews carry the settings with the flag off.