[refactor] refactor load3d #5765

jtydhr88 · 2025-09-25T01:57:37Z

Summary

Fully Refactored the Load3D module to improve architecture and maintainability by consolidating functionality into a
centralized composable pattern and simplifying component structure. and support VueNodes system

Changes

Architecture: Introduced new useLoad3d composable to centralize 3D loading logic and state
management
Component Simplification: Removed redundant components (Load3DAnimation.vue, Load3DAnimationScene.vue,
PreviewManager.ts)
Support VueNodes
improve config store
remove lineart output due Animation doesnot support it, may add it back later
remove Preview screen and keep scene in fixed ratio in load3d (not affect preview3d)
improve record video feature which will already record video by same ratio as scene
Need BE change Merge 3d animation node comfyanonymous/ComfyUI#10025

2025-09-25.22-33-36.mp4

┆Issue is synchronized with this Notion page by Unito

github-actions · 2025-09-25T01:57:58Z

🎭 Playwright Test Results

⚠️ Tests passed with flaky tests

⏰ Completed at: 10/08/2025, 07:06:42 PM UTC

📈 Summary

Total Tests: 488
Passed: 455 ✅
Failed: 0
Flaky: 3 ⚠️
Skipped: 30 ⏭️

📊 Test Reports by Browser

✅ chromium: View Report • ✅ 446 / ❌ 0 / ⚠️ 3 / ⏭️ 30
✅ chromium-2x: View Report • ✅ 2 / ❌ 0 / ⚠️ 0 / ⏭️ 0
✅ chromium-0.5x: View Report • ✅ 1 / ❌ 0 / ⚠️ 0 / ⏭️ 0
✅ mobile-chrome: View Report • ✅ 6 / ❌ 0 / ⚠️ 0 / ⏭️ 0

🎉 Click on the links above to view detailed test results for each browser configuration.

src/components/graph/GraphCanvas.vue

…el events (#5821) ## Summary Add generic wheel event capture mechanism for interactive widgets in vueNodes system to prevent event bubbling to canvas. ## Changes - What: Add event handling logic in LGraphNode.vue and GraphCanvas.vue that checks for data-capture-wheel attribute to determine whether wheel events should be forwarded to the canvas - How it works: Components that need to capture wheel events (like Three.js scenes) can add data-capture-wheel="true" attribute to prevent wheel events from bubbling up to the canvas zoom handler prerequirist for #5765 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-5821-Check-if-the-wheel-event-is-from-an-element-that-wants-to-capture-wheel-events-27b6d73d3650812493b5f13849147e6c) by [Unito](https://www.unito.io)

christian-byrne · 2025-09-30T17:54:18Z

Could you fix the merge conflict then rebase with main?

github-actions · 2025-09-30T23:44:36Z

🎨 Storybook Build Status

✅ Build completed successfully!

⏰ Completed at: 10/08/2025, 06:47:44 PM UTC

🔗 Links

📊 View Workflow Run
🎨 View Storybook

🎉 Your Storybook is ready for review!

src/composables/useLoad3d.ts

src/components/load3d/Load3D.vue

src/components/load3d/Load3DScene.vue

src/extensions/core/load3d.ts

src/services/load3dService.ts

tests-ui/tests/composables/useLoad3d.test.ts

## Summary pass nodeId to widget component, which is a prerequirist for #5765 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-5853-pass-nodeData-to-widget-component-27e6d73d3650811e9e48ceb7c3a00545) by [Unito](https://www.unito.io)

christian-byrne · 2025-10-04T04:25:21Z

src/composables/useLoad3dViewer.ts

+      const widthWidget = node.widgets?.find((w) => w.name === 'width')
+      const heightWidget = node.widgets?.find((w) => w.name === 'height')
+      if (!(widthWidget && heightWidget)) {
+        isPreview.value = true
+      }


question: what is the logic for this? If there is not a width and a height widget, then it means it's a preview node?

Yes, and it will be used in, for example, if open 3d viewer from preview3D, drag and drop feature will be disabled.

What is the heuristic? Why is it the case that, if there is no width and no height, it means it's a preview widget? It seems a bit hard to understand.

Width and height refer to the output size in load3d. They only exist in load3d, not in preview3d. However, since load3d and preview3d now share this part of the logic, and certain features (such as drag and drop file) only work for load3d, I need to use them to identify whether the source is load3d or preview3d to differentiate between them.

Why not just use the node type? You already have a reference to the node. This current setup seems hard to maintain - what happens if you want to change the name of the width widget in 6 months? Will you remember that it's going to break this logic?

good catch, yes, we can. Just updated

…el events (#5821) ## Summary Add generic wheel event capture mechanism for interactive widgets in vueNodes system to prevent event bubbling to canvas. ## Changes - What: Add event handling logic in LGraphNode.vue and GraphCanvas.vue that checks for data-capture-wheel attribute to determine whether wheel events should be forwarded to the canvas - How it works: Components that need to capture wheel events (like Three.js scenes) can add data-capture-wheel="true" attribute to prevent wheel events from bubbling up to the canvas zoom handler prerequirist for #5765 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-5821-Check-if-the-wheel-event-is-from-an-element-that-wants-to-capture-wheel-events-27b6d73d3650812493b5f13849147e6c) by [Unito](https://www.unito.io)

## Summary pass nodeId to widget component, which is a prerequirist for #5765 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-5853-pass-nodeData-to-widget-component-27e6d73d3650811e9e48ceb7c3a00545) by [Unito](https://www.unito.io)

…el events (#5821) ## Summary Add generic wheel event capture mechanism for interactive widgets in vueNodes system to prevent event bubbling to canvas. ## Changes - What: Add event handling logic in LGraphNode.vue and GraphCanvas.vue that checks for data-capture-wheel attribute to determine whether wheel events should be forwarded to the canvas - How it works: Components that need to capture wheel events (like Three.js scenes) can add data-capture-wheel="true" attribute to prevent wheel events from bubbling up to the canvas zoom handler prerequirist for #5765 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-5821-Check-if-the-wheel-event-is-from-an-element-that-wants-to-capture-wheel-events-27b6d73d3650812493b5f13849147e6c) by [Unito](https://www.unito.io)

## Summary pass nodeId to widget component, which is a prerequirist for #5765 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-5853-pass-nodeData-to-widget-component-27e6d73d3650811e9e48ceb7c3a00545) by [Unito](https://www.unito.io)

github-actions · 2025-10-08T18:36:12Z

🔧 Auto-fixes Applied

This PR has been automatically updated to fix linting and formatting issues.

⚠️ Important: Your local branch is now behind. Run git pull before making additional changes to avoid conflicts.

Changes made:

ESLint auto-fixes
Prettier formatting

christian-byrne

I have reviewed 14/42 files, taking a quick break.

src/components/load3d/controls/CameraControls.vue

src/components/load3d/controls/LightControls.vue

christian-byrne · 2025-10-08T18:08:32Z

src/components/load3d/Load3DControls.vue

+        v-if="activeCategory === 'scene' && sceneConfig"
        ref="sceneControlsRef"
-        :background-color="backgroundColor"
-        :show-grid="showGrid"
-        :has-background-image="hasBackgroundImage"
-        @toggle-grid="handleToggleGrid"
-        @update-background-color="handleBackgroundColorChange"
+        v-model:show-grid="sceneConfig.showGrid"
+        v-model:background-color="sceneConfig.backgroundColor"
+        v-model:background-image="sceneConfig.backgroundImage"
        @update-background-image="handleBackgroundImageUpdate"
      />

      <ModelControls
-        v-if="activeCategory === 'model'"
+        v-if="activeCategory === 'model' && modelConfig"
        ref="modelControlsRef"
-        :input-spec="inputSpec"
-        :up-direction="upDirection"
-        :material-mode="materialMode"
-        :edge-threshold="edgeThreshold"
-        @update-up-direction="handleUpdateUpDirection"
-        @update-material-mode="handleUpdateMaterialMode"
-        @update-edge-threshold="handleUpdateEdgeThreshold"
+        v-model:material-mode="modelConfig.materialMode"
+        v-model:up-direction="modelConfig.upDirection"
      />

      <CameraControls
-        v-if="activeCategory === 'camera'"
+        v-if="activeCategory === 'camera' && cameraConfig"
        ref="cameraControlsRef"
-        :camera-type="cameraType"
-        :fov="fov"
-        :show-f-o-v-button="showFOVButton"
-        @switch-camera="switchCamera"
-        @update-f-o-v="handleUpdateFOV"
+        v-model:camera-type="cameraConfig.cameraType"
+        v-model:fov="cameraConfig.fov"
      />

      <LightControls
-        v-if="activeCategory === 'light'"
+        v-if="activeCategory === 'light' && lightConfig && modelConfig"


for the v-if on the Controls, you can use computed to prevent re-render on every change of activeCategory and config.

christian-byrne · 2025-10-08T18:11:24Z

src/components/load3d/Load3dViewerContent.vue

+// Drag and drop state
+const isDragging = ref(false)
+const dragMessage = ref('')
+const SUPPORTED_EXTENSIONS = ['.gltf', '.glb', '.obj', '.fbx', '.stl']


question: we are defining this in both Load3dViewerContent and Load3DScene, are they always going to be the same? If so, can we put it in a constants file that both places import from?

christian-byrne · 2025-10-08T18:15:31Z

src/components/load3d/Load3dViewerContent.vue

+function handleDragOver(event: DragEvent) {
+  if (viewer.isPreview.value) return
+
+  if (!event.dataTransfer) return
+
+  const hasFiles = event.dataTransfer.types.includes('Files')
+
+  if (!hasFiles) return
+
+  isDragging.value = true
+
+  event.dataTransfer.dropEffect = 'copy'
+  dragMessage.value = t('load3d.dropToLoad')
+}
+
+function handleDragLeave() {
+  isDragging.value = false
+}
+
+async function handleDrop(event: DragEvent) {
+  isDragging.value = false
+
+  if (viewer.isPreview.value) return
+
+  if (!event.dataTransfer) return
+
+  const files = Array.from(event.dataTransfer.files)
+
+  if (files.length === 0) return
+
+  const modelFile = files.find(isValidModelFile)
+
+  if (modelFile) {
+    await viewer.handleModelDrop(modelFile)
+  } else {
+    useToastStore().addAlert(t('load3d.unsupportedFileType'))
+  }
+}
+


suggestion: This seems like something that could be moved into a hook and re-used in other components.

christian-byrne · 2025-10-08T18:24:33Z

src/composables/useLoad3d.ts

+
+    try {
+      load3d = new Load3d(containerRef, {
+        node: node as LGraphNode


question: is this type assertion still necessary if we add null check above? If so, can we use generic toRaw<LGraphNode>?

Same question for all the below type assertions as LGraphNode as well.

christian-byrne · 2025-10-08T18:26:09Z

src/composables/useLoad3d.ts

+      }
+
+      node.onRemoved = function () {
+        useLoad3dService().removeLoad3d(node as LGraphNode)


Don't we also need to clear entries in nodeToLoad3dMap and pendingCallbacks here?

christian-byrne · 2025-10-08T18:29:11Z

src/composables/useLoad3d.ts

+    }
+  }
+
+  const restoreConfigurationsFromNode = async (node: LGraphNode) => {


nit (non-blocking): all of the logic revolving around serialization, config management, config types, and restoring from configs can likely be moved to its own module.

christian-byrne · 2025-10-08T18:32:13Z

src/composables/useLoad3d.ts

+    if (modelWidget?.value) {
+      const modelUrl = getModelUrl(modelWidget.value as string)
+      if (modelUrl) {
+        loading.value = true
+        loadingMessage.value = t('load3d.reloadingModel')
+        try {
+          await load3d.loadModel(modelUrl)
+
+          if (cameraStateToRestore) {
+            await nextTick()
+            load3d.setCameraState(cameraStateToRestore)
+          }
+        } catch (error) {
+          console.error('Failed to reload model:', error)
+          useToastStore().addAlert(t('toastMessages.failedToLoadModel'))
+        } finally {
+          loading.value = false
+          loadingMessage.value = ''
+        }
+      }
+    } else if (cameraStateToRestore) {
+      load3d.setCameraState(cameraStateToRestore)
+    }
+  }


nit (non-blocking, suggestion for future): We should consider whether this needs to be blocking setup. Currently, we are synchronously restoring the model config, get the url, loading the model, setting the camera state - and everything else must wait on this to happen. I believe we could refactor a bit and have this happen in the background like with useAsyncState

christian-byrne · 2025-10-08T18:36:35Z

src/composables/useLoad3d.ts

+    pendingCallbacks.get(node as LGraphNode)!.push(callback)
+  }
+
+  watch(


question: it seems that we have these watchers which will dynamically handle updates to the config objects. But we also have the same logic (as far as I can tell) repeated in restoreConfigurationsFromNode. Is that correct?

For example, this watcher already handles updates to the CameraConfig:

watch( cameraConfig, (newValue) => { if (load3d && nodeRef.value) { nodeRef.value.properties['Camera Config'] = newValue load3d.toggleCamera(newValue.cameraType) load3d.setFOV(newValue.fov) } }, { deep: true } )

But we are also doing the same thing in the restoreConfigurationsFromNode:

const savedCameraConfig = node.properties['Camera Config'] as CameraConfig const cameraStateToRestore = savedCameraConfig?.state // Apply camera type and FOV immediately if (savedCameraConfig) { cameraConfig.value = savedCameraConfig load3d.toggleCamera(savedCameraConfig.cameraType) load3d.setFOV(savedCameraConfig.fov) }

Shouldn't this line

cameraConfig.value = savedCameraConfig

actually be triggering that watch, making the next two lines redundant?

jtydhr88 requested review from a team, KarryCharon, Yorha4D and shinshin86 as code owners September 25, 2025 01:57

jtydhr88 marked this pull request as draft September 25, 2025 01:57

dosubot bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label Sep 25, 2025

jtydhr88 force-pushed the refactor-load3d-2 branch 3 times, most recently from 5b73fbc to 40f4e5d Compare September 26, 2025 02:36

jtydhr88 marked this pull request as ready for review September 26, 2025 02:42

christian-byrne reviewed Sep 26, 2025

View reviewed changes

src/components/graph/GraphCanvas.vue Outdated Show resolved Hide resolved

This was referenced Sep 27, 2025

Check if the wheel event is from an element that wants to capture wheel events #5821

Merged

pass nodeId to widget component #5853

Merged

christian-byrne added claude-review Add to trigger a PR code review from Claude Code and removed claude-review Add to trigger a PR code review from Claude Code labels Sep 30, 2025

jtydhr88 force-pushed the refactor-load3d-2 branch from 40f4e5d to 61da21c Compare September 30, 2025 23:44

christian-byrne added the claude-review Add to trigger a PR code review from Claude Code label Oct 1, 2025