reconstructed flattened frames from rdk - #923
Conversation
🦋 Changeset detectedLatest commit: 73cfb8e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
3ee6ec7 to
630b7e3
Compare
630b7e3 to
f6f6777
Compare
3e557f7 to
ee977d3
Compare
ee977d3 to
e5a79b8
Compare
ca4d5df to
cb818b3
Compare
cb818b3 to
46652af
Compare
|
Claude (@claude) review |
|
Claude finished Devin T. Currie (@DTCurrie)'s task in 4m 26s —— View job Review
This is a well-designed PR. The core logic in Correctness
Exception ordering —
DH guard placement — The Emit order — Links before joints in IssuesMinor — Test gap — DesignThe The two-level test strategy — raw |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| * carry a numeric `0` id, which is falsy but names a frame. | ||
| */ | ||
| const nodeName = (value: string | undefined): string | undefined => | ||
| value === undefined || value === '' ? undefined : value |
There was a problem hiding this comment.
nodeName is already exported from ./jointColumns with identical semantics and the same truth-vs-explicit rationale. Worth importing rather than re-defining — the one difference is that jointColumns's doc comment says "only the empty string, never a number" (which is accurate for the plan-dump route but not for this one), so the comment here would need to stay local even if the function itself moves.
| value === undefined || value === '' ? undefined : value | |
| const nodeName = (value: string | undefined): string | undefined => | |
| value === undefined || value === '' ? undefined : value |
(no change to the body — just flagging the duplication; importing from ./jointColumns and keeping the comment here as inline context is the cleaner option)
|
|
||
| expect(frames['ok']).toBeDefined() | ||
| expect(frames['also-ok']).toBeDefined() | ||
| expect(frames['bad']).toBeUndefined() |
There was a problem hiding this comment.
The "no partial writes" invariant should be pinned here. If the try/catch ever moved or a throw happened after frames[origin] was written, bad_origin could leak into the scene as a dangling mount frame. The current ordering in addPart (model parsing at line 189, frame write at line 194) does protect against this, but the test doesn't assert it.
| expect(frames['bad']).toBeUndefined() | |
| expect(frames['ok']).toBeDefined() | |
| expect(frames['also-ok']).toBeDefined() | |
| expect(frames['bad']).toBeUndefined() | |
| expect(frames['bad_origin']).toBeUndefined() |
46652af to
73cfb8e
Compare
A joint whose parent names something the model never declares is a second root to RDK, not a broken chain: buildModelFrameSystem seeds its walk with every child whose parent is absent from transforms and hangs it off fs.World(). So stray sorts against base rather than trailing the walk, and nothing is guessed or warned about. Confirmed against go.viam.com/rdk, which builds this model and reports its schema as [stray j1].
73cfb8e to
828b660
Compare
1 participant
Reconstructs the flattened frame system RDK's motion service builds, from what a robot will actually hand a browser, so a part's configured geometry is drawn even when the part also carries a kinematic model. Stacks on #922. Nothing imports it yet; the move preview in #908 is the consumer.
The
planDoCommand answers with a trajectory and nothing else, joint values and no poses, so anything that wants to draw a plan has to supply the kinematics itself. The plan replayer gets them from RDK's debug dump, which no RPC exposes. What is exposed isrobot.frameSystemConfig: oneFrameSystemConfigper part carrying that part's offset from its parent plus, inkinematics, the very same model config the dump nests atframes['left-arm'].frame.model. The gap between the two is the flattening RDK does when it builds the frame system, and that is what this file reproduces.Stack
$lib/motion(make motion utils reusable #917)MoveFrameplugin (Motion plan preview #908)Frontend
frameSystemToPlanFrames(parts)emits the frames RDK produces per part:p_originfor the mount offset and geometry,pfor the model or an identity placeholder, andp:<link.id>/p:<joint.id>for the model's own nodes, each parented inside the model's namespace.p_origincarries the part's configured geometry unconditionally, matchingcreateFramesFromPart.pand nop:<link>frames, matching the same function.drawableModelOfvalidates a model whole before any of it is emitted, and rejects it entirely if RDK would have. That covers DH kinematics, a joint typeJointConfig.ToFramecannot build, and a node with no id.p_origincollides with another part's own name is skipped with a warning rather than overwriting it.frameDescriptors.tsgainsDECODED_FRAME_TYPEand aDecodedFrameshape, plus the switch case that turns one into a static descriptor.inferGeometryTypenow also runs for frames that came from a model config, where RDK has not yet resolved an empty"type"into a named one.Why?
How does RDK flatten a frame system, and what does this file have to reproduce?
For a part
p, RDK publishes four kinds of frame.p_originholds p's offset from its parent plus the part's configured geometry, and is parented to whatever p is parented to.pholds the model, or an identity placeholder, and is parented top_origin.p:<link.id>holds the link'sLinkConfigandp:<joint.id>holds the joint'sJointConfig, each parented to its ownp:<parent>inside the model's namespace, falling back top_originfor a node that namesworldor names nothing. A link namingworldinside a model means the model's own mount, not the scene root. This is verified against__fixtures__/plan.json, a dual-arm rig whose dump contains exactly this shape.The one asymmetry worth naming: a part without a model carries its geometry on
p_originand leavespan empty frame. That is what RDK does, and it is whypis emitted at all, since descendants are parented to the bare name and the frame has to exist for them to hang off.Why a frame type that RDK does not have?
A part's own pose and geometry arrive over this route as protobuf, not as Go's JSON marshal of a
GeometryConfig. Re-encoding them into astaticframe purely so the existing switch could decode them again would round-trip a protoGeometrythrough a lossy intermediate. Carrying them already decoded avoids that. Only the part-level frames need it: a model's links and joints come off the wire as the sameLinkConfig/JointConfigJSON the plan dump carries, so they take the normal path.Why does a jointless model drop its links?
Because RDK does. Its predicate is
len(modelFrame.DoF()) == 0 && len(offsetGeom.Geometries()) > 0, which for an SVA model is the joint count: a model with no degrees of freedom and a geometry configured on the part is not published as a model at all. The model frame is replaced with a bare static one, and a static frame is not flattenable, so nop:<link>frames exist for it downstream. The user's shape was meant to replace the model's. Drawing the links anyway shows collision volumes RDK discarded, doubled up with the one that replaced them, so the stand-in carries no geometry of its own either.The stand-in sits at identity, where RDK puts it at
modelFrame.Transform([]Input{}). That is the model's whole extent at rest rather than a small internal offset: a two-link gripper 10 mm and 40 mm long puts RDK's frame 50 mm away from this one. Computing it would mean a second forward-kinematics implementation in this file. Anything parented to the bare part name is off by that much, and nothing else is. This is called out in the code at the branch.Why reject a whole model over one bad node?
Because half-rejecting it was worse than useless. Skipping a single unsupported joint left everything below it still naming
p:<jointId>, a key with no frame behind it. Those frames were not dropped along with the joint; they resolved to no parent at all and drew at the scene origin. A fuzz of 4,000 random configs produced 933 such dangling parents.Rejecting the model is also what RDK does.
JointConfig.ToFramereturnsNewUnsupportedJointTypeErrorfor anything but revolute and prismatic, andUnmarshalModelJSONpropagates that error rather than skipping the joint, so the part ends up with no model frame at all. The part still draws here, model-less, keeping its mount offset and its configured geometry.Why guard against a part named after another part's generated origin?
originNamederives<part>_originmechanically and has no idea whether some other part in the same reply is literally named that. A partcamand a partcam_originproduce the same key:cam's mount-offset frame andcam_origin's own bare-part frame both land onframes['cam_origin']. Whichever part is processed second would silently overwrite the first's mount offset and entire collision volume and reparent its descendants, and array order decides which "second" means, so it would not even be deterministic across replies. Both keys are checked so the collision is caught from either direction.Why read each part in its own try?
Struct.toJson()throws on a non-finite number and on aValuewith no kind set, and the throw escaped the loop: one malformed part meant no preview at all rather than one part missing. RDK's own marshal refuses infinities first, but its URDF converter does produce infinite limits for a continuous joint, so the value exists upstream and only that step stands between it and here.Why does
nodeNametest forundefinedand''rather than truthiness?Nothing on this route should emit a numeric node id.
LinkConfig.IDandJointConfig.IDare Go strings, and the URDF converter readsjointElem.Name, an element's name attribute rather than its index. Butkinematicsarrives as aStruct, which can carry aNumberValuewhatever RDK meant to put there, and a0that arrived as a JSON number is falsy. A truth test would drop that joint and everything below it.Why are Go symbols named rather than cited by line?
Line numbers drift between releases, and the version in this repo's
go.modis not the one to check them against.v0.122.0has nonamedframe type at all:namedFrameis absent fromregister.goand has noMarshalJSON, so dumping a frame system there fails outright with "not a registered Frame implementation". Thenamed/inner_frameshape reproduced in this file, and theplan.jsonfixture with its 41namedframes, both postdate that version.go.modpins what this repo's own Go compiles against, not what a machine runs, so this targets RDK v1.x.What does this route not give you?
Two things, both consequences of what
FrameSystemPart.ToProtobufsends. A part can look model-less here while RDK has a real model for it, becausekinematicsis empty wheneverSimpleModel.modelConfigis nil, which includes a model assembled from frames rather than parsed from a config. Andprimary_output_framenever arrives: RDK's model envelope carries it,ToProtobufsends onlymodel, and it isframeDescriptors' primary source for a model's end effector. On this route that branch cannot fire, so a branching model that declares no output frame will not get its end-effector remap here even though the replayer does.Testing
pnpm exec vitest --runpasses 870 tests across 75 files, up 50 tests and one file from the base branch.pnpm exec svelte-checkreports 0 errors and 0 warnings.The section that makes a consumerless module reviewable is
equivalence with a real plan dump: it builds aframeSystemConfigout ofplan.json's own model, synthesizes a frame system from it, and compares the result against what the dump itself produces. That is real signal rather than a restatement, because the two sides are genuinely different JSON: the flattened entry carries a quaternion orientation and a named geometry type where the model config carries a null orientation and an empty type, so the synthesized side has to survive the model-config encoding and still land on the same pose and shape.It reads stronger than it is, though, and
the synthesized frame system itselfanda model that branchesexist to cover the gap. Both sides of the equivalence run throughbuildFrameDescriptors, so a deterministic mistake in the descriptor builder appears identically on both and cancels out. Reversing the joint walk's sibling sort passes every assertion in that block.Both behavior claims were checked by reverting them. Dropping the origin geometry for modelled parts fails 2 tests with
expected undefined to be 'sphere'. Removing the jointless branch fails 2 withexpected true to be falseandexpected undefined to be 'static'.Cases the fixtures pin that a mutation could previously get away with:
_originpose. Holding the part's offset is the entire reason that frame exists, and every fixture went through a helper that defaults the pose to identity, so it could be discarded or replaced outright without a failure. Now asserted on the frame, through the descriptor builder, and as absent from the bare part frame.dhParamsand no links or joints, which is what a real DH config looks like; a fixture that also declared a link makes both orderings of the guard pass.common.v1.Geometryand this package's actually differ, and so the only one where replacing the byte round-trip with a cast would have been caught. Every other fixture is a sphere, which a cast carries perfectly well.