petercorke / petercorke/robotics-toolbox-python

Robot/Link architecture redesign: generic LinkType, one-Robot-class design, kinematic/scene-graph decoupling

Open
#571 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

tech-debt
Dominant language
C++
Stars
3.5k
Forks
624
Avg merge
2d 4h
Merged PRs (30d)
53

Description

Migrated from tech-debt.md (deleted, see repo history via git log -- tech-debt.md). Groups everything under the long-running "Robot/Link architecture" direction -- see the "Roadmap by Peter (2026-07-31)" note that was at the end of the old file: RTB 1.5.0 plans a "major renovation of Swift backend and detangle of SG from RTB" and RTB 2.0.0 plans to "revisit the top-level robot classes. Explicit ETSRobot, Robot -> base". These subitems are the concrete groundwork for that.

Check items off as they land; close this issue once all are done (or split a subitem into its own issue if it grows a life of its own).

  • Make Robot generic over LinkType. DHLink plays two roles (DH-parameter carrier + compiled Link subclass); because Robot(BaseRobot[Link]) pins LinkType=Link, pyright can't see DH parameters on links inside DHRobot, causing 67+ type errors. Fix: class Robot(BaseRobot[RobotLinkType], RobotKinematicsMixin) generic, pinned per subclass (DHRobot(Robot[DHLink]), PoERobot(Robot[PoELink]), RobotURDF(Robot[Link])). Eliminates links property overrides and # type: ignore[union-attr] workarounds.

  • Evaluate "one Robot class, polymorphic Link.A(q)" design. Core idea: Link is an abstract base with A(q) -> SE3; DHLink/PoELink/ETSLink each implement it their own way (DH formula / exp(S*q) / ETS eval, dispatching to fknm for speed). Robot becomes a single class holding list[Link], with factory constructors (Robot.DH(...), Robot.ETS(...), Robot.PoE(...), Robot.URDF(...)) instead of subclasses. Removes ERobot (currently dead: a 5-line pass-through alias for Robot) and the Generic-iterator problem entirely. Robot is arguably a poor name for what's specifically an ETS-based robot -- ETSRobot would be accurate but is an API break; consider for a major version, keeping Robot as a deprecated alias. Preserve the fknm C-extension batch-FK path (robot._fkine_fknm(q)) as an optimised overload when all links are ETSLink -- it's a real perf win, not a design constraint.

  • Delete ERobot (dead code, no functionality) once the above lands.

  • Simplify Link.A(q): drop the _Ts cache. Link.A(q) (Link.py:1509-1516) caches the link's constant (non-joint) ETS prefix as one pre-multiplied matrix to avoid re-multiplying it every call. Benchmarked against just calling self._ets.eval(q) (200k calls, real Panda link): the cached version is ~15-18% slower (1.677us vs 1.421us/call) -- the Python-level overhead of a second method call + a separate @ dispatch for a 4x4 array costs more than the cheap Eigen constant-multiply it's avoiding. Root structural reason: URDF-derived links are at most 2 ETs (one constant, one joint) by construction, so there's nothing meaningful for _Ts to save in the common case. Not yet benchmarked: a link with a genuinely long uncompiled constant-ET prefix (hand-authored ETS models that never call .compile()) -- check that regime before removing the cache outright.

  • Wire ETS.compile() into non-URDF model construction. ETS.compile() (constant-folds consecutive constant ETs) is correct but never called anywhere in the repo, including the hand-authored model constructors that would benefit (models.ETS.Panda() and similar). Worth calling automatically at the end of Robot.__init__ for that construction path, or documenting it as a required convention for model authors.

  • Decouple Robot/Link kinematic state from scene-graph/rendering state. Directly related to desiderata.md's "Stateless over stateful" aspiration (.q retained as persistent state, "not yet achieved"). Robot/Link currently carry not just .q but SceneNode/update() (was _propogate_scene_tree()) machinery -- world-transform bookkeeping that exists purely to support rendering (Swift, PyPlot), mixed into the kinematic model classes. Agreed direction: Robot/Link become a pure kinematic model (links, DH/ETS/PoE params, joint limits, geometry attachments -- no live world-transform state); a separate viz-owned "instance handle" owns mutable per-simulation state (q, plausibly base/tool) and computes part poses via the pure FK path.

    • Partial progress already shipped: Robot.fkine_geometry(q, robot_alpha, collision_alpha) computes every geometry part's world pose purely from an explicit q (via fkine_all + each geometry's fixed local offset), verified bit-for-bit against the old SceneNode-based path to ~1e-9 precision (test_Robot.py::test_fkine_geometry_matches_scene_graph). Swift's env.add() now returns a handle (AssemblyHandle, generalized from RobotHandle) that owns q/qd and calls through to fkine_geometry -- Swift's hot render path no longer depends on SceneNode mutation. Robot/Link themselves are still unchanged (still carry .q, still have SceneNode); this is the pure alternative living alongside the stateful path, not a replacement.
    • Not yet made pure: gripper joints (fkine_geometry still reads gripper.q as ordinary state, not a parameter).
    • Remaining: actually remove SceneNode from Robot/Link, update PyPlot/teach to the handle-based model, decide base/tool ownership (see below).
  • Resolve base/tool ownership. Some models genuinely need base/tool as part of their kinematic definition (e.g. a fixed pedestal offset); for others it's purely "instance placement," indistinguishable from q. No formalized answer yet -- needed before the handle redesign above can be completed (does the instance handle own base/tool, or does the model?).

  • Remove Robot._fk_dict() (dead code). Walks every link's geometry/collision and reads each shape's cached _wT/_wq (populated by the old _update_link_tf() + scene-graph-propagate pass). Its only real caller was Swift's old _draw_all(), replaced by fkine_geometry() -- grepped, no remaining callers anywhere in this repo. Shape.fk_dict() (the per-shape method it calls) is still alive and used directly by Swift for plain shapes -- don't touch that.

  • Revisit Robot.rne()'s mdh-based misuse guard once the hierarchy above is resolved. Current guard (assert getattr(self, "mdh", True), ...) checks the mdh attribute rather than class identity, because joint-last compliance tracks the DH convention in use (DHLink._to_ets()'s MDH branch), not the DHRobot class itself -- a class-name-based check would wrongly reject a compliant mdh=True DHRobot. If the hierarchy redesign above lands (one Robot class, or DHRobot stops subclassing Robot), this question may become moot or need re-deriving structurally rather than via a runtime attribute check.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

This is a multi-part architecture effort rather than one newcomer-sized change. Start by splitting out a concrete subitem, then read Link.py:1509-1516 and test_Robot.py::test_fkine_geometry_matches_scene_graph where relevant. Done means one scoped redesign item is implemented, its existing behavior is covered, and this checklist is updated.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
backend-api-design
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
28/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.