Skip to content

Support anchor in agentic env gen#784

Open
qianl-nv wants to merge 2 commits into
mainfrom
qianl/dev/agentic_anchor
Open

Support anchor in agentic env gen#784
qianl-nv wants to merge 2 commits into
mainfrom
qianl/dev/agentic_anchor

Conversation

@qianl-nv

@qianl-nv qianl-nv commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

Summary

EnvGenAgent adds the is_anchor relation.

Detailed description

  • What was the reason for the change?
    is_anchor is currently missing from agent generated spec.
  • What has been changed?
    -- agent prompt is updated to include request for is_anchor in system prompt.
    -- reformat system prompt for better readibility.
    -- pose is required for anchor object. set pose during conversion to arena env if missing.
  • What is the impact of this change?
    -- Agent generated env spec should now work with relation solver.

@qianl-nv qianl-nv changed the title Support is_anchor is env gen agent output Support anchor in agent generated environment Jun 11, 2026
@qianl-nv qianl-nv changed the title Support anchor in agent generated environment Support anchor in agentic env gen Jun 11, 2026
@greptile-apps

greptile-apps Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR extends the agentic environment generation pipeline to produce is_anchor relations, which were previously missing and caused LLM-generated specs to fail with the relation solver.

  • Prompt update (environment_generation_agent.py): the system prompt is reformatted from a concatenated string to a multi-line literal and gains one new REQUIRED bullet instructing the model to emit an is_anchor (unary) relation for the surface that other objects rest on.
  • Conversion fix (arena_env_graph_conversion_utils.py): after attaching a unary relation to its subject asset, if the relation kind is is_anchor and the asset has no initial pose, Pose.identity() is assigned so the placement solver always has a valid fixed pose to work from.

Confidence Score: 4/5

Safe to merge with minor polish; the conversion logic is straightforward and the prompt change is additive.

Both changes are small and targeted. The conversion utility correctly gates the default-pose assignment on get_initial_pose() is None, so existing assets with an explicit pose are unaffected. The magic string is_anchor is a minor maintenance risk but poses no immediate runtime breakage. The system prompt instruction is clear enough for most cases but omits which node id the model should use as the is_anchor subject, which could lead to the model choosing the wrong asset in more complex scenes.

arena_env_graph_conversion_utils.py — the hardcoded is_anchor string and the post-add_relation default-pose block deserve a second look.

Important Files Changed

Filename Overview
isaaclab_arena/agentic_environment_generation/environment_generation_agent.py Reformats system prompt from string concatenation to multi-line literal and adds a REQUIRED is_anchor guidance bullet; logic-equivalent refactor with one new instruction.
isaaclab_arena/environments/arena_env_graph_conversion_utils.py Adds import of Pose and a post-add_relation block that defaults an is_anchor asset to Pose.identity() when no initial pose is set; uses a hardcoded string comparison for the relation kind.

Reviews (1): Last reviewed commit: "Reformat system prompt for better readib..." | Re-trigger Greptile

# An is_anchor asset must have a fixed initial pose for the placement solver.
# If the asset class does not declare one, default to the world origin so
# LLM-generated specs (which never set an explicit pose) work out of the box.
if spatial_constraint.kind == "is_anchor" and subject_asset.get_initial_pose() is None:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 The relation kind is compared to the hard-coded string "is_anchor" rather than to a registry key or a constant. If the registered name for the anchor relation ever changes (e.g. to "IS_ANCHOR" or "anchor"), this check silently stops firing and LLM-generated specs will no longer receive the default pose. Defining a named constant or deriving the value from the registry would make the coupling explicit.

Suggested change
if spatial_constraint.kind == "is_anchor" and subject_asset.get_initial_pose() is None:
_IS_ANCHOR_KIND = "is_anchor"
if spatial_constraint.kind == _IS_ANCHOR_KIND and subject_asset.get_initial_pose() is None:

Do NOT hallucinate values — the resolver tolerates nulls; it cannot fix invented data.
- For binary relations (e.g. on), subject is the placed object and reference is \
the surface it is relative to (typically the background name).
- REQUIRED: include an is_anchor (unary) relation for the surface other objects rest on.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 The new bullet says is_anchor is "REQUIRED" for "the surface other objects rest on", but the prompt does not explain what to use as the subject of the unary relation (the background node id, an object-reference id, or something else). Without that detail the LLM may apply is_anchor to an incorrect node, causing the relation solver to fail or silently misplace objects. Adding a concrete example (e.g. subject = background node id) in the same bullet would remove the ambiguity.

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant