Homogeneous Object Placement on Environment Reset

[zhx06]

Summary

Pool-based object placement with fresh layouts on environment reset

Detailed description

What changed:

• Added PlacementPool that pre-solves valid layouts at build time and serves them on reset via placement_events.py.
• Rewritten ArenaEnvBuilder._solve_relations() to use the pool, with --resolve_on_reset / --no-resolve_on_reset controlling behavior.
• Added --episode_length_s to gr1_table_multi_object_no_collision for periodic resets.

Impact

• Fresh collision-free layouts on every reset, different across envs and episodes.
• Solver runs once at build time (batched), not per-reset.

[greptile-apps]

Greptile Summary

This PR introduces homogeneous object placement on environment reset by adding a PlacementPool that pre-solves a batch of collision-free layouts at build time, then registers a reset EventTermCfg (solve_and_place_objects) that draws a fresh layout per resetting environment. The resolve_on_reset=False path retains the static per-object pose behavior via set_initial_pose. Previous review concerns (empty-tensor guard, test coverage for the reset event path) have been addressed.

Confidence Score: 5/5

Safe to merge; all remaining findings are P2 style/performance suggestions with no correctness impact.

Prior P1 concerns (empty-tensor guard, missing test coverage) are resolved. The two open findings are a P2 performance suggestion (batching write calls) and a P2 test fragility. Neither blocks correctness or data integrity.

placement_events.py (write-call batching) and test_placement_events.py (pool.remaining assertion).

Important Files Changed

Filename	Overview
isaaclab_arena/relations/placement_events.py	New reset-event function that draws layouts from the pool and writes poses; logic is correct but per-(env,object) write loop issues O(N×M) sim API calls instead of the more efficient batched-per-object approach.
isaaclab_arena/relations/placement_pool.py	Solid FIFO pool with lazy compaction and auto-refill; acquire/sample semantics, compaction logic, and RuntimeError on exhaustion are all correct.
isaaclab_arena/environments/arena_env_builder.py	Refactored _solve_relations to use PlacementPool + optional reset event; resolve_on_reset branching, init-state seeding from pool, and EventTermCfg registration look correct.
isaaclab_arena/relations/object_placer_params.py	Added resolve_on_reset and min_unique_layouts_per_env dataclass fields with clear docstrings; no issues.
isaaclab_arena/tests/test_placement_events.py	Good coverage of solve_and_place_objects (empty/None guard, single/multi-env writes, pool acquire/sample/refill); assert pool.remaining == 20 is fragile since the pool only keeps valid-success results.
isaaclab_arena/cli/isaaclab_arena_cli.py	Added --resolve_on_reset BooleanOptionalAction flag with default=None (defers to ObjectPlacerParams default); correct and consistent with other placement flags.
isaaclab_arena_environments/gr1_table_multi_object_no_collision_environment.py	Added optional --episode_length_s flag that wires in a time_out termination and sets episode length; clean opt-in pattern with no impact when flag is absent.

_{Reviews (3): Last reviewed commit: "align code style and variable name" | Re-trigger Greptile}

[greptile-apps]

None guard does not cover empty tensors

Isaac Lab's EventManager passes a torch.Tensor (never None) for reset-mode events, so the is None branch is dead code. More importantly, an empty tensor (len(env_ids) == 0) would still reach placer.place(objects, num_envs=0, ...), which calls self._solver.solve(objects, []) with no initial positions — a path whose behaviour under the solver is unverified. The guard should cover both cases:

Suggested change

	if env_ids is None:
	return
	if env_ids is None or len(env_ids) == 0:
	return

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This PR moves object placement from a one-time build step to a per-reset event, so each environment reset produces a fresh collision-free layout. The design is sound: a single EventTermCfg(mode="reset") replaces the old build-time ObjectPlacer.place() call, and the new solve_and_place_objects function handles the solver invocation + sim writes. Tests cover the ObjectPlacer randomness properties but not the reset event itself.

Design Assessment

Design is sound. Registering placement as a reset event via EventTermCfg is the right Isaac Lab pattern for per-episode randomization. The make_configclass → combine_configclass_instances approach for injecting the new event alongside existing events is clean and consistent with how other configs are composed.

The split of concerns is clear: _solve_relations() builds the config, compose_manager_cfg() wires it into the event pipeline, and solve_and_place_objects does the actual work at reset time.

Findings

🟡 Warning: Failed placements are written to sim without guarding — isaaclab_arena/relations/placement_events.py:68-98

When the solver fails for some environments (result.success == False), the code still writes those failed positions into the simulation. The old code logged a warning but didn't apply positions (since it ran at build time and the sim wasn't live yet). Now that placement happens during resets, writing invalid positions could produce overlapping objects in the running simulation.

Consider skipping pose writes for environments where the solver failed:

for local_idx, cur_env in enumerate(env_ids.tolist()):
    if not results_per_env[local_idx].success:
        continue  # keep previous layout for this env
    positions = results_per_env[local_idx].positions

This is not critical because the solver's "best effort" positions are typically close to valid even when validation fails, but it would be more defensive.

🟡 Warning: Per-env tensor allocation in hot loop — isaaclab_arena/relations/placement_events.py:90-98

Each iteration creates torch.tensor([cur_env], device=env.device) twice (for pose and velocity writes). For many environments with many objects, this is O(envs × objects × 2) small tensor allocations per reset. The existing set_object_pose_per_env in events.py has the same pattern, so this is consistent with the codebase, but worth noting for future optimization — pre-allocating the env_id tensor outside the loop would reduce GPU allocation overhead:

for local_idx, cur_env in enumerate(env_ids.tolist()):
    env_id_tensor = torch.tensor([cur_env], device=env.device)
    ...
    asset.write_root_pose_to_sim(pose_t_xyz_q_xyzw, env_ids=env_id_tensor)
    asset.write_root_velocity_to_sim(torch.zeros(1, 6, device=env.device), env_ids=env_id_tensor)

At minimum, this avoids creating the same [cur_env] tensor twice per object. A further optimization would batch all objects for a given env_id into a single write.

🔵 Suggestion: Test gap — solve_and_place_objects is untested — isaaclab_arena/tests/test_placement_events.py

The three tests are good coverage of ObjectPlacer randomness properties (unseeded produces different layouts, seeded is reproducible, multi-env diversity). However, they test the placer directly — not the solve_and_place_objects reset event function. The reset event adds important logic on top: the torch.inference_mode(False) context, the rotation computation, the sim write loop, and the env_ids mapping.

A unit test with a mock ManagerBasedEnv verifying that solve_and_place_objects correctly calls write_root_pose_to_sim for the right env_ids (and skips anchors) would strengthen confidence in the integration layer.

🔵 Suggestion: _enable_periodic_reset closure captures episode_length_s by reference — isaaclab_arena_environments/gr1_table_multi_object_no_collision_environment.py:108-115

The inner function _enable_periodic_reset captures episode_length_s from the enclosing scope. This works correctly here because the variable is only assigned once before the closure is created. No bug, just noting for awareness — if this pattern were extended with a loop, the classic closure-over-loop-variable issue could arise.

Test Coverage

• New code tested: Partial — ObjectPlacer randomness is well-tested
• Gaps: The solve_and_place_objects function (the actual new event) has no direct test. The _enable_periodic_reset callback path is also untested.
• Test quality: Good — tests are deterministic (seeded vs unseeded), isolated, and cover the key behavioral property (distinct layouts per reset/per env)

CI Status

Pre-commit check: ⏳ pending

Verdict

Minor fixes needed

The architecture is clean and follows Isaac Lab conventions. The main actionable concern is writing failed solver results to sim (🟡) — adding a guard for failed placements would prevent edge-case overlapping objects. The per-env tensor allocation (🟡) is a pre-existing pattern in the codebase but worth a small optimization. The test gap is real but not blocking since the core placer logic is well-covered.

[alexmillane]

This is looking pretty good!

I have some small comments, but the big one is about speed and the potential use of a solution pool, in order to run bigger problem sizes, less frequently.

Let's discuss this. It's a good first stab at the problem.

[alexmillane]

I'm wondering if it's an error that non-anchor objects have pose-reset events in the case of relational solving. Do you know if what cases this is actually doing something?

[zhx06]

Right now non-anchor objects don't have set_initial_pose() called, so it's not an error and these object never gets a pose-reset event

[alexmillane]

Rather than setting the their event_cfg to None should we just assert obj.event_cfg = None, "Relational object solving should not be combined with explicit setting of poses on non-anchor objects.

Right now, if a user tries to combine these things, the code will silently ignore the explicit poses. IMO it's better to fail and tell the user that they're doing something incorrect.

[alexmillane]

I think we should make an unguarded access of self.args.placement_seed. Something has gone wrong if we end up here without placement_seed.

[zhx06]

Sure, we will use self.args.placement_seed.

[alexmillane]

Why do we have to force the following params:

• save_position_history=False,
• apply_positions_to_objects=False
• verbose=False

I can see the justification for the placement_seed (although perhaps we should set that to false one level above and then warn if it's not None at this level).

[zhx06]

We have now moved them to construction into arena_env_builder.py. save_position_history is turned off for object reset because it is for verification purpose. We don't use apply_positions_to_objects because we write them directly to sim. verbose is turned off so we removed this forcing here.

[alexmillane]

This strikes me as a little strange. If we immediately convert the single environment solution to a list on return, should we just always return a MultiEnvPlacementResult?

[zhx06]

This comes from the ObjectPlacer where we have MultiEnvPlacementResult for n_envs>1 and PlacementResult for n_envs=1. Do we want to edit the design in that level?

[alexmillane]

Yea, maybe we should leave that for another MR. I think we should consider it in general.

[alexmillane]

I guess we warn if the solving failed? Seems like we should warn before writing.

[zhx06]

Warning added

[alexmillane]

Should we put this in a function somewhere if it's not already?

[zhx06]

These 4 lines are similar in some other code for setting object poses, but has slight differences. I would suggest we keep it for now and do the cleaning later since this touches lots of things in relation solver.

[alexmillane]

👍

[alexmillane]

One big question I have about this approach is that it's effectively gotten rid of our batching behaviour. One can imagine that over time, the (potentially 1000s) of environments in an evaluation will all eventually reset at different indices, which will mean that this event fires for 1 env frequently.

With the current design that will mean solving a lot of small problems frequently, rather than solving one big problem infrequently. This is likely to mean poor performance on the GPU.

How does the performance look. What do you think about the approach of solving a bigger batched problem and saving to a solution pool?

[zhx06]

I tested with num_envs=200 and num_envs=10 (both 10 max_attempts). They both take like 10 seconds. So the solver batches well, but the per-call overhead is significant for repeated resets. We could have a pool, but let me think about it and have a better design

[alexmillane]

Yea that's matches expectation. The solve time is basically independent of batch size (at the small sizes we're dealing with). I think that makes having a pool look like a good option (because we get additional solutions for free at solve-time).

Let me know when you'd like me to re-review :)

[alexmillane]

So here, we've removed the upfront solve. So at the moment we're enforcing that we have a new solution per reset.

I could imagine that some users may still want that behavior. It would be great if we had a parameter that controlled resolve_on_reset.

This actually relates to another comment further down about a solution pool. If we change the design slightly to draw solutions from a pool, implementing reset_on_solve = False is easy: we just don't remove solutions from the pool when they're used.