add shapes overlay#693
Conversation
🦋 Changeset detectedLatest commit: 403e238 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 |
|
DTCurrie
requested review from
mattmacf98 and
micheal-parks
and removed request for
micheal-parks
| {/if} | ||
|
|
||
| <div aria-label="mutable visibility"> | ||
| <Checkbox |
There was a problem hiding this comment.
Why are we adding a checkbox for invisibility? Doesn't this duplicate the Treeview invisibility button?
There was a problem hiding this comment.
consistency with other metadata rendering values, and just discoverability.
There was a problem hiding this comment.
Is that really a problem though? I worry that we're inventing the problem here - no one that I've talked to has reported the eye buttons on the tree to not be discoverable
| } | ||
| }) | ||
|
|
||
| keys.onKeys('-', () => { |
There was a problem hiding this comment.
I'd maybe leave this out until we have undo keybindings + modal, the trash button works and this could cause confusion
There was a problem hiding this comment.
we can, this was just to not lose the existing behavior for custom geos that were added with the =/+ key
There was a problem hiding this comment.
Yeah, but this expands niche behavior to something that encompasses all delete-ables
| * changes in EditedMatrix and round-trip through the robot config; local | ||
| * Removable entities write directly to Matrix and skip the config call. | ||
| */ | ||
| const getEditTarget = (entity: Entity) => { |
There was a problem hiding this comment.
These file edits concern me a lot. I don't like how entangled this is getting. I'm working on a PR that extracts all of frame editing into a plugin, and I was going to have completely separate pathway for editing local entities. Coupling them like this seems very risky for stability.
There was a problem hiding this comment.
oh yeah, and to @mattmacf98 comment this was just a quick way to get this stubbed in and working based off the old geos, but I think they should be separate
| const box = useTrait(() => entity, traits.Box) | ||
| const capsule = useTrait(() => entity, traits.Capsule) | ||
| const sphere = useTrait(() => entity, traits.Sphere) | ||
| const plane = useTrait(() => entity, traits.Plane) |
There was a problem hiding this comment.
nit: a little iffy about adding planes as a geo type since it is not a viam supported one and people may be confused why they can set geo to a plane for this flow but not for frame geos
There was a problem hiding this comment.
yeah as mentioned I would probably separate this from the frame editing stuff in general this was just a quick way to get something useable to test out.
| {@const edgesGeometry = box.current ? unitBoxEdges : unitSphereEdges} | ||
| {#if box.current || sphere.current || plane.current} | ||
| {@const meshGeometry = box.current ? unitBox : sphere.current ? unitSphere : unitPlane} | ||
| {@const edgesGeometry = box.current |
There was a problem hiding this comment.
nit: is this enough nested ternaries to warrant a helper func?
| { value: 'frame', label: 'Frame', icon: Axis3d }, | ||
| ] | ||
|
|
||
| let counters = $state<Record<ShapeKind, number>>({ |
There was a problem hiding this comment.
do we decrement this on remove? if not maybe we should rename this to shapeIds so people don't mistake this as a true count of the number of them in the scene
There was a problem hiding this comment.
not sure it matters too much, but we can. its just to avoid name collisions since these should just be temporary entities. I don't if poeple will care to much since they aren't persisted
| if (!trimmed) return | ||
|
|
||
| const entity = spawners[kind](world) | ||
| entity.set(traits.Name, trimmed) |
There was a problem hiding this comment.
is this not already handled in the baseTraits in the spawner?
const baseTraits = (kind: ShapeKind) => [
traits.Name(`custom ${kind} ${++counters[kind]}`),
traits.Matrix,
traits.Removable,
traits.Transformable,
]
could the source of truth ids just be in there instead?
| const tempPose = createPose() | ||
|
|
||
| /** | ||
| * Picks the matrix trait to edit. Config-backed frames (with FramesAPI) stage |
There was a problem hiding this comment.
I think this expands the responsibility of the FrameConfigUpdater.svelte.ts too much (it now deals with frame updates and the custom geo ones)
I think it would be good to create a new updater to just handle the custom geo stuff and use that when the focused entity is a custom geo and then we can leave this file alone to just deal with frames
3 participants
Note: Quickly put this together to support the education training effort. Open to suggestions/commits on this. It replaces the old
=hotkey.Adds an "Add Shape" dashboard button to allow creating simple geometries. Also adds color and visibility controls to the details panel.
As a follow-up, I am going to look into splitting up the detail panel into tabs, it is getting overloaded and we will want space for controls soon.