-
Notifications
You must be signed in to change notification settings - Fork 15
fix: Maintain LocalPlayer-related data on all scenes #8837
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Changes from 5 commits
d18bdc5
5741142
a400ccd
6c2dc08
7d7a2a4
ea34e55
d175dde
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,4 @@ | ||
| using Arch.Core; | ||
| using Utility.Multithreading; | ||
|
|
||
| namespace SceneRunner.Scene | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| using System.Runtime.CompilerServices; | ||
|
|
||
| [assembly: InternalsVisibleTo("DCL.EditMode.Tests")] | ||
| [assembly: InternalsVisibleTo("DCL.PlayMode.Tests")] |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| using Arch.Core; | ||
| using Arch.SystemGroups; | ||
| using CRDT; | ||
| using CrdtEcsBridge.Components; | ||
| using DCL.Diagnostics; | ||
| using DCL.Multiplayer.SDK.Components; | ||
| using DCL.Multiplayer.SDK.Systems.GlobalWorld; | ||
| using DCL.PluginSystem.World; | ||
| using DCL.Profiles; | ||
| using ECS.Abstract; | ||
| using ECS.Groups; | ||
| using SceneRunner.Scene; | ||
|
|
||
| namespace DCL.Multiplayer.SDK.Systems.SceneWorld | ||
| { | ||
| /// <summary> | ||
| /// Propagates the local player's identity and avatar data to every scene world unconditionally, | ||
| /// similar to how <see cref="DCL.CharacterMotion.Systems.WriteMainPlayerTransformSystem" /> handles transforms. | ||
| /// Initialize() seeds PlayerSceneCRDTEntity and SDKProfile onto PersistentEntities.Player before the JS | ||
| /// runtime starts, so that <see cref="WritePlayerIdentityDataSystem" />, <see cref="WriteSDKAvatarBaseSystem" />, | ||
| /// and <see cref="WriteAvatarEquippedDataSystem" /> can flush CRDT messages in their own Initialize(). | ||
| /// Update() keeps the SDKProfile in sync when Profile.IsDirty — unlike | ||
| /// <see cref="PlayerProfileDataPropagationSystem" /> which only targets the assigned scene | ||
| /// and which skips the local player entirely (filtered by PlayerComponent). | ||
| /// Runs in SyncedPreRenderingSystemGroup before the writer systems so SDKProfile | ||
| /// is up-to-date when they flush CRDT. Profile.IsDirty is guaranteed to be set | ||
| /// (set in PresentationSystemGroup, reset later in the same PreRenderingSystemGroup). | ||
| /// </summary> | ||
| [UpdateInGroup(typeof(SyncedPreRenderingSystemGroup))] | ||
| [UpdateBefore(typeof(WritePlayerIdentityDataSystem))] | ||
| [UpdateBefore(typeof(WriteSDKAvatarBaseSystem))] | ||
| [UpdateBefore(typeof(WriteAvatarEquippedDataSystem))] | ||
| [LogCategory(ReportCategory.PLAYER_SDK_DATA)] | ||
| public partial class LocalPlayerCRDTEntityHandlerSystem : BaseUnityLoopSystem | ||
| { | ||
| private readonly World globalWorld; | ||
| private readonly Entity localPlayerEntity; | ||
| private readonly CharacterDataPropagationUtility characterDataPropagationUtility; | ||
| private readonly PersistentEntities persistentEntities; | ||
|
|
||
| internal LocalPlayerCRDTEntityHandlerSystem( | ||
| World world, | ||
| World globalWorld, | ||
| Entity localPlayerEntity, | ||
| CharacterDataPropagationUtility characterDataPropagationUtility, | ||
| PersistentEntities persistentEntities) : base(world) | ||
| { | ||
| this.globalWorld = globalWorld; | ||
| this.localPlayerEntity = localPlayerEntity; | ||
| this.characterDataPropagationUtility = characterDataPropagationUtility; | ||
| this.persistentEntities = persistentEntities; | ||
| } | ||
|
|
||
| public override void Initialize() | ||
| { | ||
| if (!globalWorld.TryGet<Profile>(localPlayerEntity, out Profile? profile)) | ||
| return; | ||
|
|
||
| Entity playerEntity = persistentEntities.Player; | ||
| World.Add(playerEntity, new PlayerSceneCRDTEntity(new CRDTEntity(SpecialEntitiesID.PLAYER_ENTITY))); | ||
|
|
||
| characterDataPropagationUtility.CopyProfileToSceneEntity(profile!, new SceneEcsExecutor(World), playerEntity); | ||
|
pravusjif marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| protected override void Update(float t) | ||
| { | ||
| if (!globalWorld.TryGet<Profile>(localPlayerEntity, out Profile? profile)) | ||
| return; | ||
|
|
||
| if (!profile!.IsDirty) | ||
| return; | ||
|
|
||
| characterDataPropagationUtility.CopyProfileToSceneEntity(profile, new SceneEcsExecutor(World), persistentEntities.Player); | ||
| } | ||
| } | ||
| } | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -98,6 +98,9 @@ public void SetupPlayerCRDTEntityForPlayerInsideScene(bool isMainPlayer) | |
| Assert.IsTrue(world.TryGet(entity, out PlayerCRDTEntity playerCRDTEntity)); | ||
| Assert.IsTrue(scene1World.TryGet(playerCRDTEntity.SceneWorldEntity, out PlayerSceneCRDTEntity scenePlayerCRDTEntity)); | ||
| Assert.AreEqual(playerCRDTEntity.CRDTEntity, scenePlayerCRDTEntity.CRDTEntity); | ||
|
|
||
| if (isMainPlayer) | ||
| Assert.AreEqual(scene1Facade.PersistentEntities.Player, playerCRDTEntity.SceneWorldEntity); | ||
| } | ||
|
|
||
| [TestCase(true)] | ||
|
|
@@ -146,8 +149,19 @@ public void RemovePlayerCRDTEntityForPlayersLeavingScene(bool isMainPlayer) | |
|
|
||
| Assert.IsTrue(world.TryGet(entity, out PlayerCRDTEntity newState)); | ||
| Assert.IsFalse(newState.AssignedToScene); | ||
| Assert.IsTrue(playerCRDTEntity.SceneFacade.EcsExecutor.World.Has<PlayerSceneCRDTEntity>(playerCRDTEntity.SceneWorldEntity)); | ||
| Assert.That(playerCRDTEntity.SceneFacade.EcsExecutor.World.Has<DeleteEntityIntention>(playerCRDTEntity.SceneWorldEntity), Is.True); | ||
|
|
||
| if (isMainPlayer) | ||
| { | ||
| // Local player: PersistentEntities.Player has PlayerSceneCRDTEntity removed, not destroyed | ||
| Assert.IsFalse(scene1World.Has<DeleteEntityIntention>(playerCRDTEntity.SceneWorldEntity)); | ||
| Assert.IsFalse(scene1World.Has<PlayerSceneCRDTEntity>(playerCRDTEntity.SceneWorldEntity)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BLOCKING — test assertion contradicts current implementation. Commit // Local Player is never removed from the scene world
if (crdtEntity.Id == SpecialEntitiesID.PLAYER_ENTITY)
return;With that early return, Assert.IsFalse(scene1World.Has<PlayerSceneCRDTEntity>(playerCRDTEntity.SceneWorldEntity)); // FAILSPer 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 |
||
| } | ||
| else | ||
| { | ||
| // Remote player: separate entity gets DeleteEntityIntention | ||
| Assert.IsTrue(playerCRDTEntity.SceneFacade.EcsExecutor.World.Has<PlayerSceneCRDTEntity>(playerCRDTEntity.SceneWorldEntity)); | ||
| Assert.That(playerCRDTEntity.SceneFacade.EcsExecutor.World.Has<DeleteEntityIntention>(playerCRDTEntity.SceneWorldEntity), Is.True); | ||
| } | ||
| } | ||
|
|
||
| [TestCase(true)] | ||
|
|
@@ -180,7 +194,21 @@ public void ChangeSceneOnPlayerMove(bool isMainPlayer) | |
| Assert.IsTrue(world.TryGet(entity, out playerCRDTEntity)); | ||
| Assert.That(playerCRDTEntity.SceneFacade, Is.EqualTo(scene2Facade)); | ||
| Assert.IsTrue(scene2Facade.EcsExecutor.World.Has<PlayerSceneCRDTEntity>(playerCRDTEntity.SceneWorldEntity)); | ||
| Assert.That(scene1Facade.EcsExecutor.World.Has<DeleteEntityIntention>(scene1Entity), Is.True); | ||
|
|
||
| if (isMainPlayer) | ||
| { | ||
| // 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)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BLOCKING — same issue as line 157. |
||
|
|
||
| // New scene uses its persistent player entity | ||
| Assert.AreEqual(scene2Facade.PersistentEntities.Player, playerCRDTEntity.SceneWorldEntity); | ||
| } | ||
| else | ||
| { | ||
| // Remote player: old entity gets DeleteEntityIntention | ||
| Assert.That(scene1Facade.EcsExecutor.World.Has<DeleteEntityIntention>(scene1Entity), Is.True); | ||
| } | ||
| } | ||
|
|
||
| [Test] | ||
|
|
@@ -263,7 +291,7 @@ public void TrackReservedCRDTEntityIdsCorrectly() | |
|
|
||
| [TestCase(true)] | ||
| [TestCase(false)] | ||
| public void NotAssignPlayerWhenSceneIsStarting(bool isMainPlayer) | ||
| public void AssignPlayerWhenSceneIsStarting(bool isMainPlayer) | ||
| { | ||
| scene1Facade.SceneStateProvider.State.Returns(new Atomic<SceneState>(SceneState.Starting)); | ||
|
|
||
|
|
@@ -279,14 +307,14 @@ public void NotAssignPlayerWhenSceneIsStarting(bool isMainPlayer) | |
| system.Update(0); | ||
|
|
||
| Assert.IsTrue(world.TryGet(entity, out PlayerCRDTEntity playerCRDTEntity)); | ||
| Assert.IsFalse(playerCRDTEntity.AssignedToScene); | ||
| Assert.That(playerCRDTEntity.SceneFacade, Is.Null); | ||
| Assert.That(playerCRDTEntity.SceneWorldEntity, Is.EqualTo(Entity.Null)); | ||
| Assert.IsTrue(playerCRDTEntity.AssignedToScene); | ||
| Assert.That(playerCRDTEntity.SceneFacade, Is.EqualTo(scene1Facade)); | ||
| Assert.IsTrue(scene1World.Has<PlayerSceneCRDTEntity>(playerCRDTEntity.SceneWorldEntity)); | ||
| } | ||
|
|
||
| [TestCase(true)] | ||
| [TestCase(false)] | ||
| public void AssignPlayerWhenSceneTransitionsFromStartingToRunning(bool isMainPlayer) | ||
| public void KeepPlayerAssignedWhenSceneTransitionsFromStartingToRunning(bool isMainPlayer) | ||
| { | ||
| scene1Facade.SceneStateProvider.State.Returns(new Atomic<SceneState>(SceneState.Starting)); | ||
|
|
||
|
|
@@ -299,16 +327,16 @@ public void AssignPlayerWhenSceneTransitionsFromStartingToRunning(bool isMainPla | |
| if (isMainPlayer) | ||
| world.Add(entity, new PlayerComponent()); | ||
|
|
||
| // First tick: scene still Starting — gate blocks assignment | ||
| // First tick: scene is Starting — player is assigned immediately | ||
| system.Update(0); | ||
|
|
||
| Assert.IsTrue(world.TryGet(entity, out PlayerCRDTEntity playerCRDTEntity)); | ||
| Assert.IsFalse(playerCRDTEntity.AssignedToScene); | ||
| Assert.IsTrue(playerCRDTEntity.AssignedToScene); | ||
|
|
||
| // Scene finishes initializing | ||
| scene1Facade.SceneStateProvider.State.Returns(new Atomic<SceneState>(SceneState.Running)); | ||
|
|
||
| // Next tick: reconciliation query auto-retries and assigns | ||
| // Next tick: assignment persists through state transition | ||
| system.Update(0); | ||
|
|
||
| Assert.IsTrue(world.TryGet(entity, out playerCRDTEntity)); | ||
|
|
@@ -344,9 +372,10 @@ public void SkipSceneSideCleanupWhenPreviousSceneIsDisposing() | |
| Assert.IsTrue(world.TryGet(entity, out playerCRDTEntity)); | ||
| Assert.IsFalse(playerCRDTEntity.AssignedToScene); | ||
|
|
||
| // Scene-side cleanup write must be skipped: posting DeleteEntityIntention to a | ||
| // disposing world would race with its teardown. | ||
| // Scene-side cleanup write must be skipped: RemovePlayerFromScene gates on Running, | ||
| // so neither DeleteEntityIntention nor PlayerSceneCRDTEntity removal should happen. | ||
| Assert.That(scene1World.Has<DeleteEntityIntention>(scene1Entity), Is.False); | ||
| Assert.That(scene1World.Has<PlayerSceneCRDTEntity>(scene1Entity), Is.True); | ||
| } | ||
|
|
||
| [Test] | ||
|
|
||
There was a problem hiding this comment.
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.Startingis now accepted as valid for assignment, but two pre-existing tests assert the opposite behavior and are not updated by this PR:NotAssignPlayerWhenSceneIsStarting(lines 293–313) — assertsAssignedToScene = falsewhen the scene isStarting, but this code will now set it totrue. BothisMainPlayer = trueandisMainPlayer = falsevariants fail.AssignPlayerWhenSceneTransitionsFromStartingToRunning(lines 315–346) — the firstUpdate()tick (scene stillStarting) now assigns the player, soAssert.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 isStarting. ForAssignPlayerWhenSceneTransitionsFromStartingToRunning, the intermediate assertion after the first tick should be removed or changed toIsTrue.Fix this →