Skip to content

Use Three.js-like api for pose math - #884

Merged
Micheal Parks (micheal-parks) merged 10 commits into
mainfrom
pose-updates
Jul 28, 2026
Merged

Use Three.js-like api for pose math#884
Micheal Parks (micheal-parks) merged 10 commits into
mainfrom
pose-updates

Conversation

@micheal-parks

@micheal-parks Micheal Parks (micheal-parks) commented Jul 27, 2026

Copy link
Copy Markdown
Member

Simple refactor before moving on. This PR gathers together all of the loose pose utility functions into a Pose class that matches as best as possible Three.js math library conventions.

@changeset-bot

changeset-bot Bot commented Jul 27, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: df80245

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@viamrobotics/motion-tools Patch

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

@github-actions

github-actions Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-07-28 18:46 UTC

@micheal-parks

Copy link
Copy Markdown
Member Author

Claude (@claude) review

@claude

claude Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Claude finished Micheal Parks (@micheal-parks)'s task in 5m 13s —— View job


Review

  • Read CLAUDE.md and repository conventions
  • Read key changed files (pose.ts, transform.ts, consumers)
  • Reviewed all 41 changed files
  • Posted inline comments
  • Summary below

Overall

Clean, well-scoped refactor. The new Pose class consolidates ~150 lines of scattered utility functions into a cohesive object with a recognisable Three.js-like API (setFrom* / to*, fluent chaining, module-level scratch temporaries). The units contract (mm in Pose, m in Matrix4/Vector3) is clearly documented on the class. Implementing ViamPose so instances are directly passable to SDK APIs is a nice touch. All 41 consumers are migrated correctly — I traced several end-to-end paths (frame sync, gizmo drag, plan replay, LLM delta adapter) and the semantics are preserved.


Issues by severity

Medium

  • No unit tests for Pose itself — the class has 16+ methods with meaningful edge cases (oZ defaulting, copy(undefined) vs copy({}), round-trip consistency for all 4 orientation types, isFinite() with NaN/Infinity, equals() with partial patches). The consumer tests exercise Pose indirectly but don't pin down the class invariants. Given the scope of this migration a dedicated src/lib/math/pose.spec.ts would be a meaningful safety net. See inline comment on line 1 of pose.ts.

Low

  • setFromFrame silent catch-all — the else if (frame.orientation) branch at line 139 is intended to handle ov_degrees but also silently processes any unknown orientation type. If a new orientation type is added without updating this method, it won't warn — it'll just read .value.x/.y/.z/.th which may be undefined, producing NaN in the OV. See inline comment.

  • Module-level scratch naming — the shared ov, quaternion, translation, scale, euler temporaries follow Three.js conventions but don't carry the _ prefix Three.js uses (e.g. _v1). The re-entrancy constraint is a silent invariant. Consider prefixing or adding a comment. See inline comment on lines 9–13.

Nit

  • build-frame-descriptors.ts poseFromFrame allocates new Pose() per call, unlike the module-level tempPose pattern used everywhere else. Cold path (plan loading), so no perf concern — just a consistency note.

What's right

  • copy() semantics are carefully designed: undefined → identity reset (not no-op), with a clear doc comment explaining why. The oZ defaulting logic (default oZ = 1 only when the entire OV component is absent) is subtle but correct.
  • merge() vs copy() is a clean distinction that makes writeMatrix / FrameEditor.setPose much clearer.
  • transform.ts now contains exactly what belongs there: the two matrix-blending functions (composeLocalMatrix, solveEditedMatrix) and applyEulerDeltaToPose, none of which fit inside Pose itself.
  • SelectedTransformControls.svelte's gizmo drag path is well-adapted: refPose.setFromMatrix4(tempRefMatrix) followed by selective field writes for translate-only vs rotate-only drags.
  • The PosePatch export is a useful addition on its own — the writeMatrix utility is cleaner and the type is already being used in a few places outside this PR.

Comment thread src/lib/math/pose.ts
Comment thread src/lib/math/pose.ts
Comment thread src/lib/plugins/MotionPlanReplayer/build-frame-descriptors.ts
Comment thread src/lib/math/pose.ts
Comment thread src/lib/components/Entities/composeBoxMatrix.ts
Comment thread src/lib/math/pose.ts

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

worth just adding a bunch of unit tests to cover this, since it is pretty critical and should be very unit-testable?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yep, will add in this PR

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

actually, just going to follow up so i can make that PR more readable

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm mod unit tests on the pose class

@micheal-parks
Micheal Parks (micheal-parks) merged commit d2c58fe into main Jul 28, 2026
23 checks passed
@micheal-parks
Micheal Parks (micheal-parks) deleted the pose-updates branch July 28, 2026 18:46
@claude claude Bot mentioned this pull request Jul 28, 2026
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