diff --git a/CHANGELOG.md b/CHANGELOG.md index bf9e890764..93d544389f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +### Fixes + +- Preserve custom scene materials across save, load, clone, fork, and live sync. Materials were dropped at every persistence boundary, so a scene reopened with default surfaces. Collections were dropped on MCP import for the same reason. + ## 1.0.0-beta.1 (2026-07-30) The first Pascal Editor 1.0 beta. Relative to diff --git a/apps/editor/components/scene-loader.tsx b/apps/editor/components/scene-loader.tsx index fb891f1cbd..a0179005bd 100644 --- a/apps/editor/components/scene-loader.tsx +++ b/apps/editor/components/scene-loader.tsx @@ -14,6 +14,7 @@ import Image from 'next/image' import Link from 'next/link' import { useRouter, useSearchParams } from 'next/navigation' import { useCallback, useEffect, useRef, useState } from 'react' +import { type PersistedSceneGraph, sceneGraphSignature } from '@/lib/scene-signature' import { cn } from '@/lib/utils' import { BuildTab } from './build-tab' import { CommunityViewerToolbarLeft, CommunityViewerToolbarRight } from './viewer-toolbar' @@ -71,26 +72,13 @@ interface SceneLoaderProps { meta: SceneMeta } -type SceneGraphWithCollections = SceneGraph & { - collections?: Record -} - interface LiveSceneEvent { eventId: number sceneId: string version: number kind: string createdAt: string - graph: SceneGraphWithCollections -} - -function sceneGraphSignature(graph: SceneGraphWithCollections): string { - return JSON.stringify({ - nodes: graph.nodes, - rootNodeIds: graph.rootNodeIds, - collections: graph.collections, - installedPlugins: graph.installedPlugins, - }) + graph: PersistedSceneGraph } /** diff --git a/apps/editor/lib/graph-schema.test.ts b/apps/editor/lib/graph-schema.test.ts index deb9bba229..ca63ca9c3e 100644 --- a/apps/editor/lib/graph-schema.test.ts +++ b/apps/editor/lib/graph-schema.test.ts @@ -165,3 +165,52 @@ test('treats an unnamespaced unknown type as a foreign node', () => { .success, ).toBe(false) }) + +const MATERIAL_ID = 'mat_a1b2c3d4e5f6g7h8' +const material = (overrides: Record = {}) => ({ + id: MATERIAL_ID, + name: 'Oak', + material: { preset: 'wood', ...overrides }, +}) + +test('keeps materials in the parsed output', () => { + const graph = { ...buildGraph({}), materials: { [MATERIAL_ID]: material() } } + const res = apiGraphSchema.safeParse(graph) + + expect(res.success).toBe(true) + expect(res.data?.materials).toEqual(graph.materials) +}) + +// A material's texture is a URL the editor loads, so it is held to the same +// `AssetUrl` allowlist as every other URL-shaped field in the graph. +test('rejects a material texture URL outside the allowlist', () => { + for (const url of ['ftp://host/a.png', 'javascript:alert(1)']) { + const graph = { + ...buildGraph({}), + materials: { [MATERIAL_ID]: material({ texture: { url } }) }, + } + expect(apiGraphSchema.safeParse(graph).success).toBe(false) + } +}) + +test('accepts a material texture URL inside the allowlist', () => { + const graph = { + ...buildGraph({}), + materials: { + [MATERIAL_ID]: material({ texture: { url: 'https://cdn.example.com/oak.png' } }), + }, + } + + expect(apiGraphSchema.safeParse(graph).success).toBe(true) +}) + +// The routes persist this schema's output, so validation must not double as +// normalization: a save has to store the palette it was handed. +test('does not rewrite materials it accepts', () => { + const sparse = { id: MATERIAL_ID, name: 'Oak', material: { properties: { color: '#886644' } } } + const graph = { ...buildGraph({}), materials: { [MATERIAL_ID]: sparse } } + const res = apiGraphSchema.safeParse(graph) + + expect(res.success).toBe(true) + expect(res.data?.materials?.[MATERIAL_ID]).toEqual(sparse) +}) diff --git a/apps/editor/lib/graph-schema.ts b/apps/editor/lib/graph-schema.ts index 7f5f6c5ceb..f597bfa73c 100644 --- a/apps/editor/lib/graph-schema.ts +++ b/apps/editor/lib/graph-schema.ts @@ -1,4 +1,4 @@ -import { AnyNode, AssetUrl, BaseNode } from '@pascal-app/core/schema' +import { AnyNode, AssetUrl, BaseNode, SceneMaterial } from '@pascal-app/core/schema' import { z } from 'zod' /** @@ -100,6 +100,15 @@ export const apiGraphSchema = z nodes: z.record(z.string(), z.unknown()), rootNodeIds: z.array(z.string()), collections: z.unknown().optional(), + // `unknown` here, validated in `superRefine` below — the same split the + // nodes get, and for the same reason: the routes persist this schema's + // *output*, so a validating shape would also rewrite what gets stored. + // `SceneMaterial` injects `MaterialProperties` defaults and drops unknown + // keys, which would make every save silently normalize the caller's + // palette. Materials still have to be checked, because they carry texture + // URLs that `MaterialSchema` routes through `AssetUrl` — this schema is + // where that allowlist is enforced. + materials: z.record(z.string(), z.unknown()).optional(), installedPlugins: z.array(z.string().min(1)).optional(), }) .superRefine((value, ctx) => { @@ -113,6 +122,18 @@ export const apiGraphSchema = z } } + for (const [materialId, material] of Object.entries(value.materials ?? {})) { + const res = SceneMaterial.safeParse(material) + if (res.success) continue + for (const issue of res.error.issues) { + ctx.addIssue({ + code: 'custom', + path: ['materials', materialId, ...issue.path], + message: issue.message, + }) + } + } + // Ids of foreign nodes in this graph. Builtin container schemas name the // child kinds they accept (`BuildingNode.children`, `RoofNode.children`), // so a container holding a plugin child fails against `AnyNode` even diff --git a/apps/editor/lib/scene-signature.test.ts b/apps/editor/lib/scene-signature.test.ts new file mode 100644 index 0000000000..6bcce47df8 --- /dev/null +++ b/apps/editor/lib/scene-signature.test.ts @@ -0,0 +1,41 @@ +import { expect, test } from 'bun:test' +import { type PersistedSceneGraph, sceneGraphSignature } from './scene-signature' + +const NODE_ID = 'level_a1b2c3d4e5f6g7h8' +const MATERIAL_ID = 'mat_a1b2c3d4e5f6g7h8' + +const graph = (overrides: Partial = {}) => + ({ + nodes: { [NODE_ID]: { object: 'node', id: NODE_ID, type: 'level', level: 0 } }, + rootNodeIds: [NODE_ID], + ...overrides, + }) as PersistedSceneGraph + +// The echo check compares a raw SSE payload against the store after +// `setScene` ran, and `setScene` always writes these three keys. A payload +// that omits them — which is exactly what MCP live sync sends — must still +// match, or every remote update looks like a local edit and gets saved back. +test('an omitted field signs the same as its applied default', () => { + expect(sceneGraphSignature(graph())).toBe( + sceneGraphSignature(graph({ collections: {}, materials: {}, installedPlugins: [] })), + ) +}) + +// Conversely, every field the save body carries has to be signed. An unsigned +// field makes a local edit that touches only that field read as an echo, and +// the save is skipped — the change is silently lost. +test('changing any signed field changes the signature', () => { + const base = sceneGraphSignature(graph()) + + expect( + sceneGraphSignature( + graph({ materials: { [MATERIAL_ID]: { id: MATERIAL_ID, name: 'Oak', material: {} } } }), + ), + ).not.toBe(base) + expect( + sceneGraphSignature(graph({ collections: { col_1: { id: 'col_1', nodeIds: [] } } })), + ).not.toBe(base) + expect(sceneGraphSignature(graph({ installedPlugins: ['@pascal-app/plugin-trees'] }))).not.toBe( + base, + ) +}) diff --git a/apps/editor/lib/scene-signature.ts b/apps/editor/lib/scene-signature.ts new file mode 100644 index 0000000000..a5a4587dd0 --- /dev/null +++ b/apps/editor/lib/scene-signature.ts @@ -0,0 +1,27 @@ +import type { SceneGraph } from '@pascal-app/editor' + +export type PersistedSceneGraph = SceneGraph & { + collections?: Record +} + +/** + * Identity of a graph for echo detection, compared across a boundary that + * normalizes: one side is a raw SSE payload, the other is the editor's state + * after `applySceneGraphToEditor` ran. `setScene` always writes `collections`, + * `materials` and `installedPlugins`, so a payload that omits them (MCP live + * sync emits exactly that) has to serialize the same as the store that + * defaulted them, or the echo reads as a local edit and gets saved back. + * + * Every field the PUT body carries has to appear here. A field that is + * persisted but unsigned makes a local change to *only* that field + * indistinguishable from an echo, and the save is skipped. + */ +export function sceneGraphSignature(graph: PersistedSceneGraph): string { + return JSON.stringify({ + nodes: graph.nodes, + rootNodeIds: graph.rootNodeIds, + collections: graph.collections ?? {}, + materials: graph.materials ?? {}, + installedPlugins: graph.installedPlugins ?? [], + }) +} diff --git a/packages/core/src/utils/clone-scene-graph.test.ts b/packages/core/src/utils/clone-scene-graph.test.ts index a189af4344..c7e30961a4 100644 --- a/packages/core/src/utils/clone-scene-graph.test.ts +++ b/packages/core/src/utils/clone-scene-graph.test.ts @@ -1,5 +1,6 @@ import { describe, expect, test } from 'bun:test' import type { CollectionId } from '../schema/collections' +import type { SceneMaterialId } from '../schema/scene-material' import type { AnyNode, AnyNodeId } from '../schema/types' import { cloneLevelSubtree, @@ -46,10 +47,49 @@ function makeSceneGraph(): SceneGraph { nodeIds: ['scan_1', 'guide_1'] as AnyNodeId[], }, }, + materials: { + ['mat_1' as SceneMaterialId]: { + id: 'mat_1', + name: 'Oak', + material: { preset: 'wood' }, + }, + }, installedPlugins: ['pascal:trees'], } } +describe('scene material palette', () => { + // Nodes reference materials through `slots` values shaped `scene:mat_…`. + // Those are opaque strings to the node remapping, so the ids they point at + // have to survive a clone unchanged or every reference dangles. + test('cloneSceneGraph carries materials over with their ids intact', () => { + const source = makeSceneGraph() + const cloned = cloneSceneGraph(source) + + expect(cloned.materials).toEqual(source.materials) + }) + + test('cloneSceneGraph deep-copies materials', () => { + const source = makeSceneGraph() + const cloned = cloneSceneGraph(source) + const material = cloned.materials?.['mat_1' as SceneMaterialId] + expect(material).toBeDefined() + if (!material) return + + material.name = 'Mutated' + expect(source.materials?.['mat_1' as SceneMaterialId]?.name).toBe('Oak') + }) + + // A palette entry is authored content in its own right. Stripping the scan + // node that happened to use it must not take the material with it. + test('forkSceneGraph keeps materials when stripping scans', () => { + const source = makeSceneGraph() + const forked = forkSceneGraph(source) + + expect(forked.materials).toEqual(source.materials) + }) +}) + describe('forkSceneGraph', () => { test('strips scan and guide nodes by default', () => { const forked = forkSceneGraph(makeSceneGraph()) diff --git a/packages/core/src/utils/clone-scene-graph.ts b/packages/core/src/utils/clone-scene-graph.ts index e48f13e830..13e89ab260 100644 --- a/packages/core/src/utils/clone-scene-graph.ts +++ b/packages/core/src/utils/clone-scene-graph.ts @@ -6,11 +6,13 @@ import { import type { AnyNode, AnyNodeId } from '../schema' import { generateId } from '../schema/base' import type { Collection, CollectionId } from '../schema/collections' +import type { SceneMaterial, SceneMaterialId } from '../schema/scene-material' export type SceneGraph = { nodes: Record rootNodeIds: AnyNodeId[] collections?: Record + materials?: Record installedPlugins?: string[] } @@ -32,7 +34,7 @@ function extractIdPrefix(id: string): string { * - Multi-scene in-memory scenarios */ export function cloneSceneGraph(sceneGraph: SceneGraph): SceneGraph { - const { nodes, rootNodeIds, collections, installedPlugins } = sceneGraph + const { nodes, rootNodeIds, collections, materials, installedPlugins } = sceneGraph // Build ID mapping: old ID -> new ID const idMap = new Map() @@ -164,6 +166,12 @@ export function cloneSceneGraph(sceneGraph: SceneGraph): SceneGraph { nodes: clonedNodes, rootNodeIds: clonedRootNodeIds, ...(clonedCollections && { collections: clonedCollections }), + // Material ids are deliberately *not* remapped. Nodes point at these + // through `slots` values shaped `scene:mat_…` — opaque strings that the + // node remapping above copies verbatim, since `idMap` only covers node + // ids. Minting fresh material ids here would orphan every one of those + // refs and the clone would render with default materials. + ...(materials && { materials: structuredClone(materials) }), ...(installedPlugins && { installedPlugins: [...installedPlugins] }), } } @@ -304,7 +312,7 @@ export function forkSceneGraph( return cloneSceneGraph(sceneGraph) } - const { nodes, rootNodeIds, collections, installedPlugins } = sceneGraph + const { nodes, rootNodeIds, collections, materials, installedPlugins } = sceneGraph // First, identify scan and guide node IDs to exclude (user-uploaded imagery) const excludedNodeIds = new Set() @@ -366,6 +374,11 @@ export function forkSceneGraph( nodes: filteredNodes, rootNodeIds: filteredRootNodeIds, ...(filteredCollections && { collections: filteredCollections }), + // Kept whole rather than filtered to the surviving nodes: a palette entry + // is authored content in its own right, and dropping the scan node that + // happened to be its only user would silently delete a material the fork's + // owner can still pick from the palette. + ...(materials && { materials }), ...(installedPlugins && { installedPlugins }), }) } diff --git a/packages/mcp/src/bridge/scene-bridge.test.ts b/packages/mcp/src/bridge/scene-bridge.test.ts index 0bf7c3edf4..141f9e61f2 100644 --- a/packages/mcp/src/bridge/scene-bridge.test.ts +++ b/packages/mcp/src/bridge/scene-bridge.test.ts @@ -439,6 +439,29 @@ describe('SceneBridge', () => { expect(bridge.exportJSON().installedPlugins).toEqual(['pascal:trees']) }) + // `setScene` resets `collections` and `materials` to `{}` unless they are + // in its `extra` bag, so anything applied after it is silently discarded. + // These two round trips are what catch a regression back to that. + test('loadJSON round-trips the material palette', () => { + const materials = { + mat_1: { id: 'mat_1', name: 'Oak', material: { preset: 'wood' } }, + } + bridge.loadJSON({ ...bridge.exportJSON(), materials } as never) + + expect(bridge.exportJSON().materials).toEqual(materials) + }) + + test('loadJSON round-trips collections', () => { + const snap = bridge.exportJSON() + const nodeId = Object.keys(snap.nodes)[0]! + const collections = { + collection_1: { id: 'collection_1', name: 'Refs', nodeIds: [nodeId] }, + } + bridge.loadJSON({ ...snap, collections } as never) + + expect(bridge.exportJSON().collections).toEqual(collections) + }) + test('legacy graphs do not become explicitly uninstalled on export', () => { const snap = bridge.exportJSON() const { installedPlugins: _installedPlugins, ...legacy } = snap diff --git a/packages/mcp/src/bridge/scene-bridge.ts b/packages/mcp/src/bridge/scene-bridge.ts index f69672fcde..193ba626d0 100644 --- a/packages/mcp/src/bridge/scene-bridge.ts +++ b/packages/mcp/src/bridge/scene-bridge.ts @@ -20,6 +20,9 @@ export type ActiveSceneMeta = Pick< 'id' | 'name' | 'projectId' | 'ownerId' | 'thumbnailUrl' | 'version' > +/** The `extra` bag `setScene` accepts — collections, materials, plugin state. */ +type SetSceneExtra = Parameters['setScene']>[2] + /** * Headless bridge to the `@pascal-app/core` Zustand store. * @@ -59,11 +62,15 @@ export class SceneBridge { } /** Replace entire scene (undoable via Zundo). */ - setScene(nodes: Record, rootNodeIds: AnyNodeId[]): void { - useScene.getState().setScene(nodes, rootNodeIds) + setScene( + nodes: Record, + rootNodeIds: AnyNodeId[], + extra?: SetSceneExtra, + ): void { + useScene.getState().setScene(nodes, rootNodeIds, extra) } - /** Full snapshot for export, including collections. */ + /** Full snapshot for export, including collections and the material palette. */ exportJSON(): SceneGraph & { collections: Record } { const state = useScene.getState() // Deep-clone so callers can't mutate store state directly. @@ -72,6 +79,7 @@ export class SceneBridge { nodes: state.nodes, rootNodeIds: state.rootNodeIds, collections: state.collections ?? {}, + materials: state.materials ?? {}, ...(state.hasExplicitPluginInstallState || state.installedPlugins.length > 0 ? { installedPlugins: state.installedPlugins } : {}), @@ -119,13 +127,25 @@ export class SceneBridge { } } - this.setScene(nodes as Record, rootNodeIds as AnyNodeId[]) - if (Array.isArray(obj.installedPlugins)) { - useScene.getState().setInstalledPlugins( - obj.installedPlugins.filter((id): id is string => typeof id === 'string'), - { explicit: true }, - ) - } + const record = (value: unknown) => + value && typeof value === 'object' && !Array.isArray(value) ? value : undefined + const collections = record(obj.collections) as NonNullable['collections'] + const materials = record(obj.materials) as NonNullable['materials'] + const installedPlugins = Array.isArray(obj.installedPlugins) + ? obj.installedPlugins.filter((id): id is string => typeof id === 'string') + : undefined + + // One `setScene` call rather than a follow-up `setInstalledPlugins`: + // `setScene` overwrites `collections` and `materials` with `{}` whenever + // they aren't in the `extra` bag, so anything applied afterwards is lost. + // It also marks every node dirty at the end, and `markDirty` skips nodes + // whose plugin isn't installed — so plugin state has to be in place by + // then or plugin-owned nodes never get validated. + this.setScene(nodes as Record, rootNodeIds as AnyNodeId[], { + ...(collections && { collections }), + ...(materials && { materials }), + ...(installedPlugins && { installedPlugins, hasExplicitPluginInstallState: true }), + }) } /** Read a single node, or `null` if not present. */ diff --git a/packages/mcp/src/operations/scene-operations.test.ts b/packages/mcp/src/operations/scene-operations.test.ts index 63146664a0..7c003d1d22 100644 --- a/packages/mcp/src/operations/scene-operations.test.ts +++ b/packages/mcp/src/operations/scene-operations.test.ts @@ -3,6 +3,7 @@ import * as fs from 'node:fs/promises' import * as os from 'node:os' import * as path from 'node:path' import type { SceneGraph } from '@pascal-app/core/clone-scene-graph' +import { SceneBridge } from '../bridge/scene-bridge' import { SqliteSceneStore } from '../storage/sqlite-scene-store' import { createSceneOperations } from './scene-operations' @@ -77,3 +78,17 @@ describe('SceneOperationsFacade scene events', () => { await expect(operations.listSceneEvents('live')).rejects.toThrow('scene_events_unavailable') }) }) + +// `exportSceneGraph` hand-copies fields off `exportJSON`, so a field it omits +// is dropped from everything that persists through it — `save_scene`, +// `publishLiveSceneSnapshot`, and variant generation. +describe('SceneOperationsFacade exportSceneGraph', () => { + test('carries the material palette off the bridge', () => { + const bridge = new SceneBridge() + const materials = { mat_1: { id: 'mat_1', name: 'Oak', material: { preset: 'wood' } } } + bridge.loadJSON({ ...makeGraph(), materials } as never) + const operations = createSceneOperations({ bridge }) + + expect(operations.exportSceneGraph().materials).toEqual(materials) + }) +}) diff --git a/packages/mcp/src/operations/scene-operations.ts b/packages/mcp/src/operations/scene-operations.ts index 0167d5b743..4527c69c9b 100644 --- a/packages/mcp/src/operations/scene-operations.ts +++ b/packages/mcp/src/operations/scene-operations.ts @@ -150,6 +150,7 @@ class SceneOperationsFacade implements SceneOperations { nodes: exported.nodes, rootNodeIds: exported.rootNodeIds, collections: exported.collections as SceneGraph['collections'], + materials: exported.materials, installedPlugins: exported.installedPlugins, } } diff --git a/packages/mcp/src/storage/sqlite-scene-store.test.ts b/packages/mcp/src/storage/sqlite-scene-store.test.ts index c97c2011a3..84a003427f 100644 --- a/packages/mcp/src/storage/sqlite-scene-store.test.ts +++ b/packages/mcp/src/storage/sqlite-scene-store.test.ts @@ -113,6 +113,40 @@ describe('SqliteSceneStore', () => { expect(loaded!.name).toBe('Kitchen') }) + // `GraphSchema` strips any key it doesn't name, so a field missing from it + // is dropped on load without an error — the save looks like it worked. + test('round-trips collections, materials and installed plugins', async () => { + const graph = makeGraph({ + collections: { + collection_1: { id: 'collection_1', name: 'Refs', nodeIds: ['site_abc'] }, + } as SceneGraph['collections'], + materials: { + mat_1: { id: 'mat_1', name: 'Oak', material: { preset: 'wood' } }, + } as SceneGraph['materials'], + installedPlugins: ['pascal:trees'], + }) + await store.save({ id: 'full', name: 'Full', graph }) + + store.close() + store = createStore(rootDir) + + expect((await store.load('full'))?.graph).toEqual(graph) + }) + + // Nothing validates a graph on the way in, and `parseGraph` throws on a + // shape mismatch, so a strict read schema would make an odd stored value + // permanently unloadable rather than merely odd. + test('loads a stored scene whose material does not match the strict schema', async () => { + const graph = makeGraph({ + materials: { + mat_1: { id: 'mat_1', material: { texture: { url: 'ftp://host/a.png' } } }, + } as unknown as SceneGraph['materials'], + }) + await store.save({ id: 'odd', name: 'Odd', graph }) + + expect((await store.load('odd'))?.graph).toEqual(graph) + }) + test('stores optional metadata verbatim', async () => { await store.save({ id: 'meta-test', diff --git a/packages/mcp/src/storage/sqlite-scene-store.ts b/packages/mcp/src/storage/sqlite-scene-store.ts index f705a8793b..566ba1419a 100644 --- a/packages/mcp/src/storage/sqlite-scene-store.ts +++ b/packages/mcp/src/storage/sqlite-scene-store.ts @@ -70,10 +70,18 @@ interface ProjectPlaceholder { updatedAt: string } +// `z.object()` strips keys it doesn't name, so every field that must survive a +// save→load round trip has to be listed here. Values stay `unknown` rather than +// being validated against `SceneMaterial`/`Collection`: nothing validates on the +// way in, and `parseGraph` throws, so a strict shape here would let one odd +// stored value make a saved scene permanently unloadable. Validation belongs on +// the write path, where the caller can still react to it. const GraphSchema = z.object({ nodes: z.record(z.string(), z.unknown()), rootNodeIds: z.array(z.string()), collections: z.record(z.string(), z.unknown()).optional(), + materials: z.record(z.string(), z.unknown()).optional(), + installedPlugins: z.array(z.string()).optional(), }) /**