Skip to content

Articulation atom action axis auto generation. - #573

Open
matafela wants to merge 2 commits into
mainfrom
cj/slide-press-twist-axis-generator
Open

Articulation atom action axis auto generation.#573
matafela wants to merge 2 commits into
mainfrom
cj/slide-press-twist-axis-generator

Conversation

@matafela

@matafela matafela commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Description

  • translation_axis of Slide is now auto generated.
  • twist_axis, axis_origin of Twist is now auto generated.
  • press_axis of Press is now auto generated.
  • Fix tutorial scripts/tutorials/atomic_action/pour.py

Type of change

  • Enhancement (non-breaking change which improves an existing functionality)

Checklist

  • I have run the black . command to format the code base.
  • I have made corresponding changes to the documentation
  • Public API changes are reflected in the API docs (python docs/scripts/check_api_docs.py), if applicable
  • I have added tests that prove my fix is effective or that my feature works
  • Dependencies have been updated, if applicable.

@greptile-apps

greptile-apps Bot commented Sep 1, 2026

Copy link
Copy Markdown

Greptile Summary

The PR derives Slide, Press, and Twist action geometry from articulation point clouds and updates configured task integration, tutorials, documentation, and tests accordingly.

  • Adds initial articulation mesh sampling in a target-link-local frame.
  • Resolves action axes and selected contact/origin positions while constructing object semantics.
  • Makes the configured Slide axis optional and updates the packaged open-drawer integration.
  • Adds focused affordance, articulation, tutorial, and task-program coverage.

Confidence Score: 5/5

The PR appears safe to merge because no blocking failure remains in the eligible follow-up review scope.

No blocking failure remains.

Important Files Changed

Filename Overview
embodichain/lab/sim/objects/articulation.py Adds validated initial-state mesh transformation and Open3D surface sampling for target and whole-articulation point clouds.
embodichain/lab/sim/atomic_actions/affordance.py Adds geometry-driven resolution for Slide, Press, and Twist affordance fields, including explicit validation and legacy fallback behavior.
embodichain/lab/sim/atomic_actions/core.py Resolves geometry-derived affordance fields during ObjectSemantics initialization.
embodichain/lab/task_program/integrations/_configured_services.py Updates configured Slide lowering to sample articulation geometry and retain an optional explicit-axis fallback.
embodichain/lab/task_program/integrations/configured.py Makes translation_axis optional when decoding configured articulation-link Slide services.
embodichain/lab/sim/atomic_actions/primitives/twist.py Requires a resolved twist-axis origin before generating twist keyframes.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    A[Articulation meshes] --> B[Initial forward kinematics]
    B --> C[Target-local point clouds]
    C --> D[ObjectSemantics geometry]
    D --> E[Affordance resolution]
    E --> F[Slide translation axis]
    E --> G[Press axis and contact point]
    E --> H[Twist axis and origin]
    F --> I[Atomic-action planning]
    G --> I
    H --> I
Loading

Reviews (2): Last reviewed commit: "fix pour tutorial" | Re-trigger Greptile

@matafela
matafela requested a review from yuecideng September 1, 2026 11:05

@yuecideng yuecideng left a comment

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.

I found three medium-severity correctness and compatibility regressions. The bundled Drawer/Microwave paths and CI checks pass, but the public articulation path can infer a wrong or nondeterministic kinematic axis, Twist can rotate about an off-axis mesh centroid, and the documented legacy Slide fallback is not reachable when sampling is unsupported.

Priority Area Impact
P2 Axis inference Oblique axes are quantized to a Cartesian basis; symmetric independently sampled clouds can choose a noise-driven direction.
P2 Twist origin The mesh centroid can replace a correct revolute-joint origin and produce rotation about the wrong spatial line.
P2 Slide compatibility Providers with an explicit legacy axis still fail when point-cloud sampling is unavailable or unsupported.

Requesting changes before merge; details and suggested fixes are attached inline.

raise ValueError(f"{field_name} point-cloud neighborhood is empty.")
neighborhood_center = articulation_points[neighborhood_mask].mean(dim=0)
center_offset = neighborhood_center - target_center
axis_index = int(torch.argmax(torch.abs(center_offset)).item())

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] Resolve the motion axis from articulation kinematics

This always quantizes the neighborhood offset to one Cartesian basis vector. A valid oblique prismatic/revolute axis such as (1, 1, 0) / sqrt(2) is therefore returned as +X, and independently sampled symmetric clouds can select different signed axes from centroid noise because the rejection tolerance is only radius * 1e-6. Slide/Press/Twist then plan a wrong or nondeterministic Cartesian path. Please resolve the active parent joint through get_parent_joint_chain(), transform its exact axis into the target-link frame, and use point-cloud geometry only for sign/contact selection (with a confidence check).

)
if resolved is not None:
self.twist_axis, target_points = resolved
self.axis_origin = tuple(

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] Preserve the revolute joint's actual axis origin

The sampled target-link centroid is not guaranteed to lie on the revolute axis (for example, asymmetric or laterally offset knob meshes), so overwriting an explicit origin here makes Twist rotate the grasp around the wrong spatial line. OpenDoorAffordance.from_articulation() already shows how to transform the selected joint's origin_pose into the target-link frame. Please derive Twist's origin from the active parent joint and keep the centroid for grasp/contact geometry only.

)
if not callable(sample_initial_point_clouds):
raise TypeError("Articulation must provide sample_initial_point_clouds().")
geometry = sample_initial_point_clouds(native_link_name)

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] Make the explicit translation axis a real compatibility path

sample_initial_point_clouds() is called before the optional legacy axis can be used. Existing configured Slide providers that previously supplied a valid axis but have no PK chain, use non-unit body_scale, or do not implement this new method now fail during adapter creation; when sampling succeeds, ObjectSemantics also overwrites the explicit value. Please skip sampling when translation_axis is present, or fall back to that value when sampling is unavailable/unsupported, and cover the factory create() path rather than only decoding the field.

@yuecideng yuecideng left a comment

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.

Supplementary review of the new Articulation geometry API. The point-cloud functionality is useful, but its current ownership mixes simulation-object facts with stochastic preprocessing and Atomic Action-specific semantics.

Priority Area Recommendation
P2 API ownership Keep Articulation deterministic and domain-neutral; move action-specific sampling into a typed geometry adapter.
P3 Mixed mesh handling Preserve or explicitly reject links that have vertices but no valid triangle faces.

These comments supplement the existing correctness findings and do not duplicate the earlier joint-axis review.

verts, faces = self.body_data.link_vert_face[link_name]
return verts, faces

def sample_initial_point_clouds(

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] Keep action-specific geometry sampling out of Articulation

This public method combines responsibilities from three layers: Articulation topology/FK/raw mesh access, stochastic Open3D surface sampling, and the Atomic Action-specific geometry protocol (target_link_point_cloud and articulation_point_cloud). Its callers use the result to construct ObjectSemantics and resolve affordances, so placing it here makes the simulation-object API depend on PK-chain/unit-scale restrictions and private semantic keys that are not intrinsic properties of an articulation.

Please keep Articulation focused on deterministic, domain-neutral facts such as get_link_vert_face(), compute_fk(), and get_parent_joint_chain(). Move this operation to an atomic_actions/articulation_geometry.py adapter that accepts an articulation-like provider and returns a typed value. If a class-level API is still needed, expose a deterministic initial mesh snapshot here; perform random sampling and affordance interpretation in the higher layer.

dim=1,
)
valid_faces = face_areas > torch.finfo(vertices.dtype).eps
if not bool(valid_faces.any().item()):

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.

[P3] Preserve vertex-only links in the merged point cloud

_validate_point_cloud_mesh() accepts links with no triangles, and this helper falls back to their vertices only when the entire input has no valid face. In a mixed articulation where the target link has vertices but no (or only degenerate) faces while another link has a valid triangle, valid_faces.any() is true and only triangles[valid_faces] is passed to Open3D. The target vertices are not referenced by any sampled triangle, so target_link_point_cloud is populated while articulation_point_cloud silently omits that same link. Downstream target-neighborhood resolution can then be empty or geometrically inconsistent.

Please either apply the fallback per link before merging or reject links without valid triangle surfaces, and add a mixed-case regression test.

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.

2 participants