fix: Unable to see other user's passport card #1996
Conversation
|
Windows and Mac build successfull in Unity Cloud! You can find a link to the downloadable artifact below.
|
WalkthroughThe pull request introduces significant changes to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
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? TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
Explorer/Assets/DCL/Multiplayer/Profiles/Entities/RemoteEntities.cs (1)
92-95: Consider Adding Error Handling and LoggingThe
TryRemoveandRemoveGlobalEntityAssociationmethods lack explicit error handling or logging. In a multiplayer environment, it's crucial to ensure these operations are robust against errors and race conditions. Consider adding error handling or logging to monitor these operations effectively.
TryRemovemethod inRemoteEntities.csRemoveGlobalEntityAssociationmethod inEntityCollidersGlobalCache.csAnalysis chain
Verify the removal logic for consistency and error handling.
The
TryRemovemethod updates thecollidersByWalletIddictionary and releases resources usingremoteAvatarColliderPool. It's crucial to ensure that these operations are atomic and error-free, especially in a multiplayer environment where race conditions can occur.Consider adding error handling or logging to monitor these operations, and verify that the
RemoveGlobalEntityAssociationmethod handles exceptions or unusual states gracefully.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the error handling in the removal logic. # Test: Search for error handling patterns in the TryRemove method. rg --type cs -A 5 $'TryRemove' Explorer/Assets/DCL/Multiplayer/Profiles/Entities/RemoteEntities.csLength of output: 541
Script:
#!/bin/bash # Search for try-catch blocks and logging within the TryRemove method. rg --type cs -A 10 'TryRemove' Explorer/Assets/DCL/Multiplayer/Profiles/Entities/RemoteEntities.cs | rg -i 'try|catch|log' # Search for the implementation of the RemoveGlobalEntityAssociation method to check for error handling. ast-grep --lang csharp --pattern 'RemoveGlobalEntityAssociation($_)' --search-implementationLength of output: 751
Script:
#!/bin/bash # Correct the ast-grep command to find the implementation of the RemoveGlobalEntityAssociation method. ast-grep --lang csharp --pattern 'RemoveGlobalEntityAssociation($_)'Length of output: 70
Script:
#!/bin/bash # Search for the definition of the RemoveGlobalEntityAssociation method across the codebase. rg --type cs 'RemoveGlobalEntityAssociation' -A 5Length of output: 1527
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- Explorer/Assets/DCL/Multiplayer/Profiles/Entities/RemoteEntities.cs (3 hunks)
Additional context used
Path-based instructions (1)
Explorer/Assets/DCL/Multiplayer/Profiles/Entities/RemoteEntities.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.
What does this PR change?
Fix #1786
There were some cases in which the passport card of other users couldn't be open. The option "view profile" didn't appear when hovering over them.
The problem was that we were never releasing the collider of none of the remote avatars, so each time a new avatar was being spawned, a new collider was being added as child. If this happened several times with the same avatar, we were accumulating several colliders for the same game object and it was causing conflicts in the click detection.
How to test the changes?
Our Code Review Standards
https://github.com/decentraland/unity-renderer/blob/master/docs/code-review-standards.md
Summary by CodeRabbit
New Features
RemoteAvatarCollidertype for improved functionality related to remote avatars.Bug Fixes
Refactor
RemoteEntitiesclass to focus onRemoteAvatarCollider, enhancing maintainability and encapsulation of collider logic.