Reachability only validates tasks-relevant objects (Part 2.5/3)#947
Reachability only validates tasks-relevant objects (Part 2.5/3)#947xyao-nv wants to merge 11 commits into
Conversation
| assets_by_node_id = instantiate_assets_from_spec(graph_spec, AssetRegistry(), enable_cameras=enable_cameras) | ||
| _ensure_scene_lighting(graph_spec, assets_by_node_id) | ||
| _attach_spatial_relations_to_assets(graph_spec.relations, assets_by_node_id) | ||
| _attach_reachability_relations_to_assets(graph_spec.get_reachability_target_object_ids(), assets_by_node_id) |
There was a problem hiding this comment.
Open for discussions if any alternative to tie the reachability to object. It does not make sense to me to add it as a property to the object class. So I treat it as a Relation instead.
![arena-review-bot[bot]](https://avatars.githubusercontent.com/u/195794629?s=60&v=4){kind=small}
🤖 Isaac Lab-Arena Review BotSummaryThis PR scopes the build-time cuRobo reachability check from every movable object to only the objects a task actually needs the robot to reach (pickup + place destination for PnP). It adds a Findings🔵 Test CoverageGood. New spec-level tests cover parsing, subject validation (unknown / non-object / object-reference rejected), auto-population, explicit+auto union/dedup, and marker attachment. The cuRobo validator gains tests that only stamped objects are IK-checked and that the zero-target path passes trivially and warns exactly once. Sim-dependent conversion test uses the inner/outer VerdictShip it (address the minor nits at your discretion). |
Greptile SummaryThis PR scopes reachability validation to only the objects the robot actually needs to reach (pickup target and place destination), replacing the previous behavior of IK-checking every non-anchor movable object. The mechanism uses a new
Confidence Score: 4/5Safe to merge; all findings are non-blocking style or minor performance observations with no correctness impact. The change is well-tested and logically sound. Explicit constraints are validated against placed object ids at spec-parse time, deduplication prevents double-stamping, and the test suite is extended with targeted cases for all new paths. The only substantive tweak worth considering is moving the reachability-target check before the GPU world-update call so layouts with no targets skip the cuboid rebuild entirely. isaaclab_arena_curobo/ik_reachability_validator.py has the minor world-update ordering issue; banana_on_plate.yaml has redundant explicit constraints worth reviewing. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| task_constraints: | ||
| - type: reachable | ||
| subject: banana | ||
| - type: reachable | ||
| subject: plate |
There was a problem hiding this comment.
PickAndPlaceTask.reachability_target_objects = ("pick_up_object", "destination_location") already auto-derives the reachability targets from the task params. If the task params map pick_up_object: banana and destination_location: plate, these explicit task_constraints entries produce exactly the same result as leaving them out - get_reachability_target_object_ids() deduplicates via dict.fromkeys so no double-stamping occurs, but the YAML becomes misleading for future authors who may not realize the config is redundant. Consider removing these entries if the intent is only to validate the PickAndPlaceTask auto-derivation path.
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!
There was a problem hiding this comment.
I agree, the explicit task_constraints in yaml/env seems unnecessary
the task interface already defines which objects are reachability_target_objects, there's no need to expose this again through env/yaml. I'd suggest remove this and the task_constraints TaskConstraintSpec ect entirely.
a user can design to switch on/off reachability for an env through validator params?
There was a problem hiding this comment.
That's a valid design critique. Looking at the full implementation:
task_constraintsin the YAML only adds to what the task class already derives viareachability_target_objects— there's no use case where it's the sole source of truth for aPickAndPlaceTask- On/off control already exists via
placement_validators.enabled_checks/required_checksinbuild_checks_for_placer_params— no new mechanism needed
The one trade-off worth considering before removing TaskConstraintSpec entirely: it's currently the only hook for env authors using custom task types that don't declare reachability_target_objects on their class. If you remove it, those authors must touch the task class. But if the team standard is that all task classes own their reachability targets (which is the cleaner design), that's fine.
If you remove task_constraints, the net effect is:
TaskConstraintSpec,TaskConstraintTypedeleted fromarena_env_graph_types.pytask_constraintsfield and_assert_task_constraint_subject_is_validremoved fromarena_env_graph_spec.pyget_reachability_target_object_idssimplifies to just_select_task_declared_reachable_subjects- The
banana_on_plate.yamlentries disappear (as originally flagged) - Tests for
task_constraintsparsing/validation are removed; the auto-derivation tests remain
Would you like me to create a rule so I don't flag missing task_constraints in future PRs?
There was a problem hiding this comment.
Yes it was just for showcase. But I agree earlier to remove the task_constraints entirely.
| reachability_target_objects = ("pick_up_object", "destination_location") | ||
| """The robot must reach the object it picks up and the it places onto. | ||
|
|
||
| An object reference or the background scene (a static location, not a placed object) is skipped |
There was a problem hiding this comment.
I'm debating the tradeoff to support object reference for reachability validation. Here is my thinking process. Open for discussion.
Object reference is never a placed entity thru the solver (e.g. shelf inside the fridge). But its parent object (the fridge) could be inside the background scene (e.g. kitchen) or a foreground object (e.g. the microwave on table). It does not make sense to reject layout if it's a background for unreachable as none of them will be returned.
Can be in another MR if enable the object reference if it's a foreground placeable object is necessary.
xyao-nv
changed the title
alexmillane
left a comment
alexmillane
left a comment
There was a problem hiding this comment.
Look pretty good. I left some points for discussion.
| task_constraints: list[TaskConstraintSpec] | None = Field( | ||
| default=None, | ||
| description="Optional per-object task constraints.", | ||
| ) |
There was a problem hiding this comment.
Point for discussion. Given that the reachability is described as a relation (which is probably correct IMO) is there a good reason to introduce another field here to hold it? Could we not have the reachability relation with the other relations.
With the splitting of the graph yamls into scenes and tasks, relations can be added in a task-specific manner. They live inside the task portion of the yaml file(s) and are added to the scene if that task is used.
Wdyt?
@qianl-nv for viz
There was a problem hiding this comment.
I would actually go one step further and don't expose task_constraints at the env/yaml level.
I feel adding reachable relation to objects should be handled internally in the Task class, and user should not need to add the task-based constraints explicitly.
| class TaskConstraintType(str, Enum): | ||
| """Kind of task constraint declared on a scene object.""" | ||
|
|
||
| REACHABLE = "reachable" | ||
|
|
||
|
|
||
| class TaskConstraintSpec(BaseModel): | ||
| """One task constraint: a requirement of ``type`` on the object named by ``subject``.""" | ||
|
|
||
| type: TaskConstraintType = Field(description="Constraint kind; must match TaskConstraintType exactly.") | ||
| subject: str = Field( | ||
| min_length=1, | ||
| description="Graph node id of the object the constraint applies to. Must match an object id exactly.", | ||
| ) |
There was a problem hiding this comment.
See comment above. Any reason not to just treat the reachable constraint as another relation?
Probably later, we'll actually want it to be a binary relation, rather than a unary one. The reachable constrain relates the object and the robot.
There was a problem hiding this comment.
Removed task constraint.
| @register_object_relation | ||
| class RequiresReachability(RelationBase): | ||
| """Marker indicating the robot shall be able to reach this object. | ||
|
|
||
| Stamped onto an object from a 'reachable' task constraint. It does not affect placement geometry | ||
| during optimization, but rejects unreachable placements. | ||
|
|
||
| Usage: | ||
| banana.add_relation(RequiresReachability()) # IK-check reachability of the banana | ||
| """ | ||
|
|
||
| name = "requires_reachability" | ||
|
|
||
| @staticmethod | ||
| def is_unary() -> bool: | ||
| """Return whether the relation constrains a single object.""" | ||
| return True |
There was a problem hiding this comment.
We should possibly make this a binary constraint between the robot and the object. Right now the robot is sort of implied.
I wonder if this constraint could also have an affect on the optimization by adding a reach radius constraint to the optimization between the robot (which will soon be placeable) and the object(s).
There was a problem hiding this comment.
Perhaps this should be the subject of a future MR though.
There was a problem hiding this comment.
I would lean towards this being a "unary" constraint, since one of the end of the relation is always the robot (it doesn't make sense to be any other asset), and there's only one embodiment in an arena env.
so it's either define as unary, or as binary + asset that the reference is always THE embodiment. In that case going for unary seems simpler.
There was a problem hiding this comment.
It makes sense to me, once radius constraint is added and tested, this can be changed to subclassing BinaryRelation class. Added a TODO.
Once robot-object is in, it could be binary relation.
There was a problem hiding this comment.
On a side note, there are other better candidates for a surrogate loss than the radius constraint for reachability. Lio's MR is just 2D, and does not reflect robot's reachability map.
| The factory receives ``arm_mode`` from the env builder and returns a constructed cfg. | ||
| """ | ||
|
|
||
| reachability_target_objects = ("pick_up_object", "destination_location") |
There was a problem hiding this comment.
I'm wondering why these need to be added here?
Aren't we relying on the user to specify the reachability relation manually? I'm wondering why they're here? (maybe there's a good reason, I am just unsure).
There was a problem hiding this comment.
Ha I was suggesting the exact opposite in my comment above lol... to not let user specify the reachability relation and also add them in the PickAndPlaceTask automatically.
There was a problem hiding this comment.
I think configuring them thru Task interface is a better design. It's a requirement from the task, and task interface can easily derive the target to be checked.
| return tuple( | ||
| dict.fromkeys(( | ||
| *(c.subject for c in (self.task_constraints or []) if c.type is TaskConstraintType.REACHABLE), | ||
| *self._select_task_declared_reachable_subjects(object_ids), | ||
| )) | ||
| ) |
There was a problem hiding this comment.
nit: large compound comprehensions like this (i find) are basically impossible for a human to read. Suggestion to break things like this up over multiple lines and statements.
| assets_by_node_id = instantiate_assets_from_spec(graph_spec, AssetRegistry(), enable_cameras=enable_cameras) | ||
| _ensure_scene_lighting(graph_spec, assets_by_node_id) | ||
| _attach_spatial_relations_to_assets(graph_spec.relations, assets_by_node_id) | ||
| _attach_reachability_relations_to_assets(graph_spec.get_reachability_target_object_ids(), assets_by_node_id) |
There was a problem hiding this comment.
I'm thinking that attaching reachability constraints to the environment in the graph layer is the wrong place to do it. For starters, it means that the python way of specifying environments does get these.
A good comparison might be the non-collision constraints which are added in the env-builder / object placer.
I'm thinking that we could add to PickAndPlaceTask a function that adds reachability constraints to it's input objects? That's not perfect either but I feel it's better than having it in the graph layer.
There was a problem hiding this comment.
Yes, it was added.
And now it's only configured in the task.
qianl-nv
left a comment
qianl-nv
left a comment
There was a problem hiding this comment.
Thanks for the change, it's a good place to start refining.
Do we have any use cases where we want to add reachable relation to non-task-related object? If not it feels better to have the Task class automatically add the relation to its pick/target objects instead of always having the user specific the relation outside of the task.
The only main concern for that approach is backward compatibility: some of our existing envs (esp those hand crafted ones) might not even satisfy these relations in the first place. For those we might just explicitly switch the reachability checks off through PlacementParams instead.
wdyt? maybe I'm missing other scenarios where the user might want to have manual control over the reachability relations.
| task_constraints: | ||
| - type: reachable | ||
| subject: banana | ||
| - type: reachable | ||
| subject: plate |
There was a problem hiding this comment.
I agree, the explicit task_constraints in yaml/env seems unnecessary
the task interface already defines which objects are reachability_target_objects, there's no need to expose this again through env/yaml. I'd suggest remove this and the task_constraints TaskConstraintSpec ect entirely.
a user can design to switch on/off reachability for an env through validator params?
|
|
||
|
|
||
| @register_object_relation | ||
| class RequiresReachability(RelationBase): |
There was a problem hiding this comment.
parent class should be UnaryRelation
There was a problem hiding this comment.
I don't agree, UnaryRelation has been using for solver-optimized position constraints. If inheriting that, it will crash the solver as no loss strategy is implemented (yet) for this reachability.
We either rename the following func to be explicit on those are used in solver optimization in another MR, or change this to UnaryRelation once reachability loss is added.
If you insist suggesting UnaryRelation to use here, then a zero-loss strategy needed to be added as a no-op loss for non-geometric relations. For simplicity, I prefer subclassing RelationBase.
In object_base.py,
def get_spatial_relations(self) -> list[RelationBase]:
"""Get only spatial relations (On, NextTo, AtPosition, etc.), excluding markers like IsAnchor."""
return [r for r in self.relations if isinstance(r, (Relation, UnaryRelation))]
There was a problem hiding this comment.
Subclassing RelationBase is similiar to how is_anchor is used.
| task_constraints: list[TaskConstraintSpec] | None = Field( | ||
| default=None, | ||
| description="Optional per-object task constraints.", | ||
| ) |
There was a problem hiding this comment.
I would actually go one step further and don't expose task_constraints at the env/yaml level.
I feel adding reachable relation to objects should be handled internally in the Task class, and user should not need to add the task-based constraints explicitly.
| @register_object_relation | ||
| class RequiresReachability(RelationBase): | ||
| """Marker indicating the robot shall be able to reach this object. | ||
|
|
||
| Stamped onto an object from a 'reachable' task constraint. It does not affect placement geometry | ||
| during optimization, but rejects unreachable placements. | ||
|
|
||
| Usage: | ||
| banana.add_relation(RequiresReachability()) # IK-check reachability of the banana | ||
| """ | ||
|
|
||
| name = "requires_reachability" | ||
|
|
||
| @staticmethod | ||
| def is_unary() -> bool: | ||
| """Return whether the relation constrains a single object.""" | ||
| return True |
There was a problem hiding this comment.
I would lean towards this being a "unary" constraint, since one of the end of the relation is always the robot (it doesn't make sense to be any other asset), and there's only one embodiment in an arena env.
so it's either define as unary, or as binary + asset that the reference is always THE embodiment. In that case going for unary seems simpler.
| The factory receives ``arm_mode`` from the env builder and returns a constructed cfg. | ||
| """ | ||
|
|
||
| reachability_target_objects = ("pick_up_object", "destination_location") |
There was a problem hiding this comment.
Ha I was suggesting the exact opposite in my comment above lol... to not let user specify the reachability relation and also add them in the PickAndPlaceTask automatically.
| """True if this object has an IsAnchor relation.""" | ||
| return any(isinstance(r, IsAnchor) for r in self.relations) | ||
|
|
||
| @property |
There was a problem hiding this comment.
It might be a bad idea to add relation-type-specific API to the object_base class. Imaging we would have other task constraints and each time adding a new API to object_base sounds a bad idea.
either we make this a generic def has_relation(self, relation_type: Relation) in object_base
or make this a helper/util in the relations module instead.
There was a problem hiding this comment.
valid, updated it to has_relation.
82408e9 to
7351cb4
Compare
3 participants
Summary
Reachability used to validate over all non-anchor objects in the candidate layout (#914). In this MR, it checks reachability against task-related objects. For PnP tasks, it is auto-configured as pickup objects and place destination objects.
Detailed description
task_constraintsin the env graph spec to capture the reachability constraint.Objectis anRelationBasetied to the object, which affects the geometry of layout (only at the validation step, not solving step). Defined asRequiresReachability.PickAndPlaceTaskspecified what objects need to be reached thru a init data memberreachability_target_objects. This data is also added to theTaskBase.TODO