improvement: replace THREE Editor with a minimal local-to-package implementation - #212
Closed
timurbazhirov wants to merge 17 commits into
Closed
improvement: replace THREE Editor with a minimal local-to-package implementation#212timurbazhirov wants to merge 17 commits into
timurbazhirov wants to merge 17 commits into
Conversation
…ejs-editor package
Replace the standalone @mat3ra/threejs-editor modal with an in-viewer
edit mode driven by a new InteractiveStructureEditorMixin: select an
atom by clicking it, drag it with a transform gizmo or type its
coordinates, add/delete atoms, and undo/redo - all without leaving
the main viewer.
Also fixes the correctness bugs from the initial pass: elements read
off material.basis.elements need unwrapping from their ESSE
{id,value} shape before being fed back into
Basis.fromElementsAndCoordinates, otherwise they double-wrap and crash
any UI that renders them; the edit gizmo now survives scene rebuilds
instead of going stale after the first edit; click-to-select was
reading the wrong userData key; and the edit toolbar now uses the
app's MUI components instead of raw inline-styled buttons.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…drag - Fix crystal/cartesian units corruption when adding or removing atoms and editing coordinates: convert to the material's actual basis units instead of relabeling raw values. - Fix atomIndex/atomicIndex naming mismatch that broke atom selection and removal. - Preserve the selected atom (and gizmo attachment) across scene rebuilds instead of losing selection on every structure edit. - Keep TransformControls' camera reference in sync when toggling the orthographic camera. - Add direct click-and-drag-to-move for atoms (camera-facing plane, offset-based), independent of the TransformControls gizmo. - Refactor undo/redo to consistently use the bypassReloadViewer path and reset history on external material prop changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comprehensive spec for the editor introduced in 751a7e3: 12 user stories, the interaction state machine, UI/data-flow mockups, the host-app embedding contract, a 22-defect/17-rough-edge register, a Jest-based test-pathway matrix, 12 open product decisions, and a phased P0/P1/P2 implementation plan. Grounded in a research pass over the deleted standalone editor's git history, the current implementation, materials-designer's integration contract, and prior art (Avogadro, VESTA, Blender, three.js editor, Unity/Unreal/Figma), then adversarially fact-checked against the source. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Six critical/confirmed defects from docs/design/interactive-editor-spec.md
Section 7/10 (P0):
- D1: camera view reset after every structure edit. WaveComponent's
shouldViewerAdjust() compared the cell prop with exact JSON.stringify
equality; a lossy scene-vertex round trip reintroduces the lattice to
only ~1e-7 precision, spuriously tripping the comparison on every edit
and snapping the camera back to its default framing. Now an
epsilon-tolerant (1e-5 A) numeric comparison over the cell's scalar
fields, with an exact units check retained.
- D5: extractBasisFromScene() swept up every Mesh-typed scene object as
a phantom atom, including bond InstancedMeshes (which inherit type
"Mesh" in the installed three.js version) and boundary-condition
planes, and baked repetition clones into the material. Now scoped to
the base atoms group only, explicitly excluding InstancedMesh.
- D6: extractLatticeFromScene() located the unit cell via the first
LineSegments found by depth-first search and indexed a fixed vertex
count, crashing under non-periodic boundary conditions (a
differently-shaped cell object). Now locates the real unit-cell
LineSegments unambiguously and handles both periodic and
non-periodic geometry.
- D2: the edit-mode hotkey was dead - it referenced a settings key
(toggleThreejsEditorModal) that didn't exist in hotKeysConfig, so the
advertised "Edit [E]" tooltip did nothing (and "e" was already bound
to element labels). Added a real toggleEditMode binding ("t"), fixed
the tooltip to read the key dynamically, and fixed a related listener
leak (removeEventListener was missing the capture flag used at
registration).
- D4: the toolbar's Rotate mode button was a structural no-op (rotating
a sphere about its own center never changes atom position) that still
produced junk history entries. Removed the button, and added a
zero-delta guard on the gizmo's commit path so a zero-movement
interaction never fires a structure-modified event.
- D17: the window postMessage handler reflectively invoked any
component method with attacker-controlled arguments, from any origin,
with no allowlist. Removed - no confirmed external consumer depends
on it.
Hotkey choice (t) and postMessage removal (vs. allowlisting) follow the
spec's own stated recommendations for open decisions D-10 and D-11.
Full suite: 81 tests (79 passed, 2 pre-existing skips), tsc --noEmit
clean, lint unchanged from baseline (pre-existing unrelated debt in
measurements/*.ts and viewSettingsUrl.ts). Browser-verified: camera
position now stays fixed across a real drag; Rotate button confirmed
removed from the toolbar; hotkey tooltip confirmed reading "edit [t]".
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Formalizes the pointer-event/canvas-rect patterns already used by hand throughout tests/__tests__/mixins/interactive_structure_editor.js into tests/helpers/editor.js: stubCanvasRect, projectToScreen/ projectMeshToScreen, dispatchPointerEvent (direct-handler or real- dispatch mode), simulateClick, simulateAtomDrag (encodes the two-move drag-offset requirement), getWaveWithRecordedCallbacks, and expectVisualMatch. Also adds docs/design/MANUAL_SMOKE.md, an ~8-item checklist for the handful of things the Jest+jsdom+headless-gl stack structurally cannot prove (real event ordering, CSS/layout/DPR, GPU rendering, pointer- capture/drag-leaves-canvas, focus/hotkey/IME, scrolled-container drift), per the design spec's Section 8 test-pathway analysis. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ign spec Implements docs/design/interactive-editor-spec.md Section 10 P1: the delta-based edit architecture plus the drag robustness, affordance, host-API, and polish items that depend on it. Architecture (src/mixins/interactive_structure_editor.ts): - Replace scene re-derivation with basis mutation on the wave's own tracked structure. addAtom/removeSelectedAtom/commitMovedAtom_ now clone the current material, mutate its Basis in place (Basis.addAtom, index-based element/coordinate/label/constraint filtering, or a single coordinate swap), and write back via setBasis - never through ThreeDSceneDataToMaterial. Lattice, units, metadata, labels, and constraints on every untouched atom now survive edits exactly, and the new atom is auto-selected (US-7). - Drag robustness: pointer capture on drag start/release (defensively guarded for environments without the API), pointercancel treated as a cancel, Esc reverts an in-progress direct or gizmo drag with no history entry (US-6), LMB-only filtering (D21), orbit-controls restored to their tracked pre-drag state instead of hardcoded true (D11). - Affordances: hover and selection now render through a dedicated wireframe halo mesh instead of mutating the atom's own material.emissive, which spin-glow and measurement-hover already write to (D19); move/grabbing cursors follow hover/drag state. - Selection persists across an edit-mode disable/re-enable cycle, but not across an explicit deselect (R12). Host API and React-side fixes (src/components/ThreeDEditor.jsx): - Selection changes no longer trigger a full WaveComponent reload: wrapped in the same bypassReloadViewer guard already used for structure edits, fixing both the mid-drag mesh-orphaning bug (D3) and per-click rebuild sluggishness (R17). - Coordinate fields hold a local draft string while focused (clearable, minus-sign-capable, full precision) and commit once on blur/Enter - one history entry per edit instead of one per keystroke (D18). - UNSAFE_componentWillReceiveProps only resets history/selection when the incoming material's actual content differs (via calculateHash()) from what's already shown, so a host that echoes its own committed material back as a prop no longer wipes state on every edit (D16). - New onEditModeChanged callback and canUndo()/canRedo() methods for host-driven UI (spec Section 6). - Edit mode and measurement modes are now mutually exclusive - entering one force-exits the other (D20). - Add Atom places the new atom at the true cell center (a+b+c)/2, not the component-wise (ax/2, by/2, cz/2) diagonal, and offsets it if that position is already occupied instead of stacking duplicates (D22). - Download now exports the current edited material instead of the pre-edit snapshot (D8). - Delete/Backspace and Ctrl(Cmd)+Z/Ctrl(Cmd)+Shift+Z are now wired via a keydown listener (keypress, used for the rest of this component's hotkeys, never fires for these keys) while edit mode is active. - The edit toolbar no longer renders while the viewer's interactive cover is showing (R14). Lifecycle (src/wave.js, src/components/WaveComponent.jsx): - Replaced the leaked, never-removed window resize listener with a ResizeObserver scoped to the container and a real dispose() that disconnects it and releases the renderer; WaveComponent now calls dispose() on unmount and before constructing a replacement instance for a "Reset View" re-init (D15). tests/setupFiles.js gains a minimal ResizeObserver polyfill, since jsdom (unlike every real browser this targets) doesn't implement it. 36 new tests across the mixin, ThreeDEditor, WaveComponent, and wave suites. Full run: 115 passed (2 pre-existing skips), tsc --noEmit clean, lint unchanged from baseline aside from 2 new any-typed warnings matching this file's existing convention, build succeeds. Browser- verified end to end against the running dev server: hover halo/cursor, drag-threshold/offset behavior, orbit-state tracking, Esc-revert, Ctrl+Z/Ctrl+Shift+Z, and - the original user-reported bug - the camera staying exactly fixed across a real drag and a real undo. Not attempted: P2 (multi-select, snapping, element picker, etc.) is explicitly gated on the spec's open product decisions (Section 9) and was left for the user to resolve. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… status Persists the planning artifacts behind docs/design/interactive-editor-spec.md: the original plan for producing the spec (now marked historical) plus a new status section tracking what's landed - spec approved, P0 (97707ba) and P1 (03b1376, c2524a8) implemented/tested/committed/pushed - and what's still outstanding: P2, itemized against the spec's 12 open product decisions (§9), none of which are inferable from the spec itself. These files live under plan/, which is gitignored by default (scratch planning area); force-added here since the user asked to keep this specific plan's state trackable rather than let it stay session-local. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Implements the design spec's decision D-4 (docs/design/interactive-editor-spec.md §9): rubber-band multi-select, resolving the drag-on-empty-space conflict with camera orbit by moving orbit rotation to the right mouse button while edit mode is active (OrbitControls.mouseButtons remapped on enable, restored on disable) - freeing left-drag on empty space for a marquee selection. Mixin (src/mixins/interactive_structure_editor.ts): - selectedMeshes_ array is now the canonical selection (selectedMesh_ stays as its last-entry alias for existing single-atom callers/tests). setSelectedAtomMesh/clearSelectedAtom are now thin wrappers over the new setSelectedAtomMeshes/clearSelection. - Click selection gains modifiers: Shift+click adds, Ctrl/Cmd+click toggles; Shift/Ctrl+click on empty space is a no-op rather than clearing the selection. - A left-drag starting on empty space now opens a rubber-band marquee (an absolutely-positioned overlay div sized live during the drag) instead of immediately deselecting; releasing selects every atom whose projected screen position falls inside the rectangle, combined with the existing selection via the same replace/add/toggle modifiers. A release before the drag threshold still behaves as a plain click. - 2+ selected atoms get a pivot object (selectionPivot_) at their centroid; the gizmo attaches to it instead of an atom directly. Dragging the pivot (via the gizmo) or dragging any one of the selected atoms directly moves every selected atom by the same rigid delta, live during the drag. - Multiple atoms moved together, or removed together, commit as a single basis-delta/one history entry (commitMovedAtoms_/removeSelectedAtoms_), matching the existing one-drag-one-commit convention. - The selection highlight is now a reusable pool of halo meshes (one per selected atom) instead of a single mesh. - onSelectionChanged's contract changes from (index: number | null) to (atomicIndices: number[]) - a breaking change from the P1-documented single-index signature, made safely since no external consumer existed yet (materials-designer is still pinned to the pre-P0 export). React (src/components/ThreeDEditor.jsx): - selectedAtomIndex state becomes selectedAtomIndices (array). The coordinate panel still shows X/Y/Z only for exactly one selected atom; 2+ shows an "N atoms selected" caption instead (coordinates aren't meaningful for a group). Remove Selected / Delete already worked unchanged, since wave.removeSelectedAtom() itself now branches on selection size. 12 new tests (mixin: modifiers, marquee hit-testing and threshold gating, orbit button remap, group drag via direct-drag and via the gizmo pivot, Esc-revert for a group drag, group removal, multi-selection surviving an edit-mode toggle). Full suite: 127 passed (2 pre-existing skips, up from 115), tsc --noEmit clean, lint unchanged from baseline, build succeeds. Browser-verified end to end against the running dev server: Shift+click building a 2-atom selection, the marquee overlay rendering and correctly hit-testing both atoms, a direct drag on one selected atom moving both by an identical delta in a single history entry, and the camera staying fixed throughout. Not implemented (see docs/design/interactive-editor-spec.md §11 roadmap): group rotate (blocked on single-atom Rotate mode returning) and double-click-to-select-bonded-fragment (needs bond connectivity data). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…amera focus, element rename) Extends D-4's group translate with rotate about the shared centroid (same gizmo, same one-commit-per-drag semantics, gated to a 2+ selection). Adds clone selected atom(s), camera focus on selection (F key + button, preserves viewing angle unlike the whole-cell fit reset), and the type-to-change half of decision D-9 (editable, periodic-table-validated Element field). Also fixes a latent defect found while wiring rotate: rebuildScene() in wave.js only ever restored a single-atom selection, so any externally triggered rebuild (e.g. the host's onStructureModified round-trip) silently collapsed a group selection to one atom - breaking a second consecutive group rotate/drag. Now preserves the full multi-selection regardless of who triggers the rebuild; this also hardens D-4's group translate, which shared the same gap. vite.config.ts: extends the existing optimizeDeps.include workaround to the new toolbar icons - a dev-server-only esbuild pre-bundling/chunk-splitting issue with @emotion/styled, unrelated to the production build (which already built clean throughout). 18 new/extended tests (mixin + component), all verified live against the running dev server, including rotate math and event-order verified directly against the real (non-mocked) three.js/WebGL runtime. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…teractions Mixin-level tests drive a bare Wave instance; component-level tests drive ThreeDEditor's handler methods directly. Neither exercises a real pointer gesture against the mounted wave's canvas together with the real onStructureModified -> setState -> _applyMaterialToViewer round-trip - which is exactly the gap the rebuildScene() multi-selection bug hid in. These tests mount the real ThreeDEditor and drive its real, running wave instance with the same helpers the mixin suite uses: click-select, click-and-drag move, shift+click and marquee multi-select, group translate/rotate via the gizmo (each proven to survive a second consecutive operation through the real host round-trip, not a simulated one), undo, and clone-then-drag. Along the way, found and fixed a real gap in the test harness itself: WaveComponent renders its own inner ref'd div as the Wave's canvas container, which never inherits the outer mount container's stubbed clientWidth/clientHeight, so the renderer sized itself to 0x0 in jsdom and every raycasting-based pointer test silently missed. Fixed by forcing the renderer's drawing-buffer size after mount, before stubbing the canvas rect. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…fix) Reviewed via agent-code-review-tb (Timur Bazhirov persona + real precedent retrieval), 4 iteration rounds. Every finding independently re-verified against the repo before trusting it - one (repetition-clone picking) broke an existing test once fixed, confirming the value of that step even for a well-sourced review. Full writeup in plan/interactive-editor-spec-plan.md. Blockers fixed, each with a regression test (confirmed fails pre-fix): - Esc did not cancel a gizmo-driven drag (translate's default interaction); cancelAtomDrag_() only knew how to revert a direct body-drag. - tsconfig.json's declaration:false (confirmed added by this PR) was silently shipping a stale dist/exports.d.ts that still exported the deleted ThreejsEditorModal; restored, orphaned dist files removed. - Repetition-clone atoms were selectable/draggable with a deliberately out-of-range atomicIndex; collectAllAtoms() (renamed collectSelectableAtoms()) is now scoped to the first ATOM_GROUP_NAME group, same fix (and invariant) utils.js's extractBasisFromScene already uses. getAtomGroups() (measurements' own clone-inclusive collector) is untouched. - Undo/redo never told the host when a rebuild silently invalidated the selection (e.g. undo across an Add Atom); reselectAtomsByIndices is now the single source of truth for firing onSelectionChanged, only on actual change. - onSelectionChanged was wired into the mixin but never forwarded to a host prop, despite spec Sec6.2 listing it as public API. Should-fix applied: group direct-drag now keeps the pivot tracking the live centroid instead of freezing at the pre-drag spot; cloneSelectedAtoms() shares Add Atom's occupied-site guard (D22); the camera-focus hotkey now reads settings.hotKeysConfig instead of a bare "f" literal; undo()/redo() ref-API aliases added per spec Sec6.3; dead @codemirror/*, acorn, codemirror, esprima, signals dependencies removed (zero usage, traced to the deleted old editor's JS-scripting panel via orphaned CSS, also removed). Investigated without a code change: the three fork-to-stock swap. Compared the pinned fork commit against upstream three.js - its TransformControls.js patches are either dead for this codebase (a class this architecture never uses) or narrow edge cases; the one patch that touches live behavior (rotate-mode drag-plane computation) looks like it relied on fragile stale module state that stock three's unconditional reset avoids, and this round's extensive rotate test coverage already exercises stock three's version correctly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Completes the host-API gap the TB-persona review flagged: onEditCommit was
listed in spec §6.2 (P1 scope, alongside onSelectionChanged/
onEditModeChanged) but never implemented - only onUpdate's every-edit,
source-agnostic channel existed, which is exactly the flood that broke
materials-designer's redux history integration in the old modal editor.
Threads a `source` string through every commit path down to
onStructureModified(material, source): the mixin's commitMovedAtom_/
commitMovedAtoms_ now tag "drag" (direct body-drag, endAtomDrag_) vs
"gizmo" (TransformControls "mouseUp" listener, translate or rotate alike);
addAtom/removeSelectedAtom(s)/cloneSelectedAtoms tag "add"/"remove"/"clone".
ThreeDEditor.jsx forwards this to a new onEditCommit(material, {source})
prop via _applyMaterialToViewer, and adds "coordinate-input"/"element-input"
for its own typed-panel edits and "undo"/"redo" for handleUndo/handleRedo
(which bypass handleStructureModified entirely). Extends spec §6.2's
original 7-value source enum with "element-input" and "clone" for this
round's type-to-change and clone-atom features, which postdate the enum.
Also fixes tests/helpers/editor.js's simulateAtomDrag, whose
onStructureModified interceptor only forwarded the first (material)
argument - silently dropping source and would have masked this feature
working correctly for the direct-drag path.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
commitMovedAtoms_/addAtom/cloneSelectedAtoms placed a touched atom's coordinate by flipping the whole basis to Cartesian and back around the mutation - Basis.toCartesian()/toCrystal() round-trip every atom's coordinate via mapArrayInPlace, not just the touched one, so spec §5.2's "untouched atoms preserved bit-for-bit because they are never re-derived" claim didn't fully hold in the implementation. Convert only the touched point(s) directly via basis.cell.convertPointToCrystal/convertPointToCartesian instead, so untouched atoms' coordinate entries are genuinely never read or rewritten and each edit does O(touched) matrix work instead of O(n). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…TB-ARCH-2) Extract marquee rubber-band selection into a new MarqueeSelectionMixin and group-transform pivot/centroid math plus the group-only halves of the TransformControls change/mouseUp listeners into a new GroupTransformMixin, composed alongside InteractiveStructureEditorMixin in wave.js. The direct-drag lifecycle (beginAtomDrag_/endAtomDrag_/ cancelAtomDrag_) stays in the base mixin - it's a genuinely unified state machine covering all 4 drag shapes at once, and this round's Esc-cancel regression tests are concentrated exactly there, so forcing a single/group split there would trade real correctness coverage for a readability preference. Base file: 1460 -> 1211 lines. Pure refactor: all 181 pre-existing tests pass unchanged, and behavioral equivalence was further verified live against a real (non-jsdom) WebGL Wave instance - marquee-select, group direct-drag, and gizmo group-rotate all independently confirmed correct. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Rather than (a) a materials[] prop on ThreeDEditor or (b) a new merge-workflow component in this repo, the multi-material merge workflow belongs in materials-designer, built on wave.js's existing single-material ThreeDEditor plus this session's group-transform primitives (select/drag/rotate a rigid group of atoms). Updates the spec's decision table (§9), props table (§6.2), and roadmap (§11), plus the plan doc's outstanding-decisions table, so nothing still reads D-2 as open or as a wave.js roadmap item. Notes two integration shapes for materials-designer, neither needing wave.js changes today: pre-merging materials into one before handing ThreeDEditor a single material as usual, or building a custom viewer against lower-level primitives (Wave, createAtomsGroup, getUnitCellObject) - which would need those exported from src/exports.js, deferred until actually needed rather than grown speculatively. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Aug 13, 2026
Member
Author
|
Closing — consolidated into #216 (then #214 on top). Verified contained: this PR's head The branch Generated by Claude Code |
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
1 participant
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.
No description provided.