Code Review: PR #5627 — Remove direct visualizer blank prints

Thank you for this cleanup PR! The changes improve user experience by deferring visualizer URL display until the simulation loop starts, preventing noise during initialization. The implementation is solid overall, with good test coverage for the new functionality.

🔬 Design Assessment

Architecture fit: The deferred startup message pattern is well-designed. Queuing messages in _log_viewer_url() and flushing via flush_startup_messages() in update_visualizers() ensures URLs appear at the right time (just before the simulation loop) rather than during potentially noisy initialization.

Cross-module impact: Self-contained. The changes affect only the visualizer subsystem and don't break any existing interfaces.

🔵 Suggestions

1. flush_startup_messages() called on every frame — source/isaaclab/isaaclab/sim/simulation_context.py:764

The flush_startup_messages() method is called on every update_visualizers() invocation. While the implementation clears the list after printing (making subsequent calls no-ops), iterating over all visualizers and calling this method every frame is unnecessary overhead for a one-time operation.

Consider tracking whether startup messages have been flushed to avoid repeated calls:

# In SimulationContext
if not self._visualizers:
    return

if not getattr(self, '_startup_messages_flushed', False):
    for viz in self._visualizers:
        viz.flush_startup_messages()
    self._startup_messages_flushed = True

Alternatively, flush only once per visualizer by adding a flag in BaseVisualizer.

2. verbose parameter disconnect in NewtonViewerViser — source/isaaclab_visualizers/isaaclab_visualizers/viser/viser_visualizer.py:272

NewtonViewerViser.__init__ accepts a verbose parameter, but ViserVisualizer._create_viewer() always passes verbose=False. This means the verbose parameter in NewtonViewerViser has no effect at runtime. Consider either:

• Removing the verbose parameter from NewtonViewerViser since it's always False
• Or passing self.cfg.verbose to allow users to control internal viser logging

This is minor since the current behavior (suppress internal logs, use cfg.verbose for Isaac Lab URL logging) appears intentional.

🧪 Test Coverage

The PR includes good test coverage:

• ✅ test_rendering_without_visualizers_initializes_kit_backend — verifies rendering mode still creates Kit backend
• ✅ test_non_rendering_without_visualizers_defers_backend_initialization — verifies no backend when not needed
• ✅ test_kit_visualizer_initializes_kit_backend_when_rendering_flag_is_false — verifies visualizer overrides rendering flag
• ✅ test_rendering_context_authors_visible_usd_point_instancer — verifies USD markers work with rendering
• ✅ test_viser_visualizer_open_browser_is_opt_in — verifies open_browser=False default
• ✅ test_rerun_visualizer_open_browser_is_opt_in — verifies open_browser=False default
• ✅ test_web_visualizer_cfgs_do_not_open_browser_by_default — verifies config defaults

The deferred message flushing is exercised through integration tests. Consider adding a unit test specifically for _log_viewer_url() URL formatting with edge cases (e.g., URLs without ports).

✅ Summary

Verdict: Ship it

The changes are well-implemented with appropriate test coverage. The suggestions above are minor improvements that could be addressed in follow-up work. The fix for visualization marker backend initialization (sim.is_rendering check) is a good catch that ensures USD markers work during camera rendering even without explicit visualizers.