Skip to content

fix: support for RAW GLTF emotes#8377

Closed
pravusjif wants to merge 7 commits into
devfrom
fix/lsd-scene-emotes
Closed

fix: support for RAW GLTF emotes#8377
pravusjif wants to merge 7 commits into
devfrom
fix/lsd-scene-emotes

Conversation

@pravusjif

@pravusjif pravusjif commented Apr 21, 2026

Copy link
Copy Markdown
Member

WHY

Currently the Explorer support for playing RAW GLTF emotes is broken, so these 2 features don't work at all:

  • Preview of unreleased Builder collection emotes
  • Playing of scene-emotes during Local Scene Development mode

WHAT

  • Brought back legacy animation support for RAW GLTF emotes
  • Refactors to solve problems with the usage of unreleased Builder emotes

TEST INSTRUCTIONS

TEST 1: Scene-emotes

  1. Clone the sdk7-test-scenes repo and enter this test scene
  2. In the scene root folder open a terminal and run npm i and then npm run start
  3. Close the Explorer that auto-opened. Leave the scene running in the console/terminal.
  4. Download the build from this PR and open it connected to the local scene (keep in mind its position is 80,-1): https://github.com/decentraland/unity-explorer/wiki/How-to-connect-to-a-local-scene-(Unity-Editor---Custom-Build---Latest-Released-Build)#connecting-a-custom-build-to-the-scene
  5. Once you enter the scene go one by one clicking on the emote cubes and confirm that:
  • The first cube (the cosest to the spawn point) plays the normal Dance emote
  • The 2nd cube plays the "building a snowball" scene emote
  • The 3rd cube DOESN'T play (if you open the console you will see an error regarding the naming conventions, that's OK)
  • The 4th cube plays the love potion emote
  1. Confirm that you can interrupt any of the emotes by either playing another emote or moving the avatar with your input
Screen.Recording.2026-04-22.at.9.45.01.PM.mp4

TEST 2: Preview of unreleased Builder emotes

Follow the test instructions of this old PR but using the current PR builds

Screen.Recording.2026-04-22.at.9.40.47.PM.mp4

@pravusjif pravusjif added bug Something isn't working 1-high Severely degrades primary features. Workaround may exist but user experience is very poor. labels Apr 21, 2026
@decentraland decentraland deleted a comment from github-actions Bot Apr 21, 2026
@decentraland decentraland deleted a comment from github-actions Bot Apr 21, 2026
@pravusjif pravusjif added the force-build Used to trigger a build on draft PR label Apr 21, 2026
@github-actions

github-actions Bot commented Apr 21, 2026

Copy link
Copy Markdown
Contributor

@pravusjif pravusjif moved this from Todo to In Progress in Explorer Alpha Apr 22, 2026
@pravusjif pravusjif changed the title fix: RAW GLTF emotes fix: support for RAW GLTF emotes Apr 22, 2026
@pravusjif pravusjif added force-build Used to trigger a build on draft PR and removed force-build Used to trigger a build on draft PR labels Apr 22, 2026
@pravusjif pravusjif added force-build Used to trigger a build on draft PR and removed force-build Used to trigger a build on draft PR labels Apr 22, 2026
@pravusjif
pravusjif marked this pull request as ready for review April 22, 2026 19:48
@pravusjif
pravusjif requested review from a team as code owners April 22, 2026 19:48
@claude

claude Bot commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

Claude finished @pravusjif's task in 5m 29s —— View job


PR Review

  • Gather context (reading CLAUDE.md and docs)
  • Analyze PR diff
  • Review key changed files
  • Post inline comments for specific issues
  • Post final review summary

Root-cause assessment

This PR fixes RAW GLTF emotes by restoring the legacy animation path for two features: Local Scene Development scene-emotes and Builder Collection preview emotes. The root cause was that the Animation component (attached by GLTFast when loading without Mecanim mode) was never handled in EmotePlayer or AvatarBase. The diff adds a proper legacy execution path, gated behind a legacyAnimationsEnabled flag. This is a root-cause fix, not a workaround.


Blocking issues

PlayingEmoteDuration NullReferenceException for legacy emotesCharacterEmoteComponent.cs:18-20

For legacy emotes, EmotePlayer.CreateNewEmoteReference explicitly sets animatorComp = null and populates animationComp instead. However PlayingEmoteDuration accesses CurrentEmoteReference.animatorComp!.speed in the truthy branch (which fires whenever avatarClip is non-null). This crashes at runtime.

The crash path is CharacterPreviewControllerBase.PlayEmoteAndAwaitItAsync:319:

await UniTask.Delay((int)(emoteComponent.PlayingEmoteDuration * 1000), ct);

This is reached during emote-wheel preview when builderCollectionsPreview = true and the loaded emote is a legacy clip — exactly the case this PR enables. See inline comment for the fix.


No other blocking issues found

The overall architecture is sound:

  • EmotePlayer legacy pathPlayLegacyEmote correctly disables the Mecanim animator before playing, and StopEmote in CharacterEmoteSystem calls StopLegacyAnimation() before resetting animator triggers, ensuring the animator is re-enabled on emote stop.
  • IsLegacyAnimationPlaying guard — All Mecanim setters in AvatarBase correctly short-circuit when legacy is playing, preventing Mecanim from stomping the legacy pose each frame.
  • AvatarShapeVisibilitySystem change — The && !avatarView.IsLegacyAnimationPlaying guard prevents re-enabling the Mecanim animator mid-legacy-emote. Correct.
  • GetSceneEmoteFromLocalSceneIntention mask-aware loading — Full-body → mecanimAnimationClips: false (legacy), masked → mecanimAnimationClips: true (Mecanim). Equality includes Mask to prevent promise-cache cross-contamination. Correct.
  • ResolveBuilderEmotePromisesSystem deletion — Replaced cleanly by the static BuilderEmoteAssetPromiseFactory, which is simpler, easier to test, and properly gated in AfterBuilderItemsLoaded. The 7 new test classes provide solid coverage.
  • TryParseSceneEmoteURN — The scene-name-prefix matching approach handles scene names that contain hyphens (LSD scenes), with a first-dash fallback for deployed realm format where scene IDs don't contain hyphens.
  • CancelEmotes legacy tear-down — Non-looping legacy emotes self-terminate (!avatarView.IsLegacyAnimationPlaying); forced stops (StopEmote = true) and HiddenPlayerComponent are checked first. Correct.

REVIEW_RESULT: FAIL ❌
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches avatar rendering pipeline (legacy Animation vs Mecanim duality), EmotePlayer, CharacterEmoteComponent, AvatarBase IAvatarView contract, ECS system registration, and async emote loading from multiple sources.
QA_REQUIRED: YES

@pravusjif pravusjif moved this from In Progress to With QA / Awaiting Review in Explorer Alpha Apr 22, 2026
@claude

claude Bot commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

Claude finished @pravusjif's task in 2m 21s —— View job


PR Review

  • Gather context (reading CLAUDE.md and docs)
  • Analyze PR diff (checking fix for previously flagged NullReferenceException)
  • Review key changed files
  • Post final review summary

Root-cause assessment

This PR fixes RAW GLTF emotes by restoring the legacy Animation path for two features: Local Scene Development scene-emotes and Builder Collection preview emotes. The root cause was that GLTFast attaches a legacy Animation component (not a Mecanim Animator) when loading without Mecanim mode, and EmotePlayer/AvatarBase had no handling for that path. The diff adds a proper legacy execution path, gated behind legacyAnimationsEnabled. This is a root-cause fix.


Previous blocking issue — RESOLVED ✅

PlayingEmoteDuration NullReferenceException for legacy emotes — fixed in commit cde8431d.

The property now reads:

public float PlayingEmoteDuration => CurrentEmoteReference?.avatarClip
    ? CurrentEmoteReference.avatarClip.length * (CurrentEmoteReference.animatorComp != null ? CurrentEmoteReference.animatorComp.speed : 1f)
    : 0f;

When animatorComp is null (legacy emote), speed defaults to 1f, correctly returning avatarClip.length. The fix is functionally equivalent to the suggested approach.


No blocking issues found

The overall architecture is sound:

  • CancelEmotes forced-stop orderingwantsToCancelEmote || HiddenPlayerComponent is checked and handled (StopEmote called + early return) at lines 164–168, before the legacy guard at lines 172–177. Forced stops always work correctly.
  • PlayLegacyEmote Mecanim disableavatarView.AvatarAnimator.enabled = false is called before AddOrGetLegacyAnimation, preventing Mecanim from overwriting the legacy pose on the same frame.
  • AvatarBase Mecanim guards — All four Mecanim setters (SetAnimatorFloat, SetAnimatorInt, SetAnimatorBool, SetAnimatorTrigger, and both emote slot setters) correctly short-circuit on IsLegacyAnimationPlaying, preventing Mecanim from stomping the legacy pose each frame.
  • AvatarShapeVisibilitySystem guardshouldPlayFootstepFX && !avatarView.IsLegacyAnimationPlaying prevents re-enabling the Mecanim animator mid-legacy-emote.
  • GetSceneEmoteFromLocalSceneIntention equality — Now includes Mask to prevent promise-cache cross-contamination between full-body and masked scene emotes.
  • BuilderEmoteAssetPromiseFactory — Correctly handles unisex and gendered emotes; the StopLegacyAnimation call in AcquireEmoteReferences ensures clean state before loading a new emote reference.
  • LoadEmotesByPointersSystem sentinel guardIsLSDAsset flag correctly prevents AssetBundle promise creation for LSD emotes whose manifests lack version/build metadata.
  • AfterBuilderItemsLoaded hook — Promise creation happens before the intention entity is consumed, avoiding a race condition with the async caller.

REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches avatar rendering pipeline (legacy Animation vs Mecanim duality), EmotePlayer, CharacterEmoteComponent, AvatarBase IAvatarView contract, ECS system registration, and async emote loading from multiple sources.
QA_REQUIRED: YES

@github-actions

Copy link
Copy Markdown
Contributor

🔍 Claude reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging.

@davidejensen

Copy link
Copy Markdown
Member

Closing this as already merged as part of another PR

@github-project-automation github-project-automation Bot moved this from With QA / Awaiting Review to Done in Explorer Alpha May 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1-high Severely degrades primary features. Workaround may exist but user experience is very poor. bug Something isn't working force-build Used to trigger a build on draft PR

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants