common: add Cylinder to the Geometry oneof - #894
Draft
btshrewsbury-viam wants to merge 4 commits into
Draft
btshrewsbury-viam wants to merge 4 commits into
btshrewsbury-viam wants to merge 4 commits into
Conversation
spatialmath.Cylinder exists in the RDK as a collision primitive but has no
representation here, so it cannot cross the wire at all. Cylinder.ToProtobuf()
in the RDK is currently:
panic("Cylinder.ToProtobuf: unimplemented -- no Cylinder message in commonpb")
Any component returning a cylinder from Geometries() panics its own
GetGeometries handler, and a cylinder in a kinematics model is unrenderable by
consumers. In practice that means round hardware has to be squared off to a box
or approximated by a capsule, neither of which is the shape.
A capsule is not a substitute: its length is tip-to-tip with hemispherical
caps, so it always under-approximates a flat-ended cylinder, and it cannot be
flatter than length = 2*radius. This message has flat caps and no aspect
constraint.
Additive change: new field number 8 in the oneof, no existing field touched.
buf lint and buf breaking both clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
RDK's Cylinder has a `capped` flag: when false the end caps are omitted, producing an open tube -- a cylindrical surface with no interior volume, which collides only when something crosses its wall. It models open containers a robot must reach into. Without a field for it, that state is lost on the wire and every open tube silently decodes as a solid. Phrased as `uncapped` rather than `capped` so the proto3 default (false) is the solid cylinder. That is both the common case and the conservative one: treating an open tube as solid over-approximates, while treating a solid as open would let a caller plan a path straight through it. A consumer that ignores the field entirely still gets the safe interpretation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Regenerated common/v1/common.pb.go from the merged proto; the generated file was the only conflict and is not hand-resolvable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The comment argued the solid default was the conservative reading: an open tube read as solid over-approximates, a solid read as open would let a caller plan straight through it. Adversarial review checked that against RDK and it does not hold. Cylinder collision there goes through the tessellated mesh, which is a surface. A point fully inside a "solid" cylinder does not register a collision, where the same point inside a box, sphere or capsule does -- verified directly. So capping adds two zero-thickness cap discs rather than filling the interior, and the interior is equally invisible either way. There is also a path where over-approximating is the dangerous direction: motionplan whitelists pairs found in collision at the start pose for the whole plan, so a phantom cap can whitelist a real obstacle. The default itself is still right -- it is the common case, what NewCylinder builds, and what a cylinder means in URDF and SDF. Only the justification was wrong, and proto field comments are permanent, so it should not ship claiming a guarantee it cannot back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a
Cylindermessage to theGeometryoneof.Why
spatialmath.Cylinderexists in the RDK as a collision primitive but has no representation on the wire, so it cannot leave the process that created it.Cylinder.ToProtobuf()is currently:Not an error — a panic. Any component that returns a cylinder from
Geometries()panics its ownGetGeometrieshandler, so the response never forms and the client renders nothing. A cylinder in a kinematics model is the same problem one step removed, reachable through any RPC that ships aGeometriesInFrame.This is not hypothetical. A gripper module shipped exactly this and was broken on arrival — the 3D scene rendered nothing at all, while every unit test passed, because none of them crossed the wire.
Why not just use a capsule or a box
Both are already available and both are wrong for round, flat-ended hardware:
lengthis tip-to-tip with hemispherical caps, so it always under-approximates a flat-ended cylinder: at r=35.5, l=126 only 55mm is at full radius and the rest tapers to a point. It also cannot be flatter thanlength = 2*radius, so a short wide disc is inexpressible.A vacuum gripper's body and suction cups, a wheel, a roller, a puck — these are cylinders, and today the only options are a pill or a brick.
The change
plus
Cylinder cylinder = 8;in theGeometryoneof.Why
uncappedand notcappedRDK's
Cylindercarries acappedflag: uncapped means an open tube, a cylindrical surface modeling a container a robot reaches into. Omitting the field would silently turn every open tube into a solid.It is spelled negatively so the proto3 default (
false) is the solid cylinder — the overwhelmingly common case, whatNewCylinderbuilds, and what a cylinder means in URDF and SDF. A consumer that ignores the field reads every cylinder as solid.An earlier revision of this PR justified that default as the conservative reading. Adversarial review checked it and it doesn't hold: RDK's cylinder collision goes through the tessellated mesh, which is a surface, so a point fully inside a "solid" cylinder registers no collision where the same point inside a box, sphere or capsule does. Capping adds two zero-thickness discs rather than filling the interior. There is also a path where over-approximating is the dangerous direction — motionplan whitelists pairs found in collision at the start pose for the whole plan, so a phantom cap can whitelist a real obstacle. The default is still right; the safety argument was not, and proto comments are permanent, so it's gone.
Additive: new field number, no existing field touched or renumbered.
buf lintandbuf breaking --against mainare both clean. Regeneratedcommon/v1/common.pb.gowith the pinnedprotoc-gen-go v1.35.1;common.protodeclares no services, so no grpc/gateway output changed.Mixed-version behavior — worth a release note
An old client decoding a Cylinder from a new server puts field 8 in unknown fields (preserved on re-marshal), every oneof getter returns nil, and
NewGeometryFromProtoreturnsunsupported Geometry type. Most callers surface that loudly. One does not:robot/impl/local_robot.gologs a component'sGeometries()error at Debug and builds the frame-system part with no geometry — so an old viam-server paired with a new module returning cylinders would silently plan against a component with zero collision geometry.That is inherent to adding any oneof variant rather than specific to this one, but it should be called out when this ships.
Generated artifacts
common/v1/common.pb.gois regenerated and byte-identical to what the repo's pinned toolchain produces (buf 1.55.1, protoc-gen-go v1.35.1).common.protodeclares no services, so there is no.pb.gw.gochange.The tracked JS/TS bindings under
gen/js/are not regenerated here — that path needs the old protoc builtin js compiler thebuf-webtask pins..github/workflows/compile_protos.ymlregenerates and auto-commits them behind theready-for-protoslabel; flagging it explicitly so it isn't assumed done.Follow-up
Cylinder.ToProtobuf()+ parsing).referenceframe/xml_conversions.gorejects standalone URDF<cylinder>with "not natively supported in spatialmath" — that statement stops being true once this lands, and URDF's cylinder maps exactly onto this message. Worth a follow-up.