Skip to content

Fix invalid inline comment in Windows batch code block#5612

Merged
kellyguo11 merged 3 commits into
isaac-sim:developfrom
klakhi:klakhi/fix-windows-inline-comment-docs
May 15, 2026
Merged

Fix invalid inline comment in Windows batch code block#5612
kellyguo11 merged 3 commits into
isaac-sim:developfrom
klakhi:klakhi/fix-windows-inline-comment-docs

Conversation

@klakhi
Copy link
Copy Markdown
Contributor

@klakhi klakhi commented May 14, 2026

Summary

  • Fixed an invalid inline :: comment in the Windows batch code block on the kit-less installation page. In Windows batch, :: only works as a comment at the start of a line — when placed inline after a command, the tokens are passed as arguments, causing a runtime error. Moved the shorthand hint (or: isaaclab.bat -i) to its own comment line.

Test plan

  • Verify the rendered docs page shows the corrected batch snippet.
  • Confirm the isaaclab.bat --install command runs without unexpected extra arguments on Windows.

In Windows batch, `::` only works as a comment at the start of a line.
When placed inline after a command, it is passed as arguments. Move the
shorthand hint to a separate comment line so the documented command runs
correctly.
@github-actions github-actions Bot added bug Something isn't working documentation Improvements or additions to documentation isaac-lab Related to Isaac Lab team labels May 14, 2026
@klakhi klakhi requested review from myurasov-nv and removed request for Mayankm96 and jtigue-bdai May 14, 2026 12:29
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 14, 2026

Greptile Summary

This PR corrects a Windows batch syntax bug in the kit-less installation documentation where an inline :: comment was appended to an isaaclab.bat command — a pattern that causes batch to treat the comment tokens as command arguments rather than ignoring them.

  • The invalid isaaclab.bat --install :: or isaaclab.bat -i is split so the :: or: isaaclab.bat -i hint now lives on its own preceding comment line, matching how every other :: comment in the file is already written.
  • An empty .skip changelog fragment is added to suppress a user-facing changelog entry for this docs-only fix.

Confidence Score: 5/5

Safe to merge — this is a one-line documentation fix that corrects batch syntax with no impact on Python source or runtime behaviour.

The change is minimal and targeted: a single misplaced inline :: comment in a .rst code block is split onto its own line. The rest of the file is untouched, and the fix matches the established pattern used by every other batch comment block in the same document.

No files require special attention.

Important Files Changed

Filename Overview
docs/source/setup/installation/kitless_installation.rst Fixes invalid inline :: batch comment by moving it to its own dedicated comment line before the command, eliminating the unintended extra argument bug on Windows.
source/isaaclab/changelog.d/klakhi-fix-windows-inline-comment-docs.skip Empty .skip changelog fragment indicating this fix does not need a user-facing changelog entry.

Sequence Diagram

sequenceDiagram
    participant User
    participant CMD as Windows CMD
    participant Script as isaaclab.bat

    Note over User,Script: Before fix (inline :: comment)
    User->>CMD: isaaclab.bat --install   :: or isaaclab.bat -i
    CMD->>Script: "argv = ["--install", "::", "or", "isaaclab.bat", "-i"]"
    Script-->>User: Runtime error / unexpected arguments

    Note over User,Script: After fix (:: on its own line)
    User->>CMD: :: or: isaaclab.bat -i
    CMD->>CMD: Treats entire line as comment — skipped
    User->>CMD: isaaclab.bat --install
    CMD->>Script: "argv = ["--install"]"
    Script-->>User: Installs successfully
Loading

Reviews (1): Last reviewed commit: "Fix invalid inline comment in Windows ba..." | Re-trigger Greptile

Copy link
Copy Markdown

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

This PR has evolved significantly from the original scope (Windows batch comment fix). The latest changes (e8c56cd) now include substantial improvements to math utilities, camera sensors, and the LEAPP export infrastructure.

Findings

# Severity Finding
1 🟢 Praise Well-designed NaN-signaling for degenerate math cases
2 🟢 Praise Robust camera set_world_poses_from_view now handles edge cases gracefully
3 🟢 Praise LEAPP export refactor improves testability and modularity
4 🟢 Praise Comprehensive test coverage for new edge cases
5 🟡 Note PR scope has expanded substantially

Details

1. ✅ Math Utilities Edge Case Handling

create_rotation_matrix_from_view now properly handles:

  • eyes == targets (undefined forward direction) → returns NaN
  • Look-at direction parallel to up-axis → uses alternate reference vector
  • Non-finite input → returns NaN

quat_from_matrix now validates input:

  • Non-rotation matrices (singular, reflection, scale-error) → returns NaN
  • Unit quaternion check with 2e-5 tolerance

This is excellent defensive programming that prevents silent garbage propagation.

2. ✅ Camera Sensor Robustness

Both Camera.set_world_poses_from_view and RayCasterCamera.set_world_poses_from_view now:

  • Detect degenerate rows (eye == target)
  • Skip invalid poses with a warning
  • Raise ValueError when ALL rows are degenerate

Previously these methods would silently apply garbage transforms.

3. ✅ LEAPP Export Script Refactor

The export script has been restructured from module-level execution to a clean function-based API:

  • create_arg_parser() / parse_export_args() — argument handling
  • export_rsl_rl_agent() — core export logic
  • run_export_with_hydra() — Hydra integration
  • main_cli() — CLI entry point

Benefits:

  • Lazy runtime imports (avoids torch/NumPy fork issues with Kit startup)
  • Proper resource cleanup via try/finally
  • Better testability (export tests now run batches in-process)

4. ✅ Test Coverage

New tests for:

  • test_create_rotation_matrix_from_view_lookat_along_up_axis_z/y
  • test_create_rotation_matrix_from_view_zero_forward_returns_nan
  • test_create_rotation_matrix_from_view_batched_partial_failure
  • test_quat_from_matrix_singular_matrix_returns_nan
  • test_quat_from_matrix_reflection_returns_nan
  • test_quat_from_matrix_non_orthonormal_returns_nan

Export tests refactored to use batching, reducing Kit startup overhead.

5. 📝 Scope Note

The PR title is "Fix invalid inline comment in Windows batch code block" but now includes:

  • Isaac Sim AppLauncher CUDA device deferral fix
  • Math utility edge case handling
  • Camera sensor robustness improvements
  • PVA debug visualizer fixes
  • LEAPP export script refactor
  • Multiple version bumps and changelog entries

Consider whether this should be split into multiple PRs for cleaner git history. However, if these changes are interrelated (the LEAPP export tests likely surfaced the math utility edge cases), keeping them together is reasonable.

Minor Observations

  1. Import style change: from isaaclab.utils import configclassfrom isaaclab.utils.configclass import configclass (multiple files). This appears to be fixing a lazy-loader interaction issue.

  2. Test fixture cleanup: Removed unnecessary AppLauncher from test_noise.py and test_wrench_composer.py — good simplification.

  3. Changelog validation: New orphan-paragraph detection in tools/changelog/cli.py prevents Sphinx build failures.

Verdict

Approve — The changes are well-implemented with proper error handling, comprehensive tests, and good documentation in the changelogs. The expanded scope is noted but the changes appear coherent and beneficial.

@kellyguo11 kellyguo11 merged commit 2bdccb2 into isaac-sim:develop May 15, 2026
33 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working documentation Improvements or additions to documentation isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants