Skip to content

fix: preserve scene materials across every persistence boundary - #597

Merged
Aymericr merged 1 commit into
mainfrom
fix/preserve-scene-materials-persistence
Aug 5, 2026
Merged

fix: preserve scene materials across every persistence boundary#597
Aymericr merged 1 commit into
mainfrom
fix/preserve-scene-materials-persistence

Conversation

@Aymericr

@Aymericr Aymericr commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Rebased and reworked version of #560, co-authored with @ShiroKSH, whose diagnosis and layering this follows. Closes #560.

The bug

A custom scene material survived nothing. materials was missing from the SceneGraph type, so every layer that rebuilds a graph field-by-field silently omitted it. Reopen a scene and it came back with default surfaces.

Probed on main before touching anything — all eight of these dropped the palette:

Boundary On main
cloneSceneGraph keys: collections, installedPlugins, nodes, rootNodeIds
forkSceneGraph same
bridge exportJSON same
bridge loadJSON materials{}
bridge loadJSON collections{} (see below)
exportSceneGraph materials absent — this is the MCP persist path
SQLite saveload z.object() strips it
API graph-schema z.object() strips it

Nodes reference materials through slots values shaped scene:mat_…. Those are opaque strings to the clone remapping, so material ids are carried over unchanged — minting fresh ones would orphan every reference and the clone would render with defaults anyway.

Two bugs found while fixing the round trip

1. loadJSON dropped collections too, for a different reason than materials. It applied plugin state in a second call after setScene, and setScene resets collections and materials to {} whenever they aren't in its extra bag — so anything applied afterwards is discarded. Everything now goes in one call.

That also fixes dirty-tracking for plugin-owned nodes: setScene marks every node dirty at the end, and markDirty returns early for a node whose plugin isn't in installedPlugins. Installing plugins afterwards meant those nodes were never marked, so they never got validated.

2. The echo-suppression signature omitted materials, which lost user edits. scene-loader.tsx skips a save when the graph signature matches the last remote payload. Since the signature ignored materials, a local edit that touched only the palette signed identically to that payload — so it read as an echo and the save was skipped. Verified against main's signature function: a material-only change produced a byte-identical signature.

The signature now also defaults the three fields setScene unconditionally writes. Without that, a payload that omits them — which is exactly what MCP live sync sends — can never match the store that defaulted them, so every remote update looks like a local edit and gets saved back, bumping the version. scene-signature.ts exists so this is testable; the two tests there both fail against main's version.

Where materials get validated

At the network boundary, not on read-back:

  • API schema (graph-schema.ts) holds materials to SceneMaterial in superRefine. They carry texture URLs, and MaterialSchema routes every one through AssetUrl — this schema is where that allowlist is enforced, so passing materials through as unknown would have reopened the Phase 3 risk on a field the editor loads. Confirmed ftp:// and javascript: are rejected.
  • It validates without transforming, matching how nodes are already handled here. The routes persist this schema's output, and SceneMaterial injects MaterialProperties defaults and strips unknown keys — so a validating shape would make every save silently rewrite the caller's palette. Measured: a sparse material grew from 1 property to 6.
  • SQLite read path stays permissive (z.record(z.string(), z.unknown())). Nothing validates on write and parseGraph throws, so a strict read shape would turn one odd stored value into a permanently unloadable scene. There's a test that saves a material with an ftp:// texture and asserts it still loads — a stored row must never become unreadable. This is the one place I diverged from fix: preserve scene material document state #560, which put SceneMaterial on the read path.

Verification

End-to-end probe over the real store, all eight boundaries — every one failed on main, all pass here:

PASS  cloneSceneGraph keeps materials          PASS  exportSceneGraph keeps materials
PASS  forkSceneGraph keeps materials           PASS  sqlite save->reopen->load keeps materials
PASS  bridge loadJSON->exportJSON materials    PASS  sqlite keeps collections
PASS  bridge loadJSON->exportJSON collections  PASS  sqlite keeps installedPlugins

Full CI gate green locally: bun run check 1603 files clean, check-types 9/9, bun run test 12/12 tasks, bun run build 7/7.

11 tests added across the five layers, each pinning a boundary that silently dropped data rather than erroring — that's the failure mode here, so a type alone wouldn't hold the line.

Credit

The diagnosis is @ShiroKSH's, and so is the call to fix it in packages/core and packages/mcp rather than only in apps/editor — that matters, because the hosted app forks scenes through @pascal-app/core/clone-scene-graph, so an app-only fix would have left both npm consumers and production broken. What I changed from #560: kept the echo-suppression timer instead of removing it, moved material validation from the SQLite read path to the API write path, dropped the one-line isRemoteSceneEcho wrapper, and rebased onto the 1.0-beta CHANGELOG.


Note

Medium Risk
Touches scene persistence, live-sync save suppression, and API graph validation (material texture URLs); behavior is well covered by new boundary tests but mistakes could still lose data or reject saves.

Overview
Fixes custom scene materials being stripped whenever a graph crossed a persistence boundary (save/load, clone/fork, MCP export/import, SQLite round-trip, API validation). materials is now part of the core SceneGraph and is deep-copied on clone/fork with stable material ids (node scene:mat_… refs are not remapped).

MCP bridge: exportJSON / exportSceneGraph include materials; loadJSON passes collections, materials, and installedPlugins in a single setScene extra bag so they are not wiped when setScene defaults missing fields.

Editor: Shared sceneGraphSignature includes materials, collections, and installedPlugins (with defaults matching setScene) so live-sync echo suppression does not skip saves for palette-only changes or treat every remote update as local.

API apiGraphSchema: Accepts and validates materials via SceneMaterial.safeParse (texture URLs / AssetUrl) without normalizing stored palette entries. SQLite GraphSchema lists materials (and installedPlugins) for round-trip but stays permissive on read.

Reviewed by Cursor Bugbot for commit 15a80fa. Bugbot is set up for automated code reviews on this repo. Configure here.

A custom scene material survived nothing. `materials` was absent from the
`SceneGraph` type, so every layer that rebuilds a graph field-by-field
silently omitted it: `cloneSceneGraph`/`forkSceneGraph`, the MCP bridge's
`exportJSON`/`loadJSON`, `exportSceneGraph` (which `save_scene`,
`publishLiveSceneSnapshot` and variant generation all persist through), the
SQLite read schema, and the editor's API graph schema. Reopen a scene and it
came back with default surfaces.

Nodes point at materials through `slots` values shaped `scene:mat_…`. Those
are opaque strings to the clone remapping, so material ids are carried over
unchanged — minting new ones would orphan every reference.

Two things fell out of fixing the round trip:

`loadJSON` dropped collections too, for a different reason: it applied plugin
state in a second call after `setScene`, and `setScene` resets `collections`
and `materials` to `{}` whenever they aren't in its `extra` bag. Everything
now goes in one call, which also fixes dirty-tracking for plugin-owned nodes
— `setScene` marks nodes dirty at the end, and `markDirty` skips nodes whose
plugin isn't installed yet.

The editor's echo-suppression signature omitted `materials`, so a local edit
that touched only the palette signed identically to the last remote payload
and the save was skipped — the edit was lost. The signature now defaults the
fields `setScene` always writes, so a payload that omits them (MCP live sync
sends exactly that) still matches the store that defaulted them.

Materials are validated where they enter from the network, not where they are
read back from disk. The API schema holds them to `SceneMaterial` in
`superRefine` — they carry texture URLs, and that schema is where the
`AssetUrl` allowlist is enforced — but keeps the parsed value untransformed,
since the routes persist this schema's output and `SceneMaterial` injects
defaults and strips unknown keys. The SQLite read path stays permissive:
nothing validates on write and `parseGraph` throws, so a strict read shape
would turn one odd stored value into a permanently unloadable scene.

Co-authored-by: ShiroKSH <kushidashiro@gmail.com>
@Aymericr
Aymericr merged commit 2e526e9 into main Aug 5, 2026
3 checks passed
@Aymericr
Aymericr deleted the fix/preserve-scene-materials-persistence branch August 5, 2026 03:27
Aymericr added a commit that referenced this pull request Aug 5, 2026
…fixes (#598)

Both #596 and #597 landed without a CHANGELOG line. Match the 0.6.0 style
and name the contributors, since the release notes are how credit for an
outside fix actually surfaces.
ovurrsl pushed a commit to ovurrsl/editor that referenced this pull request Aug 10, 2026
Ayna 7 Ağustos'tan beri donmuştu; `MIRROR_TOKEN` ile açıldıktan sonra
biriken 56 upstream commit'i entegrasyon dalına alındı. On üç dosya
çakıştı; beşinin kuralı `UPSTREAM.md`'de yazılıydı, sekizinde yoktu ve
kararlar tek tek verilip artık oraya yazıldı.

**İki sessiz veri kaybı düzeltildi.** Upstream'in pascalorg#597'si `materials`
alanının kalıcılık katmanlarında hiç adlandırılmadığını bulmuş: `z.object()`
adlandırmadığı anahtarı düşürdüğü için özel yüzeyler kaydetme hatası
vermeden siliniyordu. Fork'ta aynı eksik iki yerdeydi
(`apps/editor/lib/graph-schema.ts`, `scene-store-shared.ts`) ve ikincisinde
`installedPlugins` de eksikti — yani bir depo sahnesi hangi eklentiye
ihtiyaç duyduğunu da unutuyordu. Belirtisi yoktu: sahne açılıyor, yalnız
kullanıcının verdiği malzemeler varsayılana dönmüş oluyordu.

**Yayın kapısı korundu.** Sağlık kontrolü bizde kaldı — upstream'inki
veritabanına dokunmadan `ok` diyor, alınsaydı `deploy-bundle`'ın ikinci
duman testi bozuk bir veritabanıyla da yeşil yanardı. Upstream'in `version`
ve `instanceId` alanları eklendi; hangi derlemenin canlıda olduğunu okumanın
başka yolu yoktu. Aynı gerekçeyle `output: 'standalone'` koşulsuz bırakıldı:
upstream onu `PASCAL_PORTABLE_BUILD` arkasına almış, alınsaydı derleme yeşil
biter ve sunulacak dizin oluşmazdı.

**Eklentiler.** `mintPlugin` ile `warehousePlugin` artık birlikte kayıtlı —
`extendPluginDiscovery` zincirleme olduğu için bu bir seçim değil iki çağrı.
Mint bağımlılığı, external paket listesi ve kayıt satırı birlikte geldi.

**Kök `package.json`.** `test` betiği fork'ta düşmüştü, yani depo kökünden
`bun test` hiçbir şey koşmuyordu; geri geldi. Node alt sınırı 20.9'a,
`packageManager` güncel bun'a (1.3.14) çıktı, Next 16.3.0'a sabitlendi.
`release:cli` bilerek alınmadı. `build`/`dev`/`sync-panel` bizden.

**Viewer.** GPU yetenek denetimi ile desteklenmeyen-GPU yedeği upstream'de
ayrı modüllere çıkarılmış; bizim satır içi kopyalarımız aynı kodun eski
hâliydi. Upstream'inki alındı, yalnız yedek metnindeki ürün adı geri
"DigitalTwin" yapıldı.

**Testler.** `graph-schema.test.ts` iki tarafta bağımsız yazılmış; ikisi de
tutuldu (upstream'in 12 zarf testi + bizim 5 eklenti-kind testimiz).
Merge sonrası: core 950, editor 600, viewer 177 test yeşil, biome 1797
dosyada temiz.

`bun.lock` bizim tarafımızdan alındı ve bağımlılıklar elle değiştiği için
Relock workflow'uyla gerçek bir runner'da yeniden üretilmesi gerekiyor —
`UPSTREAM.md`'nin kendi kuralı.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JzwsRNyjsWXrn95Y7K4voB
tahacagrimen pushed a commit to tahacagrimen/editor that referenced this pull request Aug 13, 2026
…alorg#7)

Named bookmarks that restore a camera pose, per-collection visibility and
lock, the active section plane, and the view/level/wall/theme presentation
state. SketchUp calls these "Scenes"; that word is the scene graph here, so
they are `SavedView`.

Scene-side state like `collections` and `materials` — persisted with the
document, not a node — which means it had to be carried across every
persistence boundary, the bug class CHANGELOG pascalorg#597 shipped twice:

  save        `use-auto-save` graph construction *and* its `beforeunload`
              flush, which builds a second graph a few lines down
  load        `applySceneGraphToEditor` + `setScene`'s extra bag
  clone/fork  `cloneSceneGraph` (see below) and `forkSceneGraph`
  live sync   `sceneGraphSignature`, whose comment already warned that an
              unsigned field makes a local edit read as an echo
  history     `SceneSnapshot` + `areSceneSnapshotsEqual`, so a view edit
              is undoable like a collection edit

Cloning is not a copy: a view points at a section-plane node and at
collections, both of which get renumbered, so `cloneSceneGraph` remaps both
and records "no cut" when the plane didn't survive a fork.

Core stores the viewer/editor half of a view as an opaque `presentation`
bag — view mode, level mode, wall mode and theme are rendering and editing
concepts core is not allowed to know about. `packages/editor/src/lib/
saved-views.ts` is the only place that gives the bag a shape, and it drops
values it doesn't recognise rather than restoring nonsense.

Restoring writes the scene inside one `runAsSingleSceneHistoryStep`: a view
that flips three collections and swaps the cut is one action to the user,
not four undos. An absent `sectionPlaneId` (a view saved before section
planes existed) leaves the current cut alone; an explicit `null` clears it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VhnKW1ZmnSjGdLZCx5kvY7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant