newton/sim/ik.py (2)

29-31: Address the TODO comment about enable_backward

This TODO suggests a temporary workaround. Please clarify why enable_backward needs to be enabled globally and consider a more localized solution if possible.

Would you like me to investigate alternative approaches to avoid setting this globally, or open an issue to track this technical debt?

---

454-454: Remove unused noqa directive

The noqa: PLC0415 directive appears to be unnecessary in the current linting configuration.

-        from newton.solvers.featherstone.kernels import (  # noqa: PLC0415
+        from newton.solvers.featherstone.kernels import (

newton/examples/example_ik_benchmark.py (4)

76-77: Consider renaming df to derivative for clarity.

The variable name df could be misleading as it commonly refers to DataFrames in data science contexts.

-        df = (dim + 1) * x**dim - 1.0
-        x_next = x - f / df
+        derivative = (dim + 1) * x**dim - 1.0
+        x_next = x - f / derivative

---

89-89: Use float('inf') instead of magic number for clarity.

Using float('inf') is more idiomatic and self-documenting than a large magic number.

-    best_cost = float(1.0e30)
+    best_cost = float('inf')

---

246-257: Extract quaternion multiplication to a utility function.

The inline quaternion multiplication implementation could be moved to a reusable utility function for better modularity.

Consider extracting this to a utility function in the newton.utils module or at the module level:

def quaternion_multiply(a, b):
    """Multiply two quaternions represented as [w, x, y, z] arrays."""
    w1, x1, y1, z1 = np.moveaxis(a, -1, 0)
    w2, x2, y2, z2 = np.moveaxis(b, -1, 0)
    return np.stack(
        (
            w1 * w2 - x1 * x2 - y1 * y2 - z1 * z2,
            w1 * x2 + x1 * w2 + y1 * z2 - z1 * y2,
            w1 * y2 - x1 * z2 + y1 * w2 + z1 * x2,
            w1 * z2 + x1 * y2 - y1 * x2 + z1 * w2,
        ),
        axis=-1,
    )

---

261-261: Add explanatory comment for rotation error calculation.

The rotation error calculation using arctan2 is mathematically correct but could benefit from an explanation.

+        # Calculate rotation error as the angle of the relative rotation quaternion
+        # Using 2*atan2(||v||, |w|) formula for quaternion angle extraction
         rot_err = (2 * np.arctan2(np.linalg.norm(rel[..., 1:], axis=-1), np.abs(rel[..., 0]))).max(axis=-1)

newton/examples/gizmo_utils.py (4)

268-269: Maintain consistency in sentinel values.

The code uses 1e9 as a threshold here but initializes with 1e10 elsewhere. These values should be consistent.

-    if min_capsule_dist < 1e9:
+    if min_capsule_dist < 1e10:

---

373-379: Move function definition outside the loop for better performance.

Defining a function inside a loop creates a new function object on each iteration, which can impact performance.

Move the arc_point function outside the loop and pass the required parameters:

def _arc_point(self, t, from_world, to_world, visual_pos, radius):
    sin_t1 = math.sin((1 - t) * (math.pi / 2))
    sin_t2 = math.sin(t * (math.pi / 2))
    direction = sin_t1 * from_world + sin_t2 * to_world
    return visual_pos + direction * radius

---

643-644: Consider making array size limits configurable.

The hardcoded limits could restrict usage in larger scenes. Consider making these configurable.

-        self.MAX_GIZMOS = 800
-        self.MAX_CAPSULES = self.MAX_GIZMOS * 27
+        self.MAX_GIZMOS = max_gizmos if hasattr(self, 'max_gizmos') else 800
+        self.MAX_CAPSULES = self.MAX_GIZMOS * 27

Or add parameters to the constructor:

def __init__(self, renderer, scale_factor=1.0, rotation_sensitivity=0.01, max_gizmos=800):

---

333-333: Add comment explaining the underscore convention.

The fill_ method uses an underscore suffix, which is a PyTorch/Warp convention for in-place operations but may be unfamiliar to some Python developers.

+                    # fill_ is an in-place operation (PyTorch/Warp convention)
                     off_d.fill_(sb_start)

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

newton/examples/gizmo_utils.py (1)

197-200: Good variable naming improvement!

The variable names axis_length_sq, axis_ray_dot, and axis_origin_dot are clear and descriptive, properly addressing the previous review feedback about cryptic names.

newton/examples/gizmo_utils.py (1)

584-584: Clarify the double scaling factor for rotation sensitivity.

The rotation delta is computed as -tangent_movement * rotation_sensitivity * 0.01, which applies two separate scaling factors. Consider either documenting why both factors are necessary or consolidating them into a single, well-documented parameter.

newton/sim/ik.py (2)

455-455: Remove unused noqa directive.

The noqa: PLC0415 directive appears to be unnecessary as indicated by static analysis. If the import-outside-toplevel rule is not enabled in your linter configuration, this directive can be safely removed.

-        from newton.solvers.featherstone.kernels import (  # noqa: PLC0415
+        from newton.solvers.featherstone.kernels import (

---

29-30: Address the TODO about backward mode configuration.

The TODO comment indicates uncertainty about why wp.config.enable_backward = True is necessary. This should be investigated and either:

1. Document why backward mode is required (e.g., for autodiff Jacobian computation)
2. Remove it if not needed, as it may impact performance
3. Enable it only when needed (e.g., when jacobian_mode includes AUTODIFF)

Would you like me to help investigate whether backward mode is required for all use cases or only for specific Jacobian modes?

newton/examples/example_ik_benchmark.py (2)

141-146: Consider using pathlib for more robust path handling.

The current check for path separators using "/" in self.cfg.asset or "\\" in self.cfg.asset is platform-specific. Consider using pathlib for more robust cross-platform path handling.

-        asset_path = (
-            Path(self.cfg.asset) / self.cfg.file
-            if ("/" in self.cfg.asset or "\\" in self.cfg.asset)
-            else newton.utils.download_asset(self.cfg.asset) / self.cfg.file
-        )
+        asset_path_obj = Path(self.cfg.asset)
+        if asset_path_obj.parts and len(asset_path_obj.parts) > 1:
+            # It's a path
+            asset_path = asset_path_obj / self.cfg.file
+        else:
+            # It's an asset name
+            asset_path = newton.utils.download_asset(self.cfg.asset) / self.cfg.file

---

367-407: Consider using a table formatting library for cleaner code.

The manual ASCII table construction works but could be simplified significantly using a library like tabulate or rich. This would improve maintainability and reduce code complexity.

Example with tabulate:

from tabulate import tabulate

def print_results(self):
    headers = ["", "robot", "batch", "newton-time (ms)", "newton-success (%)", 
               "newton-pos-err (mm)", "newton-ori-err (rad)"]
    
    table_data = [
        [i, r, b, f"{t:.6g}", f"{s:.3f}", 
         "nan" if np.isnan(pe) else f"{pe:.6g}",
         "nan" if np.isnan(oe) else f"{oe:.6g}"]
        for i, (r, b, t, s, pe, oe) in enumerate(self.results)
    ]
    
    print("\nReported errors are 98-percentile of successful solves\n")
    print(tabulate(table_data, headers=headers, tablefmt="grid"))

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

newton/sim/ik.py (2)

29-30: Address the TODO comment and configuration setting.

The TODO comment indicates uncertainty about why wp.config.enable_backward = True is necessary. This suggests a potential workaround that should be investigated and documented properly.

Do you want me to help investigate why this configuration is needed and provide proper documentation, or create an issue to track this technical debt?

---

454-454: Remove unused noqa directive.

The static analysis tool correctly identifies that the noqa directive for PLC0415 is unused and should be removed.

-        from newton.solvers.featherstone.kernels import (  # noqa: PLC0415
+        from newton.solvers.featherstone.kernels import (

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

Learnt from: dylanturpin
PR: newton-physics/newton#394
File: newton/examples/gizmo_utils.py:889-889
Timestamp: 2025-07-13T21:36:00.273Z
Learning: In Newton's gizmo system (newton/examples/gizmo_utils.py), the return value `return self.drag_state.mode == "rotate"` in the mouse drag handler is intentional design. It returns False for translation mode to allow the underlying pan/view callback to fire (so the view follows gizmo movement), and True for rotation mode to prevent the pan callback from firing (avoiding awkward view changes during rotation).