diff --git a/packages/core/tests/io/formats/fig/session/page-instance-sync.test.ts b/packages/core/tests/io/formats/fig/session/page-instance-sync.test.ts index 6235e77a1..6dbd2cf21 100644 --- a/packages/core/tests/io/formats/fig/session/page-instance-sync.test.ts +++ b/packages/core/tests/io/formats/fig/session/page-instance-sync.test.ts @@ -5,12 +5,76 @@ import { initCodec } from '@open-pencil/core/kiwi' import { createFigDocumentSession } from '@open-pencil/fig' import { SceneGraph } from '@open-pencil/scene-graph' +test('loading a later page keeps the derived sizes of instances earlier pages placed', async () => { + await initCodec() + const source = new SceneGraph() + const first = source.getPages()[0] + const component = source.createNode('COMPONENT', first.id, { name: 'Field', width: 200 }) + source.createNode('RECTANGLE', component.id, { name: 'Box', width: 100, height: 20 }) + const placed = source.createInstance(component.id, first.id) + if (!placed) throw new Error('Missing instance') + // The instance's box is wider than the component's, as a fill child of a resized instance is; + // export saves that as the size Figma derived for it. + const box = source.getChildren(placed.id)[0] + source.updateNode(box.id, { width: 150 }) + source.createInstance(component.id, source.addPage('Second').id) + const bytes = await exportFigFile(source) + + // The app's sessions read the sizes Figma derived, as this one does. + const session = createFigDocumentSession(bytes.buffer as ArrayBuffer, { derivedBounds: true }) + session.loadPage(session.pages[0].id) + const firstPageId = session.graphPageId(session.pages[0].id) + const instance = session.graph + .getChildren(firstPageId ?? '') + .find((node) => node.type === 'INSTANCE') + if (!instance) throw new Error('Missing loaded instance') + const loadedBox = () => session.graph.getChildren(instance.id)[0] + expect(loadedBox().width).toBe(150) + + session.loadPage(session.pages[1].id) + expect(loadedBox().width).toBe(150) +}) + +test('loading a later page keeps a live edit to a nested instance layer over its derived size', async () => { + await initCodec() + const source = new SceneGraph() + const library = source.getPages()[0] + const inner = source.createNode('COMPONENT', library.id, { name: 'Inner', width: 100 }) + source.createNode('RECTANGLE', inner.id, { name: 'Box', width: 100, height: 20 }) + const outer = source.createNode('COMPONENT', library.id, { name: 'Outer', width: 300 }) + const layer = source.createInstance(inner.id, outer.id) + if (!layer) throw new Error('Missing nested instance layer') + const placed = source.createInstance(outer.id, source.addPage('Placed').id) + if (!placed) throw new Error('Missing placed instance') + // Figma derived a wider nested instance in the placed copy, as a fill child would be. + const nested = source.getChildren(placed.id)[0] + source.updateNode(nested.id, { width: 150 }) + const bytes = await exportFigFile(source) + + const session = createFigDocumentSession(bytes.buffer as ArrayBuffer, { derivedBounds: true }) + session.loadPage(session.pages[0].id) + const libraryId = session.graphPageId(session.pages[0].id) ?? '' + const loadedOuter = session.graph.getChildren(libraryId).find((node) => node.name === 'Outer') + const loadedLayer = loadedOuter ? session.graph.getChildren(loadedOuter.id)[0] : undefined + if (!loadedLayer) throw new Error('Missing loaded nested instance layer') + // Editing the component's layer live, after the file opened. + session.graph.updateNode(loadedLayer.id, { width: 120 }) + expect(session.graph.getNode(loadedLayer.id)?.source.editedFields).toContain('width') + + const placedPage = session.pages.find((page) => page.name === 'Placed') + if (!placedPage) throw new Error('Missing placed page') + session.loadPage(placedPage.id) + const placedId = session.graphPageId(placedPage.id) ?? '' + const loadedPlaced = session.graph.getChildren(placedId)[0] + expect(session.graph.getChildren(loadedPlaced.id)[0].width).toBe(120) +}) + /** - * Syncing visits every instance of a component, so doing it per placed instance costs a - * pass over the whole graph for each one. A page placing many instances of one component - * must still sync it once. + * Syncing a whole component visits every instance of it in the graph, including those earlier + * pages placed, whose sizes Figma derived. A resumed page syncs only the instances it places, + * each once. */ -test('a resumed page load syncs each component once, not once per instance it places', async () => { +test('a resumed page load syncs each instance it places once, never a whole component', async () => { await initCodec() const source = new SceneGraph() const library = source.getPages()[0] @@ -22,21 +86,27 @@ test('a resumed page load syncs each component once, not once per instance it pl const session = createFigDocumentSession(bytes.buffer as ArrayBuffer) const synced: string[] = [] + const wholeComponents: string[] = [] + const syncInstance = session.graph.syncInstance.bind(session.graph) + session.graph.syncInstance = (instanceId: string) => { + synced.push(instanceId) + return syncInstance(instanceId) + } const syncInstances = session.graph.syncInstances.bind(session.graph) session.graph.syncInstances = (componentId: string) => { - synced.push(componentId) + wholeComponents.push(componentId) return syncInstances(componentId) } const placed = session.pages.find((candidate) => candidate.name === 'Placed') if (!placed) throw new Error('Missing placed page') session.loadPage(placed.id) - // One component placed eight times is one sync, not eight. - expect(synced.length).toBe(1) const pageId = session.graphPageId(placed.id) if (!pageId) throw new Error('Missing materialized page') const instances = session.graph.getChildren(pageId) expect(instances).toHaveLength(8) + expect(wholeComponents).toEqual([]) + expect(synced.toSorted()).toEqual(instances.map((instance) => instance.id).toSorted()) for (const instance of instances) expect(session.graph.getChildren(instance.id)[0].text).toBe('Badge') }) diff --git a/packages/fig/src/document/materialize.ts b/packages/fig/src/document/materialize.ts index 617c9f649..fea158bcb 100644 --- a/packages/fig/src/document/materialize.ts +++ b/packages/fig/src/document/materialize.ts @@ -1,7 +1,10 @@ import type { NodeChange } from '@open-pencil/kiwi/fig/codec' import { SceneGraph, type SceneNode } from '@open-pencil/scene-graph' -import { reconcileLiveComponentEdits } from '../instance-overrides/live-component-edits' +import { + reconcileLiveComponentEdits, + syncSourceLayers +} from '../instance-overrides/live-component-edits' import { materializeInstance } from '../instance-overrides/materialize-instance' import type { InstanceOccurrence, @@ -169,6 +172,31 @@ function createAssemblyState( } } +/** + * Syncing a resumed load's new instances copies their component's sizes over the sizes Figma + * derived for them, such as a field filling its resized instance. Puts the derived sizes back, + * except along an axis whose component layer was edited live, which syncing rightly carried over: + * the layer an owning instance maps it to, or its own component. + */ +function restoreDerivedSizes( + graph: SceneGraph, + derivedSizes: ReadonlyMap +): void { + graph.preserveSourceMetadataDuring(() => { + for (const [id, size] of derivedSizes) { + const node = graph.getNode(id) + if (!node) continue + const edited = new Set( + syncSourceLayers(graph, node).flatMap((layer) => layer.source.editedFields) + ) + const updates: Partial = {} + if (node.width !== size.width && !edited.has('width')) updates.width = size.width + if (node.height !== size.height && !edited.has('height')) updates.height = size.height + if (Object.keys(updates).length > 0) graph.updateNode(id, updates) + } + }) +} + function materializeReader( reader: ReturnType, blobs: Uint8Array[], @@ -186,9 +214,14 @@ function materializeReader( const { graph, sources, components, savedSizeNodes, componentIds } = state const existingNodeIds = new Set(graph.nodes.keys()) const layoutScales = new Map() + /** The sizes Figma derived for this pass's instance layers, as materialized. */ + const derivedSizes = new Map() const rememberDerivedSizes = (nodes: ReadonlyMap): void => { for (const [occurrence, node] of nodes) { - if (occurrence.derivedSize) savedSizeNodes.add(node.id) + if (occurrence.derivedSize) { + savedSizeNodes.add(node.id) + derivedSizes.set(node.id, { width: node.width, height: node.height }) + } if (occurrence.layoutScale !== undefined) layoutScales.set(node.id, occurrence.layoutScale) } } @@ -232,9 +265,9 @@ function materializeReader( sources.set(item.sourceId, materialized.root.id) } /** - * Components whose instances a resumed load must re-sync. Syncing visits every instance - * of a component, so a page placing many instances of one component collects it once - * and syncs after the page is built rather than per instance. + * Instances a resumed load must sync with their components, which may have changed live since + * the file loaded. Only this page's new instances: syncing a whole component would copy its + * sizes over the sizes Figma derived for instances earlier pages already placed. */ const resync = new Set() const populateInstances = (occurrence: InstanceOccurrence): void => { @@ -253,7 +286,7 @@ function materializeReader( linkInstanceSourceChildren(child, materialized, components) if (previous) { reconcileLiveComponentEdits(graph, materialized) - if (materialized.root.componentId) resync.add(materialized.root.componentId) + if (materialized.root.componentId) resync.add(materialized.root.id) } sources.set(child.sourceId, materialized.root.id) } else if (child.mainComponentId === null && child.properties.type !== 'SYMBOL') @@ -267,7 +300,8 @@ function materializeReader( parent.childIds = [...ordered, ...parent.childIds.filter((id) => !ordered.includes(id))] } for (const page of pages) populateInstances(page) - for (const componentId of resync) graph.syncInstances(componentId) + for (const instanceId of resync) graph.syncInstance(instanceId) + if (resync.size > 0) restoreDerivedSizes(graph, derivedSizes) for (const node of graph.getAllNodes()) { if (existingNodeIds.has(node.id)) continue for (const field of [ diff --git a/packages/fig/src/instance-overrides/live-component-edits.ts b/packages/fig/src/instance-overrides/live-component-edits.ts index 298d8037d..dc802a636 100644 --- a/packages/fig/src/instance-overrides/live-component-edits.ts +++ b/packages/fig/src/instance-overrides/live-component-edits.ts @@ -7,6 +7,38 @@ import { import type { MaterializedInstance } from './materialize-instance' +/** The instances that own `target`, nearest first, starting with `target` when it is one. */ +function owningInstances(graph: SceneGraph, target: SceneNode): SceneNode[] { + const owners: SceneNode[] = [] + let parent = target.parentId ? graph.getNode(target.parentId) : undefined + while (parent) { + if (parent.type === 'INSTANCE') owners.push(parent) + parent = parent.parentId ? graph.getNode(parent.parentId) : undefined + } + if (target.type === 'INSTANCE') owners.unshift(target) + return owners +} + +/** + * The component layers `target` syncs from, one per owning instance: the layer the owner maps it + * to, or its own component. + */ +export function syncSourceLayers(graph: SceneGraph, target: SceneNode): SceneNode[] { + const sources: SceneNode[] = [] + for (const owner of owningInstances(graph, target)) { + const mapped = getInstanceOverride( + owner.instanceOverrides, + owner.id, + target.id, + 'sourceComponentId' + ) + const sourceId = typeof mapped === 'string' ? mapped : target.componentId + const source = sourceId ? graph.getNode(sourceId) : undefined + if (source) sources.push(source) + } + return sources +} + /** Apply live component edits to new occurrences, never repaint or resync existing pages. */ export function reconcileLiveComponentEdits( graph: SceneGraph, @@ -14,13 +46,7 @@ export function reconcileLiveComponentEdits( ): void { graph.preserveSourceMetadataDuring(() => { for (const target of materialized.nodes.values()) { - const owners: SceneNode[] = [] - let parent = target.parentId ? graph.getNode(target.parentId) : undefined - while (parent) { - if (parent.type === 'INSTANCE') owners.push(parent) - parent = parent.parentId ? graph.getNode(parent.parentId) : undefined - } - if (target.type === 'INSTANCE') owners.unshift(target) + const owners = owningInstances(graph, target) const protectedFields = new Set() for (const owner of owners) { const fields = @@ -30,16 +56,7 @@ export function reconcileLiveComponentEdits( for (const field of fields?.keys() ?? []) protectedFields.add(field) } const updates: Partial = {} - for (const owner of owners) { - const mapped = getInstanceOverride( - owner.instanceOverrides, - owner.id, - target.id, - 'sourceComponentId' - ) - const sourceId = typeof mapped === 'string' ? mapped : target.componentId - const source = sourceId ? graph.getNode(sourceId) : undefined - if (!source) continue + for (const source of syncSourceLayers(graph, target)) { for (const field of INSTANCE_SYNC_FIELDS) { if (!source.source.editedFields.includes(field) || protectedFields.has(field)) continue Object.assign(updates, { [field]: structuredClone(source[field]) }) diff --git a/packages/scene-graph/src/index.ts b/packages/scene-graph/src/index.ts index 0bf172fcb..f96880b09 100644 --- a/packages/scene-graph/src/index.ts +++ b/packages/scene-graph/src/index.ts @@ -835,6 +835,10 @@ export class SceneGraph { Instances.syncInstances(this, componentId) } + syncInstance(instanceId: string): void { + Instances.syncInstance(this, instanceId) + } + detachInstance(instanceId: string): void { Instances.detachInstance(this, instanceId) } diff --git a/packages/scene-graph/src/instances.ts b/packages/scene-graph/src/instances.ts index 2dad1e9ab..1d52a6c1a 100644 --- a/packages/scene-graph/src/instances.ts +++ b/packages/scene-graph/src/instances.ts @@ -125,6 +125,21 @@ export function swapInstanceComponent( const syncingComponentsByGraph = new WeakMap>() export function syncInstances(graph: SceneGraph, componentId: string): void { + syncInstancesOf(graph, componentId, getInstances(graph, componentId)) +} + +/** Syncs one instance from its component, as `syncInstances` does for every instance. */ +export function syncInstance(graph: SceneGraph, instanceId: string): void { + const instance = graph.nodes.get(instanceId) + if (instance?.type !== 'INSTANCE' || !instance.componentId) return + syncInstancesOf(graph, instance.componentId, [instance]) +} + +function syncInstancesOf( + graph: SceneGraph, + componentId: string, + instances: Iterable +): void { const component = graph.nodes.get(componentId) if (component?.type !== 'COMPONENT') return let syncing = syncingComponentsByGraph.get(graph) @@ -135,7 +150,7 @@ export function syncInstances(graph: SceneGraph, componentId: string): void { if (syncing.has(componentId)) return syncing.add(componentId) try { - for (const instance of getInstances(graph, componentId)) { + for (const instance of instances) { const enclosing = enclosingInstanceOverrideFields(graph, instance) enclosing.push(new Set(instance.instanceOverrides.self.keys())) const protectedField = bindingProtection(enclosing) diff --git a/tests/engine/io/fig/import/instance-regressions.test.ts b/tests/engine/io/fig/import/instance-regressions.test.ts index d0e2d2f74..3c99f1de7 100644 --- a/tests/engine/io/fig/import/instance-regressions.test.ts +++ b/tests/engine/io/fig/import/instance-regressions.test.ts @@ -57,14 +57,18 @@ describe('derived instance layout regressions', () => { expect(inputFrame?.width).toBeCloseTo(375.7498, 3) expect(inputFrame?.height).toBeCloseTo(39.3803, 3) expect(content).toMatchObject({ x: 0, y: 0 }) + expect(content?.width).toBeCloseTo(374.8589782714844, 3) // Original Figma capture: gold-input-layout.json. The outer instance scales padding too. expect(firstBadge?.x).toBeCloseTo(7.126753330230713, 3) expect(firstBadge?.y).toBeCloseTo(5.345065116882324, 3) expect(firstBadge?.width).toBeCloseTo(85.3239, 3) expect(firstBadge?.height).toBeCloseTo(28.6901, 3) expect(firstBadgeContent).toMatchObject({ x: 0, y: 0 }) + // Figma derived the placeholder at 97.507 × 39.3803, as tall as Content, in the file. expect(placeholderFrame?.x).toBeCloseTo(277.352, 3) - expect(placeholderFrame?.y).toBeCloseTo(0.0915584564, 3) + expect(placeholderFrame?.y).toBeCloseTo(0, 3) + expect(placeholderFrame?.width).toBeCloseTo(97.50701141357422, 3) + expect(placeholderFrame?.height).toBeCloseTo(39.3802604675293, 3) expect(placeholderText?.x).toBeCloseTo(14.2535, 3) expect(placeholderText?.y).toBeCloseTo(10.6901, 3) })