-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat: sdk camera control components #1744
Conversation
…at/sdk-camera-control-components
…at/sdk-camera-control-components
…at/sdk-camera-control-components
…at/sdk-camera-control-components
…d world plugin setting configuration refering to that prefab
… MainCamera due to protocol update
…at/sdk-camera-control-components
…at/sdk-camera-control-components
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.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- Explorer/Assets/DCL/PerformanceAndDiagnostics/Diagnostics/ReportsHandling/ReportCategory.cs (1 hunks)
- Explorer/Assets/DCL/SDKComponents/AvatarModifierArea/Systems/AvatarModifierAreaHandlerSystem.cs (2 hunks)
- Explorer/Assets/DCL/SDKComponents/CameraControl/CameraModeArea/Systems/CameraModeAreaHandlerSystem.cs (4 hunks)
- Explorer/Assets/DCL/SDKComponents/CameraControl/MainCamera/Systems/MainCameraSystem.cs (1 hunks)
- Explorer/Assets/DCL/SDKComponents/CameraControl/MainCamera/Systems/VirtualCameraSystem.cs (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- Explorer/Assets/DCL/PerformanceAndDiagnostics/Diagnostics/ReportsHandling/ReportCategory.cs
- Explorer/Assets/DCL/SDKComponents/AvatarModifierArea/Systems/AvatarModifierAreaHandlerSystem.cs
Additional context used
Path-based instructions (3)
Explorer/Assets/DCL/SDKComponents/CameraControl/MainCamera/Systems/VirtualCameraSystem.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/SDKComponents/CameraControl/MainCamera/Systems/MainCameraSystem.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 (8)
Explorer/Assets/DCL/SDKComponents/CameraControl/MainCamera/Systems/VirtualCameraSystem.cs (1)
1-86
: LGTM!The
VirtualCameraSystem
class is well-implemented and follows good practices. The use of dependency injection, attribute usage, and component pools is commendable. The class effectively handles the setup, removal, and finalization of virtual cameras. The code is clean, readable, and follows a clear structure.The changes are approved.
Explorer/Assets/DCL/SDKComponents/CameraControl/MainCamera/Systems/MainCameraSystem.cs (1)
101-101
: **Add a check for `sceneStateProvider.Explorer/Assets/DCL/SDKComponents/CameraControl/CameraModeArea/Systems/CameraModeAreaHandlerSystem.cs (6)
29-35
: ****The following comment from the previous review is still valid:
coderabbitai[bot]: Refactor suggestion: Consider using dependency injection for
cameraData
.The constructor now includes an additional parameter
IExposedCameraData cameraData
which is a significant change. To enhance testability and maintainability, consider using dependency injection forcameraData
instead of directly passing it through the constructor. This approach would make it easier to manage dependencies and mock them during testing.
59-83
: ****The following comment from the previous review is still valid:
coderabbitai[bot]: Optimization: Reduce complexity in
UpdateCameraModeArea
.The method
UpdateCameraModeArea
has been expanded significantly to handle new checks forEnteredAvatarsToBeProcessed
andExitedAvatarsToBeProcessed
. Consider refactoring this method to reduce complexity and improve readability. Splitting the method into smaller, more focused methods could help manage the different conditions and actions more clearly.
66-66
: ****The following comment from the previous review is still valid:
mikhail-dcl: You can remove nullable suppression as the lists are defined as non-nullables
25-25
: ****The following comment from the previous review is still valid:
mikhail-dcl: It would be great to pool this hashset on Initialize/Dispose
88-91
: LGTM!The code changes are approved.
97-101
: LGTM!The code changes are approved.
Also applies to: 126-130
Explorer/Assets/DCL/SDKComponents/CameraControl/MainCamera/Systems/MainCameraSystem.cs
Outdated
Show resolved
Hide resolved
Explorer/Assets/DCL/SDKComponents/CameraControl/MainCamera/Systems/MainCameraSystem.cs
Outdated
Show resolved
Hide resolved
Explorer/Assets/DCL/SDKComponents/CameraControl/MainCamera/Systems/MainCameraSystem.cs
Outdated
Show resolved
Hide resolved
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.
Actionable comments posted: 6
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- Explorer/Assets/DCL/SDKComponents/CameraControl/MainCamera/Systems/MainCameraSystem.cs (1 hunks)
Additional context used
Path-based instructions (1)
Explorer/Assets/DCL/SDKComponents/CameraControl/MainCamera/Systems/MainCameraSystem.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 (5)
Explorer/Assets/DCL/SDKComponents/CameraControl/MainCamera/Systems/MainCameraSystem.cs (5)
48-58
: LGTM!The
Update
method looks good. It delegates the work to specific query methods, which is a good practice. There are no obvious issues or potential optimizations in this method.
113-119
: LGTM!The
DisableVirtualCameraOnSceneLeave
method looks good. It has a clear purpose and delegates the actual disabling of the virtual camera to a separate method, which is a good practice. There are no obvious issues or potential optimizations in this method.
121-129
: LGTM!The
SetupMainCamera
method looks good. It has a clear purpose and adds the necessary component to the camera entity. There are no obvious issues or potential optimizations in this method.
131-137
: LGTM!The
HandleMainCameraRemoval
method looks good. It has a clear purpose and removes the necessary component from the camera entity. There are no obvious issues or potential optimizations in this method.
139-144
: LGTM!The
HandleMainCameraEntityDestruction
method looks good. It has a clear purpose and disables the active virtual camera when the main camera entity is destroyed. There are no obvious issues or potential optimizations in this method.
Explorer/Assets/DCL/SDKComponents/CameraControl/MainCamera/Systems/MainCameraSystem.cs
Show resolved
Hide resolved
Explorer/Assets/DCL/SDKComponents/CameraControl/MainCamera/Systems/MainCameraSystem.cs
Show resolved
Hide resolved
Explorer/Assets/DCL/SDKComponents/CameraControl/MainCamera/Systems/MainCameraSystem.cs
Outdated
Show resolved
Hide resolved
Explorer/Assets/DCL/SDKComponents/CameraControl/MainCamera/Systems/MainCameraSystem.cs
Show resolved
Hide resolved
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.
Good job 💪 I only have one question but can approve upfront
Explorer/Assets/DCL/SDKComponents/CameraControl/MainCamera/Systems/MainCameraSystem.cs
Outdated
Show resolved
Hide resolved
restored branch temporarily until I adapt the Test Coverage PR that currently is based on this branch |
Implements support for new SDK Camera Control components, high level changes:
MainCamera
plugin andMainCameraSystem
to handle the new SDK componentsMainCamera
andVirtualCamera
SDKComponents/CameraModeArea/
into newSDKComponents/CameraControl/CameraModeArea
(many file changes come from that)WriteCameraComponentSystem
to put theMainCamera
component on the scene camera CRDT Entity along with the other components already being set up during initialization** ANY CAMERA **
, maintaining the same existing easing and time. Behaviour-wise nothing changes regarding the existing character cameras.AvatarShapeVisibilitySystem
to change the main player avatar visibility based on camera distance instead of camera mode, since of course SDK Virtual Cameras are enabled far away from the main player. Commit.CameraMode.SDKCamera
to easily handle avoiding First Person chara input controls inCalculateCharacterVelocitySystem
when an SDK Virtual Cam is activated. This also automatically avoids any problem withAvatarModifierArea
. Commit.CameraMode
tracking inExposedCameraData
and injected it intoUpdateCursorInputSystem
to be able to hide the corsshair when an SDK camera is being used. Commit.BillboardSystem
to avoid it conflicting with a VirtualCamera on the same entity or too close to the billboard entity (e.g. in the parent entity). Commit.CharacterTriggerArea
plugin systems to avoid conflicting with the new SDK Camera, going from a "per frame" detected avatars collection to a "to be processed" avatars collection. Commit + Commit.Related PRs
QA TEST INSTRUCTIONS
pravus.dcl.eth
world for running the test scene (/goto pravus
)Things to test:
Using the Character default camera
Using the SDK Camera 1
Using the SDK Camera 2
Using the SDK Camera 3
Using the SDK Camera 4
Camera Mode Area
-> move inside the CameraModeArea that will force 1st person cam
-> press 'E' to activate any SDK camera (doesn't matter which one)
-> cycle through the cameras until you get back to the character camera
-> confirm that you are again under the effect of the Camera Mode Area
-> get out of the Camera Mode Area and confirm you can control the camera again
-> press 'E' to activate any SDK camera (doesn't matter which one)
-> move the character inside the CameraModeArea
-> cycle through the cameras until you get back to the character camera
-> confirm that you are now under the effect of the Camera Mode Area
-> get out of the Camera Mode Area and confirm you can control the camera again
-> move inside the CameraModeArea that will force 1st person cam
-> press 'E' to activate any SDK camera (doesn't matter which one)
-> move the character outside the Camera Mode Area
-> cycle through the cameras until you get back to the character camera
-> confirm that you free from the effect of the Camera Mode Area and you can control the camera again
-> move inside the CameraModeArea that will force 1st person cam
-> press 'E' to activate any SDK camera (doesn't matter which one)
-> move the character outside the Camera Mode Area
-> move the character inside the Camera Mode Area again
-> cycle through the cameras until you get back to the character camera
-> confirm that you are still under the effect of the Camera Mode Area
-> get out of the Camera Mode Area and confirm you can control the camera again
-> press 'E' to activate any SDK camera (doesn't matter which one)
-> move the character inside the Camera Mode Area
-> move the character outside the Camera Mode Area again
-> cycle through the cameras until you get back to the character camera
-> confirm that you are still free from the effect of the Camera Mode Area
Video reference for testing
SDKCameraControlDemo-NO-CONTROLLABLE-CAM.mp4
Summary by CodeRabbit
New Features
MainCameraSystem
for improved camera management.MainCameraComponent
for advanced camera control.MainCameraPlugin
for enhanced camera functionalities.Bug Fixes
Documentation
Chores