-
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
Open
mikhail-dcl
wants to merge
7
commits into
dev
Choose a base branch
from
feat/seed-local-player-crdt-on-scene-init
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
d18bdc5
Maintain LocalPlayer-related data on all scenes throughout their whol…
mikhail-dcl 5741142
Fix RemovePlayerFromScene
mikhail-dcl a400ccd
Merge branch 'dev' into feat/seed-local-player-crdt-on-scene-init
mikhail-dcl 6c2dc08
@
mikhail-dcl 7d7a2a4
Merge remote-tracking branch 'origin/feat/seed-local-player-crdt-on-s…
mikhail-dcl ea34e55
Handle Local Player more carefully & clean-up some paths
mikhail-dcl d175dde
@
mikhail-dcl File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1 change: 0 additions & 1 deletion
1
Explorer/Assets/DCL/Infrastructure/SceneRunner/Scene/SceneEcsExecutor.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,4 @@ | ||
| using Arch.Core; | ||
| using Utility.Multithreading; | ||
|
|
||
| namespace SceneRunner.Scene | ||
| { | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
76 changes: 76 additions & 0 deletions
76
Explorer/Assets/DCL/Multiplayer/SDK/Systems/SceneWorld/LocalPlayerCRDTEntityHandlerSystem.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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(localPlayerEntity, out Profile? profile)) | ||
| return; | ||
|
|
||
| Entity playerEntity = persistentEntities.Player; | ||
| World.Add(playerEntity, new PlayerSceneCRDTEntity(new CRDTEntity(SpecialEntitiesID.PLAYER_ENTITY))); | ||
|
|
||
| characterDataPropagationUtility.CopyProfileToSceneEntity(profile!, World, playerEntity); | ||
| } | ||
|
|
||
| protected override void Update(float t) | ||
| { | ||
| if (!globalWorld.TryGet(localPlayerEntity, out Profile? profile)) | ||
| return; | ||
|
|
||
| if (!profile!.IsDirty) | ||
| return; | ||
|
|
||
| characterDataPropagationUtility.CopyProfileToSceneEntity(profile, World, persistentEntities.Player); | ||
| } | ||
| } | ||
| } |
2 changes: 2 additions & 0 deletions
2
.../Assets/DCL/Multiplayer/SDK/Systems/SceneWorld/LocalPlayerCRDTEntityHandlerSystem.cs.meta
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 →