fix(VR): disable hmd menu for incompatible openvr#1309
Conversation
|
""" WalkthroughThis update introduces robust OpenVR version detection and compatibility enforcement within the VR feature. It adds methods for gathering and validating OpenVR DLL information, integrates compatibility checks throughout the VR overlay and menu UI, and refines input and overlay logic to conditionally enable VR features only when a whitelisted OpenVR version is detected. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Menu
participant VR
participant OpenVR DLL
User->>Menu: Open VR-related UI or overlay
Menu->>VR: Check IsOpenVRCompatible()
VR->>OpenVR DLL: DetectOpenVRInfo()
VR-->>Menu: Return compatibility status
alt OpenVR compatible
Menu->>VR: Show VR menus, overlays, process input
else OpenVR not compatible
Menu-->>User: Hide/disable VR menus and overlays
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
![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
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Features/VR.cpp(15 hunks)src/Features/VR.h(7 hunks)src/Hooks.cpp(2 hunks)src/Menu.cpp(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
src/Hooks.cpp (5)
Learnt from: alandtse
PR: #577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.
Learnt from: alandtse
PR: #577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use uint type instead of bool for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
Learnt from: alandtse
PR: #577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use uint type instead of bool for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-01T18:01:07.079Z
Learning: In the skyrim-community-shaders project, simple scalar constants in HLSL shaders use #define (e.g., #define NTHREADS 128), while more complex constants use static const within namespaces (e.g., Math namespace in Math.hlsli). For epsilon standardization, #define is the appropriate choice since epsilon values are simple scalar constants.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
src/Menu.cpp (1)
Learnt from: alandtse
PR: #1157
File: src/Feature.cpp:42-49
Timestamp: 2025-06-17T09:27:49.594Z
Learning: In src/Feature.cpp, when an obsolete feature's INI file is deleted, the feature should be silently disabled without surfacing any issues to the user. This is the intended behavior because a deleted INI file for an obsolete feature indicates that the user has properly cleaned up the obsolete feature.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (10)
src/Hooks.cpp (2)
15-15: LGTM: VR header include added correctly.The include is properly positioned with other feature headers and is necessary to access the VR compatibility methods used in the input polling logic.
591-592: VR input blocking logic is correctThe existing expression correctly:
- Blocks VR devices only when OpenVR is compatible (i.e., the in-game VR menu is active), preventing underlying game input.
- Allows all VR input when OpenVR is incompatible (disabling the HMD menu fallback to normal controls).
No changes required here.
src/Menu.cpp (6)
304-306: LGTM: Proper VR compatibility checking in overlay initialization.The change from
REL::Module::IsVR()toglobals::features::vr && globals::features::vr->IsOpenVRCompatible()correctly implements the new compatibility validation system while ensuring null safety.
1623-1625: LGTM: Consistent VR feature presence check for Frame Generation.The updated logic correctly disables Frame Generation when the VR feature is present, maintaining consistency with the new VR feature architecture.
1659-1661: LGTM: Proper VR compatibility gating for overlay texture recreation.The compatibility check ensures overlay textures are only recreated when OpenVR is compatible, preventing unnecessary operations on incompatible systems.
1663-1665: LGTM: Consistent VR compatibility check for controller input.The controller input processing is correctly gated by the same compatibility check pattern used throughout the file.
1677-1682: LGTM: Updated VR input event filtering logic.The input event filtering now correctly uses the new compatibility checking mechanism while maintaining the existing VR controller device detection logic.
1782-1784: LGTM: Proper VR compatibility check for overlay frame submission.The overlay frame submission is appropriately gated by the compatibility check, ensuring VR overlay operations only occur when OpenVR is compatible.
src/Features/VR.cpp (1)
74-96: Good defensive programming with early OpenVR detection.The implementation correctly detects OpenVR compatibility early in the initialization process and provides detailed logging. This prevents potential CTDs by avoiding incompatible OpenVR operations later.
src/Features/VR.h (1)
311-311: Well-designed overlay visibility logic.The updated condition properly checks OpenVR compatibility before allowing overlay visibility, preventing potential crashes. The integration with auto-hide seconds and menu state provides good user experience control.
|
✅ A pre-release build is available for this PR: |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Features/VR.cpp (1)
2306-2332: Overly restrictive compatibility approach remains unchanged.The strict whitelist still only allows a single specific OpenVR version, which may unnecessarily block users with potentially compatible versions. This matches the concern raised in previous reviews about being too restrictive.
Consider implementing the suggestions from the previous review:
- Support version ranges instead of exact matches
- Add user override options for advanced users
- Use a blacklist for known incompatible versions instead
The current approach may require frequent maintenance as new OpenVR versions are released and could prevent users from accessing VR features with compatible newer versions.
🧹 Nitpick comments (1)
src/Features/VR.cpp (1)
74-95: Good proactive compatibility detection approach.The early OpenVR detection and comprehensive logging will help users understand compatibility issues. Consider using debug level for some of the detailed logging (lines 82-86) to reduce verbosity in normal operation.
- logger::info("OpenVR DLL detected:"); - logger::info(" Path: {}", openVRInfo.dllPath); - logger::info(" Version: {}", openVRInfo.version); - logger::info(" Size: {} bytes", openVRInfo.fileSize); - logger::info(" Modified: {}", openVRInfo.modificationTime); + logger::info("OpenVR DLL detected: {} (Compatible: {})", openVRInfo.version, openVRInfo.isCompatible ? "Yes" : "No"); + logger::debug(" Path: {}", openVRInfo.dllPath); + logger::debug(" Size: {} bytes", openVRInfo.fileSize); + logger::debug(" Modified: {}", openVRInfo.modificationTime);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Features/VR.cpp(21 hunks)src/Utils/UI.h(1 hunks)src/Utils/VRUtils.cpp(2 hunks)src/Utils/VRUtils.h(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/Utils/UI.h
- src/Utils/VRUtils.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Utils/VRUtils.cpp
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Features/VR.cpp (6)
src/Features/VR.h (3)
GetSingleton(267-271)Both(120-120)Primary(106-106)src/Menu.h (1)
GetSingleton(16-20)src/Utils/UI.h (1)
HoverTooltipWrapper(53-62)src/Utils/UI.cpp (2)
HoverTooltipWrapper(25-32)HoverTooltipWrapper(34-40)src/Utils/VRUtils.cpp (2)
DrawButtonCombo(9-72)DrawButtonCombo(9-9)src/Utils/VRUtils.h (3)
GetControllerBothColor(35-35)GetControllerPrimaryColor(33-33)GetControllerSecondaryColor(34-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (9)
src/Features/VR.cpp (9)
20-21: LGTM - Necessary includes for version detection.The addition of
winver.hand linkingversion.libare required for the new OpenVR DLL version detection functionality.
51-51: Good improvement from boolean to configurable timeout.Changing from
ShowHowToUseMessagetokAutoHideSecondsprovides users with granular control over the overlay display duration instead of just on/off.
120-169: Excellent compatibility gating and timeout logic improvements.The early return for incompatible OpenVR versions prevents crashes, and the conversion from boolean to configurable timeout seconds provides better user control. The UI text appropriately directs users to the new setting location.
203-211: Consistent compatibility gating across VR-specific UI elements.The approach properly prevents users from configuring VR-specific features when OpenVR is incompatible, avoiding confusion and potential issues.
422-543: Excellent improvement to controller input instructions.The new table-based layout with color-coded controller information and comprehensive thumbstick instructions greatly improves usability and clarity for VR users.
773-789: Valuable debug information for troubleshooting compatibility.The OpenVR information section with color-coded compatibility status will greatly help users and developers diagnose VR-related issues.
1287-1291: Proper safety gating for overlay initialization.The compatibility check prevents overlay creation attempts with incompatible OpenVR versions, avoiding potential crashes.
1394-1397: Consistent safety gating for overlay operations.Early return for incompatible OpenVR maintains consistency with other overlay methods and prevents undefined behavior.
2241-2304: Robust OpenVR detection implementation.The OpenVR DLL detection logic correctly uses Windows APIs to gather comprehensive version and file information. The implementation handles edge cases well and provides detailed diagnostic data.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Features/VR.cpp (1)
2266-2357: Consider the restrictive whitelist approachThe current implementation uses a strict whitelist that only allows one specific OpenVR version (1.0.10.0). While this ensures stability, it may be overly restrictive and could unnecessarily disable VR features for users with potentially compatible versions.
Consider these improvements:
- Version range support instead of exact matches
- User override option for advanced users
- Blacklist approach for known incompatible versions
The detection logic itself is well-implemented with comprehensive version information gathering.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Features/VR.cpp(22 hunks)src/Features/VR.h(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/Features/VR.h (2)
src/Features/VR.cpp (6)
SetupResources(75-96)SetupResources(75-75)DetectOpenVRInfo(2266-2329)DetectOpenVRInfo(2266-2266)IsOpenVRCompatible(2331-2357)IsOpenVRCompatible(2331-2331)src/Globals.h (1)
globals(47-145)
src/Features/VR.cpp (6)
src/Features/VR.h (3)
GetSingleton(267-271)Both(120-120)Primary(106-106)src/Menu.h (1)
GetSingleton(16-20)src/Utils/UI.h (1)
HoverTooltipWrapper(53-62)src/Utils/UI.cpp (2)
HoverTooltipWrapper(25-32)HoverTooltipWrapper(34-40)src/Utils/VRUtils.cpp (2)
DrawButtonCombo(9-72)DrawButtonCombo(9-9)src/Utils/VRUtils.h (3)
GetControllerBothColor(35-35)GetControllerPrimaryColor(33-33)GetControllerSecondaryColor(34-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (26)
src/Features/VR.h (7)
245-246: LGTM! Well-chosen default and maximum values.The 30-second default auto-hide timeout provides a good balance between visibility and user experience, while the 5-minute maximum prevents indefinite overlay display.
291-291: LGTM! Proper override declaration for OpenVR compatibility detection.The SetupResources() override follows the framework pattern and will handle early OpenVR detection to prevent crashes with incompatible versions.
311-311: Excellent safety improvement! OpenVR compatibility gating prevents crashes.The updated visibility logic properly gates overlay display on OpenVR compatibility, preventing potential crashes with incompatible OpenVR versions while maintaining the existing auto-hide and menu state logic.
333-333: Good UX improvement! Fixed world positioning provides better stability.Changing the default from HMD-relative (0) to fixed world position (1) improves user experience by providing more stable overlay positioning that's less disorienting during head movements.
381-385: Well-designed settings additions for enhanced VR functionality.The new settings provide good configurability:
kAutoHideSecondsoffers precise timeout control replacing the boolean approachEnableDragToRepositiongives users control over drag functionalityVRMenuAutoResetDistance(1000 units ≈ 14.3m) is a sensible default for auto-reset detection
418-418: Proper validation! Auto-hide timeout clamping maintains data integrity.The clamping of
kAutoHideSecondsto [0, kMaxAutoHideSeconds] follows the established pattern and prevents invalid timeout values that could cause issues.
596-596: Well-designed additions for OpenVR compatibility and auto-reset functionality.The new member variables and method declarations provide clean interfaces for:
savedPlayerWorldPos: Reference point for auto-reset distance calculationsDetectOpenVRInfo(): OpenVR DLL detection and metadata gatheringIsOpenVRCompatible(): Compatibility validation against whitelistThe naming follows conventions and the design supports the safety-focused approach.
Also applies to: 604-605
src/Features/VR.cpp (19)
20-21: LGTM - Required includes for version detectionThe winver.h header and version.lib linkage are correctly added to support the new OpenVR version detection functionality.
51-52: Settings structure properly extendedThe changes correctly add the new
kAutoHideSecondstimeout setting andVRMenuAutoResetDistancefor the auto-reset functionality.
75-96: Well-implemented OpenVR compatibility detectionThe new
SetupResources()method correctly implements early OpenVR version detection with comprehensive logging. The approach of detecting compatibility early helps prevent crashes with incompatible versions.
121-122: Good early compatibility checkThe early return for incompatible OpenVR versions prevents unnecessary overlay operations and potential crashes.
126-149: Timeout logic correctly updatedThe change from boolean to integer-based timeout logic is properly implemented with consistent variable naming and calculations.
204-212: Smart conditional UI renderingHiding the "Bindings" tab when OpenVR is incompatible is a good UX decision that prevents user confusion and avoids exposing non-functional VR controls.
426-427: Consistent compatibility checkThe early return for incompatible OpenVR maintains consistency with other UI sections.
430-434: Well-designed auto-hide timeout controlThe slider with tooltip provides clear user control over the overlay timeout with helpful explanatory text.
435-542: Excellent UI reorganizationThe table-based layout for controller input instructions is much more organized and readable than the previous format. The color-coded controller information enhances usability.
564-565: Consistent compatibility enforcementThe early return maintains consistency with other settings sections when OpenVR is incompatible.
604-615: Valuable auto-reset functionalityThe auto-reset distance slider and manual reset button provide users with good control over fixed world position behavior. The formatted distance in the tooltip is a nice UX touch.
621-622: Consistent compatibility check in mouse settingsThe early return maintains proper gating of VR-specific functionality.
633-634: Consistent compatibility check in drag settingsThe early return maintains proper gating of VR-specific functionality.
787-803: Valuable diagnostic informationThe OpenVR Information section provides comprehensive debugging details including compatibility status, DLL version, and metadata. This will be very helpful for troubleshooting VR issues.
1095-1102: Useful debugging addressesThe OpenVR Addresses section provides developers with debugging information about internal OpenVR objects and their memory locations.
1312-1316: Essential compatibility checkThe early return in overlay initialization prevents potential crashes with incompatible OpenVR versions.
1419-1422: Important safety checkSkipping overlay operations when OpenVR is incompatible prevents crashes and undefined behavior.
1208-1220: Well-implemented auto-reset logicThe distance-based auto-reset functionality correctly tracks player movement and resets the overlay position when the configured threshold is exceeded. The position update after reset is handled properly.
1644-1644: Helpful debugging informationThe handedness change logging will assist in troubleshooting VR controller issues.
| // OpenVR version and compatibility information | ||
| struct OpenVRInfo | ||
| { | ||
| bool isAvailable = false; | ||
| bool isCompatible = true; | ||
| std::string dllPath; | ||
| std::string version; | ||
| uint64_t fileSize = 0; | ||
| std::string modificationTime; | ||
| } openVRInfo; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider defaulting isCompatible to false for better safety.
The OpenVRInfo struct design is excellent for tracking OpenVR metadata. However, consider changing isCompatible = true to isCompatible = false as the default. This ensures that OpenVR features are disabled by default and only enabled after explicit compatibility validation, providing better fail-safe behavior.
- bool isCompatible = true;
+ bool isCompatible = false;🤖 Prompt for AI Agents
In src/Features/VR.h around lines 585 to 594, change the default value of the
isCompatible member in the OpenVRInfo struct from true to false. This means
updating the initialization from isCompatible = true to isCompatible = false to
ensure OpenVR features are disabled by default until compatibility is explicitly
verified.
2 participants
Summary by CodeRabbit
New Features
Improvements