feat: sdk other players transform propagation

[pravusjif]

• This PR enables propagating the transform information of other players (as we do with PBPlayerIdentityData etc...)
• This should work with the avatar bots tool when instantiating other fake players

Snippet for testing the reading of the other players transform in a scene using SDK:

import { engine,  Transform, PlayerIdentityData } from '@dcl/sdk/ecs'

let interval = 1
engine.addSystem((dt)=> {
  interval -= dt
  if (interval <= 0) {
    interval = 1

    for (const [entity, data, transform] of engine.getEntitiesWith(PlayerIdentityData, Transform)) {
      // skip main player
      if (entity === 1) continue

      console.log(`PLAYERIDENTITYDATA - entityId: ${entity}; Address: ${data.address}; IsGuest? ${data.isGuest}`)
      console.log(`Player transform pos: (${transform.position.x}, ${transform.position.y}, ${transform.position.z}); `)
    }
  }
})

Summary by CodeRabbit

• New Features

  • Introduced properties for avatar position and rotation in the avatar interface, enhancing spatial attribute access.
  • Added PlayerTransformPropagationSystem to manage player transform data in multiplayer environments.
  • Introduced WritePlayerTransformSystem for real-time updates of player positions and rotations.
  • Added mock implementations for testing, including Fake classes for IAvatarView and ISceneFacade.
  • Enhanced MultiplayerPlugin to include new systems for improved player transformation handling.

• Bug Fixes

  • Updated method calls to reflect renamed systems, ensuring proper functionality in player transformation handling.

• Tests

  • Added unit tests for player transformation propagation to validate functionality in multiplayer scenarios.

[github-actions]

Windows and Mac build successfull in Unity Cloud! You can find a link to the downloadable artifact below.

Name	Link
Commit	3b13941
Logs	https://github.com/decentraland/unity-explorer/actions/runs/10909118182
Download Windows	https://github.com/decentraland/unity-explorer/suites/28505757126/artifacts/1944458326
Download Mac	https://github.com/decentraland/unity-explorer/suites/28505757126/artifacts/1944437652
Built on	2024-09-17T19:23:04Z

[mikhail-dcl]

Please merge the recent changes from main

[pravusjif]

I'll tackle this when I have some bandwith

[coderabbitai]

Walkthrough

The pull request introduces several modifications across multiple files, primarily focusing on enhancing the functionality of avatar and player transform systems within a multiplayer environment. Key changes include the addition of new properties in the IAvatarView interface, renaming of classes for clarity, and the introduction of new systems for managing player transformations. Additionally, mock implementations for testing purposes have been added, along with metadata files for better asset management.

Changes

File Path	Change Summary
Explorer/Assets/DCL/AvatarRendering/AvatarShape/UnityInterface/AvatarBase.cs	Added Position and Rotation properties to IAvatarView and AvatarBase, introduced a mock Fake class implementing IAvatarView.
Explorer/Assets/DCL/Character/CharacterMotion/Systems/WriteMainPlayerTransformSystem.cs	Renamed WritePlayerTransformSystem to WriteMainPlayerTransformSystem, updated constructor accordingly.
Explorer/Assets/DCL/Character/Plugin/CharacterContainer.cs	Updated method call from WritePlayerTransformSystem.InjectToWorld to WriteMainPlayerTransformSystem.InjectToWorld.
Explorer/Assets/DCL/Multiplayer/SDK/Systems/GlobalWorld/PlayerTransformPropagationSystem.cs	Introduced PlayerTransformPropagationSystem for managing player transform data propagation in multiplayer.
Explorer/Assets/DCL/Multiplayer/SDK/Systems/SceneWorld/WritePlayerTransformSystem.cs	Added WritePlayerTransformSystem for managing player transform updates in a multiplayer context.
Explorer/Assets/DCL/Multiplayer/SDK/Tests/PropagateTransformTest.cs	Created unit tests for PlayerTransformPropagationSystem and WritePlayerTransformSystem to validate transformation propagation functionality.
Explorer/Assets/DCL/PluginSystem/Global/MultiplayerPlugin.cs	Added call to PlayerTransformPropagationSystem.InjectToWorld in MultiplayerPlugin.
Explorer/Assets/DCL/PluginSystem/World/MultiplayerPlugin.cs	Added injection call for WritePlayerTransformSystem in MultiplayerPlugin.
Explorer/Assets/Scripts/SceneRunner/Scene/ISceneFacade.cs	Introduced a mock Fake class that implements ISceneFacade for testing purposes.

Sequence Diagram(s)

sequenceDiagram
    participant Player
    participant PlayerTransformPropagationSystem
    participant WritePlayerTransformSystem
    participant CRDTLayer
    
    Player->>PlayerTransformPropagationSystem: Update Transform
    PlayerTransformPropagationSystem->>WritePlayerTransformSystem: Propagate Transform
    WritePlayerTransformSystem->>CRDTLayer: Send Transform Data
    CRDTLayer-->>WritePlayerTransformSystem: Acknowledge
    WritePlayerTransformSystem-->>PlayerTransformPropagationSystem: Acknowledge
    PlayerTransformPropagationSystem-->>Player: Update Complete

Loading

🐰 In the code, I hop and play,
New features bloom, hip-hip-hooray!
With transforms that dance and twirl,
In the multiplayer world, they swirl!
So let’s celebrate with a cheer,
For changes bright, bring joy and cheer! 🎉

Possibly related PRs

• feat: support raw gltf loading for GLTFContainer SDK component #1435: This PR enhances the AvatarBase class and IAvatarView interface, which could relate to avatar rendering and positioning.
• feat: sdk camera control components #1744: This PR focuses on camera control components, which may indirectly relate to avatar positioning and rendering.
• feat: block player movement #1777: This PR introduces changes to input handling systems, which could affect avatar interactions.
• Refactor: initial state sync #1936: This PR involves changes to player data propagation systems, which could relate to avatar management.
• Optimization: avatar area system #1998: This PR optimizes the avatar area system, which is directly related to the handling of avatar visibility and positioning.
• Fix: avatar turning with compression, no floaty avatars #2033: This PR addresses avatar movement and compression, which is relevant to the overall handling of avatar behavior and may connect to the changes in AvatarBase.cs.

Suggested labels

feature

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 3

Outside diff range and nitpick comments (3)

Explorer/Assets/Scripts/SceneRunner/Scene/ISceneFacade.cs (1)

43-89: LGTM, but consider adding a comment to clarify the purpose of the class.

The Fake class serves as a mock implementation of the ISceneFacade interface, which can be useful for testing or as a placeholder implementation. The changes are approved.

However, consider adding a comment to clarify the purpose of the class, such as:

/// <summary>
/// A mock implementation of the ISceneFacade interface for testing or placeholder purposes.
/// </summary>
class Fake : ISceneFacade
{
    // ...
}

Explorer/Assets/DCL/Multiplayer/SDK/Tests/PropagateTransformTest.cs (1)

24-69: Test method looks good, but consider adding more test cases.

The test method PropagateTransform is well-structured and follows the Arrange-Act-Assert pattern. It is testing the integration between the PlayerTransformPropagationSystem and the WritePlayerTransformSystem to ensure that the player transform is correctly propagated and written to the CRDT.

The test method is using fake implementations for dependencies, which is a good practice for unit testing.

However, consider adding more test cases to cover edge cases and error scenarios, such as:

• Test with multiple players
• Test with different transform values
• Test with invalid or missing components
• Test error handling and logging

Explorer/Assets/DCL/AvatarRendering/AvatarShape/UnityInterface/AvatarBase.cs (1)

Line range hint 1-241: Consider the following optimizations for the Unity engine:

1. The AvatarBase class has a significant number of serialized fields, which can impact performance and memory usage. Consider optimizing the structure by grouping related fields into separate components or using a more efficient serialization approach.

2. The Awake method in AvatarBase class creates a new AnimatorOverrideController and initializes the animationOverrides list. Ensure that this initialization is necessary and consider moving it to a lazy initialization approach to avoid unnecessary overhead.

3. The ReplaceEmoteAnimation method in AvatarBase class sets the runtimeAnimatorController property of the Animator component. Frequently changing the runtimeAnimatorController can be expensive, so consider optimizing this approach if it is called frequently.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 3143466 and 6cb76e1.

Files selected for processing (12)

• Explorer/Assets/DCL/AvatarRendering/AvatarShape/UnityInterface/AvatarBase.cs (3 hunks)
• Explorer/Assets/DCL/Character/CharacterMotion/Systems/WriteMainPlayerTransformSystem.cs (2 hunks)
• Explorer/Assets/DCL/Character/Plugin/CharacterContainer.cs (1 hunks)
• Explorer/Assets/DCL/Multiplayer/SDK/Systems/GlobalWorld/PlayerTransformPropagationSystem.cs (1 hunks)
• Explorer/Assets/DCL/Multiplayer/SDK/Systems/GlobalWorld/PlayerTransformPropagationSystem.cs.meta (1 hunks)
• Explorer/Assets/DCL/Multiplayer/SDK/Systems/SceneWorld/WritePlayerTransformSystem.cs (1 hunks)
• Explorer/Assets/DCL/Multiplayer/SDK/Systems/SceneWorld/WritePlayerTransformSystem.cs.meta (1 hunks)
• Explorer/Assets/DCL/Multiplayer/SDK/Tests/PropagateTransformTest.cs (1 hunks)
• Explorer/Assets/DCL/Multiplayer/SDK/Tests/PropagateTransformTest.cs.meta (1 hunks)
• Explorer/Assets/DCL/PluginSystem/Global/MultiplayerPlugin.cs (1 hunks)
• Explorer/Assets/DCL/PluginSystem/World/MultiplayerPlugin.cs (1 hunks)
• Explorer/Assets/Scripts/SceneRunner/Scene/ISceneFacade.cs (1 hunks)

Files skipped from review due to trivial changes (4)

• Explorer/Assets/DCL/Character/CharacterMotion/Systems/WriteMainPlayerTransformSystem.cs
• Explorer/Assets/DCL/Multiplayer/SDK/Systems/GlobalWorld/PlayerTransformPropagationSystem.cs.meta
• Explorer/Assets/DCL/Multiplayer/SDK/Systems/SceneWorld/WritePlayerTransformSystem.cs.meta
• Explorer/Assets/DCL/Multiplayer/SDK/Tests/PropagateTransformTest.cs.meta

Additional context used

Path-based instructions (8)

Explorer/Assets/DCL/PluginSystem/World/MultiplayerPlugin.cs (2)

Pattern **/*.cs: Review the code for heap allocations and suggest potential improvements to avoid runtime allocations.

---

Pattern **/*.cs: Review the code for specific unity engine optimizations and suggest potential improvements.

Explorer/Assets/DCL/Multiplayer/SDK/Systems/GlobalWorld/PlayerTransformPropagationSystem.cs (2)

Pattern **/*.cs: Review the code for heap allocations and suggest potential improvements to avoid runtime allocations.

---

Pattern **/*.cs: Review the code for specific unity engine optimizations and suggest potential improvements.

Explorer/Assets/DCL/Multiplayer/SDK/Systems/SceneWorld/WritePlayerTransformSystem.cs (2)

Pattern **/*.cs: Review the code for heap allocations and suggest potential improvements to avoid runtime allocations.

---

Pattern **/*.cs: Review the code for specific unity engine optimizations and suggest potential improvements.

Explorer/Assets/Scripts/SceneRunner/Scene/ISceneFacade.cs (2)

Pattern **/*.cs: Review the code for heap allocations and suggest potential improvements to avoid runtime allocations.

---

Pattern **/*.cs: Review the code for specific unity engine optimizations and suggest potential improvements.

Explorer/Assets/DCL/Multiplayer/SDK/Tests/PropagateTransformTest.cs (2)

Pattern **/*.cs: Review the code for heap allocations and suggest potential improvements to avoid runtime allocations.

---

Pattern **/*.cs: Review the code for specific unity engine optimizations and suggest potential improvements.

Explorer/Assets/DCL/Character/Plugin/CharacterContainer.cs (2)

Pattern **/*.cs: Review the code for heap allocations and suggest potential improvements to avoid runtime allocations.

---

Pattern **/*.cs: Review the code for specific unity engine optimizations and suggest potential improvements.

Explorer/Assets/DCL/PluginSystem/Global/MultiplayerPlugin.cs (2)

Pattern **/*.cs: Review the code for heap allocations and suggest potential improvements to avoid runtime allocations.

---

Pattern **/*.cs: Review the code for specific unity engine optimizations and suggest potential improvements.

Explorer/Assets/DCL/AvatarRendering/AvatarShape/UnityInterface/AvatarBase.cs (2)

Pattern **/*.cs: Review the code for heap allocations and suggest potential improvements to avoid runtime allocations.

---

Pattern **/*.cs: Review the code for specific unity engine optimizations and suggest potential improvements.

Additional comments not posted (12)

Explorer/Assets/DCL/PluginSystem/World/MultiplayerPlugin.cs (1)

34-34: LGTM!

The code change is approved for the following reasons:

1. The injection of WritePlayerTransformSystem into the world builder is consistent with the existing code structure and follows the pattern of injecting systems.
2. The change does not introduce any heap allocations or require specific Unity engine optimizations.
3. The AI-generated summary provides additional context, indicating that this change enhances the functionality by integrating the WritePlayerTransformSystem into the existing architecture and allowing it to utilize the sharedDependencies.SceneData alongside the previously injected systems.

Explorer/Assets/DCL/Multiplayer/SDK/Systems/GlobalWorld/PlayerTransformPropagationSystem.cs (1)

28-28: Consider using TransformComponent instead of IAvatarView.

As mentioned in the previous comment by mikhail-dcl, consider using TransformComponent instead of IAvatarView for consistency with other systems and to avoid potential issues with the avatar system.

Explorer/Assets/DCL/Multiplayer/SDK/Systems/SceneWorld/WritePlayerTransformSystem.cs (4)

39-39: The code still uses PlayerCRDTEntity, indicating that the suggested change to PlayerSceneCRDTEntity has not been made.

---

47-47: The code does not use ExposedTransformUtils as suggested. Could you please clarify the benefits of using ExposedTransformUtils in this context?

---

37-52: LGTM!

The UpdateSDKTransform query correctly updates the SDK transform for player entities, excluding the main player. The position conversion to scene-relative coordinates and the usage of ecsToCRDTWriter.PutMessage are appropriate for proper synchronization.

---

54-59: LGTM!

The HandleComponentRemoval query correctly handles the removal of the SDKTransform component when a DeleteEntityIntention component is present. The usage of ecsToCRDTWriter.DeleteMessage is appropriate for removing the component from the CRDT layer.

Explorer/Assets/DCL/Multiplayer/SDK/Tests/PropagateTransformTest.cs (1)

71-107: Helper class looks good.

The helper class FakeECSToCRDTWriter is a simple fake implementation of IECSToCRDTWriter interface for testing purposes. It is storing the messages sent to the writer in a list of Message objects, which is a good way to store the message data for assertions.

The class is not implementing all the methods of the interface, but that is acceptable for the current test case. The class can be extended to support more test cases in the future if needed.

Explorer/Assets/DCL/Character/Plugin/CharacterContainer.cs (1)

125-125: LGTM, but verify the impact of the change.

The renaming of the method from WritePlayerTransformSystem.InjectToWorld to WriteMainPlayerTransformSystem.InjectToWorld suggests a shift in functionality or focus of the system being invoked. This change is approved.

However, ensure that this change is thoroughly tested to confirm that it behaves as expected and does not introduce any unintended side effects or performance regressions in the handling of player transformations within the world-building process.

Run the following script to verify the impact of the change:

Verification successful

---

Change appears consistent, but recommend runtime testing.

The renaming of the method to WriteMainPlayerTransformSystem.InjectToWorld aligns with its role in handling the main player transform, as indicated by the comments and usage in the codebase. No immediate issues or performance regressions were identified from the code analysis. However, it is advisable to conduct runtime testing to ensure that no performance regressions occur due to this change.

• Ensure runtime testing is performed to confirm the absence of performance regressions.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the impact of the change on the functionality and performance of the player transformation handling.

# Test 1: Search for potential side effects or performance regressions related to the change.
rg --type csharp -A 5 $'WriteMainPlayerTransformSystem'

# Test 2: Search for potential issues in the handling of player transformations within the world-building process.
ast-grep --lang csharp --pattern $'InjectToWorld($_, $_, $_, $_, $_, $_, $_, $_)'

Length of output: 4611

Explorer/Assets/DCL/PluginSystem/Global/MultiplayerPlugin.cs (1)

133-133: LGTM! Verify the implementation of PlayerTransformPropagationSystem.InjectToWorld.

The addition of the PlayerTransformPropagationSystem.InjectToWorld call is approved, as it enhances the functionality related to player transformations in the multiplayer environment.

However, please ensure that the implementation of PlayerTransformPropagationSystem.InjectToWorld follows best practices:

• Review the code for unnecessary heap allocations and optimize them if needed.
• Review the code for Unity-specific optimizations and apply them if needed.

Explorer/Assets/DCL/AvatarRendering/AvatarShape/UnityInterface/AvatarBase.cs (3)

160-162: LGTM!

The Position and Rotation properties are consistent with the AvatarBase class and provide a way to access the avatar's position and rotation through the interface.

---

186-195: LGTM!

The Fake class provides a mock implementation of the IAvatarView interface, which can be useful for testing or as a placeholder during development. The constructor allows creating instances with specific position and rotation values.

---

197-240: LGTM!

The Fake class provides a complete implementation of the IAvatarView interface, with each method throwing a NotImplementedException. This is expected behavior for a mock implementation and serves as a placeholder for future development.

[coderabbitai]

Actionable comments posted: 0

Outside diff range and nitpick comments (1)

Explorer/Assets/Scripts/ECS/TestSuite/EcsTestsUtils.cs (1)

25-25: Heap allocations and consistency improvement suggestions

The code changes are creating new SDKTransform, CanBeDirty<Vector3>, and CanBeDirty<Quaternion> objects which will result in heap allocations. Consider the following suggestions to potentially improve performance and consistency:

1. If possible, consider reusing existing SDKTransform objects instead of creating new ones to reduce heap allocations. This may require changes to the calling code to pass in an existing SDKTransform object.

2. For consistency with the Position and Rotation properties, consider wrapping the Scale property in a CanBeDirty<Vector3> type as well:

-Scale = Vector3.one
+Scale = new CanBeDirty<Vector3>(Vector3.one)

Overall, the changes look good and enhance the functionality by allowing for more nuanced state management of the transform properties.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 6cb76e1 and b5c6df3.

Files selected for processing (9)

• Explorer/Assets/DCL/Multiplayer/SDK/Systems/GlobalWorld/PlayerTransformPropagationSystem.cs (1 hunks)
• Explorer/Assets/DCL/Multiplayer/SDK/Systems/SceneWorld/WritePlayerTransformSystem.cs (1 hunks)
• Explorer/Assets/DCL/SDKComponents/Tween/Components/CustomTweener/CustomTweenerImpl.cs (3 hunks)
• Explorer/Assets/Scripts/CrdtEcsBridge/Components/Transform/SDKTransform.cs (2 hunks)
• Explorer/Assets/Scripts/CrdtEcsBridge/Components/Transform/SDKTransformSerializer.cs (1 hunks)
• Explorer/Assets/Scripts/ECS/TestSuite/ECS.TestSuite.asmdef (1 hunks)
• Explorer/Assets/Scripts/ECS/TestSuite/EcsTestsUtils.cs (2 hunks)
• Explorer/Assets/Scripts/ECS/Unity/Transforms/ExposedTransformUtils.cs (1 hunks)
• Explorer/Assets/Scripts/ECS/Unity/Transforms/Tests/UpdateTransformSystemShould.cs (2 hunks)

Files skipped from review as they are similar to previous changes (1)

• Explorer/Assets/DCL/Multiplayer/SDK/Systems/GlobalWorld/PlayerTransformPropagationSystem.cs

Additional context used

Path-based instructions (7)

Explorer/Assets/Scripts/ECS/Unity/Transforms/ExposedTransformUtils.cs (2)

Pattern **/*.cs: Review the code for heap allocations and suggest potential improvements to avoid runtime allocations.

---

Pattern **/*.cs: Review the code for specific unity engine optimizations and suggest potential improvements.

Explorer/Assets/Scripts/CrdtEcsBridge/Components/Transform/SDKTransformSerializer.cs (2)

Pattern **/*.cs: Review the code for heap allocations and suggest potential improvements to avoid runtime allocations.

---

Pattern **/*.cs: Review the code for specific unity engine optimizations and suggest potential improvements.

Explorer/Assets/Scripts/ECS/TestSuite/EcsTestsUtils.cs (2)

Pattern **/*.cs: Review the code for heap allocations and suggest potential improvements to avoid runtime allocations.

---

Pattern **/*.cs: Review the code for specific unity engine optimizations and suggest potential improvements.

Explorer/Assets/Scripts/ECS/Unity/Transforms/Tests/UpdateTransformSystemShould.cs (2)

Pattern **/*.cs: Review the code for heap allocations and suggest potential improvements to avoid runtime allocations.

---

Pattern **/*.cs: Review the code for specific unity engine optimizations and suggest potential improvements.

Explorer/Assets/Scripts/CrdtEcsBridge/Components/Transform/SDKTransform.cs (2)

Pattern **/*.cs: Review the code for heap allocations and suggest potential improvements to avoid runtime allocations.

---

Pattern **/*.cs: Review the code for specific unity engine optimizations and suggest potential improvements.

Explorer/Assets/DCL/Multiplayer/SDK/Systems/SceneWorld/WritePlayerTransformSystem.cs (2)

Pattern **/*.cs: Review the code for heap allocations and suggest potential improvements to avoid runtime allocations.

---

Pattern **/*.cs: Review the code for specific unity engine optimizations and suggest potential improvements.

Explorer/Assets/DCL/SDKComponents/Tween/Components/CustomTweener/CustomTweenerImpl.cs (2)

Pattern **/*.cs: Review the code for heap allocations and suggest potential improvements to avoid runtime allocations.

---

Pattern **/*.cs: Review the code for specific unity engine optimizations and suggest potential improvements.

Additional comments not posted (16)

Explorer/Assets/Scripts/ECS/Unity/Transforms/ExposedTransformUtils.cs (2)

18-18: LGTM!

The code changes are approved. The position is being correctly assigned using the ParcelMathHelper.GetSceneRelativePosition method to convert from local space to scene space. There are no heap allocations or Unity engine optimizations needed here.

---

19-19: LGTM!

The code changes are approved. The rotation is being correctly assigned directly from data.Item1.Rotation.Value. There are no heap allocations or Unity engine optimizations needed here.

Explorer/Assets/Scripts/CrdtEcsBridge/Components/Transform/SDKTransformSerializer.cs (2)

14-15: LGTM!

The code changes are approved. The deserialization logic for Position and Rotation properties is correctly implemented by accessing their Value property from the pointer variable.

---

23-24: LGTM!

The code changes are approved. The serialization logic for Position and Rotation properties is correctly implemented by writing their Value property to the pointer variable.

Explorer/Assets/Scripts/ECS/TestSuite/ECS.TestSuite.asmdef (1)

21-22: LGTM!

The addition of new GUID entries to the list of referenced assemblies is a valid change that expands the project's capabilities without introducing any issues. The existing dependencies remain intact.

The code changes are approved.

Explorer/Assets/Scripts/ECS/Unity/Transforms/Tests/UpdateTransformSystemShould.cs (2)

10-10: LGTM!

The addition of the Utility namespace is approved.

---

23-23: LGTM, but verify the impact on the transform update system.

The change to initialize the Position property using the CanBeDirty<Vector3> wrapper class is approved. This modification suggests an enhancement in managing the position data by allowing more sophisticated handling of the "dirty" state of the transform.

However, please ensure that this change does not introduce any unintended side effects on the transform update system. Verify that the system correctly handles the new CanBeDirty<Vector3> wrapper and that the position updates are propagated as expected.

Explorer/Assets/Scripts/CrdtEcsBridge/Components/Transform/SDKTransform.cs (4)

16-16: LGTM!

The code changes are approved.

---

18-19: LGTM!

The code changes are approved. The use of CanBeDirty<T> wrapper for Position and Rotation properties is a good practice for serialization/deserialization. The default values are assigned using Vector3.zero and Quaternion.identity, which are static readonly fields and do not introduce any heap allocations.

---

20-21: LGTM!

The code changes are approved. The explicit interface implementations for IExposedTransform.Position and IExposedTransform.Rotation properties provide a way to access the Position and Rotation properties through the IExposedTransform interface. The implementations do not introduce any heap allocations or impact Unity engine optimizations.

---

45-46: LGTM!

The code changes are approved. Resetting the values of Position and Rotation properties through their Value properties in the Clear method ensures that the dirty state is managed correctly. The changes do not introduce any heap allocations or impact Unity engine optimizations.

Explorer/Assets/DCL/Multiplayer/SDK/Systems/SceneWorld/WritePlayerTransformSystem.cs (2)

1-61: LGTM!

The code changes are approved. The system is well-structured, follows best practices, and correctly handles updating player transform data and component removal.

---

1-61: Addressing past review comments:

mikhail-dcl: In the scene context PlayerCRDTEntity is changed to PlayerSceneCRDTEntity

The code correctly uses PlayerSceneCRDTEntity throughout the system.

mikhail-dcl: Can't you use ExposedTransformUtils instead?

The code correctly uses ExposedTransformUtils to update the transform data.

Explorer/Assets/DCL/SDKComponents/Tween/Components/CustomTweener/CustomTweenerImpl.cs (3)

30-34: LGTM!

The code changes in the SetResult method are approved. The changes align with the goal of improving the encapsulation of the SDKTransform structure by accessing the Position and Rotation properties through their respective Value properties.

---

57-58: LGTM!

The code changes in the SetResult method are approved. The changes align with the goal of improving the encapsulation of the SDKTransform structure by accessing the Position and Rotation properties through their respective Value properties.

---

82-84: LGTM!

The code changes in the SetResult method are approved. The changes align with the goal of improving the encapsulation of the SDKTransform structure by accessing the Rotation and Position properties through their respective Value properties.

[coderabbitai]

Actionable comments posted: 1

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 82608fb and 931e5e2.

Files selected for processing (1)

• Explorer/Assets/DCL/Multiplayer/SDK/Tests/PropagateTransformTest.cs (1 hunks)

Additional context used

Path-based instructions (1)

Explorer/Assets/DCL/Multiplayer/SDK/Tests/PropagateTransformTest.cs (2)

Pattern **/*.cs: Review the code for heap allocations and suggest potential improvements to avoid runtime allocations.

---

Pattern **/*.cs: Review the code for specific unity engine optimizations and suggest potential improvements.

Additional comments not posted (1)

Explorer/Assets/DCL/Multiplayer/SDK/Tests/PropagateTransformTest.cs (1)

77-113: LGTM!

The FakeECSToCRDTWriter class is a fake implementation used for testing purposes, and it looks good as is. It correctly implements the PutMessage method and stores the messages in a list for later verification. The other methods of the interface are not implemented, but that's expected for a fake implementation.

[pravusjif]

I'll try to replace the ISceneFacade.Fake with NSubstitute usage in the test suite