OMPE-96097: Simplify kitless visual color randomization

[ooctipus]

Supersedes #6269 while preserving the two original Jessica Martinez commits and authorship.

Why

The original implementation proved the feature, but the first follow-up over-generalized it with shared clone-plan machinery, private renderer discovery, broad lifecycle changes, and excessive test duplication. This revision keeps the feature while removing those layers.

Scope

• The only framework change is a five-line optional EventManager hook that runs before PHYSICS_READY. OVRTX must author materials before its camera callback snapshots the USD stage; normal reset/startup class terms are instantiated after that snapshot.
• There are no net changes to ManagerBase, ManagerBasedEnv, or RenderContext.
• Backend selection, event orchestration, and the existing Kit/Replicator path remain in events.py.
• Newton and OVRTX writers remain in their optional extension packages, avoiding optional backend imports from core.
• Newton maps shapes once and updates model.shape_color through a zero-copy Torch view with linear-to-sRGB conversion.
• OVRTX pre-bakes clone-local strongly bound OmniPBR materials and performs runtime writes through one public OVRTXRenderer.write_visual_colors method.
• Ambiguous cases fail explicitly instead of adding generalized machinery: multiple renderer instances, heterogeneous OVRTX clone plans, body-id-only pre-physics targeting, and missing OVRTX material preparation.

Size

• 587 production additions / 119 deletions.
• 700 test and fixture additions / 2 deletions.
• 25 changelog lines.
• 1,312 total additions, down from 3,260 in the previous revision.

Validation

• 16 passed: lifecycle, target selection, Newton writer, and OVRTX writer focused suites.
• 6 passed: full real Isaac RTX color-randomization suite.
• 1 passed: real RTX render-readback contract.
• All pre-commit hooks pass on the 22 changed files.
• The native OVRTX render contract is collected from the isaaclab_ov package test tree; it skips locally because the optional ovrtx wheel is absent and will run in the package CI job.

Deliberate constraints

• Per-environment visual color randomization requires replicate_physics=False.
• Kitless Newton and OVRTX support startup and reset, not prestartup.
• A term supports exactly one active renderer instance.
• OVRTX currently supports homogeneous InteractiveScene clone plans and requires a conventional /visuals layout or explicit mesh_name.
• OVRTX targets must have a bound OmniPBR diffuse color so pre-baking cannot silently change untouched objects.

[greptile-apps]

Greptile Summary

This PR replaces the original Kit-only Replicator event with a fully refactored, three-backend visual-color randomization system (RTX/Isaac, kitless Newton, OVRTX). It introduces a shared backend-neutral VisualColorTargetPlan, a new prepare_before_physics_ready lifecycle hook on ManagerTermBase, and three independent writer classes that each own their renderer-specific stage mutations.

• Architecture: EventManager now calls prepare_before_physics_ready on all class-based terms before PHYSICS_READY, enabling OVRTX/RTX to author USD materials before the renderer bakes the stage; the resolved plan is passed through cfg.__dict__ to __init__, which constructs a lazy writer after physics starts.
• Newton writer: resolves shape rows once at construction into a padded index tensor, then performs a zero-copy wp.to_torch scatter with GPU-side sRGB encoding; the test suite includes a source-inspection assertion that forbids any host-sync call on the write path.
• OVRTX writer: pre-bakes deterministic OmniPBR materials under a SHA-256-keyed scope before prepare_stage, then limits runtime writes to the new OVRTXRenderer.write_visual_colors boundary.

Confidence Score: 4/5

Safe to merge with minor follow-up; the new multi-backend architecture is solid and the test suite is thorough across all three renderer paths.

The rework is well-structured and heavily tested (69 tests across RTX, Newton, OVRTX, and unit suites). Two style-level concerns are worth noting: the colors identity dispatch in randomize_visual_color.call creates an undocumented coupling to the EventManager argument-passing behaviour, and the _find_matching_visual_prims token regex can misparse alternation body-name patterns, adding unexpected false-positive prims to the proxy-walk candidates. Neither causes incorrect output on any known codepath.

source/isaaclab/isaaclab/utils/visual_color.py (regex token BFS) and source/isaaclab/isaaclab/envs/mdp/events.py (call color dispatch) warrant a second look; all other files are clean.

Important Files Changed

Filename	Overview
source/isaaclab/isaaclab/envs/mdp/events.py	Refactored randomize_visual_color to a multi-backend architecture; added backend resolution, pre-physics-ready hook, and lazy writer construction. One subtle fragility in the per-call color-dispatch path.
source/isaaclab/isaaclab/utils/visual_color.py	New backend-neutral target plan resolver; prim traversal, clone-plan integration, initial color reading, and env-id normalization. Token-level regex compilation inside _find_matching_visual_prims is unescaped.
source/isaaclab/isaaclab/managers/event_manager.py	Adds prepare_before_physics_ready hook dispatch and promotes mode-validation checks before the hook runs; logic is correct.
source/isaaclab/isaaclab/managers/manager_base.py	Adds no-op prepare_before_physics_ready classmethod to ManagerTermBase and guards callback registration against None cfg; both changes are safe.
source/isaaclab_newton/isaaclab_newton/visual/newton_shape_color_writer.py	New kitless Newton sink: resolves shape rows once, pads into a fixed-width index tensor, and performs zero-copy Torch scatter with sRGB encoding. Logic is correct and the test suite validates the hot path sync-free contract.
source/isaaclab_ov/isaaclab_ov/visual/ovrtx_visual_color_writer.py	OVRTX writer pre-bakes deterministic OmniPBR materials and drives runtime writes through OVRTXRenderer.write_visual_colors. Validation and de-instancing logic is thorough.
source/isaaclab_ov/isaaclab_ov/renderers/ovrtx_renderer.py	Adds write_visual_colors public boundary that forwards to the private renderer handle; encapsulation is correct.
source/isaaclab/isaaclab/renderers/render_context.py	Adds renderer_entries property returning a read-only tuple snapshot; straightforward and safe.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant U as User / EnvCfg
    participant EM as EventManager
    participant RVC as randomize_visual_color
    participant VCU as visual_color utils
    participant W as Backend Writer

    U->>EM: __init__(cfg)
    EM->>EM: deepcopy(term_cfg) + validate mode
    EM->>RVC: prepare_before_physics_ready(name, cfg, env)
    RVC->>VCU: resolve_visual_color_target_plan()
    VCU-->>RVC: VisualColorTargetPlan
    RVC->>W: pre_physics_ready_setup(env, plan)
    Note over W: RTX: de-instance prims
    Note over W: OVRTX: author USD materials
    Note over W: Newton: no-op
    RVC->>RVC: store plan on cfg.__dict__
    Note over EM,RVC: PHYSICS_READY
    EM->>RVC: __init__(cfg, env)
    RVC->>RVC: pop plan from cfg.__dict__
    Note over EM,W: First reset
    EM->>RVC: __call__(env, env_ids, colors)
    RVC->>RVC: normalize env_ids to target_indices
    RVC->>RVC: _ensure_writer() lazy construct
    RVC->>W: write_colors(target_indices, random_colors)
    W-->>RVC: colors written to renderer

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant U as User / EnvCfg
    participant EM as EventManager
    participant RVC as randomize_visual_color
    participant VCU as visual_color utils
    participant W as Backend Writer

    U->>EM: __init__(cfg)
    EM->>EM: deepcopy(term_cfg) + validate mode
    EM->>RVC: prepare_before_physics_ready(name, cfg, env)
    RVC->>VCU: resolve_visual_color_target_plan()
    VCU-->>RVC: VisualColorTargetPlan
    RVC->>W: pre_physics_ready_setup(env, plan)
    Note over W: RTX: de-instance prims
    Note over W: OVRTX: author USD materials
    Note over W: Newton: no-op
    RVC->>RVC: store plan on cfg.__dict__
    Note over EM,RVC: PHYSICS_READY
    EM->>RVC: __init__(cfg, env)
    RVC->>RVC: pop plan from cfg.__dict__
    Note over EM,W: First reset
    EM->>RVC: __call__(env, env_ids, colors)
    RVC->>RVC: normalize env_ids to target_indices
    RVC->>RVC: _ensure_writer() lazy construct
    RVC->>W: write_colors(target_indices, random_colors)
    W-->>RVC: colors written to renderer

Loading

Comments Outside Diff (1)

1. source/isaaclab/isaaclab/utils/visual_color.py, line 921-933 (link)

   |-alternation tokens compile as incorrect regexes. When mesh_prim_path is built from a list of body names (e.g. body_0|body_1), splitting on / yields a single token like body_0|body_1. Compiling ^body_0|body_1$ as a regex matches any string that starts with body_0 or ends with body_1 — not just the two intended names. The proxy-walk candidates would include spuriously matched prim names. The primary find_matching_prims result is unaffected, and downstream _resolve_visual_root filtering limits the blast radius to false positives in the proxy-augmented list, but targets with odd names could be added to the plan unexpectedly. Wrapping the token in a non-capturing group ((?:body_0|body_1)) before compiling fixes the alternation precedence.

_{Reviews (1): Last reviewed commit: "OMPE-96097: Simplify kitless visual colo..." | Re-trigger Greptile}

[greptile-apps]

Object-identity cache bypass is fragile. colors is self.cfg.params.get("colors") works today because EventManager passes kwarg values directly from cfg.params without copying, so the colors argument and the stored value are the same Python object. If the dispatch layer ever wraps or reconstructs params, this check would always be False, silently moving every call site onto the per-call _visual_color_range validation path and allocating new device tensors every reset. A sentinel or an explicit _override_colors keyword would make the contract visible.

Suggested change

	if colors is self.cfg.params.get("colors"):
	color_low, color_high = self._color_low, self._color_high
	else:
	colors = colors if colors else self._cfg.params.get("colors")
	# parse the colors into replicator format
	if isinstance(colors, dict):
	# (r, g, b) - low, high --> (low_r, low_g, low_b) and (high_r, high_g, high_b)
	color_low = [colors[key][0] for key in ["r", "g", "b"]]
	color_high = [colors[key][1] for key in ["r", "g", "b"]]
	colors = [color_low, color_high]
	else:
	colors = list(colors)
	num_prims = len(self.material_prims)
	random_colors = self.color_rng.generator.uniform(colors[0], colors[1], size=(num_prims, 3))
	rep.functional.modify.attribute(self.material_prims, "diffuse_color_constant", random_colors)
	color_low_cpu, color_high_cpu = _visual_color_range(colors, "cpu")
	color_low = color_low_cpu.to(env.device)
	color_high = color_high_cpu.to(env.device)
	# Use cached bounds when the caller passes the config's own colors object (the common
	# manager path). Any other object is treated as a per-call override and re-validated.
	if colors is self.cfg.params.get("colors"):
	color_low, color_high = self._color_low, self._color_high
	else:
	color_low_cpu, color_high_cpu = _visual_color_range(colors, "cpu")
	color_low = color_low_cpu.to(env.device)
	color_high = color_high_cpu.to(env.device)

[greptile-apps]

Token-level regex compilation uses raw token strings as patterns without escaping. re.compile(f"^{token}$") means a path component like env.0 would generate the pattern ^env.0$, which matches envX0, env_0, etc. USD prim names conventionally contain only [A-Za-z0-9_], so this is not an active bug, but adding re.escape for literal components makes the intent explicit and prevents silent mismatch if paths ever contain literal dots or other regex metacharacters.

Suggested change

	for token in normalized_expr.split("/")[1:]:
	token_pattern = re.compile(f"^{token}$")
	for token in normalized_expr.split("/")[1:]:
	# Escape literal components; leave explicit regex wildcards (.*) intact.
	safe_token = token if token == ".*" else re.escape(token)
	token_pattern = re.compile(f"^{safe_token}$")

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!