Updates for VIROO integration #292
Conversation
|
/review |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
@coderabbitai review |
WalkthroughThis pull request introduces a series of changes across the codebase. It enhances debugging clarity by adding explicit namespace prefixes and adjusting code formatting. New properties and methods are added to improve runtime configuration and lock state management. A suite of UI files, including UXML layouts, themes, and supporting C# classes and interfaces, have been introduced to build an in-world log console whose position updates relative to the user. Additionally, several new utility methods and extensive metadata updates for assets and XRInteraction files have been incorporated. Changes
✨ Finishing Touches
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: 3
🧹 Nitpick comments (10)
Source/Core/Runtime/UI/Console/LogConsole.uxml (2)
2-8: Consider making the UI more responsive and less hardcoded.The UI layout uses fixed positioning and hardcoded dimensions (e.g., width: 1016px, height: 54px) which might not scale well across different displays or VR headsets. Consider using relative units or flex layouts for better adaptability.
8-8: ListView might need more explicit styling.The LogList element doesn't have explicit width settings, which might cause layout issues in different contexts. Consider adding width constraints or using flex properties to ensure it displays properly within its parent container.
Source/Core/Runtime/UI/Console/LogConsole.cs (3)
13-15: Enforce non-null logItemTemplate.The serialized
logItemTemplateshould be validated to avoid null reference exceptions.[SerializeField] -private VisualTreeAsset logItemTemplate = null; +[SerializeField, Tooltip("Template for log items in the console")] +private VisualTreeAsset logItemTemplate = null; +private void OnValidate() +{ + if (logItemTemplate == null) + { + Debug.LogWarning("LogItemTemplate is not assigned in LogConsole. The console will not function correctly.", this); + } +}
61-79: Remove or implement the commented-out icon code.The commented code for setting icons appears to be using editor-only functionality (
EditorGUIUtility.IconContent), which won't work at runtime. Either implement a runtime-compatible version or remove the commented code.if (image != null) { - //switch (logs[index].LogType) - //{ - // case LogType.Log: - // image.style.backgroundImage = - // new StyleBackground((Texture2D)EditorGUIUtility.IconContent("console.infoicon").image); - // break; - // case LogType.Warning: - // image.image = EditorGUIUtility.IconContent("console.warnicon").image; - // break; - // case LogType.Error: - // image.image = EditorGUIUtility.IconContent("console.erroricon").image; - // break; - // case LogType.Exception: - // image.image = EditorGUIUtility.IconContent("console.erroricon").image; - // break; - //} + // TODO: Implement runtime icon display for different log types }
101-106: Add overflow protection for logs list.Consider adding a maximum capacity for the logs list to prevent memory issues with excessive logging.
+private const int MaxLogEntries = 100; public void LogMessage(string message, string details, LogType logType) { logs.Add(new LogMessage(message, details, logType)); + // Trim log if it exceeds maximum capacity + if (logs.Count > MaxLogEntries) + { + logs.RemoveAt(0); + } listView?.RefreshItems(); }Source/Core/Runtime/Utils/ComponentUtils.cs (1)
26-51: Improve error message for runtime persistent listener.The error message when attempting to add a persistent listener at runtime could be more informative.
#else - UnityEngine.Debug.LogError($"{target.name} attempted to add a persistent listener to {unityEvent.ToString()} at runtime. This is supported only at editor time."); + UnityEngine.Debug.LogError($"{target.name} attempted to add a persistent listener to event '{call.Method.Name}' at runtime. Adding persistent listeners is only supported during editor time."); return false; #endifSource/Core/Runtime/Utils/WorldConsole.cs (2)
48-143: Refactor duplicate code in logging methodsThe Log, LogWarning, LogError, and LogException methods contain very similar code patterns. Consider extracting the common functionality into a private helper method to improve maintainability.
+ private static void LogInternal(string message, string details, LogType logType, bool show) + { + if (console is null) + { + Debug.LogWarning($"World console not initialized. Message not logged: {message}"); + return; + } + + lock (executionQueue) + { + executionQueue.Enqueue(() => + { + console.LogMessage(message, details, logType); + + if (show) + { + console.Show(); + } + }); + } + + console.SetDirty(); + } public static void Log(string message, string details = "", bool show = false) { - lock (executionQueue) - { - executionQueue.Enqueue(() => - { - console.LogMessage(message, details, LogType.Log); - - if (show) - { - console.Show(); - } - }); - } - - console.SetDirty(); + LogInternal(message, details, LogType.Log, show); } // Similar changes for LogWarning, LogError, and LogException methods
146-147: Remove empty line at the end of the fileThere's an empty line at the end of the file that should be removed.
Source/Core/Runtime/Properties/LockableProperty.cs (2)
61-69: Clarify parameter naming in event emission methodsThe parameter name
isLockedin bothEmitLockedandEmitUnlockedmethods might be confusing since it's always set to theIsLockedproperty value. Consider renaming it tolockStatefor clarity or removing the parameter entirely if the methods are always called withIsLocked.protected void EmitLocked(bool isLocked) { - Locked?.Invoke(this, new LockStateChangedEventArgs(isLocked)); + Locked?.Invoke(this, new LockStateChangedEventArgs(lockState)); } protected void EmitUnlocked(bool isLocked) { - Unlocked?.Invoke(this, new LockStateChangedEventArgs(isLocked)); + Unlocked?.Invoke(this, new LockStateChangedEventArgs(lockState)); }
115-133: Consider making LogLockState virtualThe
LogLockStatemethod has been extracted fromRequestLockedand made protected, which is good for extensibility. However, to fully support customization in derived classes, consider making it virtual as well.- protected void LogLockState(bool lockState, IStepData stepData, bool canLock) + protected virtual void LogLockState(bool lockState, IStepData stepData, bool canLock)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Source/Core/Resources/Prefabs/DefaultWorldConsole.prefabis excluded by!**/*.prefabSource/Core/Runtime/UI/Console/LogConsoleMaterial.matis excluded by!**/*.matSource/Core/Runtime/UI/Console/LogConsolePanelSettings.assetis excluded by!**/*.asset
📒 Files selected for processing (34)
Source/Core/Editor/Input/InputEditorUtils.cs(4 hunks)Source/Core/Resources/Prefabs/DefaultWorldConsole.prefab.meta(1 hunks)Source/Core/Runtime/Configuration/BaseRuntimeConfiguration.cs(2 hunks)Source/Core/Runtime/ProcessController/BasicProcessLoader.cs(1 hunks)Source/Core/Runtime/Properties/LockableProperty.cs(6 hunks)Source/Core/Runtime/UI/Console.meta(1 hunks)Source/Core/Runtime/UI/Console/ConsoleElement.uxml(1 hunks)Source/Core/Runtime/UI/Console/ConsoleElement.uxml.meta(1 hunks)Source/Core/Runtime/UI/Console/ConsoleTransformHandler.cs(1 hunks)Source/Core/Runtime/UI/Console/ConsoleTransformHandler.cs.meta(1 hunks)Source/Core/Runtime/UI/Console/DefaultRuntimeTheme.tss(1 hunks)Source/Core/Runtime/UI/Console/DefaultRuntimeTheme.tss.meta(1 hunks)Source/Core/Runtime/UI/Console/ILogConsole.cs(1 hunks)Source/Core/Runtime/UI/Console/ILogConsole.cs.meta(1 hunks)Source/Core/Runtime/UI/Console/LogConsole.cs(1 hunks)Source/Core/Runtime/UI/Console/LogConsole.cs.meta(1 hunks)Source/Core/Runtime/UI/Console/LogConsole.renderTexture(1 hunks)Source/Core/Runtime/UI/Console/LogConsole.renderTexture.meta(1 hunks)Source/Core/Runtime/UI/Console/LogConsole.uxml(1 hunks)Source/Core/Runtime/UI/Console/LogConsole.uxml.meta(1 hunks)Source/Core/Runtime/UI/Console/LogConsoleMaterial.mat.meta(1 hunks)Source/Core/Runtime/UI/Console/LogConsolePanelSettings.asset.meta(1 hunks)Source/Core/Runtime/UI/Console/LogMessage.cs(1 hunks)Source/Core/Runtime/UI/Console/LogMessage.cs.meta(1 hunks)Source/Core/Runtime/Utils/ComponentUtils.cs(1 hunks)Source/Core/Runtime/Utils/ComponentUtils.cs.meta(1 hunks)Source/Core/Runtime/Utils/WorldConsole.cs(1 hunks)Source/Core/Runtime/Utils/WorldConsole.cs.meta(1 hunks)Source/XRInteraction/Source/Runtime/XRI/ClimbTeleportDestinationIndicator.cs.meta(1 hunks)Source/XRInteraction/Source/Runtime/XRI/ComponentLocatorUtility.cs.meta(1 hunks)Source/XRInteraction/Source/Runtime/XRI/StarterAssets/ControllerAnimator.cs.meta(1 hunks)Source/XRInteraction/Source/Runtime/XRI/StarterAssets/ControllerInputActionManager.cs.meta(1 hunks)Source/XRInteraction/Source/Runtime/XRI/StarterAssets/DynamicMoveProvider.cs.meta(1 hunks)Source/XRInteraction/Source/Runtime/XRI/StarterAssets/GazeInputManager.cs.meta(1 hunks)
✅ Files skipped from review due to trivial changes (24)
- Source/Core/Runtime/ProcessController/BasicProcessLoader.cs
- Source/Core/Runtime/Utils/WorldConsole.cs.meta
- Source/Core/Runtime/Utils/ComponentUtils.cs.meta
- Source/Core/Runtime/UI/Console/LogConsoleMaterial.mat.meta
- Source/Core/Runtime/UI/Console/LogConsolePanelSettings.asset.meta
- Source/Core/Runtime/UI/Console/ConsoleElement.uxml
- Source/Core/Runtime/UI/Console/LogConsole.renderTexture
- Source/Core/Runtime/UI/Console.meta
- Source/Core/Runtime/UI/Console/DefaultRuntimeTheme.tss
- Source/XRInteraction/Source/Runtime/XRI/StarterAssets/GazeInputManager.cs.meta
- Source/Core/Runtime/UI/Console/LogMessage.cs.meta
- Source/XRInteraction/Source/Runtime/XRI/StarterAssets/DynamicMoveProvider.cs.meta
- Source/XRInteraction/Source/Runtime/XRI/StarterAssets/ControllerAnimator.cs.meta
- Source/Core/Resources/Prefabs/DefaultWorldConsole.prefab.meta
- Source/Core/Runtime/UI/Console/ConsoleTransformHandler.cs.meta
- Source/Core/Runtime/UI/Console/LogConsole.uxml.meta
- Source/Core/Runtime/UI/Console/LogConsole.cs.meta
- Source/XRInteraction/Source/Runtime/XRI/ComponentLocatorUtility.cs.meta
- Source/XRInteraction/Source/Runtime/XRI/ClimbTeleportDestinationIndicator.cs.meta
- Source/Core/Runtime/UI/Console/LogConsole.renderTexture.meta
- Source/Core/Runtime/UI/Console/ILogConsole.cs.meta
- Source/XRInteraction/Source/Runtime/XRI/StarterAssets/ControllerInputActionManager.cs.meta
- Source/Core/Runtime/UI/Console/DefaultRuntimeTheme.tss.meta
- Source/Core/Runtime/UI/Console/ConsoleElement.uxml.meta
🔇 Additional comments (12)
Source/Core/Runtime/UI/Console/LogMessage.cs (1)
1-32: LGTM! Well-structured and properly documented struct.The
LogMessagestruct is well-designed with immutable properties (private setters) and clear documentation. The structure appropriately encapsulates the required data for console log messages with a clean interface.Source/Core/Runtime/UI/Console/LogConsole.uxml (1)
5-6: Potential resource dependency risks.The UI references specific resource paths using the url function which could break if resources are moved or renamed:
-unity-font-definition: url("project://database/Library/unity default resources?fileID=10102&guid=0000000000000000e000000000000000&type=0#LegacyRuntime");Consider using more robust resource loading mechanisms or style variables.
Source/Core/Editor/Input/InputEditorUtils.cs (3)
7-7: LGTM! Added appropriate namespace import.The addition of the
VRBuilder.Core.Configurationnamespace import is appropriate for accessing the RuntimeConfigurator class directly.
24-24: LGTM! Improved code formatting.This change improves code readability by adding spaces around the equality operator.
79-79: LGTM! Added explicit namespace qualification.The addition of the
UnityEngine.prefix toDebug.LogErrorcalls explicitly clarifies which Debug class is being used, preventing potential ambiguity with other Debug classes from different namespaces.Also applies to: 87-87, 96-96
Source/Core/Runtime/UI/Console/ILogConsole.cs (1)
1-38: LGTM! Well-structured interface for logging.The interface design is clean, with well-documented methods that clearly define the contract for a logging console. The separation of concerns is appropriate, with distinct methods for adding log entries, controlling visibility, and managing the console state.
Source/Core/Runtime/Configuration/BaseRuntimeConfiguration.cs (2)
16-16: Appropriate namespace added.The addition of the UI.Console namespace import is necessary for the new WorldConsole functionality.
121-142: Well-implemented lazy initialization pattern.The implementation of the WorldConsole property correctly follows the lazy initialization pattern, instantiating the console only when needed and providing appropriate error handling if the prefab can't be loaded.
Source/Core/Runtime/UI/Console/LogConsole.cs (1)
36-43:✅ Verification successful
Check WorldConsole.Refresh reference.
There's a reference to
WorldConsole.Refresh()which isn't clearly defined. Ensure this method exists and is accessible from this context.
🏁 Script executed:
#!/bin/bash # Check if WorldConsole.Refresh exists in the codebase echo "Searching for WorldConsole.Refresh method definition..." rg -A 3 -B 3 "public static void Refresh" --glob "*.cs"Length of output: 691
WorldConsole.Refresh Reference Verified
We confirmed that
WorldConsole.Refresh()is defined as a public static method inSource/Core/Runtime/Utils/WorldConsole.cs, ensuring it's accessible from theLogConsolecontext. No further changes are necessary.Source/Core/Runtime/Utils/ComponentUtils.cs (1)
1-24: LGTM! Well-implemented event listener check.The
HasPersistentListenermethod is well-structured and thoroughly checks for existing listeners.Source/Core/Runtime/Properties/LockableProperty.cs (2)
39-40: Potential risk with protected setter for IsLockedMaking the
IsLockedsetter protected allows derived classes to directly modify the lock state without going through theSetLockedmethod, potentially bypassing event notifications. Consider documenting this responsibility in a comment or ensuring derived classes useSetLockedinstead of directly modifying the property./// <inheritdoc/> - public virtual bool IsLocked { get; protected set; } + /// <summary> + /// The current lock state. Derived classes should use SetLocked method + /// rather than directly modifying this property to ensure events are fired. + /// </summary> + public virtual bool IsLocked { get; protected set; }
28-40: LGTM! Good approach to enhancing extensibilityThe changes to make
unlockersprotected, extract event invocation to dedicated methods, and make handler methods protected virtual significantly improve the extensibility of theLockablePropertyclass. This aligns well with the objective of opening up this class for extension.Also applies to: 61-90, 141-155
| void Update() | ||
| { | ||
| if (RuntimeConfigurator.Exists == false) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (RuntimeConfigurator.Configuration.User == null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| transform.position = RuntimeConfigurator.Configuration.User.Head.position + new Vector3(0, 0, distanceFromHead); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing console positioning and orientation.
The current implementation has a few potential improvements:
-
The
Updatemethod runs every frame, which is inefficient for this task. Consider using less frequent updates or event-based positioning. -
The console only updates its position but not its rotation, so it won't face the user as they turn their head. Consider adding rotation alignment:
- transform.position = RuntimeConfigurator.Configuration.User.Head.position + new Vector3(0, 0, distanceFromHead);
+ Transform headTransform = RuntimeConfigurator.Configuration.User.Head;
+ transform.position = headTransform.position + headTransform.forward * distanceFromHead;
+ transform.rotation = Quaternion.LookRotation(transform.position - headTransform.position);- Add a null check for the
Headproperty to prevent potential NullReferenceExceptions.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void Update() | |
| { | |
| if (RuntimeConfigurator.Exists == false) | |
| { | |
| return; | |
| } | |
| if (RuntimeConfigurator.Configuration.User == null) | |
| { | |
| return; | |
| } | |
| transform.position = RuntimeConfigurator.Configuration.User.Head.position + new Vector3(0, 0, distanceFromHead); | |
| } | |
| void Update() | |
| { | |
| if (RuntimeConfigurator.Exists == false) | |
| { | |
| return; | |
| } | |
| if (RuntimeConfigurator.Configuration.User == null) | |
| { | |
| return; | |
| } | |
| Transform headTransform = RuntimeConfigurator.Configuration.User.Head; | |
| transform.position = headTransform.position + headTransform.forward * distanceFromHead; | |
| transform.rotation = Quaternion.LookRotation(transform.position - headTransform.position); | |
| } |
There was a problem hiding this comment.
I would not add rotation at the moment as locking rotation to head rotation can be annoying for the user.
There was a problem hiding this comment.
@VaLiuM09 I understand your point about rotation - you're right that having UI elements constantly rotate with head movement can be disorienting for users in VR.
Would you still be interested in the other optimizations? For example:
void Update()
{
if (RuntimeConfigurator.Exists == false ||
RuntimeConfigurator.Configuration.User == null ||
RuntimeConfigurator.Configuration.User.Head == null)
{
return;
}
transform.position = RuntimeConfigurator.Configuration.User.Head.position + new Vector3(0, 0, distanceFromHead);
}This adds a null check for the Head property and combines the conditional checks for better readability.
✏️ Learnings added
Learnt from: VaLiuM09
PR: MindPort-GmbH/VR-Builder#292
File: Source/Core/Runtime/UI/Console/ConsoleTransformHandler.cs:13-26
Timestamp: 2025-02-28T13:35:54.716Z
Learning: In VR Builder's console implementation, locking UI rotation to head rotation is avoided as it can be annoying for users in VR environments.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| /// <summary> | ||
| /// Logs a message in the console. | ||
| /// </summary> | ||
| /// <param name="message">The message to be logged.</param> | ||
| /// <param name="details">Additional details to show when expanding the message.</param> | ||
| /// <param name="show">If true, show the console when the message is logged.</param> | ||
| public static void Log(string message, string details = "", bool show = false) | ||
| { | ||
| lock (executionQueue) | ||
| { | ||
| executionQueue.Enqueue(() => | ||
| { | ||
| console.LogMessage(message, details, LogType.Log); | ||
|
|
||
| if (show) | ||
| { | ||
| console.Show(); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| console.SetDirty(); | ||
| } |
There was a problem hiding this comment.
Add null check before accessing console properties
The Log method (and similarly other logging methods) doesn't verify if console is null before calling console.SetDirty() on line 69. While there's a null check in the Refresh method, the same protection should be applied in all methods that access the console.
public static void Log(string message, string details = "", bool show = false)
{
if (console is null)
{
Debug.LogWarning($"World console not initialized. Message not logged: {message}");
return;
}
lock (executionQueue)
{
executionQueue.Enqueue(() =>
{
console.LogMessage(message, details, LogType.Log);
if (show)
{
console.Show();
}
});
}
console.SetDirty();
}There was a problem hiding this comment.
We assume that the console has been correctly initialized in the constructor, else an error message is displayed at that time.
There was a problem hiding this comment.
@VaLiuM09 I understand your approach. You've designed the class to validate the console during initialization and report any issues at that time rather than adding checks throughout the code. Thanks for clarifying!
✏️ Learnings added
Learnt from: VaLiuM09
PR: MindPort-GmbH/VR-Builder#292
File: Source/Core/Runtime/Utils/WorldConsole.cs:48-70
Timestamp: 2025-02-28T13:37:25.567Z
Learning: In the WorldConsole class, it's intentionally designed to assume the console has been correctly initialized in the constructor. If initialization fails, an error message is displayed at that time rather than adding null checks in each method that uses the console.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Added basic console to show log messages in VR. It can be used by calling
WorldConsole.Log,WorldConsole.LogWarningetc, mirroring Unity's Debug.Log functions. It supports displaying logs from outside the main thread.Opened the LockableProperty to extension.
Added utility to add a persistent listener to an event at editor time:
ComponentUtils.AddPersistentListener.Summary by CodeRabbit