Skip to content

Add tianji marvin | Fix solver tcp inversion - #686

Merged
matafela merged 8 commits into
mainfrom
cj/add-tianji-marvin
Sep 28, 2026
Merged

matafela merged 8 commits into
mainfrom
cj/add-tianji-marvin

Conversation

@matafela

@matafela matafela commented Sep 24, 2026 •

Copy link
Copy Markdown
Collaborator

Description

  • Add Tianji Marvin: embodichain/lab/sim/robots/tianji_marvin.py
  • Fix PytorchSolver get_ik tcp pose inversion error.
  • Unify the TCP pose inversion operation across solvers.

Type of change

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

Checklist

  • I have run the black . command to format the code base.
  • I reviewed affected documentation and agent context, updated it where needed, or explained why no update was needed.
  • 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.

@matafela
matafela requested a review from yuecideng September 24, 2026 10:57
@greptile-apps

greptile-apps Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 2/5

[High risk] Replaces matrix inversion with validated rigid-transform function across solvers.

The PR does not appear safe to merge while Tianji Marvin can select or download the wrong asset and a failed USDZ rebuild can leave a stale deliverable.

Fix All in CodexFindings

  1. P1 Local overrides trigger downloads ▶
  2. P1 Variant can load wrong URDF ▶
Fix with agent prompt
### Issue 1
embodichain/lab/sim/robots/tianji_marvin.py:undefined-109
If a caller supplies a local `fpath` on a machine without the Tianji Marvin asset cached, this line resolves the default asset before the override is applied. That starts a download, so configuration creation fails offline even though the caller provided a usable local URDF. Resolve the default asset only when no path override was supplied.

### Issue 2
embodichain/lab/sim/robots/tianji_marvin.py:undefined-109
If someone changes `with_gripper` in a saved configuration but keeps its serialized `fpath`, the merge restores the old variant’s URDF after this method selects control parts and solver frames for the new variant. Simulation then loads a model that does not match those controls and FK/IK frames. Reconcile the asset path with the selected variant when rebuilding the configuration.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

The PR adds the Tianji Marvin robot and a shared rigid-TCP inversion path. It also expands Gym action and recording contracts, physical objectives, scene USD delivery, task-program articulation bindings, and asset handling.

  • A failed scene USDZ rebuild can leave an obsolete package beside the new scene export.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  E[Updated scene export] --> U[Build scene.usda]
  U --> Z{Build scene.usdz}
  Z -- succeeds --> N[Replace old USDZ]
  Z -- fails --> O[Old USDZ remains]
  O --> P[Preview or distribution may use stale scene]
Loading

Reviews (6) · Last reviewed commit: "Merge branch 'main' into cj/add-tianji-m..."

raise TypeError("with_gripper must be a boolean.")

self.uid = "TianjiMarvin"
self.fpath = self._pk_urdf_path

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Local overrides trigger downloads

If a caller supplies a local fpath on a machine without the Tianji Marvin asset cached, this line resolves the default asset before the override is applied. That starts a download, so configuration creation fails offline even though the caller provided a usable local URDF. Resolve the default asset only when no path override was supplied.

Knowledge Base Used: Asset and dataset management

Prompt To Fix With AI
This is a comment left during a code review.
Path: embodichain/lab/sim/robots/tianji_marvin.py
Line: 99

Comment:
**Local overrides trigger downloads**

If a caller supplies a local `fpath` on a machine without the Tianji Marvin asset cached, this line resolves the default asset before the override is applied. That starts a download, so configuration creation fails offline even though the caller provided a usable local URDF. Resolve the default asset only when no path override was supplied.

**Knowledge Base Used:** [Asset and dataset management](https://app.greptile.com/dexforce/-/custom-context/knowledge-base/dexforce/embodichain/-/docs/asset-data-management.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Codex Fix in Claude Code

raise TypeError("with_gripper must be a boolean.")

self.uid = "TianjiMarvin"
self.fpath = self._pk_urdf_path

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Variant can load wrong URDF

If someone changes with_gripper in a saved configuration but keeps its serialized fpath, the merge restores the old variant’s URDF after this method selects control parts and solver frames for the new variant. Simulation then loads a model that does not match those controls and FK/IK frames. Reconcile the asset path with the selected variant when rebuilding the configuration.

Knowledge Base Used:

Prompt To Fix With AI
This is a comment left during a code review.
Path: embodichain/lab/sim/robots/tianji_marvin.py
Line: 99

Comment:
**Variant can load wrong URDF**

If someone changes `with_gripper` in a saved configuration but keeps its serialized `fpath`, the merge restores the old variant’s URDF after this method selects control parts and solver frames for the new variant. Simulation then loads a model that does not match those controls and FK/IK frames. Reconcile the asset path with the selected variant when rebuilding the configuration.

**Knowledge Base Used:**
- [Simulation lab](https://app.greptile.com/dexforce/-/custom-context/knowledge-base/dexforce/embodichain/-/docs/simulation-lab.md)
- [Motion planning and kinematics](https://app.greptile.com/dexforce/-/custom-context/knowledge-base/dexforce/embodichain/-/docs/motion-planning-and-kinematics.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Codex Fix in Claude Code

Comment thread embodichain/lab/sim/robots/tianji_marvin.py
@matafela matafela changed the title Add tianji marvin Add tianji marvin | Fix solver tcp inversion Sep 28, 2026
Comment thread embodichain/lab/sim/motion/solvers/srs_solver.py
Comment thread docs/source/api_reference/index.rst Outdated
@matafela
matafela merged commit 9a2e613 into main Sep 28, 2026
9 checks passed
@matafela
matafela deleted the cj/add-tianji-marvin branch September 28, 2026 10:26
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