app-16292: add Gizmos plugin#738
Conversation
🦋 Changeset detectedLatest commit: 744e462 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 |
Foundation for the upcoming Gizmos plugin. No callers yet; these symbols are consumed by follow-up PRs in the stack. * writeMatrix(entity, patch): patch a Pose onto an entity's Matrix trait via createPose/matrixToPose/poseToMatrix, then dirty the trait. Used by the refactored Details panel and the Gizmos plugin. * CustomDetails trait: marker so the Details panel suppresses its default frame/pose UI for entities that render their own. * useMouseRaycaster firstHitOnly option (forwarded to three-mesh-bvh). * 'gizmo' value added to Settings.interactionMode. * Re-exports of DEFAULT_LINE_WIDTH and ARROW_LENGTH for downstream consumers.
The Details overlay component had grown to handle four entirely different entity shapes (color, geometry, line, matrix) inline. Split each into its own component under details/ so callers and reviewers can reason about one entity type at a time. * ColorDetails.svelte, GeometryDetails.svelte, MatrixDetails.svelte and LineDetails/LineDetails.svelte are now individual focused components. * linePositions.ts extracts the line-position derivation with its own spec. * MatrixDetails uses writeMatrix() added in the prior PR. * vitest-setup-client.ts stubs @threlte/extras Portal / PortalTarget / HTML so the refactored panel can render in unit tests. Behavior should be unchanged; existing Details.svelte.spec.ts continues to pass with the new mocks.
Forward-looking changes to keep the next PR (Gizmos plugin) small.
* Entities/Line.svelte, Entities/Mesh.svelte: default opacity 1
instead of 0.7. The 0.7 default predated transparent materials
being opt-in; entities that want transparency now set
`opacity.current` explicitly.
* Entities/LineDots.svelte: track the IDs returned by `addInstance`
on each effect run rather than assuming sequential 0..N-1. Fixes
"Invalid instanceId" when positions change.
* Scene.svelte: enable BVH raycasting when `interactionMode` is
'gizmo', matching the existing 'measure' / 'select' branches.
* SelectedTransformControls.svelte: read `ref.position` /
`ref.quaternion` directly instead of `getWorldPosition` /
`getWorldQuaternion`. Frame-style renderers with
`matrixAutoUpdate = false` had stale world matrices, so the gizmo
visuals moved but the entity Matrix never updated.
* Popover.svelte: extend the trigger / children snippets with
`{ isOpen }` and `{ close }` props so consumers can react to open
state and close programmatically.
* dashboard/Button.svelte: add the `shapes` icon (lucide) and a
`disableTooltip` prop so tools with persistent active state can
suppress the noisy hover tooltip.
DTCurrie
force-pushed
the
app-16292/3-component-tweaks
branch
from
cddf5ac to
030b969
Compare
New opt-in plugin that lets users place editable geometry primitives (arrow, plane, line, coordinate system, geometry, normals) in the scene by clicking on existing surfaces. Mounted by the consumer like the other plugins under src/lib/plugins/. Plugin entry: Gizmos.svelte. Each tool under tools/ is a small Svelte component that places its primitive type via the shared usePlace, useGestures and usePending hooks. Per-entity Details UIs (e.g. GizmoArrow, GizmoPlane, GizmoPolylineMeasure) render through the Details panel's `details-extensions` portal target and use the `CustomDetails` trait added in PR 1 to suppress the default frame UI. Wiring: * src/lib/plugins/index.ts: re-export Gizmos. * src/routes/+layout.svelte: mount Gizmos in the demo route so the dev app exercises it. Tests cover the pure helpers (cursor, surface, matrix, spawn, buildNormalsMesh, buildSurfaceNormalsGeometry).
DTCurrie
force-pushed
the
app-16292/4-gizmos-plugin
branch
from
043b1e2 to
50cffd7
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- Move writeMatrix below updateGeometryTrait for consistent file layout - Add JSDoc to writeMatrix documenting round-trip semantics and no-op behavior - Filter undefined values from patch before Object.assign to prevent NaN in matrix - Add comment clarifying firstHitOnly affects both hover and click handlers - Strengthen orientation test with oX/oY/oZ assertions Co-authored-by: Devin T. Currie <DTCurrie@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
- LineTool: capture pending.current before pending.set(undefined) so finalizePending returns the committed entity - LineTool: select committed entity on closed-loop path in onclick - GizmoDetails: move entity $derived declaration before useTrait call - Gizmos: replace <fieldset> (no legend) with <div> - spawn: add comment noting nextIndex is O(n) intentionally - GizmoNormals: change color-update $effect to $effect.pre for consistency Co-authored-by: Devin T. Currie <DTCurrie@users.noreply.github.com>
- SelectedTransformControls: expand onChange comment to document the
flat-scene-hierarchy contract (non-FramesAPI entities must mount under
the scene root for local ≈ world; points to stageFrameTransform as the
parent-inverse reference if that ever changes)
- Button.svelte.spec.ts: add test verifying aria/role contract is
preserved when disableTooltip=true and active=true
Popover children type: Snippet<[]> (implicit children) is assignable to
Snippet<[{ close: () => void }]> under TypeScript callback-parameter
compatibility — pnpm check is clean, no change needed.
Co-authored-by: Devin T. Currie <DTCurrie@users.noreply.github.com>
- linePositions.ts: add bounds check to writeLinePosition; return copy unchanged on out-of-bounds index - Details.svelte: await navigator.clipboard.writeText() and wrap in try/catch to handle permission denial - MatrixDetails.svelte: add comments explaining useQuery vs configFrames and unconditional rendering - linePositions.spec.ts: add edge cases for out-of-bounds write and single-point removal guard - Add component tests for ColorDetails, GeometryDetails, MatrixDetails, and LineDetails Co-authored-by: Devin T. Currie <DTCurrie@users.noreply.github.com>
Co-authored-by: Devin T. Currie <DTCurrie@users.noreply.github.com>
…otion-tools into app-16292/3-component-tweaks
…otion-tools into app-16292/3-component-tweaks
…otion-tools into app-16292/4-gizmos-plugin
…92/3-component-tweaks
…92/4-gizmos-plugin
…otion-tools into app-16292/4-gizmos-plugin
| export const PendingGizmo = trait(() => true) | ||
|
|
||
| /** Renderable plane gizmo; normal is +Z in the entity's local frame. */ | ||
| export const ReferencePlane = trait({ width: 500, height: 500 }) |
There was a problem hiding this comment.
| export const ReferencePlane = trait({ width: 500, height: 500 }) | |
| export const Plane = trait({ width: 500, height: 500 }) |
I'd maybe put this with the rest of the geometry? when looking at the props it seems weird to load it with use case
There was a problem hiding this comment.
I wanted to really make sure it was obvious this was not a viam-supported geometry type, thus the separation. they also have some additional options that it didn't want to have to thread into the geometry tool and make it more complicated.
There was a problem hiding this comment.
wouldn't the additional options be other traits though? and I get that - but that sounds like something that would be surfaced to the users, not the underlying data itself. Plane to me is more ECS-forward, it's a trait that communicates a geometry type and therefore a shape to render - traits (components) should be as stripped from complicated ideas as possible
There was a problem hiding this comment.
No it has specific placement options that are different than the geometries, like the plane it is on and how far to offset it on that. So it literally has different placement options in the gizmo menu than geometries. The name doesn't really matter, but it is a different user-facing experience.
I combined them into a "reference shape" menu and just dynamically swap out the options when plane is selected, made the menu more complicated but its not the end of the world unless we add another geometry type with its own options. check it out. honestly not sure if I like it, because now the plane option is buried and hard to discover. If I saw "shape" I am not sure I think "plane" because that isn't a thing in viam, so people will have to find that. I think it would be even worse if we kept the name "reference geometry"
honestly after using it, I am thinking of reverting that change and going back to "reference plane" being its own menu section.
…92/4-gizmos-plugin
|
|
||
| The right question to ask for when to use `$effect` vs `$effect.pre` is "does anything downstream in the same flush need to read this before render/DOM-commit?" If yes,`.pre`; if it's a pure side-effect with nothing observing the result inside the same flush, plain $effect is correct. | ||
|
|
||
| **Do not reach for `.pre` to "avoid a one-frame render lag."** That reasoning applies to continuously-rendered scenes; it does **not** apply here. Because rendering is on-demand, a scene mutation is followed by `invalidate()`, which schedules a render for the _next_ `requestAnimationFrame`. |
There was a problem hiding this comment.
This doesn't make sense for continuously rendered scenes either. $effect.pre runs synchronously and immediately. $effect runs in a batched manner in a microtask. But the guaranteed order of operations for JS per frame is:
- Sync tasks
- Microtasks
- requestAnimationFrame
Microtasks always run before requestAnimationFrame. Therefore even in continuously running apps $effect() is fine. The only time it's not fine is when it's writing Three.js props that other effects will potentially read from. Then $effect.pre is more necessary to get ahead of the other reads. But this is rare and sort of dangerous.
| > | ||
| <div {...api.getContentProps()}> | ||
| <div | ||
| class="z-max" |
There was a problem hiding this comment.
Do we need z-max? Can we use a less aggressive value? (I think we use around z-5) for other floating UI
There was a problem hiding this comment.
I was hitting some weirdness but I can lower it. though IMO I think we may want to introduce some semantic z-layering classes to reduce having to go search the codebase for every z-x usage and determine how to layer. like z-popover, z-panel, z-label or something.
| useAddNextInput(() => options().onAddNext?.()) | ||
| useUndoInput(() => options().onUndo?.()) | ||
|
|
||
| onDestroy(() => { |
There was a problem hiding this comment.
nit: I'd do $effect(() => { return ... }) since running in a microtask is a bit safer with the reactivity runtime, and (less applicable here) onDestroy runs in SSR
| /** Undo the last pending tool action. */ | ||
| export const useUndoInput = (handler: () => void) => { | ||
| const onKey = (event: KeyboardEvent) => { | ||
| if (event.key !== 'Backspace') return |
There was a problem hiding this comment.
Why not control + z ? I think most people would try that before backspace for undo
There was a problem hiding this comment.
I tried both and from a user standpoint it feels less like "undo" and more like "go back" so the backspace felt natural, especially with space and enter for confirm and submit. but we can swap it out if we decide we want ctrl+z/ctrl+y to be the global undo/redo commands when we apply them more broadly.
| const plugin = useGizmos() | ||
|
|
||
| const onKey = (event: KeyboardEvent) => { | ||
| if (event.key === 'Escape') handler() |
There was a problem hiding this comment.
I believe we'll need to stop propagation here since there are other keybindings to this
There was a problem hiding this comment.
do you know which? I didn't see any when searching. I can still stop propagation either way.
|
@DTCurrie getting a call stack exceeded error after clicking on a finished polyline in the deploy preview, probably an infinite effect loop: |
|
Looks like arrow labeling is wonky? I'm betting that this has to do with the force-directed-graph label PR, which removed labels as a child of each entity and made them entirely separated: 
|
yeah I think the label changes broke a few things, I think that is where in the infinite render loop is coming from as well. I will take a look |
2 participants
Adds the
Gizmosplugin: a dashboard toolset for placing editable scene aids (coordinate systems, reference planes, reference geometries, polylines, arrows, vertex normals, surface normals) by clicking on existing surfaces. Each placed entity is auto-selected so its plugin-supplied Details panel opens for in-place editing. Stacks on #737.Note
Use this PR preview to view all of the changes from this stack: https://viamrobotics.github.io/visualization/pr-preview/pr-739/snapshot
Stack
Frontend
src/lib/plugins/Gizmos/Gizmos.svelte: plugin entry. Mounts a dashboard popover, a Threlte portal that hosts the active tool, and per-entity renderers viaGizmoEntities.svelte. Switches the visualizer into'gizmo'interaction mode while a tool is armed and reverts to'navigate'on exit.src/lib/plugins/Gizmos/tools/(ArrowTool,CoordinateSystemTool,GeometryTool,LineTool,NormalsTool,PlaneTool): one component per gizmo kind. Each delegates the click / drag pipeline to the sharedusePlace,useInputs, andusePendinghooks.src/lib/plugins/Gizmos/GizmoArrow.svelte,GizmoPlane.svelte,GizmoPolylineMeasure.svelte,GizmoNormals.svelte,GizmoDetails.svelte: per-entity renderers and Details editors. They carry theCustomDetailstrait from app-16292: shared ECS traits and helpers for entity pose plugins #735 so the default frame and pose blocks in the Details panel are suppressed.GizmoDetails.svelte: renders the sharedMatrixDetailsfrom app-16292: refactor Details panel into focused subcomponents #736 withonPoseChange={traits.writeMatrix}andonParentChange={hierarchy.setParent}. Gizmos commit pose / parent edits immediately; a future frame edit plugin would pass session-buffered handlers instead.src/lib/plugins/Gizmos/useGizmos.svelte.ts: provider plususeGizmos()hook exposing the activemode, per-tool options (plane axis / construction / offset, geometry shape / construction / wireframe, line space / measure, arrow axis, vertex and surface normals length), and anexit()action.src/lib/plugins/Gizmos/traits.ts: marker traitsGizmo,PendingGizmo,ReferencePlane,GizmoArrow,VertexNormals,SurfaceNormals, andPolylineMeasurefor queries against placed gizmos.cursor.ts,surface.ts,matrix.ts,spawn.ts,buildNormalsMesh.ts, andbuildSurfaceNormalsGeometry.tseach have a spec under__tests__/.src/lib/plugins/index.ts: re-exportsGizmos,gizmoTraits, anduseGizmosfrom the plugin barrel.src/routes/+layout.svelte: mounts<Gizmos />in the demo route so the dev app exercises the plugin.Testing
New specs under
src/lib/plugins/Gizmos/__tests__/cover the pure helpers (buildNormalsMesh,buildSurfaceNormalsGeometry,cursor,matrix,spawn,surface). The full vitest suite passes withpnpm test.pnpm checkis clean.Manual smoke test plan: exercise each tool from the dashboard popover (coordinate system; reference plane in free and offset modes; reference geometry box / sphere / capsule, solid and wireframe, at-origin and free; polyline in world and screen space with each measure mode; arrow on each axis and on the surface normal; vertex normals; surface normals). Confirm
Escand clicking the dashboard toggle exit gizmo mode.