mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-05-25 23:25:02 +00:00
> Final architectural step in the PLY / 3D Gaussian Splatting series. Previous PR introduced `ModelAdapter` with a dormant `capabilities` field; this PR makes those capabilities load-bearing and replaces the remaining `instanceof SplatMesh` / `instanceof BufferGeometry` switches with adapter-driven dispatch. Together with previous one it removes the last of the format-specific branching from `SceneModelManager` / `Load3d`. Seventh in the series splitting up the https://github.com/Comfy-Org/ComfyUI_frontend/pull/11495. ## Summary Drive viewer behavior (fit-to-viewer, default camera pose, world bounds, GPU dispose, material rebuild) from `ModelAdapterCapabilities` + 3 new optional adapter methods, instead of `SceneModelManager` reflecting on model shape. `Load3d` is rewritten to take its 13 managers as injected `Load3dDeps`; a new `createLoad3d` factory assembles them and threads a single `AdapterRef` between `LoaderManager` (writer) and `SceneModelManager` + `Load3d` (readers). Splat orientation + decoder-race + sizing bugs are fixed as a side effect — splats now render upright, fill the grid, and don't lock the OrbitControls target on first frame. ## Changes - **`ModelAdapter.ts`**: add `AdapterRef = { current: ModelAdapter | null }` shared handle and 3 optional adapter methods — `computeBounds(model)`, `disposeModel(model)`, `defaultCameraPose()`. - **`SplatModelAdapter.ts`**: implement all 3 optional methods; `await splatMesh.initialized` so first-frame bounds are populated (fixes a decoder race that collapsed the OrbitControls target onto the camera); `quaternion.set(1, 0, 0, 0)` to convert sparkjs's OpenCV (Y-down, Z-forward) to three.js (Y-up, Z-back); flip `fitToViewer` back to `true` and bump `fitTargetSize` to `20` so splats fill the 20-unit grid instead of shrinking to 1/4 of it. - **`PointCloudModelAdapter.ts`**: extract `buildPointCloudForMaterialMode` so `SceneModelManager` rebuilds via the same code path the initial load uses; `setPath` so PLYs that reference relative assets resolve correctly. - **`LoaderManager.ts`**: accept optional `AdapterRef`; write through it instead of the internal `_currentAdapter` field. `clearModel()` now runs while the old adapter is still current so its `disposeModel()` can release renderer-owned resources. - **`SceneModelManager.ts`**: accept 4 capability lambdas (`getCurrentCapabilities` / `getBoundsFromAdapter` / `disposeModelViaAdapter` / `getDefaultCameraPose`) with `DEFAULT_MODEL_CAPABILITIES` / null fallbacks. `setupModel`, `fitToViewer`, and material-mode rebuild are now capability-driven; `containsSplatMesh` (30 lines of `instanceof` traversal) and `handlePLYModeSwitch` (90 lines of duplicated PLY rebuild) are gone. - **`Load3d.ts`**: ctor switches from manager-creation to deps-injected (`Load3dDeps`); add `getCurrentModelCapabilities()` reader; gate `setGizmoEnabled` / `setGizmoMode` / `resetGizmoTransform` / `applyGizmoTransform` on `capabilities.gizmoTransform`; `isSplatModel` / `isPlyModel` now read `adapterRef.current?.kind` directly. - **`createLoad3d.ts`** (new): single factory that builds the renderer (`createRenderer`), assembles all 13 managers in dependency order, and threads one shared `AdapterRef` through `LoaderManager` and `SceneModelManager`'s 4 capability lambdas. - **`useLoad3d.ts` / `useLoad3dViewer.ts`**: switch from `new Load3d(container, options)` to `createLoad3d(container, options)`. No other call-site changes. ## Review Focus - **Capability dispatch parity**: walk each former hardcoded branch in `SceneModelManager` and confirm it now falls out of the right capability: - `containsSplatMesh()` → `!capabilities.fitToViewer` + `getDefaultCameraPose()` - `handlePLYModeSwitch()` → `capabilities.requiresMaterialRebuild` + `buildPointCloudForMaterialMode()` - `Box3.setFromObject(model)` for sizing → `getBoundsFromAdapter(model) ?? Box3.setFromObject(model)` - Mesh/Points geometry+material disposal in `clearModel` → still happens, plus `disposeModelViaAdapter(obj)` for adapter-owned resources (sparkjs SplatMesh internal GPU state) - **`AdapterRef` lifecycle**: one ref is created in `createLoad3d`, passed to `LoaderManager` (writer) and `SceneModelManager` (read via 4 closures). `LoaderManager.loadModel` clears via the *old* adapter first (so `disposeModel` runs), then null-resets the ref before picking the new one. Test `keeps the old adapter current while clearModel runs` pins this ordering. - **Splat fixes are user-visible, not pure refactor**: - Orientation: `quaternion.set(1, 0, 0, 0)` matches the sparkjs README convention. Without it splats render upside-down and mirrored on Z. Same rotation is applied to the camera-from-matrices output in PR-E so a future splat + camera-pose pair lines up. - Decoder race: `await splatMesh.initialized` ensures `getBoundingBox` returns a non-zero box on the first call. Without it `setupModel`'s bounds → camera pipeline placed the OrbitControls target on the camera origin, locking the view. - Sizing: `fitTargetSize: 20` (vs. the mesh default of 5) means splat geometry spans the full 20-unit grid footprint instead of ~1/4 of it. Mesh assets are unaffected. - **Gizmo gating**: `setGizmoEnabled(true)` early-returns when `capabilities.gizmoTransform` is false. Internal `setGizmoEnabled(false)` still runs (so we can always disable). `setGizmoMode` / `resetGizmoTransform` / `applyGizmoTransform` no-op when the capability is off. - **`createLoad3d` is the single ctor entry**: `new Load3d(...)` is no longer callable from app code (ctor signature changed to `(container, deps, options)`). All call sites use `createLoad3d`. Test scaffolding still uses `Object.create(Load3d.prototype)` + property injection where it needs to bypass renderer creation. - **Backwards compatibility**: `LoaderManager`'s `adapterRef` and `SceneModelManager`'s 4 capability lambdas all have defaults (`createAdapterRef()` and `() => DEFAULT_MODEL_CAPABILITIES` etc.), so the existing test suites that construct these classes with the old signatures still compile and pass without modification beyond what's in this PR. ## Coverage | File | Stmts | Branch | Funcs | Lines | |---|---|---|---|---| | `ModelAdapter.ts` (modified) | **100%** | **100%** | **100%** | **100%** | | `LoaderManager.ts` (modified) | **100%** | 91.7% | 86.7% | **100%** | | `MeshModelAdapter.ts` (unchanged) | **100%** | **100%** | **100%** | **100%** | | `PointCloudModelAdapter.ts` (modified) | **97.9%** | 69.2% | 71.4% | **97.9%** | | `SplatModelAdapter.ts` (modified) | **100%** | **100%** | **100%** | **100%** | | `SceneModelManager.ts` (modified) | 75.4% | 67.2% | 72.2% | 75.4% | | `Load3d.ts` (modified) | 29.5% | 30.6% | 26.7% | 30.1% | | `createLoad3d.ts` (new) | 83.8% | **100%** | 58.3% | 83.8% | | `useLoad3d.ts` (modified) | 78.2% | 65.1% | 71.4% | 82.2% | | `useLoad3dViewer.ts` (modified) | 75.2% | 52.1% | 65.9% | 79.4% | `SplatModelAdapter.ts` jumps to 100% via 6 new tests covering the orientation set, the `await initialized` decoder wait, `computeBounds` (world-space transform + null fallback), `disposeModel` (per-SplatMesh dispose + no-op on non-splat trees), and `defaultCameraPose`. `createLoad3d.ts` hits 100% branch via a new test file with 12 cases — `WebGLRenderer` config, `Load3DOptions` forwarding, `AdapterRef` identity between `LoaderManager` and `SceneModelManager`, and the 4 capability lambdas in both adapter-null and adapter-published states (each delegates correctly to the adapter's optional methods or falls back to defaults). The remaining func% reflects the inline `gizmoTransformChange` callback — not a deliberate skip, just out of scope for the dispatch-wiring tests. `SceneModelManager.ts` and `Load3d.ts` numbers are the pre-existing baseline — the existing `*.test.ts` files cover façade methods via prototype injection rather than instantiating the classes (`Load3d` constructor needs `THREE.WebGLRenderer`, which happy-dom can't provide; `SceneModelManager` covers the new capability paths via its existing `createManager(overrides)` helper). All new branches (capability gating, capability-driven `setupModel` / `fitToViewer` / rebuild, adapter-driven `isSplatModel` / `isPlyModel`) have dedicated tests. Net diff: **+846 / −370** across 16 files (10 production, 6 test). ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11660-refactor-load3d-drive-viewer-behavior-from-ModelAdapter-capabilities-34f6d73d36508130b0ece884add182b9) by [Unito](https://www.unito.io)
189 lines
5.8 KiB
TypeScript
189 lines
5.8 KiB
TypeScript
import * as THREE from 'three'
|
|
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
|
|
|
import * as ModelAdapterModule from './ModelAdapter'
|
|
import type { ModelLoadContext } from './ModelAdapter'
|
|
import { SplatModelAdapter } from './SplatModelAdapter'
|
|
|
|
const splatMeshSpies = {
|
|
ctor: vi.fn<(opts: { fileBytes: ArrayBuffer }) => void>(),
|
|
dispose: vi.fn(),
|
|
getBoundingBox: vi.fn(
|
|
() =>
|
|
new THREE.Box3(new THREE.Vector3(-1, -1, -1), new THREE.Vector3(1, 1, 1))
|
|
),
|
|
updateWorldMatrix: vi.fn()
|
|
}
|
|
|
|
vi.mock('@sparkjsdev/spark', async () => {
|
|
const three = await import('three')
|
|
return {
|
|
SplatMesh: class extends three.Object3D {
|
|
initialized = Promise.resolve()
|
|
dispose = splatMeshSpies.dispose
|
|
getBoundingBox = splatMeshSpies.getBoundingBox
|
|
|
|
constructor(opts: { fileBytes: ArrayBuffer }) {
|
|
super()
|
|
splatMeshSpies.ctor(opts)
|
|
}
|
|
|
|
override updateWorldMatrix(
|
|
force: boolean,
|
|
updateChildren: boolean
|
|
): void {
|
|
splatMeshSpies.updateWorldMatrix(force, updateChildren)
|
|
super.updateWorldMatrix(force, updateChildren)
|
|
}
|
|
}
|
|
}
|
|
})
|
|
|
|
function makeContext(): ModelLoadContext {
|
|
return {
|
|
setOriginalModel: vi.fn(),
|
|
registerOriginalMaterial: vi.fn(),
|
|
standardMaterial: new THREE.MeshStandardMaterial(),
|
|
materialMode: 'original'
|
|
}
|
|
}
|
|
|
|
describe('SplatModelAdapter', () => {
|
|
beforeEach(() => {
|
|
splatMeshSpies.ctor.mockClear()
|
|
splatMeshSpies.dispose.mockClear()
|
|
splatMeshSpies.getBoundingBox.mockClear()
|
|
splatMeshSpies.updateWorldMatrix.mockClear()
|
|
vi.spyOn(ModelAdapterModule, 'fetchModelData').mockResolvedValue(
|
|
new ArrayBuffer(8)
|
|
)
|
|
})
|
|
|
|
it('exposes splat capabilities on the adapter', () => {
|
|
const adapter = new SplatModelAdapter()
|
|
expect(adapter.kind).toBe('splat')
|
|
expect(adapter.capabilities.lighting).toBe(false)
|
|
expect(adapter.capabilities.exportable).toBe(false)
|
|
expect([...adapter.capabilities.materialModes]).toEqual([])
|
|
})
|
|
|
|
it('handles the Gaussian splat extensions', () => {
|
|
const adapter = new SplatModelAdapter()
|
|
expect([...adapter.extensions]).toEqual(['spz', 'splat', 'ksplat'])
|
|
})
|
|
|
|
it('fetches the file, builds a SplatMesh, and wraps it in a Group', async () => {
|
|
const buf = new ArrayBuffer(128)
|
|
vi.spyOn(ModelAdapterModule, 'fetchModelData').mockResolvedValue(buf)
|
|
|
|
const adapter = new SplatModelAdapter()
|
|
const ctx = makeContext()
|
|
|
|
const result = await adapter.load(ctx, '/api/view?', 'scene.splat')
|
|
|
|
expect(ModelAdapterModule.fetchModelData).toHaveBeenCalledWith(
|
|
'/api/view?',
|
|
'scene.splat'
|
|
)
|
|
expect(splatMeshSpies.ctor).toHaveBeenCalledWith({ fileBytes: buf })
|
|
expect(result).toBeInstanceOf(THREE.Group)
|
|
expect(result.children).toHaveLength(1)
|
|
|
|
expect(ctx.setOriginalModel).toHaveBeenCalledTimes(1)
|
|
expect(ctx.setOriginalModel).toHaveBeenCalledWith(result.children[0])
|
|
})
|
|
|
|
it('rotates the splat 180° around X (OpenCV → three.js convention)', async () => {
|
|
const result = await new SplatModelAdapter().load(
|
|
makeContext(),
|
|
'/api/view?',
|
|
'scene.splat'
|
|
)
|
|
|
|
const splat = result.children[0]
|
|
expect(splat.quaternion.x).toBe(1)
|
|
expect(splat.quaternion.y).toBe(0)
|
|
expect(splat.quaternion.z).toBe(0)
|
|
expect(splat.quaternion.w).toBe(0)
|
|
})
|
|
|
|
it('propagates fetch errors', async () => {
|
|
vi.spyOn(ModelAdapterModule, 'fetchModelData').mockRejectedValue(
|
|
new Error('Failed to fetch model: 500')
|
|
)
|
|
|
|
const adapter = new SplatModelAdapter()
|
|
await expect(
|
|
adapter.load(makeContext(), '/api/view?', 'scene.splat')
|
|
).rejects.toThrow('Failed to fetch model: 500')
|
|
})
|
|
|
|
describe('computeBounds', () => {
|
|
it('returns the SplatMesh bounding box transformed to world space', async () => {
|
|
const adapter = new SplatModelAdapter()
|
|
const group = await adapter.load(
|
|
makeContext(),
|
|
'/api/view?',
|
|
'scene.splat'
|
|
)
|
|
const splat = group.children[0]
|
|
splat.position.set(10, 0, 0)
|
|
|
|
const bounds = adapter.computeBounds(group)
|
|
|
|
expect(bounds).toBeInstanceOf(THREE.Box3)
|
|
expect(splatMeshSpies.getBoundingBox).toHaveBeenCalledWith(false)
|
|
expect(splatMeshSpies.updateWorldMatrix).toHaveBeenCalledWith(true, false)
|
|
// Local bbox was [-1,-1,-1]→[1,1,1]; world matrix translates by +10 X
|
|
// (with the splat's quaternion applied to the inner mesh).
|
|
expect(bounds!.min.x).toBeCloseTo(9)
|
|
expect(bounds!.max.x).toBeCloseTo(11)
|
|
})
|
|
|
|
it('returns null when the first child is not a SplatMesh', () => {
|
|
const adapter = new SplatModelAdapter()
|
|
const group = new THREE.Group()
|
|
group.add(new THREE.Mesh())
|
|
|
|
expect(adapter.computeBounds(group)).toBeNull()
|
|
})
|
|
})
|
|
|
|
describe('disposeModel', () => {
|
|
it('calls dispose on every SplatMesh in the model tree', async () => {
|
|
const adapter = new SplatModelAdapter()
|
|
const group = await adapter.load(
|
|
makeContext(),
|
|
'/api/view?',
|
|
'scene.splat'
|
|
)
|
|
|
|
adapter.disposeModel(group)
|
|
|
|
expect(splatMeshSpies.dispose).toHaveBeenCalledOnce()
|
|
})
|
|
|
|
it('is a no-op when the tree has no SplatMesh', () => {
|
|
const adapter = new SplatModelAdapter()
|
|
const group = new THREE.Group()
|
|
group.add(new THREE.Mesh())
|
|
|
|
expect(() => adapter.disposeModel(group)).not.toThrow()
|
|
})
|
|
})
|
|
|
|
describe('defaultCameraPose', () => {
|
|
it('returns the (5,5,5) / (0,2.5,0) seat for self-sized splats', () => {
|
|
const adapter = new SplatModelAdapter()
|
|
const pose = adapter.defaultCameraPose()
|
|
|
|
expect(pose.size.x).toBe(5)
|
|
expect(pose.size.y).toBe(5)
|
|
expect(pose.size.z).toBe(5)
|
|
expect(pose.center.x).toBe(0)
|
|
expect(pose.center.y).toBe(2.5)
|
|
expect(pose.center.z).toBe(0)
|
|
})
|
|
})
|
|
})
|