Skip to content

fix: Maintain LocalPlayer-related data on all scenes#8837

Open
mikhail-dcl wants to merge 7 commits into
devfrom
feat/seed-local-player-crdt-on-scene-init
Open

fix: Maintain LocalPlayer-related data on all scenes#8837
mikhail-dcl wants to merge 7 commits into
devfrom
feat/seed-local-player-crdt-on-scene-init

Conversation

@mikhail-dcl
Copy link
Copy Markdown
Collaborator

@mikhail-dcl mikhail-dcl commented May 20, 2026

Fixes decentraland/creator-hub#264

Problem

SDKProfile is propagated to the scene with 1 frame delay, making the following code fail onStart:

      const playerData = PlayerIdentityData.getOrNull(userEntity)
      const avatarData = AvatarBase.getOrNull(userEntity)
      const wearablesData = AvatarEquippedData.getOrNull(userEntity)

Solution

Make the whole set of Avatar-related components available on the systems' Initialize which is called before the scen starts. It's right the same flow seen in WriteMainPlayerTransformSystem

Sensitive changes

  • User-related data now will be available for the scene even if it's not current
  • To make it symmetric if the user profile is updated the corresponding data will be pushed to the scene as well (even if it's not current)
  • The grey zone existed before: TransformComponent was available for the player entity right away whereas PlayerIdentityData, AvatarBase and AvatarEquippedData were not available until the player steps on the current scene

P.S. Behavior for remote peers has not changed.

How to QA

import { getPlayer } from '@dcl/sdk/src/players'

export function main() {

	let myPlayer = getPlayer()

	if (myPlayer) {
		console.log('Name : ', myPlayer.name)
		console.log('UserId : ', myPlayer.userId)
	}
}
  • Execute the scene with the given script, you should see data properly logged.
  • Perform regression on the scenes which are relying on the players' data

…e lifecycle

- Local Player is propagated on Initialize from the scene world so it's available on `onStart`
- Local Player data updated in every scene even if it's not current
@mikhail-dcl mikhail-dcl requested review from a team as code owners May 20, 2026 17:55
@github-actions github-actions Bot requested a review from anicalbano May 20, 2026 17:55
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 20, 2026

@claude

This comment has been minimized.

@m3taphysics
Copy link
Copy Markdown
Collaborator

PR #8837, run #26182654059

Builds: Windows change, Windows baseline, macOS change, macOS baseline

Framework 13 i7

Metric Change Baseline Delta Improvement
Samples 2700 2700
CPU average 33.3 ms 33.3 ms -0.0 ms 0.0%
CPU 1% worst 33.9 ms 34.0 ms -0.1 ms 0.2%
CPU 0.1% worst 38.2 ms 37.2 ms 1.0 ms -2.6%
GPU average 7.8 ms 7.9 ms -0.1 ms 1.4%
GPU 1% worst 19.2 ms 18.6 ms 0.6 ms -3.4% 🔴
GPU 0.1% worst 25.4 ms 23.5 ms 1.9 ms -8.1% 🔴

@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.

@claude

This comment has been minimized.

@mikhail-dcl mikhail-dcl changed the title Fix: Maintain LocalPlayer-related data on all scenes fix: Maintain LocalPlayer-related data on all scenes May 21, 2026
{
bool newSceneIsValid = scenesCache.TryGetByParcel(characterTransform.Transform.ParcelPosition(), out ISceneFacade currentScene)
&& currentScene.SceneStateProvider.State == SceneState.Running
&& currentScene.SceneStateProvider.State.Value() is SceneState.Running or SceneState.Starting
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failing tests not updated for this behavior change.

SceneState.Starting is now accepted as valid for assignment, but two pre-existing tests assert the opposite behavior and are not updated by this PR:

  1. NotAssignPlayerWhenSceneIsStarting (lines 293–313) — asserts AssignedToScene = false when the scene is Starting, but this code will now set it to true. Both isMainPlayer = true and isMainPlayer = false variants fail.

  2. AssignPlayerWhenSceneTransitionsFromStartingToRunning (lines 315–346) — the first Update() tick (scene still Starting) now assigns the player, so Assert.IsFalse(playerCRDTEntity.AssignedToScene) at line 334 fails.

These tests need to be updated to cover the new semantics. For NotAssignPlayerWhenSceneIsStarting, the test should be renamed and updated to verify that the player is assigned when the scene is Starting. For AssignPlayerWhenSceneTransitionsFromStartingToRunning, the intermediate assertion after the first tick should be removed or changed to IsTrue.

Fix this →

mikhail-dcl and others added 2 commits May 21, 2026 12:43
Update tests to expect player assignment during SceneState.Starting

The system now assigns players to scenes in the Starting state,
so tests asserting the opposite are updated to match.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@
…cene-init' into feat/seed-local-player-crdt-on-scene-init
@claude

This comment has been minimized.

@m3taphysics
Copy link
Copy Markdown
Collaborator

PR #8837, run #26220267637

Builds: Windows change, Windows baseline, macOS change, macOS baseline

Framework 13 i7

Metric Change Baseline Delta Improvement
Samples 2699 2700
CPU average 33.3 ms 33.3 ms 0.0 ms -0.0%
CPU 1% worst 34.0 ms 33.7 ms 0.3 ms -0.8%
CPU 0.1% worst 36.9 ms 36.0 ms 0.9 ms -2.6%
GPU average 7.9 ms 7.7 ms 0.1 ms -1.8%
GPU 1% worst 19.2 ms 20.3 ms -1.2 ms 5.8% 🟢
GPU 0.1% worst 25.2 ms 24.5 ms 0.7 ms -3.0% 🔴

@pravusjif pravusjif moved this from Todo to With QA / Awaiting Review in Explorer Alpha May 21, 2026
@pravusjif pravusjif added the enhancement Enhancement of an existing feature label May 21, 2026
@claude

This comment has been minimized.

{
// Local player: PersistentEntities.Player has PlayerSceneCRDTEntity removed, not destroyed
Assert.IsFalse(scene1World.Has<DeleteEntityIntention>(playerCRDTEntity.SceneWorldEntity));
Assert.IsFalse(scene1World.Has<PlayerSceneCRDTEntity>(playerCRDTEntity.SceneWorldEntity));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BLOCKING — test assertion contradicts current implementation.

Commit ea34e551 removed the World.Remove<PlayerSceneCRDTEntity> call from RemovePlayerFromScene for the local player:

// Local Player is never removed from the scene world
if (crdtEntity.Id == SpecialEntitiesID.PLAYER_ENTITY)
    return;

With that early return, PlayerSceneCRDTEntity is never removed from PersistentEntities.Player when the local player leaves a scene. The assertion below will therefore always fail (component is still present):

Assert.IsFalse(scene1World.Has<PlayerSceneCRDTEntity>(playerCRDTEntity.SceneWorldEntity)); // FAILS

Per the PR's stated intent ("user-related data will be available for the scene even if it's not current"), the expected behavior is now that the component persists. Fix: change to Assert.IsTrue and update the comment on line 154 accordingly.

Fix this →

{
// Local player: old scene's persistent player loses PlayerSceneCRDTEntity (not destroyed)
Assert.IsFalse(scene1Facade.EcsExecutor.World.Has<DeleteEntityIntention>(scene1Entity));
Assert.IsFalse(scene1Facade.EcsExecutor.World.Has<PlayerSceneCRDTEntity>(scene1Entity));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BLOCKING — same issue as line 157. RemovePlayerFromScene returns early for local player without removing PlayerSceneCRDTEntity, so scene1Entity (which is scene1Facade.PersistentEntities.Player) still has the component after the player moves to scene2. Assert.IsFalse will fail. Change to Assert.IsTrue and update the comment on line 200 to say the persistent entity retains PlayerSceneCRDTEntity.

@github-project-automation github-project-automation Bot moved this from With QA / Awaiting Review to In Progress in Explorer Alpha May 21, 2026
@m3taphysics
Copy link
Copy Markdown
Collaborator

PR #8837, run #26226730238

Builds: Windows change, Windows baseline, macOS change, macOS baseline

Framework 13 i7

Metric Change Baseline Delta Improvement
Samples 2700 2699
CPU average 33.3 ms 33.3 ms -0.0 ms 0.0%
CPU 1% worst 33.5 ms 33.9 ms -0.4 ms 1.3%
CPU 0.1% worst 33.8 ms 37.6 ms -3.8 ms 10.2% 🟢
GPU average 7.6 ms 7.9 ms -0.3 ms 3.8% 🟢
GPU 1% worst 19.2 ms 19.6 ms -0.4 ms 1.9%
GPU 0.1% worst 25.3 ms 25.1 ms 0.2 ms -0.9%

Copy link
Copy Markdown

@DafGreco DafGreco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✔️ 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
  • [ ✔️] Skybox
  • [ ✔️] Settings

In order to re-create this one :

  1. Open the index of a scene folder
  2. Copy the text and erase the other one inside index
  3. Save the new text
  4. npm i on the src code
  5. npm run start
  6. check that when enteering to the scene the console mentions the name and wallet of the user

Evidence:

20260522-0824-01.1221605.mp4

Fix test assertions for local player PlayerSceneCRDTEntity persistence

Local player PlayerSceneCRDTEntity now persists on the persistent
entity after leaving a scene, matching the RemovePlayerFromScene
early-return behavior from ea34e55.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 22, 2026

Claude finished @mikhail-dcl's task in 4m 7s —— View job


PR Review (Re-review after sync)

  • Gather context (read CLAUDE.md, docs/README.md)
  • Read changed files and full diff
  • Check if previous blocking issues were resolved
  • Verify current state of all changed files
  • Post final review summary

Root-cause check

PASS. getPlayer() returned null in scene main() because SDKProfile / PlayerSceneCRDTEntity were seeded one frame after the JS runtime started. The fix is structural: LocalPlayerCRDTEntityHandlerSystem.Initialize() seeds both components onto PersistentEntities.Player in every scene world before the runtime starts. This addresses the root cause directly.


Previous blocking issue 1 — RESOLVED ✅

Commit 6c2dc08f correctly updated the SceneState.Starting tests:

  • NotAssignPlayerWhenSceneIsStarting → renamed AssignPlayerWhenSceneIsStarting, all assertions inverted to confirm assignment
  • AssignPlayerWhenSceneTransitionsFromStartingToRunning → renamed KeepPlayerAssignedWhenSceneTransitionsFromStartingToRunning, intermediate IsFalse changed to IsTrue

Previous blocking issue 2 — RESOLVED ✅

Commit d175ddef correctly updated the two assertions that were broken by ea34e551's early-return for local player in RemovePlayerFromScene:

  • RemovePlayerCRDTEntityForPlayersLeavingScene(isMainPlayer=true) — line 157 now Assert.IsTrue(scene1World.Has<PlayerSceneCRDTEntity>(...))
  • ChangeSceneOnPlayerMove(isMainPlayer=true) — line 202 now Assert.IsTrue(scene1Facade.EcsExecutor.World.Has<PlayerSceneCRDTEntity>(...))

Both match the new intended behavior: the persistent player entity retains PlayerSceneCRDTEntity even when the local player leaves a scene.


No new blocking issues

The full current diff is clean:

  • LocalPlayerCRDTEntityHandlerSystemInitialize() seeds PlayerSceneCRDTEntity + SDKProfile before JS runtime; Update() re-propagates on IsDirty. System ordering ([UpdateBefore] on WriteSDKAvatarBaseSystem etc.) ensures the seed is visible to writer systems.
  • PlayerCRDTEntitiesHandlerSystemRemovePlayerFromScene early-returns for local player (preserving persistent entity data). Remote-player path is unchanged.
  • WriteSDKAvatarBaseSystemInitialize() calls UpdateAvatarBaseQuery(World, true) with force=true; first Update() may write again because IsDirty is still set, but CRDT is idempotent — no impact.
  • MultiplayerPluginLocalPlayerCRDTEntityHandlerSystem is now wired first in InjectToWorld, ensuring its Initialize() runs before the writer systems'.
  • Test coverageSeedLocalPlayerCRDTSystemShould (new) covers the seeding path and the no-profile early exit. All PlayerCRDTEntitiesHandlerSystemShould cases are consistent with current implementation.

Non-blocking observations (unchanged from previous review)

CharacterDataPropagationUtility.PropagateGlobalPlayerToScenePlayer is dead production code (only called from PlayerProfileDataPropagationSystemShould test). It uses an unguarded World.Add for PlayerSceneCRDTEntity — if it ever acquired a production caller after Initialize() runs, it would throw. No immediate risk since it has no production callers, but the method could be removed or converted to AddOrSet to prevent future confusion.


REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches cross-world ECS access (global ↔ scene propagation via PersistentEntities), CRDT bridge seeding order, plugin/DI wiring, scene lifecycle state machine, and system execution ordering.
QA_REQUIRED: YES

@m3taphysics
Copy link
Copy Markdown
Collaborator

PR #8837, run #26280529239

Builds: Windows change, Windows baseline, macOS change, macOS baseline

Framework 13 i7

Metric Change Baseline Delta Improvement
Samples 2700 2700
CPU average 33.3 ms 33.3 ms 0.0 ms -0.0%
CPU 1% worst 33.8 ms 33.7 ms 0.1 ms -0.2%
CPU 0.1% worst 37.1 ms 37.3 ms -0.3 ms 0.7%
GPU average 8.0 ms 8.0 ms -0.0 ms 0.5%
GPU 1% worst 19.7 ms 18.6 ms 1.1 ms -5.8% 🔴
GPU 0.1% worst 29.2 ms 25.0 ms 4.2 ms -16.8% 🔴

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Enhancement of an existing feature

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

getPlayer() function not available on first tick

4 participants