Prioritize XDev hand tracker sources#483
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughThis PR extends Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
|
📝 Docs preview is not auto-deployed for fork PRs. A maintainer with write access to |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/live_trackers/cpp/live_hand_tracker_impl.cpp`:
- Around line 316-323: Before calling pfn_create_hand_tracker_ and push_back,
ensure the vector cannot reallocate: call trackers.reserve(trackers.size() + 1)
prior to creating the XrHandTrackerEXT so that if push_back would otherwise
trigger reallocation it won't and the newly-created handle won't be leaked;
apply the same reserve-before-create change for the other identical creation
site (the second XrHandTrackerEXT creation block) that uses session,
create_info, pfn_create_hand_tracker_, and trackers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f16693ef-8f2f-4642-8fc4-49ff1a5bd8a1
📒 Files selected for processing (3)
src/core/live_trackers/cpp/CMakeLists.txtsrc/core/live_trackers/cpp/live_hand_tracker_impl.cppsrc/core/live_trackers/cpp/live_hand_tracker_impl.hpp
| XrHandTrackerEXT tracker = XR_NULL_HANDLE; | ||
| const XrResult result = pfn_create_hand_tracker_(session, &create_info, &tracker); | ||
| if (XR_FAILED(result)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| trackers.push_back(tracker); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the full functions mentioned in the review
cd src/core/live_trackers/cpp && wc -l live_hand_tracker_impl.cppRepository: NVIDIA/IsaacTeleop
Length of output: 93
🏁 Script executed:
# Get the full function implementations including cleanup/error paths
sed -n '300,350p' src/core/live_trackers/cpp/live_hand_tracker_impl.cpp | cat -nRepository: NVIDIA/IsaacTeleop
Length of output: 2104
🏁 Script executed:
# Check if there are any RAII wrappers or cleanup mechanisms for XrHandTrackerEXT
rg -A 10 "XrHandTrackerEXT.*{" src/core/live_trackers/cpp/live_hand_tracker_impl.cpp | head -60Repository: NVIDIA/IsaacTeleop
Length of output: 164
🏁 Script executed:
# Search for destroy or cleanup patterns for XrHandTrackerEXT in this file
rg "xrDestroyHandTracker|destroy.*tracker" src/core/live_trackers/cpp/live_hand_tracker_impl.cppRepository: NVIDIA/IsaacTeleop
Length of output: 829
🏁 Script executed:
# Check the header file for class definition and resource management patterns
find src/core/live_trackers -name "*.h" -o -name "*.hpp" | xargs rg -l "LiveHandTrackerImpl|XrHandTrackerEXT" | head -5Repository: NVIDIA/IsaacTeleop
Length of output: 116
🏁 Script executed:
# Check the header file for class definition and resource management
cat -n src/core/live_trackers/cpp/live_hand_tracker_impl.hpp | head -100Repository: NVIDIA/IsaacTeleop
Length of output: 3568
🏁 Script executed:
# Look for the full class definition and member variables
rg -A 30 "class LiveHandTrackerImpl" src/core/live_trackers/cpp/live_hand_tracker_impl.hppRepository: NVIDIA/IsaacTeleop
Length of output: 1678
🏁 Script executed:
# Check how trackers vector is initialized and managed
rg "left_hand_trackers_|right_hand_trackers_" src/core/live_trackers/cpp/live_hand_tracker_impl.hppRepository: NVIDIA/IsaacTeleop
Length of output: 173
🏁 Script executed:
# Look at the destructor or cleanup to understand resource ownership
rg -B 5 -A 10 "~LiveHandTrackerImpl|destroy_hand_trackers" src/core/live_trackers/cpp/live_hand_tracker_impl.cpp | head -50Repository: NVIDIA/IsaacTeleop
Length of output: 1493
🏁 Script executed:
# Check if there's any reserve() calls on the vectors before adding trackers
rg "reserve\|shrink_to_fit" src/core/live_trackers/cpp/live_hand_tracker_impl.cppRepository: NVIDIA/IsaacTeleop
Length of output: 44
🏁 Script executed:
# Check where these functions are called and if there's any exception handling around them
rg -B 5 -A 5 "try_create_xdev_hand_tracker|try_create_default_hand_tracker" src/core/live_trackers/cpp/live_hand_tracker_impl.cpp | head -80Repository: NVIDIA/IsaacTeleop
Length of output: 1806
🏁 Script executed:
# Get more context around the initialize function to see the full try-catch
sed -n '180,280p' src/core/live_trackers/cpp/live_hand_tracker_impl.cpp | cat -nRepository: NVIDIA/IsaacTeleop
Length of output: 4172
🏁 Script executed:
# Check if there's any documentation or comments about xdev_id being a sentinel
rg "xdev_id|XrXDevIdMNDX" src/core/live_trackers/cpp/live_hand_tracker_impl.cpp | grep -i "0\|zero\|sentinel\|invalid"Repository: NVIDIA/IsaacTeleop
Length of output: 116
🏁 Script executed:
# Look for the XR_MNDX_xdev_space header to see if there's documentation
find . -name "XR_MNDX_xdev_space.h" -o -name "*xdev*" -type f | head -10Repository: NVIDIA/IsaacTeleop
Length of output: 116
🏁 Script executed:
# Check what XrXDevIdMNDX actually is
rg "XrXDevIdMNDX" --type h | head -20Repository: NVIDIA/IsaacTeleop
Length of output: 730
🏁 Script executed:
# Search for any comments explaining the zero check logic
rg -B 3 "xdev_id == 0" src/core/live_trackers/cpp/live_hand_tracker_impl.cppRepository: NVIDIA/IsaacTeleop
Length of output: 285
🌐 Web query:
OpenXR XrXDevIdMNDX zero sentinel value invalid documentation
💡 Result:
XrXDevIdMNDX is a type from the provisional OpenXR extension XR_MNDX_xdev_space, primarily implemented by the Monado OpenXR runtime and derivatives like WiVRn [1][2][3][4]. It represents an opaque handle or ID for "XDev" entities (likely external or experimental devices/spaces), defined as a struct in Rust bindings [5]. No official Khronos specification page was found, consistent with its provisional status not listed in standard registries [6][7]. Regarding zero as a sentinel/invalid value: OpenXR commonly uses 0 as invalid for handles (e.g., XR_NULL_HANDLE), and UINT64 types often treat 0 as invalid [8]. However, no source explicitly states this for XrXDevIdMNDX; documentation appears sparse or absent, potentially "invalid" in the sense of incomplete/missing details [9][5]. Likely, 0 is the invalid sentinel by convention, but check runtime-specific behavior in Monado source.
Citations:
- 1: https://lib.rs/crates/openxr_mndx_xdev_space
- 2: https://crates.io/crates/openxr_mndx_xdev_space
- 3: https://github.com/schmarni-dev/openxr_mndx_xdev_space
- 4: https://www.monado.dev
- 5: https://docs.rs/openxr_mndx_xdev_space/latest/openxr_mndx_xdev_space/all.html
- 6: https://github.khronos.org/OpenXR-Inventory/runtime_extension_support.html
- 7: https://registry.khronos.org/OpenXR/specs/1.0/man/html/XR_MND_headless.html
- 8: Section 2.13.1 XrDuration should be an unsigned value KhronosGroup/OpenXR-Docs#2
- 9: https://docs.rs/openxr_mndx_xdev_space
🌐 Web query:
OpenXR XR_MNDX_xdev_space specification XrXDevIdMNDX
💡 Result:
The OpenXR extension XR_MNDX_xdev_space is not documented in the official Khronos OpenXR registry/spec pages (no authored “XR_MNDX_xdev_space” section/manpage found in the OpenXR 1.1 specification index), so there is no authoritative “XR_MNDX_xdev_space specification” text to quote for XrXDevIdMNDX from Khronos here. [1] What can be grounded from available sources: 1) XR_MNDX_xdev_space is treated as an OpenXR extension implemented by Monado and Monado-based runtimes, and community bindings exist under the name “openxr_mndx_xdev_space”. [2][3][4] 2) The identifier XrXDevIdMNDX exists in those bindings (Rust crate) as a struct within the bindings module list (it’s one of the exported binding types). [5] 3) XrXDevIdMNDX and the related Xdev/XdevId space concept appear to be part of “XDev spaces” APIs used for “xdev” device/tracker space handling in Monado-based runtimes (this is implied by the crate’s purpose and Monado-related discussion). [2][4] However, due to missing Khronos spec text in the official spec pages we searched, I cannot reliably provide the exact OpenXR-level specification details (e.g., the precise typedef/struct definition for XrXDevIdMNDX, associated constants/enumerant values, or the exact function/parameter signatures for “XR_MNDX_xdev_space” and how to create/locate these spaces) without risking hallucination. If you want, paste the contents of XR_MNDX_xdev_space.h (from the OpenXR include headers you’re using) and I can extract the exact XrXDevIdMNDX definition/spec semantics from it precisely.
Citations:
- 1: https://registry.khronos.org/OpenXR/specs/1.1/html/xrspec.html
- 2: https://github.com/Schmarni-Dev/openxr_mndx_xdev_space
- 3: https://lib.rs/crates/openxr_mndx_xdev_space
- 4: Add support for the OpenXR tracker API supplied by Monado for Linux users godotengine/godot-proposals#13284
- 5: https://docs.rs/openxr_mndx_xdev_space/latest/openxr_mndx_xdev_space/all.html
🏁 Script executed:
# Verify that the catch handler doesn't somehow access the leaked handle
sed -n '120,180p' src/core/live_trackers/cpp/live_hand_tracker_impl.cpp | cat -nRepository: NVIDIA/IsaacTeleop
Length of output: 3008
🏁 Script executed:
# Check if there's any guard or RAII wrapper around tracker creation
rg -B 5 "XrHandTrackerEXT tracker = XR_NULL_HANDLE" src/core/live_trackers/cpp/live_hand_tracker_impl.cppRepository: NVIDIA/IsaacTeleop
Length of output: 555
Guard tracker creation against vector reallocation exceptions.
When trackers.push_back(tracker) reallocs and throws, the newly created XrHandTrackerEXT handle is not yet stored in the vector. The exception handler only destroys handles already in the vector, leaking the uninserted one. Call trackers.reserve(trackers.size() + 1) before creating the tracker to prevent reallocation during push.
Possible fix
bool LiveHandTrackerImpl::try_create_xdev_hand_tracker(XrSession session,
XrXDevIdMNDX xdev_id,
XrHandEXT hand,
std::vector<XrHandTrackerEXT>& trackers)
{
if (xdev_list_ == XR_NULL_HANDLE || xdev_id == 0)
{
return false;
}
+
+ trackers.reserve(trackers.size() + 1);
XrCreateHandTrackerXDevMNDX xdev_create_info{ XR_TYPE_CREATE_HAND_TRACKER_XDEV_MNDX };
xdev_create_info.xdevList = xdev_list_;
xdev_create_info.id = xdev_id;
@@
bool LiveHandTrackerImpl::try_create_default_hand_tracker(XrSession session,
XrHandEXT hand,
std::vector<XrHandTrackerEXT>& trackers)
{
+ trackers.reserve(trackers.size() + 1);
+
XrHandTrackerCreateInfoEXT create_info{ XR_TYPE_HAND_TRACKER_CREATE_INFO_EXT };
create_info.hand = hand;
create_info.handJointSet = XR_HAND_JOINT_SET_DEFAULT_EXT;Also applies to: 335-342
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/live_trackers/cpp/live_hand_tracker_impl.cpp` around lines 316 -
323, Before calling pfn_create_hand_tracker_ and push_back, ensure the vector
cannot reallocate: call trackers.reserve(trackers.size() + 1) prior to creating
the XrHandTrackerEXT so that if push_back would otherwise trigger reallocation
it won't and the newly-created handle won't be leaked; apply the same
reserve-before-create change for the other identical creation site (the second
XrHandTrackerEXT creation block) that uses session, create_info,
pfn_create_hand_tracker_, and trackers.
2 participants
Closes #344
This is a temporary fix until the CloudXR runtime has the right behaviour.
Summary by CodeRabbit
Release Notes