Promote Segment/sidebearings to domain classes; align Editor accessors#42
Merged
Conversation
Segment becomes a first-class reactive-friendly class in lib/model/Segment.ts, alongside Glyph and Contour. Adds Contour.segments() and Glyph.segments() generators, and a Selection.#bounds computed signal that tracks glyph contours + selection ids — fixes the stale TransformSection/ScaleSection X/Y during drag. - New Segment class wraps discriminated SegmentType data and exposes .id, .bounds, .pointIds, .anchor1/2, .toCurve(), .hitTest(), plus static parse() and hitTestMultiple() - Contour.selectionBounds(ids) and Selection.$bounds replace the free getSegmentAwareBounds / editor.getSelectionBounds / editor.getSelectionCenter - Deleted lib/geo/Segment.ts + lib/geo/Segments.ts duplicate static namespaces - Deleted lib/transform/SelectionBounds.ts (logic lives on Selection now) - SegmentHit.segment, Editor#$segmentIndex, getSegmentAt, splitSegment, upgradeLineToCubic, indicators/Segments renderer all take Segment instances - SnapManager/DebugOverlays/DebugPanel/Editor/bench swap Glyphs.* statics for Glyph instance methods (glyph.allPoints, glyph.point(id), etc.) Lint: - Update no-raw-contour-access message to point at domain methods - Add prefer-instance-method-on-glyph (catches static Glyphs.*/Contours.* called on class instances) - Add no-raw-segment-parse (use contour.segments()/glyph.segments() instead of Segment.parse / parseContourSegments in app code)
- Segment gains instance methods: pointAt, closestPoint, splitAt, sample, length All delegate to Curve.* on this.toCurve(). Callers drop manual toCurve() + Curve.x(curve, ...) pairs. - asLine/asQuad/asCubic narrow the raw discriminated data without a cast. Replaces (segment.raw as CubicSegment) in BendCurve and SplitSegmentCommand. - BendCurve and SplitSegmentCommand migrated to the new API. - Curve.ts stays as the pure-geometry layer for composite contours and other shapeless callers; Segment layers id-aware methods on top.
- Glyph gains sidebearings (computed) + $sidebearings / $xAdvance signal getters. Sidebar LSB/RSB now live-update during drag — same staleness bug that TransformSection had. - GlyphSection splits into an outer (subscribes to glyph identity) and inner view (subscribes to glyph.$sidebearings and $xAdvance) so the reactive subscription is established on every glyph load. - Editor.setLeft/RightSidebearing read glyph.sidebearings directly. - Delete lib/editor/sidebearings.ts free functions + test. - displayAdvance(advance, glyphName, unicode) moved to unicode.ts as the shared non-spacing fallback helper; replaces editor.getVisualGlyphAdvance (pure rendering concern, doesn't belong on the Glyph class). - Rename isLikelyNonSpacingGlyphRef → isNonSpacingGlyph (stale "Ref" suffix from when it was tied to composite glyph refs).
Editor had the accessor convention inverted from Glyph/Contour: - `editor.previewMode` returned Signal<boolean>, `isPreviewMode()` returned the value - `editor.getIsHoveringNode()` / `getSnapIndicator()` / etc. were methods with no corresponding signal accessor at all Now matches the domain-class convention: - `foo` getter returns the value (auto-unwraps) - `$foo` getter returns the Signal<T> (for React useSignalState) Converted pairs: - previewMode / $previewMode (was isPreviewMode() + get previewMode: Signal) - handlesVisible / $handlesVisible (was isHandlesVisible() + get handlesVisible: Signal) - gpuHandlesEnabled / $gpuHandlesEnabled - isHoveringNode / $isHoveringNode (was getIsHoveringNode()) - snapIndicator / $snapIndicator (was getSnapIndicator()) - currentModifiers / $currentModifiers - debugOverlays / $debugOverlays - cursor / $cursor (was getCursor()) - zoom / $zoom - drawOffset / $drawOffset One-shot computes (no signal): - hoveredBoundingBoxHandle, hoveredSegmentId activeTool / activeToolState left on old pattern pending tool refactor, silenced with oxlint-disable comments. Private signal fields renamed from `$foo` → `#foo` to match Glyph convention. New lint rule: shift/no-get-signal-value-method — flags `getFoo()` / `isFoo()` methods that just return `this.#signal.value`. Prevents regression.
…ription The sidebar's \`useSignalState(selection.\$bounds)\` / \`glyph.\$sidebearings\` subscription was forcing the expensive bounds computeds to run inside the drag hot path every frame. At 50K points all-selected this blew the p95 ceiling on translate/nudge/undo tests by 4-10x. Pixi's pattern: per-entity bounds caches are invalidated lazily, the aggregate (\`stage.getBounds()\`) is called once per render frame, and each child returns its cached value unless individually dirty. The render loop pulls; nothing subscribes to bounds reactively. Translated to our sidebar: - New \`useSignalTrigger(signal)\` hook: subscribes purely for re-render side effects, returns nothing. Lets a component re-render on a coarse "something changed" signal without forcing expensive derivations to run inside the subscribe callback. - Expose \`glyph.\$contours\` as the coarse signal. Fires once per \`#patchPositions\` batch (already how \`#contours\` behaves). - \`TransformSection\`, \`ScaleSection\`, \`GlyphSection\` subscribe to \`glyph.\$contours\` for re-render, then pull \`selection.bounds\` / \`glyph.sidebearings\` on demand at render time. During a tight-loop drag (perf tests): setPositions → fires #contours → useSignalTrigger effect bumps a useSyncExternalStore version → React queues a render for next rAF → no compute happens in the loop. The bounds computed only runs when React actually renders, at most once per animation frame. Result: all 8 perf tests pass, matching main's 53s baseline. Note: \`selection.\$bounds\`, \`glyph.\$sidebearings\`, \`glyph.\$xAdvance\` all remain as public APIs for non-sidebar callers or future use once incremental signals land (see \`incremental-signals.md\` ticket).
…foo signals
Previous commit fixed perf by subscribing to \`glyph.\$contours\` and pulling
\`selection.bounds\` / \`glyph.sidebearings\` on demand. That worked but was
dishonest — the \`useSignalTrigger(glyph.\$contours)\` call had a hidden
side-effect making unrelated reads reactive. A new reader couldn't tell
what subscription was keeping the bounds live.
Clean up:
- Remove public \`\$\`-accessors for expensive derived signals:
* \`selection.\$bounds\` (footgun: expensive ComputedSignal looked cheap)
* \`glyph.\$sidebearings\` (same)
These stay as plain getters \`selection.bounds\` / \`glyph.sidebearings\`
for pull access. Internal ComputedSignals remain (caching) but aren't
exposed for subscription.
- Add purpose-specific React hooks that bundle "subscribe to raw inputs
+ pull derived value" into one honest unit:
* \`useSelectionBounds()\`
* \`useGlyphSidebearings()\`
* \`useGlyphXAdvance()\`
- Migrate sidebar components. Each line now expresses what it genuinely
depends on; no hidden subscription side-effects.
- Make \`glyph.#sidebearings\` computed point-based (matches the bounds
treatment) so neither warms the bezier \`#bbox\` chain.
- \`Editor.setLeft/RightSidebearing\` still need bezier-accurate values for
correct delta computation — pull \`glyph.bbox\` directly (one-shot on
command execution, fine to pay the cost).
Convention in \`lib/reactive/docs/DOCS.md\`:
\`\$foo\` accessor = raw state, safe to subscribe via useSignalState/Trigger.
\`.foo\` getter = derived value, pull at read time.
For React live display of derived values, write a purpose-specific hook.
Perf: 51.6s — matches main baseline.
5 tasks
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.
Summary
Four-commit sweep promoting free functions and static namespaces into the reactive domain classes (
Glyph,Contour,Segment), and aligningEditor's accessor convention to match. Also introduces three new oxlint rules that catch the old patterns at authoring time.Commits (read in order)
Segment domain class + selection bounds signal (9708a78)
Segmentclass inlib/model/Segment.tsreplaces duplicateSegment/Segmentsstatic namespaces inlib/geo/.Contour.segments()andGlyph.segments()generators.Contour.selectionBounds(ids)+Selection.\$boundsreactive computed — fixes the stale X/Y inTransformSection/ScaleSectionduring drag.lib/transform/SelectionBounds.tsandeditor.getSelectionBounds()/getSelectionCenter().Curve method promotion onto Segment (c7073c6)
segment.pointAt(t),closestPoint(pos),splitAt(t),sample(n),lengthdelegate toCurve.*onthis.toCurve().asLine() / asQuad() / asCubic()narrow the raw discriminated data without a cast — replaces(segment.raw as CubicSegment)inBendCurveandSplitSegmentCommand.Curve.tsstays as the pure-geometry layer for composite contours and other shapeless callers.glyph.sidebearings reactive computed (5a54de0)
Glyph.sidebearingscomputed + `$sidebearings` / `$xAdvance` signal getters. Sidebar LSB/RSB now live-update during drag.GlyphSectionsplits outer/inner so the reactive subscription is re-established on glyph load.lib/editor/sidebearings.tsfree functions.isLikelyNonSpacingGlyphRef→isNonSpacingGlyph.displayAdvance(advance, glyphName, unicode)moved tounicode.ts(shared withlayout.ts).Editor accessor alignment (545c95a)
hoveredBoundingBoxHandle/hoveredSegmentId.Net change
Test plan
splitAt/pointAtinstance methods🤖 Generated with Claude Code