Conversation
🚦 CI StatusWindows and Mac built successfully in Unity Cloud.
Warnings not reduced: 12091 => 12285 — remove at least 195 warnings to merge. Warnings/errors in files changed by this PR (18)Lint run · took 28m 12s 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 #34153790084. Full reportPR #10024, run #34153790084 Overall: ✅ no significant changes Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
On demand — comment |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — feat: add cinematic goto departure and landing animations
STEP 2 — Root-cause check
✅ PASS. This is a new feature, not a bug fix. The PR adds cinematic departure and landing animations to the /goto chat command. No symptom-masking or workaround detected.
STEP 3 — Design & integration
✅ PASS.
OWNER SEARCH — GotoTeleportAnimationSystem:
- The animation wraps around the existing teleport flow (
ChatTeleporter→RealmNavigator→TeleportController→PlayerTeleportIntentECS component). - Existing owners searched:
TeleportCharacterSystem(Explorer/Assets/DCL/Character/CharacterMotion/Systems/TeleportCharacterSystem.cs),TeleportPositionCalculationSystem(Explorer/Assets/DCL/Character/CharacterMotion/Systems/TeleportPositionCalculationSystem.cs),InterpolateCharacterSystem. These handle position changes, not cinematic transitions. - No existing lifecycle owner can host this work — the animation is a new presentation layer that coordinates departure visuals → teleport → landing visuals. A new system is justified.
OWNER SEARCH — GotoTeleportAnimation (coordination object):
- Shared between async command (
GoToChatCommand.ExecuteCommandAsync) and ECS system (GotoTeleportAnimationSystem.Update). Both run on the Unity main thread — no threading concern. - The completion-source coordination pattern (command awaits, system resolves) is a known pattern in the codebase.
OWNER SEARCH — GotoTeleportState (ECS component):
- Stored on the player entity via
World.Add(player, new GotoTeleportState(...)). Reference type is justified because it holds Unity object references (AvatarBase,AnimationClip) and aGotoTeleportTrailswith GameObjects.
TEARDOWN / CONSUMPTION TRACE:
| Opener | Mirror | Location |
|---|---|---|
new UniTaskCompletionSource() (Departure) |
TrySetResult() / TrySetCanceled() in Finish |
GotoTeleportAnimation.cs:23→51, System.Update, GotoTeleportAnimation.Finish |
new UniTaskCompletionSource() (Arrival) |
TrySetResult() / TrySetCanceled() in Finish |
GotoTeleportAnimation.cs:42→51, System.Update |
AddCameraInputLock() |
RemoveCameraInputLock() in Restore |
System.cs:167→188 |
BlockInput(kind) |
UnblockInput(kind) in Restore |
System.cs:169→191 |
World.Add(player, StopCharacterMotion) |
World.Remove<StopCharacterMotion> in Restore (if owned) |
System.cs:146→194 |
new GameObject("Goto teleport trails") |
SafeDestroy(root) in Dispose |
GotoTeleportTrails.cs:21→47 |
new Material(shader) |
SafeDestroy(material) in Dispose |
GotoTeleportTrails.cs:22→48 |
Shader.SetGlobalVector(EFFECT_ID, ...) |
Reset to Vector4.zero in Restore + OnDispose |
System.cs:55,175 |
All openers have matching teardown. ✅
Structural change safety (CLAUDE.md §5):
Begin(): structural changes (World.Add<StopCharacterMotion>,AddOrGet<CharacterEmoteIntent>) onplayerhappen beforeref CameraComponentoncameraentity — different entities, safe. ✅Restore():GotoTeleportEmote.Restore()doesRemove<CharacterEmoteIntent>onplayerafter writing throughref CharacterEmoteComponent(write completes before the Remove). Thenref CameraComponent/ref InputMapComponenton different entities. FinalRemove<StopCharacterMotion>onplayeris last. ✅
STEP 4 — Member audit
GotoTeleportAnimation public API:
IsRequested— 3 consumers (BeginAsync, Finish, System.Update). Guards concurrent teleports. ✅CancellationToken— 2 consumers (BeginAsync, System.Update). Lets system check cancellation. ✅Departure/Arrival— 4+ consumers each. The completion sources the system resolves. Required by the coordination protocol. ✅BeginAsync/ArriveAsync/Finish— the command-side API. Each has 1-3 consumers. ✅
No single-use intermediates, no absent-≠-false conflation, no redundant guards.
STEP 5 — Line-level findings
All findings are P2 (minor). See inline comments for suggestions.
| # | Sev | File | Description |
|---|---|---|---|
| 1 | P2 | GotoTeleportTrails.cs:60 |
material.SetFloat("_Intensity", ...) uses string lookup per frame — cache PropertyToID |
| 2 | P2 | GotoTeleportTrails.cs:32–34 |
Inline color/width literals should be named constants (code-style-guidelines § constants first) |
| 3 | P2 | GotoTeleportPresentation.cs:30,85 |
Camera framing magic numbers should be named constants |
| 4 | P2 | GotoTeleportAnimationSystem.cs:56 |
Resources.Load<Shader> result not null-checked — null shader → broken material |
Security review: No issues. No secrets committed, no user-controlled input reaches asset loading (hardcoded shader name), global shader properties are gated by avatar buffer index (_lastAvatarVertCount == _DCLTeleportAvatar), input blocking is ref-counted and restored in all code paths (including exceptions and disposal).
STEP 6 — Complexity
COMPLEX — New ECS system with component lifecycle, shader pipeline integration across both avatar shader families (Toon + Facial Features), camera/input control, async coordination via completion sources, and assembly boundary changes across 37 files.
STEP 7 — QA assessment
QA_REQUIRED: YES — Changes affect user-visible behavior (new teleport animations), modify runtime code (avatar shaders, camera, input handling), and touch the rendering pipeline.
STEP 8 — Non-blocking warnings
None. Main scene not modified.
STEP 9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: New ECS system with component lifecycle, shader pipeline integration, camera/input control, async coordination, and assembly boundary changes across 37 files.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
| root.transform.position = origin; | ||
| float launch = Mathf.SmoothStep(0f, 1f, Mathf.InverseLerp(1.35f, 2.6f, elapsed)); | ||
| float alpha = Mathf.SmoothStep(0f, 1f, elapsed / 0.6f) * (1f - Mathf.InverseLerp(2.5f, 2.8f, elapsed)); | ||
| material.SetFloat("_Intensity", alpha); |
There was a problem hiding this comment.
[P2] Per-frame string lookup. material.SetFloat("_Intensity", ...) performs a string-to-ID lookup every frame during the animation. Cache the property ID in a static field, matching the pattern already used in GotoTeleportPresentation (CLAUDE.md §4: allocation-free Update).
Add a static field at the top of the class and use it here:
| material.SetFloat("_Intensity", alpha); | |
| material.SetFloat(INTENSITY_ID, alpha); |
Also add this field alongside the existing constants:
private static readonly int INTENSITY_ID = Shader.PropertyToID("_Intensity");| line.widthMultiplier = i % 3 == 0 ? 0.045f : 0.018f; | ||
| line.startColor = new Color(0.05f, 0.6f, 1f, 0f); | ||
| line.endColor = new Color(0.3f, 1f, 1f, 1f); |
There was a problem hiding this comment.
[P2] Inline color/width literals. These visual constants should be named constants declared at the top of the type (code-style-guidelines § member ordering: consts first).
| line.widthMultiplier = i % 3 == 0 ? 0.045f : 0.018f; | |
| line.startColor = new Color(0.05f, 0.6f, 1f, 0f); | |
| line.endColor = new Color(0.3f, 1f, 1f, 1f); | |
| line.widthMultiplier = i % 3 == 0 ? WIDE_WIDTH : NARROW_WIDTH; | |
| line.startColor = START_COLOR; | |
| line.endColor = END_COLOR; |
Add these constants at the top of the class:
private const float WIDE_WIDTH = 0.045f;
private const float NARROW_WIDTH = 0.018f;
private static readonly Color START_COLOR = new (0.05f, 0.6f, 1f, 0f);
private static readonly Color END_COLOR = new (0.3f, 1f, 1f, 1f);| Vector3 backward = state.CameraRotation * Vector3.back; | ||
| backward.y = 0f; | ||
| if (backward.sqrMagnitude < 0.01f) backward = Vector3.back; | ||
| return state.Origin + (Vector3.up * 1.2f) + (backward.normalized * 4.5f); |
There was a problem hiding this comment.
[P2] Camera framing magic numbers. The framing offset values (1.2f up, 4.5f back) define the cinematic camera setup and should be named constants at the top of the class (code-style-guidelines § constants first). Same for the -82f sky-look angle at lines 30 and 48.
| return state.Origin + (Vector3.up * 1.2f) + (backward.normalized * 4.5f); | |
| return state.Origin + (Vector3.up * FRAMING_HEIGHT) + (backward.normalized * FRAMING_DISTANCE); |
Add these constants alongside EFFECT_TIMELINE_DURATION:
private const float FRAMING_HEIGHT = 1.2f;
private const float FRAMING_DISTANCE = 4.5f;
private const float SKY_LOOK_PITCH = -82f;And update lines 30 and 48: Quaternion.Euler(SKY_LOOK_PITCH, ...)
| Shader shader = Resources.Load<Shader>("GotoTeleportTrail"); | ||
| World.Add(player, new GotoTeleportState(new GotoTeleportTrails(shader))); |
There was a problem hiding this comment.
[P2] Missing null-check on Resources.Load. If the shader fails to load (e.g. missing from the Resources folder after a refactor), shader is null and new Material(null) produces a broken material with a Unity error. Add a defensive check.
| Shader shader = Resources.Load<Shader>("GotoTeleportTrail"); | |
| World.Add(player, new GotoTeleportState(new GotoTeleportTrails(shader))); | |
| Shader shader = Resources.Load<Shader>("GotoTeleportTrail"); | |
| if (shader == null) { ReportHub.LogError(ReportCategory.CHAT, "GotoTeleportTrail shader not found in Resources"); return; } | |
| World.Add(player, new GotoTeleportState(new GotoTeleportTrails(shader))); |
Note: You'll need to add using DCL.Diagnostics; for ReportHub and choose an appropriate ReportCategory — CHAT or a new category if one fits better.
|
Avatar Preview Renderer — Vercel Preview is ready!
|
2 participants
Pull Request Description
What does this PR change?
Test Instructions
Steps (standard run):
metaforge explorer run XXXX # ← replace with this PR numberExpected result:
Steps (fresh account):
metaforge account create --clear metaforge explorer run XXXX # ← replace with this PR numberExpected result:
Automation (if applicable):
metaforge explorer test XXXXPrerequisites
Test Steps
Additional Testing Notes
Quality Checklist
Code Review Reference
Please review our Branch & PR Standards before submitting. It explains the automated review flow, QA/DEV approval requirements, and what each label does — especially useful for first-time contributors.