Fix: avatar turning with compression, no floaty avatars #2033
Conversation
WalkthroughThe pull request introduces several modifications across multiple files related to the multiplayer movement system. Key changes include renaming parameters for clarity, refactoring compression and decompression logic, and enhancing data structures to optimize memory usage. The Changes
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 (
|
|
Windows and Mac build successfull in Unity Cloud! You can find a link to the downloadable artifact below.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
Explorer/Assets/DCL/Multiplayer/Movement/Systems/csc.rsp (1)
1-1: Consider enabling nullable only for new code to avoid overwhelming warnings.Enabling nullable reference types is a good practice as it helps prevent null reference exceptions and improves code quality. However, enabling this option for the entire project might lead to many warnings in existing code that hasn't been updated to use nullable reference types.
Consider using
#nullable enableand#nullable disabledirectives to enable nullable reference types only for new code. This way, you can gradually update the existing codebase to use nullable reference types without being overwhelmed by warnings.Explorer/Assets/DCL/Multiplayer/Movement/Encoder/NetworkMessageEncoder.cs (1)
97-132: LGTM!The redefined
CompressMovementDatamethod enhances the clarity and maintainability of the code while ensuring that velocity data is accurately represented. The safety check for small velocity magnitudes and the refactored velocity compression logic are great improvements.Consider avoiding the allocation of a new
Vector2object on lines 103-106 by using thexandzcomponents of thepositionvector directly:-var relativePosition = new Vector2( - position.x - (parcel.x * ParcelMathHelper.PARCEL_SIZE), - position.z - (parcel.y * ParcelMathHelper.PARCEL_SIZE) // Y is Z in this case -); +float relativeX = position.x - (parcel.x * ParcelMathHelper.PARCEL_SIZE); +float relativeZ = position.z - (parcel.y * ParcelMathHelper.PARCEL_SIZE); -int compressedX = FloatQuantizer.Compress(relativePosition.x, 0, ParcelMathHelper.PARCEL_SIZE, xzBits); -int compressedZ = FloatQuantizer.Compress(relativePosition.y, 0, ParcelMathHelper.PARCEL_SIZE, xzBits); +int compressedX = FloatQuantizer.Compress(relativeX, 0, ParcelMathHelper.PARCEL_SIZE, xzBits); +int compressedZ = FloatQuantizer.Compress(relativeZ, 0, ParcelMathHelper.PARCEL_SIZE, xzBits);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- Explorer/Assets/DCL/Multiplayer/Movement/Encoder/FloatQuantizer.cs (1 hunks)
- Explorer/Assets/DCL/Multiplayer/Movement/Encoder/NetworkMessageEncoder.cs (4 hunks)
- Explorer/Assets/DCL/Multiplayer/Movement/NetworkMovementMessage.cs (1 hunks)
- Explorer/Assets/DCL/Multiplayer/Movement/Settings/MessageEncodingSettings.cs (1 hunks)
- Explorer/Assets/DCL/Multiplayer/Movement/Systems/MultiplayerMovementMessageBus.cs (2 hunks)
- Explorer/Assets/DCL/Multiplayer/Movement/Systems/PlayerMovementNetSendSystem.cs (3 hunks)
- Explorer/Assets/DCL/Multiplayer/Movement/Systems/csc.rsp (1 hunks)
- Explorer/Assets/DCL/Multiplayer/Movement/Systems/csc.rsp.meta (1 hunks)
- Explorer/Assets/DCL/Multiplayer/Movement/Tests/MovementMessageCompressionTests.cs (7 hunks)
- Explorer/Assets/DCL/Multiplayer/Movement/Tests/csc.rsp (1 hunks)
- Explorer/Assets/DCL/Multiplayer/Movement/Tests/csc.rsp.meta (1 hunks)
- Explorer/Assets/DCL/Multiplayer/Movement/csc.rsp (1 hunks)
- Explorer/Assets/DCL/Multiplayer/Movement/csc.rsp.meta (1 hunks)
- Explorer/Assets/Scripts/Global/Dynamic/DynamicWorldContainer.cs (1 hunks)
Files skipped from review due to trivial changes (4)
- Explorer/Assets/DCL/Multiplayer/Movement/Encoder/FloatQuantizer.cs
- Explorer/Assets/DCL/Multiplayer/Movement/Systems/csc.rsp.meta
- Explorer/Assets/DCL/Multiplayer/Movement/Tests/csc.rsp.meta
- Explorer/Assets/DCL/Multiplayer/Movement/csc.rsp.meta
Additional context used
Path-based instructions (7)
Explorer/Assets/DCL/Multiplayer/Movement/Settings/MessageEncodingSettings.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/Movement/NetworkMovementMessage.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/Movement/Systems/MultiplayerMovementMessageBus.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/Movement/Systems/PlayerMovementNetSendSystem.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/Movement/Encoder/NetworkMessageEncoder.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/Movement/Tests/MovementMessageCompressionTests.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/Global/Dynamic/DynamicWorldContainer.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 (33)
Explorer/Assets/DCL/Multiplayer/Movement/Tests/csc.rsp (1)
1-1: Enabling nullable reference types is a good practice, but be prepared to address warnings.Enabling nullable reference types using the
-nullable:enablecompiler option is a good practice as it helps prevent null reference exceptions and improves code quality by enforcing null-safety.However, enabling this feature might lead to new warnings in the existing codebase. Be prepared to address these warnings by updating the code to properly handle null values and adding necessary null checks.
Also, ensure that all team members are aware of this change and understand how to work with nullable reference types effectively.
Explorer/Assets/DCL/Multiplayer/Movement/csc.rsp (1)
1-1: Ensure project compatibility with nullable reference types.Enabling nullable reference types is a good practice to improve code quality and catch potential null reference exceptions at compile time. However, please ensure that the rest of the project is updated to properly annotate nullable and non-nullable reference types to avoid compilation errors.
Verification successful
Project is compatible with nullable reference types.
The codebase contains multiple files with the
#nullable enabledirective, and no related compilation errors were found. This suggests that the project is compatible with nullable reference types. Ensure ongoing code updates maintain this compatibility.
- Files with
#nullable enabledirective:
Explorer/Assets/Scripts/SceneRunner/Tests/SceneFacadeShould.csExplorer/Assets/Scripts/ECS/TestSuite/TestWebRequestController.csExplorer/Assets/Scripts/ECS/Unity/Materials/Pooling/MaterialsPool.csExplorer/Assets/Scripts/ECS/StreamableLoading/Tests/Urns/UrnsTest.csExplorer/Assets/Scripts/ECS/ComponentsPooling/Systems/FinalizeWorldSystemExtensions.csExplorer/Assets/DCL/WebRequests/WebContentSizes/ContentLengthBasedWebContentSizes.csExplorer/Assets/DCL/WebRequests/WebContentSizes/SeveralWebContentSizes.csExplorer/Assets/DCL/WebRequests/WebContentSizes/RangeBasedWebContentSizes.csExplorer/Assets/DCL/WebRequests/WebContentSizes/Playground/WebContentsSizesPlayground.csExplorer/Assets/DCL/Tests/Editor/CodeConventionTests.csExplorer/Assets/DCL/Tests/Editor/ValidationTests.csExplorer/Assets/DCL/SDKComponents/AudioSources/AudioSourceExtensions.csExplorer/Assets/DCL/SDKComponents/AudioSources/AudioUtils.csExplorer/Assets/DCL/SDKComponents/AudioSources/Components/AudioSourceComponent.csExplorer/Assets/DCL/Profiles/DataModels/ProfileJsonDto.csExplorer/Assets/DCL/PlacesAPIService/IPlacesAPIService.csExplorer/Assets/Plugins/SocketIO/SocketIOClient/Transport/WebSockets/DefaultClientWebSocket.csExplorer/Assets/Plugins/SocketIO/SocketIOClient/Transport/WebSockets/WebSocketReceiveResult.csExplorer/Assets/DCL/PerformanceAndDiagnostics/Optimization/PerformanceBudgeting/Memory/MemoryBudget.csExplorer/Assets/DCL/PerformanceAndDiagnostics/Optimization/PerformanceBudgeting/FrameTimeCapBudget.csExplorer/Assets/DCL/PerformanceAndDiagnostics/Analytics/AnalyticsController.csExplorer/Assets/DCL/PerformanceAndDiagnostics/Analytics/IAnalyticsController.csExplorer/Assets/DCL/Multiplayer/Movement/Components/RemotePlayerMovementComponent.csExplorer/Assets/DCL/Input/Utils/InteractionCache.csExplorer/Assets/DCL/AvatarRendering/Wearables/Helpers/WearablesConstants.csExplorer/Assets/DCL/AvatarRendering/AvatarShape/ComputeShader/FixedComputeBufferHandler.csScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the project compiles successfully with nullable reference types enabled. # Test: Search for compilation errors related to nullable reference types. rg --type cs $'#nullable enable' -A 10 | rg $'error CS8600|error CS8602|error CS8603|error CS8604|error CS8618|error CS8625'Length of output: 124
Script:
#!/bin/bash # Description: Verify the presence of `#nullable enable` directives and check for related errors in the codebase. # Search for files with `#nullable enable` directive. rg --type cs '#nullable enable' > nullable_files.txt # Check for nullable reference type errors in the identified files. if [ -s nullable_files.txt ]; then rg --file nullable_files.txt -A 10 | rg 'error CS8600|error CS8602|error CS8603|error CS8604|error CS8618|error CS8625' else echo "No files with #nullable enable directive found." fiLength of output: 187
Script:
#!/bin/bash # Description: List files containing the `#nullable enable` directive. # List files with `#nullable enable` directive. rg --type cs '#nullable enable' --files-with-matchesLength of output: 2030
Explorer/Assets/DCL/Multiplayer/Movement/Settings/MessageEncodingSettings.cs (1)
57-76: LGTM!The changes improve the encapsulation of the
MovementEncodingConfigclass by:
- Making the fields private and adding the
SerializeFieldattribute to allow serialization by Unity.- Introducing public properties to provide read-only access to the private fields.
These changes align with the best practices of object-oriented programming and do not introduce any issues. The code segment does not involve any heap allocations or require any Unity engine-specific optimizations.
Explorer/Assets/DCL/Multiplayer/Movement/NetworkMovementMessage.cs (4)
23-24: LGTM!The change from
inttobytefor thevelocityTierfield is a good optimization to reduce memory usage. The declaration looks good.
25-38: LGTM!The constructor update to include the new
velocityTierparameter looks good. The initialization of all the fields in the constructor is correct.
41-42: LGTM!The usage of the null-forgiving operator in the
ToStringmethod is a good indication that the JSON serialization is guaranteed to not yield a null result. The change looks good.
43-69: LGTM!The updates to the
GetHashCodeandEqualsmethods look good:
- Including the new
velocityTierfield in theGetHashCodemethod ensures that hash codes are unique and account for all relevant data.- The additional comparisons in the
Equalsmethod improve the accuracy of equality checks between instances of the struct.The implementation of both methods is correct.
Explorer/Assets/DCL/Multiplayer/Movement/Systems/MultiplayerMovementMessageBus.cs (5)
21-21: LGTM!The
null!operator is correctly used to suppress the null-state warning for themessageEncoderfield, which is later initialized in theInitializeEncodermethod.
Line range hint
27-37: LGTM!The constructor has been correctly simplified by removing the
schemeparameter and associated logic for handling different message compression schemes. The changes align with the PR objective of focusing on a single message format.
80-82: LGTM!The
WriteAndSendmethod has been correctly refactored to remove the conditional logic for handling uncompressed and compressed messages. The changes align with the PR objective of focusing on a single message format.
Line range hint
85-90: LGTM!The
WriteToProtomethod has been correctly updated to accept aCompressedNetworkMovementMessage, reflecting the new focus on compressed messages. The change aligns with the PR objective.
Line range hint
1-115: No issues found related to heap allocations or Unity engine optimizations.I reviewed the code for potential heap allocation improvements and Unity engine optimizations as per the additional instructions. Here are my findings:
- Heap allocations:
- The code does not appear to have any unnecessary heap allocations.
- The
CompressedNetworkMovementMessageobject is created using object initializer syntax, which does not cause additional heap allocations.
- Unity engine optimizations:
- The code does not directly interact with Unity engine APIs, so there are no specific Unity engine optimizations to suggest.
Overall, the code looks good from the perspective of heap allocations and Unity engine optimizations.
Explorer/Assets/DCL/Multiplayer/Movement/Systems/PlayerMovementNetSendSystem.cs (2)
Line range hint
115-125: LGTM!The refactoring of the velocity tier calculation logic into the
VelocityTierFromSpeedmethod improves code readability and modularity. The functionality remains unchanged, and the naming convention is consistent.
157-164: LGTM!The
VelocityTierFromSpeedmethod encapsulates the velocity tier calculation logic, which improves code readability and modularity. The implementation using a while loop is simple and efficient, and it does not allocate any memory on the heap, which is good for performance.Explorer/Assets/DCL/Multiplayer/Movement/Encoder/NetworkMessageEncoder.cs (11)
24-25: LGTM!The changes to the
Compressmethod and the refactoredCompressMovementDatamethod improve the accuracy and maintainability of the movement data compression logic.
62-63: LGTM!The change improves the readability of the code for extracting the
movementKindfrom the compressed temporal data.
64-65: LGTM!The change improves the readability of the code by assigning the decompressed velocity to a new variable
correctedVelocity.
68-69: LGTM!The change aligns with the updated compression logic that uses
velocityTierinstead oftier.
72-73: LGTM!The change aligns with the updated decompression logic that assigns the decompressed velocity to a new variable
correctedVelocity.
78-78: LGTM!The change aligns with the updated decompression logic that extracts the
movementKindfrom the compressed temporal data using a bitwise operation and a mask.
139-143: LGTM!The change aligns with the updated decompression logic that uses the
settingsobject to retrieve the encoding configuration.
146-147: LGTM!The change aligns with the updated decompression logic that uses the
settingsobject to retrieve the encoding configuration.
170-172: LGTM!The change aligns with the updated decompression logic that handles the sign of the velocity and its absolute value separately using the new
DecompressedVelocitymethod.
177-185: LGTM!The new
CompressedVelocitymethod enhances the clarity and maintainability of the code while ensuring that velocity data is accurately represented by handling the sign of the velocity and its absolute value separately.
187-200: LGTM!The new
DecompressedVelocitymethod enhances the clarity and maintainability of the code while ensuring that velocity data is accurately decompressed by handling the sign of the velocity and its absolute value separately. TheNegativeSignFlaghelper method also improves the readability of the code.Explorer/Assets/DCL/Multiplayer/Movement/Tests/MovementMessageCompressionTests.cs (7)
Line range hint
131-145: LGTM!The test case method looks good and aligns with the updated property name
XZ__BITSin thesettings.tier3object.
148-154: LGTM!The method looks good and aligns with the updated property name
Y__MAXin theSettings.tier3object.
167-173: LGTM!The test case method looks good and aligns with the updated property name
Y__MAXin thesettings.tier3object.
181-181: LGTM!The assertion statement looks good and aligns with the updated property name
Y__MAXin thesettings.tier3object.
188-191: LGTM!The method looks good and aligns with the updated property name
MAX__VELOCITYin theSettings.tier3object.
203-212: LGTM!The test case method looks good and aligns with the updated property name
MAX__VELOCITYin thesettings.tier3object.
241-272: LGTM!The new test case method
FloatCompressDecompressand the corresponding test case data generator methodFloatCompressDecompressTestCaseslook good. They enhance the test suite by providing coverage for the compression logic using theFloatQuantizerclass.Explorer/Assets/Scripts/Global/Dynamic/DynamicWorldContainer.cs (1)
438-438: Verify the implications of using the default scheme for MultiplayerMovementMessageBus.Removing the
MultiplayerMovementMessageBus.Scheme.Uncompressedparameter suggests the intention is to use compression for theMultiplayerMovementMessageBus. This change could impact:
- Network bandwidth usage
- CPU overhead for compressing/decompressing messages
Please verify if this change aligns with the expected behavior and performance requirements.
Run the following script to verify if compression is being used and assess the performance impact:
What does this PR change?
Fixed FloatQuantizer for approx zero values.
What's the issue was: the issue was in the way how Encoder compresses velocity. Previously it worked like: Encoder takes velocity and clamps it between range (for example -40 and 40) but Quantizer has a precision loss in the compression. When the value is '0' it cannot be encoded as '0', the compression step won't allow it (for example step is 0.33). And it encoded it as 0.33 instead of 0 what caused a constant movement of the avatar in the one direction by 3 axis. And it's the reason why avatars floated in the sky: the value was always positive and unprecised.
It's solved by the setting range from 0 to MaxValue and using one bit as a negative sign, [0; 40] in this example. So when value is actually 0 it's encoded as 0 because it's the closest value. And Encoder uses a sign bit to support negative vectors.
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
Release Notes
New Features
velocityTierfield for better categorization of movement data.Improvements
NetworkMovementMessagestruct for optimized memory usage.MultiplayerMovementMessageBus.Bug Fixes
Documentation
Tests