fix: avatar modifier area & avatar swap animations sync #2048
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces several modifications aimed at improving avatar visibility management and character control dynamics. Key changes include adjusting the Changes
Assessment against linked issues
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 (
|
github-actions
bot
requested review from
dalkia,
Ludmilafantaniella and
pravusjif
|
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: 1
Outside diff range and nitpick comments (1)
Explorer/Assets/DCL/AvatarRendering/AvatarShape/Components/AvatarShapeComponent.cs (1)
14-14: Consider the implications of makingHiddenByModifierAreaa public field.The change from a property with a private setter to a public field simplifies the management of the
HiddenByModifierAreastate by allowing direct access to its value. This eliminates the need for theUpdateHiddenStatusmethod, reducing overhead and improving readability.However, making the field public exposes it to potential unintended modifications from external code, which may lead to unexpected behavior or inconsistencies in the avatar's visibility state.
To maintain encapsulation while simplifying the code, consider the following alternative approach:
public bool HiddenByModifierArea { get; internal set; }By using an
internalsetter instead of making the field public, you can restrict access to the property within the same assembly, preventing unintended modifications from external code while still allowing direct access within the relevant scope.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- Explorer/Assets/DCL/AvatarRendering/AvatarShape/Components/AvatarShapeComponent.cs (1 hunks)
- Explorer/Assets/DCL/SDKComponents/AvatarModifierArea/Systems/AvatarModifierAreaHandlerSystem.cs (5 hunks)
- Explorer/Assets/DCL/SDKComponents/AvatarModifierArea/Tests/AvatarModifierAreaHandlerSystemShould.cs (2 hunks)
Additional context used
Path-based instructions (3)
Explorer/Assets/DCL/AvatarRendering/AvatarShape/Components/AvatarShapeComponent.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/AvatarModifierArea/Systems/AvatarModifierAreaHandlerSystem.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/AvatarModifierArea/Tests/AvatarModifierAreaHandlerSystemShould.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 (15)
Explorer/Assets/DCL/SDKComponents/AvatarModifierArea/Systems/AvatarModifierAreaHandlerSystem.cs (12)
45-48: LGTM!The
FinalizeComponentsmethod is correctly implemented as part of theIFinalizeWorldSysteminterface. It resets the affected entities when the system is finalized.
50-57: LGTM!The
ResetAffectedEntitiesmethod is correctly implemented. It shows the avatars that are currently inside the trigger area and removes theAvatarModifierAreaComponentfrom the entity, effectively resetting the state of the affected entities.
81-82: LGTM!The change to invoke the
HideAvatarmethod for each avatar transform inside the trigger area is correct. It updates the effect on now excluded/non-excluded avatars, aligning with the PR objectives.
85-86: LGTM!The change to invoke the
ShowAvatarmethod for each avatar transform that has exited the trigger area is correct. It shows the avatars that have exited the trigger area, aligning with the PR objectives.
90-92: LGTM!The change to invoke the
HideAvatarmethod for each avatar transform that has entered the trigger area is correct. It hides the avatars that have entered the trigger area, aligning with the PR objectives.
101-102: LGTM!The change to invoke the
ShowAvatarmethod for each avatar transform inside the trigger area is correct. It resets the state of affected entities when the entity is destroyed, aligning with the PR objectives.
113-113: LGTM!The change to invoke the
ShowAvatarmethod for each avatar transform inside the trigger area is correct. It resets the state of affected entities when the component is removed, aligning with the PR objectives.
120-130: LGTM!The change to set the
HiddenByModifierAreaproperty of theAvatarShapeComponenttofalseis correct. It explicitly shows the avatar, aligning with the PR objectives.
133-146: LGTM!The change to determine whether an avatar should be hidden based on whether its user ID is in the excluded IDs list is correct. It hides the avatar based on the exclusion criteria, aligning with the PR objectives. The
HiddenByModifierAreaproperty is set correctly based on theshouldHideflag.
177-178: LGTM!The change to check if the avatar's transform parent matches the required transform is correct. It finds the avatar entity based on the transform hierarchy, aligning with the PR objectives. The condition is correctly checked using the
transform.parentproperty.
Line range hint
1-181: No significant heap allocations found.The code in this file does not appear to introduce any significant heap allocations. The methods iterate over existing collections and query components using the ECS framework, which is designed to minimize allocations. No new collections or memory allocations on the heap are observed.
Line range hint
1-181: Code follows good practices for optimizing performance in Unity.The code in this file follows good practices for optimizing performance in Unity. It utilizes the ECS framework, which is designed for efficient querying and component access. The methods query components and entities efficiently using the ECS query system and work directly with transforms and components, avoiding unnecessary lookups or traversals.
Based on the current implementation, no specific Unity engine optimizations are required. The code already leverages the performance benefits of the ECS framework.
Explorer/Assets/DCL/SDKComponents/AvatarModifierArea/Tests/AvatarModifierAreaHandlerSystemShould.cs (3)
203-204: [Needs clarification] Change in avatar visibility management.The
ShowAvatarmethod call has been replaced with anUpdatemethod call. Can you provide more context on the rationale behind this change? Does theUpdatemethod achieve the desired behavior of hiding the avatar, as previously done byShowAvatar?To verify if the
Updatemethod achieves the desired behavior, consider adding a breakpoint or logging statement to check the state of theHiddenByModifierAreaproperty after theUpdatemethod call.
208-209: [Needs verification] Consistency in avatar visibility management.The change in this code segment is consistent with the previous one, replacing the
ShowAvatarmethod call with anUpdatemethod call. Please ensure that theUpdatemethod correctly handles showing the avatar, as previously done byShowAvatar.To verify if the
Updatemethod achieves the desired behavior, consider adding a breakpoint or logging statement to check the state of theHiddenByModifierAreaproperty after theUpdatemethod call.
229-230: [Needs verification] Consistency in avatar visibility management and filtering by excluded IDs.The change in this code segment is consistent with the previous ones, replacing the
ShowAvatarmethod call with anUpdatemethod call. Please ensure that theUpdatemethod correctly handles the filtering of avatars by excluded IDs, as previously done byShowAvatar.To verify if the
Updatemethod achieves the desired behavior, consider adding a breakpoint or logging statement to check the state of theHiddenByModifierAreaproperty for avatars with and without excluded IDs after theUpdatemethod call.
...r/Assets/DCL/SDKComponents/AvatarModifierArea/Tests/AvatarModifierAreaHandlerSystemShould.cs
Outdated
Show resolved
Hide resolved
lorux0
changed the title
This reverts commit cc1a722
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (4)
Explorer/Assets/DCL/CharacterTriggerArea/Components/CharacterTriggerAreaComponent.cs (1)
Line range hint
1-108: Good practices for minimizing heap allocations and optimizing performance in Unity.The code follows several good practices to minimize heap allocations and optimize performance in the Unity engine:
The use of object pooling (
IComponentPool) to manageCharacterTriggerAreainstances helps reduce heap allocations by reusing objects instead of creating new ones.The use of
IReadOnlyCollectionfor read-only access to collections avoids unnecessary allocations and clearly communicates the intent of read-only usage.The use of
TransformandTransformComponentfor efficient access to transform data minimizes the need for additional allocations.These practices contribute to better performance and reduced memory pressure in the Unity environment.
As an additional optimization, consider using
Vector3Intinstead ofVector3for integer-based positions if applicable.Vector3Intis more efficient for integer calculations and can help avoid floating-point precision issues.Explorer/Assets/DCL/SDKComponents/CameraControl/CameraModeArea/Systems/CameraModeAreaHandlerSystem.cs (1)
58-59: Looks good, but consider adding null checks.The change allows for a more direct response to player interactions without maintaining a separate collection of active areas. However, the use of the
!operator on theEnteredThisFrameandExitedThisFrameproperties suggests that these properties are nullable. Consider adding null checks to handle cases where these properties might be null to avoid potential null reference exceptions.-if (characterTriggerAreaComponent.EnteredThisFrame!.Count > 0) { OnEnteredCameraModeArea((CameraMode)pbCameraModeArea.Mode); } -else if (characterTriggerAreaComponent.ExitedThisFrame!.Count > 0) { OnExitedCameraModeArea(); } +if (characterTriggerAreaComponent.EnteredThisFrame?.Count > 0) { OnEnteredCameraModeArea((CameraMode)pbCameraModeArea.Mode); } +else if (characterTriggerAreaComponent.ExitedThisFrame?.Count > 0) { OnExitedCameraModeArea(); }Explorer/Assets/Scenes/Main.unity (2)
464-465: Clarify the meaning of theinitialRealmidentifier and improve documentation.The
initialRealmproperty has been updated from0to2. It's unclear what realm identifier2represents without additional context.Consider adding comments or documentation to clarify the meaning of the different realm identifiers used in the
launchSettings. This will make the code more maintainable and easier to understand for other developers.
493-495: Consider the implications of disabling the splash, authentication, and loading screens.The
showSplash,showAuthentication, andshowLoadingflags in thedebugSettingshave been set to0, disabling the corresponding screens. While this may streamline the user experience during development or testing, it's important to consider the implications in a production environment.
- Disabling the splash screen may affect branding and initial user perception.
- Disabling the authentication screen may skip important login or verification steps.
- Disabling the loading screen may lead to a jarring experience if there are significant load times.
Recommendation:
- Keep these flags enabled in production builds to ensure a smooth and secure user experience.
- Only disable these screens in development or testing environments for faster iteration and debugging.
- Consider using conditional compilation or build settings to toggle these flags based on the environment.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- Explorer/Assets/DCL/CharacterTriggerArea/Components/CharacterTriggerAreaComponent.cs (2 hunks)
- Explorer/Assets/DCL/CharacterTriggerArea/Tests/PlayMode/CharacterTriggerAreaHandlerSystemShould.cs (7 hunks)
- Explorer/Assets/DCL/PerformanceAndDiagnostics/Diagnostics/ReportsHandling/ReportsHandlingSettingsDevelopment.asset (1 hunks)
- Explorer/Assets/DCL/PluginSystem/World/CharacterTriggerAreaPlugin.cs (1 hunks)
- Explorer/Assets/DCL/SDKComponents/AvatarModifierArea/Systems/AvatarModifierAreaHandlerSystem.cs (5 hunks)
- Explorer/Assets/DCL/SDKComponents/CameraControl/CameraModeArea/Systems/CameraModeAreaHandlerSystem.cs (4 hunks)
- Explorer/Assets/DCL/SDKComponents/CameraControl/CameraModeArea/Tests/CameraModeAreaHandlerSystemShould.cs (1 hunks)
- Explorer/Assets/DCL/SDKComponents/csc.rsp (0 hunks)
- Explorer/Assets/DCL/SDKComponents/csc.rsp.meta (0 hunks)
- Explorer/Assets/DCL/SceneLoadingScreens/Assets/BackgroundMaterial.mat (1 hunks)
- Explorer/Assets/DCL/StylizedSkybox/Materials/GenesisSkybox.mat (3 hunks)
- Explorer/Assets/DCL/UI/LoadingSpinner/M_ChargeBarV2.mat (1 hunks)
- Explorer/Assets/Scenes/Main.unity (2 hunks)
- Explorer/ProjectSettings/QualitySettings.asset (2 hunks)
Files not reviewed due to no reviewable changes (2)
- Explorer/Assets/DCL/SDKComponents/csc.rsp
- Explorer/Assets/DCL/SDKComponents/csc.rsp.meta
Files skipped from review as they are similar to previous changes (1)
- Explorer/Assets/DCL/SDKComponents/AvatarModifierArea/Systems/AvatarModifierAreaHandlerSystem.cs
Additional context used
Path-based instructions (5)
Explorer/Assets/DCL/CharacterTriggerArea/Components/CharacterTriggerAreaComponent.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/CameraControl/CameraModeArea/Systems/CameraModeAreaHandlerSystem.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/World/CharacterTriggerAreaPlugin.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/CameraControl/CameraModeArea/Tests/CameraModeAreaHandlerSystemShould.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/CharacterTriggerArea/Tests/PlayMode/CharacterTriggerAreaHandlerSystemShould.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 (41)
Explorer/Assets/DCL/SceneLoadingScreens/Assets/BackgroundMaterial.mat (1)
46-46: Verify the reason for including this color change.The change in the
_Color1value indicates a color modification. However, it's unclear how this change is related to the PR objectives of fixing the avatar modifier area and avatar swap animations sync.Please provide more context on why this change is included in this PR. If it's unrelated, consider moving it to a separate PR to keep the changes focused and aligned with the PR objectives.
Explorer/Assets/DCL/UI/LoadingSpinner/M_ChargeBarV2.mat (2)
57-57: LGTM!Setting
_fillHeadto1ensures that the loading spinner appears fully filled when the loading is complete. This change aligns with the PR objective to accurately reflect the loading progress.
64-64: Verify if the color change is intended.The
_color01property has been changed from a light orange to a darker purple. This significantly affects the visual appearance of the loading spinner.Please confirm if this color change is intended, as the PR objectives and linked issues do not mention any requirements for changing the color scheme. If the change is unintended, consider reverting it to maintain consistency with the existing UI design.
Explorer/Assets/DCL/CharacterTriggerArea/Components/CharacterTriggerAreaComponent.cs (4)
14-15: Verify the necessity and impact of makingAreaSizepublic andmonoBehaviourinternal.Making
AreaSizepublic allows external modification, which could lead to unexpected behavior if not properly validated. Similarly, changingmonoBehaviourto internal enhances visibility within the assembly but could also lead to unintended access.Please ensure that these changes are necessary and consider the potential impact on encapsulation and maintainability. If external modification is required, consider implementing validation or using methods to control the modification of these fields.
18-20: LGTM!The property renaming from
EnteredAvatarsToBeProcessedtoEnteredThisFrameimproves code readability and clearly conveys the purpose of the collection. This change enhances maintainability and reduces potential confusion.
22-24: LGTM!The property renaming from
ExitedAvatarsToBeProcessedtoExitedThisFrameimproves code readability and clearly conveys the purpose of the collection. This change enhances maintainability and reduces potential confusion.
88-92: LGTM!The simplification of the
TryClearmethod improves code readability and maintainability by consolidating the functionality into a single conditional check. This change does not introduce any heap allocations or impact Unity engine optimizations.Explorer/Assets/DCL/SDKComponents/CameraControl/CameraModeArea/Systems/CameraModeAreaHandlerSystem.cs (10)
4-4: LGTM!The added using statement is necessary and does not introduce any runtime overhead.
7-7: LGTM!The added using statement is likely necessary and does not introduce any runtime overhead.
24-24: Good simplification!The removal of the
activeAreasHashSet simplifies the management of camera mode area entries and exits and reduces the memory footprint of the class.
29-29: Looks good, but please clarify the new camera data management approach.The removal of the
IExposedCameraDataparameter suggests a shift in how camera data is managed within the system. Please provide some context on how camera data is now handled and how this change affects the system's functionality.
56-56: LGTM!The simplification of the
UpdateCameraModeAreamethod by removing theentityparameter suggests a more streamlined approach to handling camera mode transitions. The change does not introduce any heap allocations or runtime allocations.
61-61: LGTM!The handling of the
pbCameraModeArea'sIsDirtyflag ensures that updates to the area size are still processed correctly. Setting theIsDirtyflag on thecharacterTriggerAreaComponentlikely triggers a recalculation or update of the trigger area. The change does not introduce any heap allocations or runtime allocations.Also applies to: 63-64
70-70: LGTM!The removal of the
entityparameter from theHandleEntityDestructionmethod reflects a shift towards a more generalized handling of entity destruction without needing to reference specific entities. The change does not introduce any heap allocations or runtime allocations.
78-78: LGTM!The modification of the
HandleComponentRemovalmethod to remove theentityparameter and replace it withEntity ereflects a shift towards a more generalized handling of component removal without needing to reference specific entities. Removing theCameraModeAreaComponentfrom the entity after exiting the camera mode area ensures proper cleanup. The change does not introduce any heap allocations or runtime allocations.Also applies to: 81-81
105-105: LGTM!The removal of the
entityparameter from theFinalizeComponentsmethod suggests a more generalized handling of component finalization without needing to reference specific entities. The change does not introduce any heap allocations or runtime allocations.
113-113: LGTM!Removing the
CameraModeAreaComponentduring finalization ensures that the component is properly cleaned up when the system is finalized. The change does not introduce any heap allocations or runtime allocations.Explorer/Assets/DCL/PluginSystem/World/CharacterTriggerAreaPlugin.cs (1)
76-76: LGTM!The removal of the
cameraDataparameter from theCameraModeAreaHandlerSystem.InjectToWorldmethod call is a valid simplification of the method's interface. It doesn't introduce any issues or impact the overall logic flow.Explorer/ProjectSettings/QualitySettings.asset (3)
131-131: LGTM!Reducing the LOD bias for the High quality level is a reasonable change to optimize performance, even if it slightly impacts visual fidelity at greater distances. This aligns with the PR's optimization goals.
148-148: LGTM!Decreasing the terrain detail density scale for the High quality level is a good change to enhance performance by reducing the rendering load for terrain details. This contributes to the PR's optimization goals.
150-150: Looks good, but consider the visual impact.Halving the terrain detail distance for the High quality level can optimize performance by rendering terrain details at a shorter distance, which aligns with the PR's optimization goals. However, be aware that this change may affect visual quality at greater distances. If the visual impact is acceptable, then this change is good to go.
Explorer/Assets/DCL/PerformanceAndDiagnostics/Diagnostics/ReportsHandling/ReportsHandlingSettingsDevelopment.asset (6)
301-302: LGTM!The addition of the
SDK_LOCAL_SCENE_DEVELOPMENTcategory with a severity level of 4 enhances the diagnostics settings for capturing and reporting logs related to local scene development in the SDK.
303-304: LGTM!The addition of the
SDK_CAMERAcategory with a severity level of 4 enhances the diagnostics settings for capturing and reporting logs related to the camera in the SDK.
305-306: LGTM!The addition of the
SDK_CAMERAcategory with a severity level of 0 enhances the diagnostics settings by explicitly specifying that logs related to the camera in the SDK should not be captured or reported.
307-308: LGTM!The addition of the
SDK_LOCAL_SCENE_DEVELOPMENTcategory with a severity level of 0 enhances the diagnostics settings by explicitly specifying that logs related to local scene development in the SDK should not be captured or reported.
309-310: LGTM!The addition of the
THUMBNAILScategory with a severity level of 0 enhances the diagnostics settings by explicitly specifying that logs related to thumbnails should not be captured or reported.
311-312: LGTM!The addition of the
THUMBNAILScategory with a severity level of 4 enhances the diagnostics settings for capturing and reporting logs related to thumbnails.Explorer/Assets/DCL/StylizedSkybox/Materials/GenesisSkybox.mat (4)
157-157: LGTM!The increased
_Cloud_Highlightsvalue enhances the visual prominence of the clouds in the skybox.
202-202: LGTM!The decreased
_SunSizevalue reduces the visual size of the sun in the skybox.
209-210: LGTM!The modifications to
_Sun_Radianceand_Sun_Radiance_Intensityadjust the brightness and intensity of the sun's appearance in the skybox.
224-224: LGTM!The comprehensive updates to the color properties (
_CloudsColor,_HorizonColor,_NadirColor,_RimColor,_SunColor, and_ZenitColor) refine the visual appearance of the skybox.Also applies to: 228-228, 232-233, 236-236, 238-238
Explorer/Assets/DCL/SDKComponents/CameraControl/CameraModeArea/Tests/CameraModeAreaHandlerSystemShould.cs (1)
38-38: LGTM!The removal of the
IExposedCameraDataparameter from theCameraModeAreaHandlerSystemconstructor simplifies the system's initialization for the test context. This change reduces the system's dependencies and streamlines the setup process without introducing any apparent issues or side effects within the test class.Explorer/Assets/DCL/CharacterTriggerArea/Tests/PlayMode/CharacterTriggerAreaHandlerSystemShould.cs (6)
137-138: LGTM!The code segment correctly updates the
AreaSizeproperty and setsIsDirtytotrueto trigger the update in the system. The test method verifies the expected behavior.
195-196: LGTM!The code segments correctly assert the expected counts of the
EnteredThisFrameandExitedThisFramecollections based on the character's position relative to the trigger area. The test method verifies the expected behavior of tracking the entered and exited avatars.Also applies to: 203-204, 212-213
231-232: LGTM!The code segments correctly assert the expected counts of the
EnteredThisFrameandExitedThisFramecollections based on the character's position and the scene state. The test method verifies the expected behavior of handling the player leaving the scene correctly, including incrementing the exited avatar count and disabling the box collider when the scene state is set to not current.Also applies to: 239-240, 248-249
268-269: LGTM!The code segments correctly assert the expected counts of the
EnteredThisFrameandExitedThisFramecollections based on the character's position. The test method verifies the expected behavior of detecting the character when already inside the trigger area.Also applies to: 275-276
298-299: LGTM!The code segments correctly assert the expected counts of the
EnteredThisFrameandExitedThisFramecollections based on the character's position and theAvatarBaseconfiguration. The test method verifies the expected behavior of waiting until theAvatarBaseis configured before detecting the character.Also applies to: 305-306, 314-315
338-339: LGTM!The code segments correctly assert the expected counts of the
EnteredThisFrameandExitedThisFramecollections based on the character's position and thetargetOnlyMainPlayerflag. The test method verifies the expected behavior of discriminating the character type correctly based on thetargetOnlyMainPlayerflag.Also applies to: 347-348, 357-358
Explorer/Assets/Scenes/Main.unity (3)
467-467: Ensure the predefined scenes are properly configured and thoroughly tested.The
enabledflag forpredefinedSceneshas been set to1, indicating that the feature is now active. Enabling predefined scenes allows specifying a set of parcels or locations to load by default.Please make sure to:
- Verify that the desired parcels are correctly specified in the
parcelsarray.- Test the loading and rendering of the predefined scenes to ensure a smooth user experience.
- Check for any performance impact or increased loading times when enabling this feature.
- Update the documentation or release notes to reflect the change in behavior.
469-469: Coordinates update ensures consistency betweentargetSceneandpredefinedScenes.The
parcelscoordinates in thepredefinedScenessection have been updated to match the newtargetScenecoordinates{x: 75, y: -7}. This change ensures consistency between the two settings.
465-465: Verify the new target scene coordinates are valid and accessible.The
targetScenecoordinates have been updated to{x: 75, y: -7}, likely representing a specific location within the scene.To ensure a smooth user experience, please verify that:
- The new coordinates are valid and within the bounds of the scene.
- The location at the specified coordinates is accessible and does not have any blockers or obstacles.
- The content at the new location is properly loaded and rendered.
You can use the following script to retrieve the scene's bounds and check if the new coordinates are within those bounds:
There was a problem hiding this comment.
It looks like you accidentally reverted the changes from main (encapsulation in AvatarModifierAreaHandlerSystem and names in AvatarModifierArea).
Can you please take you changes only and rebase them onto main (without merge)? it will help avoid all unnecessary changes.
There was a problem hiding this comment.
🟢 Approved by QA. This fix was verified on Windows and Mac. A smoke test was performed, covering:
- Backpack (change wereable isn't working but is not related to this PR)
- Emotes
- Nav bar (all shortcuts)
- Badges/Profile Customization
- Multiplayer
- Social interaction
- Map navigation
- Scenes/worlds (Doll House/Seed/Casa Roustan/Metadynelabs).
26.09.2024_14.29.16_REC.mp4
lorux0
dismissed
mikhail-dcl’s stale review
requested changes were fixed, since they were test changes and got reverted
What does this PR change?
Fixes #1643
This issue was related to a problem in the mutex (which was fixed in another pr). The
AvatarModifierAreaHandlerSystemwas never executed because the mutex was not acquired when needed.Additionally sets the character de-acceleration damping to zero. This forces the character to stop whenever the player releases the movement input.
The avatar swap scene plays the run & idle animations depending on the character position registry. If the character changed the position in the last frame, then it will execute the run animation. Since our character has a de-acceleration damping formula, it makes the character to make a minimal position change when the player stops moving, producing a desync bug.
Additional notes: if we want to keep the de-acceleration behaviour, the scene will require to change the movement checking by adding a threshold. However this works fine on unity-renderer, so this pr achieves the same result with no scene changes.
How to test the changes?
We cannot pre-download the AB of the avatar, so we expect a delay before the model appears. After you see it the first time, it should swap instantly by getting in and out of the area. Try to have a good internet connection.
Our Code Review Standards
https://github.com/decentraland/unity-renderer/blob/master/docs/code-review-standards.md
Summary by CodeRabbit
Summary by CodeRabbit
New Features
HiddenByModifierAreaproperty, improving performance and clarity.Bug Fixes