feat: portable experiences #1354
Conversation
Signed-off-by: Fran Colarich <[email protected]>
…raland/unity-explorer into feat/portable_experiences
|
Windows and Mac build successfull in Unity Cloud! You can find a link to the downloadable artifact below.
|
fcolarich
requested review from
mikhail-dcl,
lorux0 and
dalkia
and removed request for
mikhail-dcl and
lorux0
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- Explorer/Assets/Scripts/Global/Dynamic/Bootstraper.cs (2 hunks)
- Explorer/Assets/Scripts/Global/Dynamic/ChatCommands/LoadPortableExperienceChatCommand.cs (1 hunks)
- Explorer/Assets/Scripts/Global/Dynamic/ChatCommands/LoadPortableExperienceChatCommand.cs.meta (1 hunks)
- Explorer/Assets/Scripts/Global/Dynamic/DynamicWorldContainer.cs (8 hunks)
- Explorer/Assets/Scripts/Global/Dynamic/PortableExperiences/ECSPortableExperiencesController.cs (1 hunks)
- Explorer/Assets/Scripts/Global/Dynamic/PortableExperiences/IPortableExperiencesController.cs (1 hunks)
Files skipped from review due to trivial changes (1)
- Explorer/Assets/Scripts/Global/Dynamic/ChatCommands/LoadPortableExperienceChatCommand.cs.meta
Files skipped from review as they are similar to previous changes (3)
- Explorer/Assets/Scripts/Global/Dynamic/DynamicWorldContainer.cs
- Explorer/Assets/Scripts/Global/Dynamic/PortableExperiences/ECSPortableExperiencesController.cs
- Explorer/Assets/Scripts/Global/Dynamic/PortableExperiences/IPortableExperiencesController.cs
Additional context used
Path-based instructions (2)
Explorer/Assets/Scripts/Global/Dynamic/ChatCommands/LoadPortableExperienceChatCommand.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/Bootstraper.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 (2)
Explorer/Assets/Scripts/Global/Dynamic/ChatCommands/LoadPortableExperienceChatCommand.cs (1)
1-60: LGTM!The code for the
LoadPortableExperienceChatCommandclass is well-structured, follows best practices, and effectively implements the chat command functionality for loading portable experiences.Some key observations:
- The command syntax is clearly defined using a regular expression pattern, which is well-documented with an example.
- The
ExecuteAsyncmethod handles the command execution asynchronously, providing appropriate error handling and user feedback.- The code uses the
IPortableExperiencesControllerto create the portable experience, promoting a modular and decoupled design.- The code does not contain any obvious performance issues or potential runtime allocations.
Overall, the code changes are approved.
Explorer/Assets/Scripts/Global/Dynamic/Bootstraper.cs (1)
125-125: LGTM!The addition of
staticContainer.PortableExperiencesControlleras an argument to theDynamicWorldContainer.CreateAsyncmethod is approved.This change allows the
DynamicWorldContainerto access thePortableExperiencesControllerduring its creation, enabling the integration of portable experiences functionality into the dynamic world container.The modification is consistent with the overall structure and purpose of the
LoadDynamicWorldContainerAsyncmethod.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
Explorer/Assets/Scripts/Global/Dynamic/ChatCommands/KillPortableExperienceChatCommand.cs (1)
26-43: Consider avoiding runtime allocations.The
ExecuteAsyncmethod is allocating a newENSobject on every invocation. Consider reusing theENSobject to avoid runtime allocations.You can apply this diff to avoid runtime allocations:
-var response = portableExperiencesController.UnloadPortableExperienceByEns(new ENS(pxName)); +var ens = new ENS(pxName); +var response = portableExperiencesController.UnloadPortableExperienceByEns(ens);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- Explorer/Assets/Scripts/Global/Dynamic/ChatCommands/KillPortableExperienceChatCommand.cs (1 hunks)
- Explorer/Assets/Scripts/Global/Dynamic/ChatCommands/KillPortableExperienceChatCommand.cs.meta (1 hunks)
- Explorer/Assets/Scripts/Global/Dynamic/ChatCommands/LoadPortableExperienceChatCommand.cs (1 hunks)
- Explorer/Assets/Scripts/Global/Dynamic/DynamicWorldContainer.cs (8 hunks)
Files skipped from review due to trivial changes (1)
- Explorer/Assets/Scripts/Global/Dynamic/ChatCommands/KillPortableExperienceChatCommand.cs.meta
Files skipped from review as they are similar to previous changes (2)
- Explorer/Assets/Scripts/Global/Dynamic/ChatCommands/LoadPortableExperienceChatCommand.cs
- Explorer/Assets/Scripts/Global/Dynamic/DynamicWorldContainer.cs
Additional context used
Path-based instructions (1)
Explorer/Assets/Scripts/Global/Dynamic/ChatCommands/KillPortableExperienceChatCommand.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 (2)
Explorer/Assets/Scripts/Global/Dynamic/ChatCommands/KillPortableExperienceChatCommand.cs (2)
10-44: LGTM!The
KillPortableExperienceChatCommandclass is correctly implementing theIChatCommandinterface and injecting theIPortableExperiencesControllerdependency.
26-43: LGTM!The
ExecuteAsyncmethod is correctly handling theCancellationTokenand returning appropriate messages. It is also correctly calling theUnloadPortableExperienceByEnsmethod of theIPortableExperiencesControllerto unload a portable experience.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
Explorer/Assets/Scripts/ECS/SceneLifeCycle/SceneDefinition/Components/SceneDefinitionComponent.cs (1)
15-22: LGTM! Consider using auto-properties for brevity.The changes to the
SceneDefinitionComponentstruct look good:
- Changing fields to properties with getters improves encapsulation.
- The new constructor provides more flexibility in initializing the struct.
- The
IsPortableExperienceproperty is a useful addition.Consider using auto-properties for brevity:
-public SceneEntityDefinition Definition { get; } +public SceneEntityDefinition Definition { get; init; }This can be applied to all the properties in the struct.
Also applies to: 26-41
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- Explorer/Assets/Scripts/ECS/SceneLifeCycle/SceneDefinition/Components/SceneDefinitionComponent.cs (1 hunks)
- Explorer/Assets/Scripts/ECS/SceneLifeCycle/Systems/LoadPortableExperiencePointersSystem.cs (1 hunks)
- Explorer/Assets/Scripts/ECS/SceneLifeCycle/Systems/LoadScenePointerSystemBase.cs (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- Explorer/Assets/Scripts/ECS/SceneLifeCycle/Systems/LoadPortableExperiencePointersSystem.cs
- Explorer/Assets/Scripts/ECS/SceneLifeCycle/Systems/LoadScenePointerSystemBase.cs
Additional context used
Path-based instructions (1)
Explorer/Assets/Scripts/ECS/SceneLifeCycle/SceneDefinition/Components/SceneDefinitionComponent.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 (4)
Explorer/Assets/Scripts/ECS/SceneLifeCycle/SceneDefinition/Components/SceneDefinitionComponent.cs (4)
47-60: Verify the hardcoded values for portable experiences.The new constants and static readonly fields related to portable experiences have hardcoded values.
Please ensure that these values are correct and will not need to change in the future. If there's a possibility of these values changing, consider moving them to a configuration file.
62-65: LGTM!The changes to the
CreateFromDefinitionmethod look good. Introducing a factory method pattern for creatingSceneDefinitionComponentinstances based on theSceneEntityDefinitionis a nice way to encapsulate the initialization logic for different scenarios.
67-77: LGTM!The new
CreatePortableExperienceSceneDefinitionComponentmethod looks good. It encapsulates the logic for creating aSceneDefinitionComponentspecifically for portable experiences, using the predefinedPORTABLE_EXPERIENCES_PARCEL_CORNERSandPORTABLE_EXPERIENCES_SCENE_GEOMETRYfields for initialization.
86-104: LGTM!The changes to the
CreateEmptymethod look good. Utilizing theCreateSceneDefinitionComponentfactory method for creating aSceneDefinitionComponenthelps centralize the creation logic. Constructing theSceneEntityDefinitionwithin the method and passing it to the factory method is a nice way to encapsulate the initialization logic.
Explorer/Assets/Scripts/Global/Dynamic/ChatCommands/KillPortableExperienceChatCommand.cs
Outdated
Show resolved
Hide resolved
Explorer/Assets/Scripts/Global/Dynamic/ChatCommands/KillPortableExperienceChatCommand.cs
Outdated
Show resolved
Hide resolved
Explorer/Assets/Scripts/Global/Dynamic/PortableExperiences/ECSPortableExperiencesController.cs
Outdated
Show resolved
Hide resolved
Explorer/Assets/Scripts/Global/Dynamic/PortableExperiences/ECSPortableExperiencesController.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
Explorer/Assets/Scripts/Global/Dynamic/MainSceneLoader.cs (2)
Line range hint
216-266: Consider avoiding runtime allocations.The code creates new instances of
ENSandIPortableExperiencesController.SpawnResponsefor each portable experience. Consider reusing existing instances or object pooling to avoid unnecessary allocations at runtime.
Line range hint
216-266: Consider Unity engine optimizations.The code uses
foreachloops to iterate over the portable experiences. Consider usingforloops orIJobParallelForfor better performance, especially if the number of portable experiences is large.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- Explorer/Assets/Scripts/Global/Dynamic/ChatCommands/LoadPortableExperienceChatCommand.cs (1 hunks)
- Explorer/Assets/Scripts/Global/Dynamic/MainSceneLoader.cs (8 hunks)
- Explorer/Assets/Scripts/Global/Dynamic/PortableExperiences/ECSPortableExperiencesController.cs (1 hunks)
- Explorer/Assets/Scripts/Global/Dynamic/PortableExperiences/IPortableExperiencesController.cs (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- Explorer/Assets/Scripts/Global/Dynamic/ChatCommands/LoadPortableExperienceChatCommand.cs
- Explorer/Assets/Scripts/Global/Dynamic/PortableExperiences/ECSPortableExperiencesController.cs
- Explorer/Assets/Scripts/Global/Dynamic/PortableExperiences/IPortableExperiencesController.cs
Additional context used
Path-based instructions (1)
Explorer/Assets/Scripts/Global/Dynamic/MainSceneLoader.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 (6)
Explorer/Assets/Scripts/Global/Dynamic/MainSceneLoader.cs (6)
31-31: LGTM!The new property follows the coding conventions and is correctly defined.
40-40: LGTM!The new property follows the coding conventions and is correctly defined.
60-63: LGTM!The new fields follow the coding conventions and are correctly defined. The tooltips provide useful information.
78-82: LGTM!The modifications to the
Releasemethod correctly provide default values for the new fields.
86-87: LGTM!The new properties follow the existing pattern for debug settings and are correctly implemented.
Line range hint
216-266: LGTM!The code changes are well-structured and follow the existing coding conventions. The loading of portable experiences is implemented correctly based on the debug settings and feature flags.
What does this PR change?
In this PR we are adding the Portable Experiences functionality. It includes the API so scenes can trigger it and also enables temporarily the possibility to load the global PX that we will use for exploration minigames. This will be replaced with a connection to Unleash and downloading from there the PXs we want at the start of the session, but for now this is hardcoded.
Also, for now PX can only be started by using their ens, as I havent implemented loading from urns yet. It requires some further digging that for now is not needed for the exploration minigames.
The functionality of the PX is pretty non well defined, but basically they run all the time in all scenes and keep running even if players switch worlds.
So each PX is a new realm and also a new scene running on that realm using its own scene facade for it.
For now it looks practically the same as the normal scene facade, but as new functionality or compatibility is added to the PX, it will be changed.
Another thing that was not implemented as its not needed for now is disabling PXs when entering scenes that dont allow them. This will also be added further along the road when refining this and the exploration minigames shape is delivered.
...
How to test the changes?
The global PX will be loaded as soon as the game starts. I want to modify it so we can kill it by pressing a button on the PX UI, but that requires help from Pejo. So I hope tomorrow afternoon it can be done already.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style
Tests