newton/_src/viewer/gl/opengl.py (1)

689-707: Bounds checks and active-count-based launches are sound; consider a small guard for the zero-active case

The new active_count logic and validation (active_count <= self.num_instances and len(scalings) == active_count when provided) make the API safer and prevent silent overruns; using dim=active_count and then rendering with self.active_instances is consistent. One small refinement you might consider: when active_count == 0 and both colors and materials are None, you could early-return instead of calling _update_vbo, avoiding an unnecessary host upload on empty updates.

     if active_count > 0:
         wp.launch(
             update_vbo_transforms,
             dim=active_count,
             inputs=[transforms, scalings],
             outputs=[self.world_xforms],
             device=self.device,
             record_tape=False,
         )

-    self.active_instances = active_count
-    # Upload the full buffer; only the first `active_instances` rows are rendered
-    self._update_vbo(self.world_xforms, colors, materials)
+    self.active_instances = active_count
+    # Upload the full buffer; only the first `active_instances` rows are rendered
+    if active_count > 0 or colors is not None or materials is not None:
+        self._update_vbo(self.world_xforms, colors, materials)

Also applies to: 711-725

newton/_src/viewer/viewer_gl.py (1)

287-303: Dynamic capacity growth is reasonable; consider explicit cleanup of old instancers on resize

The new resizing logic in both log_instances and log_points (initial capacity with headroom and doubling when exceeded) is a good fit for streaming workloads and avoids constant reallocations. Two follow‑ups worth considering:

1. Explicitly destroy old instancers when upsizing.
   In both methods, when you create a new MeshInstancerGL because the requested count exceeds num_instances, the old instancer is simply dropped from self.objects. Relying on GC and __del__ to free GL resources is less predictable than calling an explicit cleanup method (similar to how log_lines calls LinesGL.destroy() before reallocating).

   A simple pattern would be:

   elif transform_count > instancer.num_instances:

•  new_capacity = max(transform_count, instancer.num_instances * 2)
  

•  instancer = MeshInstancerGL(new_capacity, self.objects[mesh])
  

•  self.objects[name] = instancer
  

•  old_instancer = instancer
  

•  new_capacity = max(transform_count, instancer.num_instances * 2)
  

•  instancer = MeshInstancerGL(new_capacity, self.objects[mesh])
  

•  self.objects[name] = instancer
  

•  # If MeshInstancerGL grows a destroy() method mirroring MeshGL/LinesGL,
  

•  # call it here to promptly free GL resources.
  

•  if hasattr(old_instancer, "destroy"):
  

•      old_instancer.destroy()
  

  
  and analogously in `log_points`.
  
  

2. Hidden flag behavior.
   Using needs_update = resized or not hidden in log_instances means you still update GPU state when hidden only if capacity changed, which is a sensible trade‑off. Just keep in mind that while an instancer is hidden, calls to log_instances won’t refresh transforms/colors unless a resize happens; if you ever rely on “hidden but still up‑to‑date” instance data, you’ll want to drop the or not hidden part.

Overall the new behavior is correct; the cleanup tweak would mainly tighten resource management for long sessions.

Also applies to: 381-390

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.