Skip to content

Move frame plugin UX improvements - #886

Open
Micheal Parks (micheal-parks) wants to merge 7 commits into
mainfrom
move-frame
Open

Move frame plugin UX improvements#886
Micheal Parks (micheal-parks) wants to merge 7 commits into
mainfrom
move-frame

Conversation

@micheal-parks

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

Copy link
Copy Markdown
Member

Overview

Updates the move frame plugin to allow much more flexibility. Users can now drag around the moved frame to translate and rotate it in world / local space. Additionally, users can tweak final values in the floating pane for fine grained edits.

A ghost image of the destination of the the frame and all of its children now shows as the user chooses the desired destination spot. This will be a very useful first step ux-wise for users who want to avoid collisions during movement.

Screenshot 2026-07-28 at 14 59 11

@changeset-bot

changeset-bot Bot commented Jul 28, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 05daa7d

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 Minor

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 28, 2026

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1

QR code for preview link

🚀 View preview at
https://viamrobotics.github.io/visualization/pr-preview/pr-886/

Built to branch gh-pages at 2026-07-28 22:16 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@micheal-parks

Copy link
Copy Markdown
Member Author

Claude (@claude) review

@claude

claude Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

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


PR Review

  • Read CLAUDE.md
  • Read changed files
  • Run diff analysis
  • Post inline comments
  • Write review summary

Overall this is a high-quality rewrite. The architectural choices are sound — gizmo ownership via a module singleton, NonSelectable for ghost pass-through, ECS entities for ghost rendering (no custom material budget), $state.raw for Three.js instances, and the post-save fold-into-world fix in usePartConfig. Test coverage is thorough and the edge-case matrix in moveGhosts.spec.ts is excellent.

Issues to address

Fragile module-level scratch in useMovedFrameMatrix.svelte.ts (inline comment on line 10)
const tempPose = new Pose() is module-level and shared across all call sites. No data race exists today (Svelte 5 derived computations are synchronous), but the fragility is unnecessary. Move it inside the factory function.

Ghost colour duplicated (inline comment on MoveTargetGhost.svelte line 11)
'#37a06f' appears verbatim in both MoveTargetGhost.svelte and moveGhosts.ts. Extract to a single exported constant.

Nits

GC pressure at drag-frame rates (inline comments on MoveControls.svelte:181 and MoveTargetGhost.svelte:24)
ghostDelta allocates a new Matrix4().copy(...).invert() inner scratch on each drag frame. origin and target in MoveTargetGhost allocate new Vector3/Quaternion on each currentWorldMatrix update. All straightforward to fix with module-level scratch variables — moveTargetPose.ts already demonstrates the pattern.

Misleading function name (inline comment on MoveControls.svelte:217)
applyEulerDeltaToPose performs an absolute replacement of Euler channels, not an incremental delta. Usage here is correct, but the name will mislead future callers. setOrientationFromEuler or setEulerOnPose would be more accurate.

shapeKind bitmask comment (inline comment on moveGhosts.ts:70)
The reason Center is in the bitmask (presence triggers respawn; value updates are handled by syncShape) isn't obvious — a one-liner would prevent future confusion.

Map semantics in useMoveGhosts (inline comment on useMoveGhosts.svelte.ts:39)
The !ghosts.has(entity) guard works correctly because ghosts are values not keys, but this is subtle enough to warrant a clarifying comment.

Looks good ✓

  • Early-return-before-stopPropagation in useEntityEvents is the correct pattern for letting events propagate through NonSelectable geometry — comment explains it well.
  • onDestroy cleanup in MoveGizmo for the dragging guard is the right defensive move for panel-close mid-drag.
  • The wantsGizmoarmedshowGizmo derivation chain, with the preemption UX ("Another move panel has the gizmo"), is well-designed for the multi-panel case.
  • Ghost respawn on shape kind change vs. in-place patch on dimension change is a sensible optimization.
  • The wasDirty edge-trigger in usePartConfig correctly solves the save-then-snap-back regression without fighting usePoses during live monitoring.
  • $effect.pre for seeding anchor in MoveGizmo is correct — needs to run before child effects that read the scene graph in the same flush.

import { RefreshRates, useSettings } from '$lib/hooks/useSettings.svelte'
import { Pose } from '$lib/math'

const tempPose = new Pose()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Module-level scratch variable shared across all instances of useMovedFrameMatrix. If two panels are open simultaneously, their $derived computations run sequentially (Svelte 5 is single-threaded), so there's no actual corruption — but it's fragile coupling. Move tempPose inside the function body to make each call site fully independent:

Suggested change
const tempPose = new Pose()
export const useMovedFrameMatrix = (
partID: () => string,
frameName: () => string,
enabled: () => boolean
): MovedFrameMatrix => {
const settings = useSettings()
const client = useRobotClient(partID)
const tempPose = new Pose()

interface Props {
/** Where the frame is right now — world space, metres. */
currentWorldMatrix: Matrix4
/** Where the gizmo has staged it — world space, metres. */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The ghost colour '#37a06f' is also hardcoded verbatim in moveGhosts.ts:31. They'll drift apart on the first colour tweak. Extract to a shared constant (e.g. in a moveGhostColor.ts or at the top of moveGhosts.ts and re-exported).

Comment thread src/lib/plugins/MoveFrame/MoveControls.svelte Outdated
Comment thread src/lib/plugins/MoveFrame/MoveControls.svelte
Comment thread src/lib/plugins/MoveFrame/useMoveGhosts.svelte.ts
Comment thread src/lib/plugins/MoveFrame/moveGhosts.ts
Comment thread src/lib/plugins/MoveFrame/MoveTargetGhost.svelte Outdated
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
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