hotfix: revert avatar shape and admin filtering#8385
Conversation
This reverts commit 8267afb.
|
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. |
|
New build in progress, come back later! |
This reverts commit e1daec8.
davidejensen
changed the title
|
@claude review |
|
Claude finished @Maurizio-dcl's task in 7m 13s —— View job PR Review
Root-cause assessmentProblem: LSD (Local Scene Development) scene reloads permanently freeze the avatar. Root cause: The Blocking Issues1. Non-looping legacy emotes never clean up → Mecanim Animator permanently disabled (LSD mode)Location:
Net effect: after a non-looping scene emote plays to completion in LSD mode, the avatar's Mecanim Animator stays permanently disabled. Idle, walk, and run animations stop working. The avatar appears frozen until a teleport or scene reload. Scope: LSD mode only, non-looping emotes only. Looping emotes are correctly cancelled by movement input (since Fix: In Non-Blocking Warnings
SummaryThe PR correctly reverts the two features and makes the targeted REVIEW_RESULT: FAIL ❌ |
Maurizio-dcl
added
force-build
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| public AudioSource? audioSource; | ||
|
|
||
| public void Initialize(AnimationClip? animationClip, AnimationClip? propClip, Animator? animatorComp, int propClipHash) | ||
| public void Initialize(AnimationClip? animationClip, AnimationClip? propCLip, Animator? animatorComp, Animation? animationComp, int propClipHash, bool legacy) |
There was a problem hiding this comment.
Parameter propCLip has an accidental capital 'L'. Per CLAUDE.md code standards, parameters must follow camelCase.
| public void Initialize(AnimationClip? animationClip, AnimationClip? propCLip, Animator? animatorComp, Animation? animationComp, int propClipHash, bool legacy) | |
| public void Initialize(AnimationClip? animationClip, AnimationClip? propClip, Animator? animatorComp, Animation? animationComp, int propClipHash, bool legacy) |
| } | ||
|
|
||
| if (!emoteComponent.IsPlayingEmote) | ||
| if (!emoteReference.legacy) |
There was a problem hiding this comment.
Bug: non-looping legacy emotes never clean up — The if (!emoteReference.legacy) guard completely skips the completion-detection logic for legacy animations. When a non-looping legacy emote plays to its end naturally, animationComp.isPlaying becomes false (IsPlayingEmote returns false), but CurrentEmoteReference is never cleared: nothing sets emoteComponent.StopEmote = true, CancelEmotesByMovementInput short-circuits on !IsPlayingEmote, and DisableAnimatorWhenPlayingLegacyAnimations keeps firing because CurrentEmoteReference.legacy is still true. The net result is that the Mecanim AvatarAnimator stays permanently disabled until an explicit cancellation event (teleport, scene reload) occurs.
Fix — add a completion branch for legacy:
| if (!emoteReference.legacy) | |
| if (!emoteReference.legacy) | |
| { | |
| if (!emoteComponent.IsPlayingEmote) | |
| { | |
| avatarView.ResetAnimatorTrigger(AnimationHashes.EMOTE_STOP); | |
| return; | |
| } | |
| int animatorCurrentStateTag = avatarView.GetAnimatorCurrentStateTag(); | |
| bool isOnAnotherTag = animatorCurrentStateTag != AnimationHashes.EMOTE && animatorCurrentStateTag != AnimationHashes.EMOTE_LOOP; | |
| if (isOnAnotherTag) | |
| StopEmote(ref emoteComponent, avatarView); | |
| } | |
| else if (!emoteReference.avatarClip!.isLooping && !emoteComponent.IsPlayingEmote) | |
| { | |
| // Non-looping legacy emote finished naturally — clean up so the Mecanim Animator is not left disabled. | |
| StopEmote(ref emoteComponent, avatarView); | |
| } |
| using DCL.AvatarRendering.AvatarShape.UnityInterface; | ||
| using DCL.ECSComponents; | ||
| using DCL.Optimization.Pools; | ||
| using System; |
There was a problem hiding this comment.
Code smell: static shared mutable array — LEGACY_ANIMATION_CLIPS is a static array used as a scratch buffer inside CreateNewEmoteReference. If a future refactor moves this off the main thread (or the pool factory is called from a background thread), two simultaneous calls would corrupt each other. Consider allocating a local AnimationClip[2] inside the method instead; it is tiny and short-lived. Fix this →
|
|
||
| return !string.IsNullOrEmpty(sceneId) && !string.IsNullOrEmpty(emoteHash); | ||
| } | ||
|
|
There was a problem hiding this comment.
Double blank line — should be a single blank line per code style. Fix this →
DafGreco
left a comment
DafGreco
left a comment
There was a problem hiding this comment.
✔️ PR reviewed and approved by QA on both platforms following instructions playing both happy and un-happy path
Regressions for this ticket had been performed in order to verify that the normal flow is working as expected:
- [✔️ ] Backpack and wearables in world
- [✔️ ] Emotes in world and in backpack
- [✔️ ] Teleport with map/coordinates/Jump In
- [✔️ ] Chat and multiplayer
- [✔️ ] Profile card
- [✔️ ] Camera
- [✔️ ] Skybox
- [ ✔️] Settings
This PR has been tested with @Maurizio-dcl in both platforms and we are ready to ship this one 🚀
Evidence on the following thread
5 participants
Pull Request Description
What does this PR change?
Hotfix aimed at stabilizing local scene development:
feat: avatar masks (#7462)commit755408034. Scene emotes go back to pre-feat: avatar masks #7462 behavior (full-body only, with the known emote flicker, which is addressed separately in this PR and not part of this PR).fix: admin tools filtering (#8154)commit4b04f9edf. Rolls back the admin-tools filtering changes.fix: multiple LSD and asset load issues (#8293)commit65a7f71b7.This PR should expect these outcomes:
88,-13-avatar-masks(or not play at all incentral-plazarunning locally), these are pre-existent issues.nebi.dcl.ethTest Instructions
1. Local Scene Development (Genesis-Plaza-2025)
Test scene: https://github.com/decentraland-scenes/Genesis-Plaza-2025/tree/main/central-plaza
Run the scene locally and verify with this PR's build that:
🟢 Current scene has been reloaded. No error message should appear in chat, from subsequent reloads onwards there might be assets missing and the scene might not load correctly, this is expected and it is heavily dependent on hardware right now./reloadchat command behaves identically.2. Local Scene Development (88,-13-avatar-masks)
Test scene: https://github.com/decentraland/sdk7-test-scenes/tree/main/scenes/88%2C-13-avatar-masks
Run the scene locally and verify with this PR's build that:
🟢 Current scene has been reloaded. No error message should appear in chat, and subsequent reloads must also succeed./reloadchat command behaves identically.🟢 Current scene has been reloadedmessage in chat).3. nebi.dcl.eth
nebi.dcl.eth.🟢 Current scene has been reloaded.4. Genesis Plaza
🟢 Current scene has been reloadedmessage appears in chat.Quality Checklist
nebi.dcl.ethCode Review Reference
Please review our Branch & PR Standards before submitting.