[refactor] refactor load3d #5765
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jtydhr88
requested review from
a team,
KarryCharon,
Yorha4D and
shinshin86
as code owners
dosubot
bot
added
the
size:XXL
🎭 Playwright Test Results⏰ Completed at: 10/31/2025, 02:19:50 PM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
jtydhr88
force-pushed
the
refactor-load3d-2
branch
3 times, most recently
from
5b73fbc to
40f4e5d
Compare
This was referenced Sep 27, 2025
christian-byrne
pushed a commit
that referenced
this pull request
Sep 30, 2025
…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
added
claude-review
|
Could you fix the merge conflict then rebase with |
jtydhr88
force-pushed
the
refactor-load3d-2
branch
from
40f4e5d to
61da21c
Compare
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 10/31/2025, 01:55:54 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
christian-byrne
added
the
claude-review
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
jtydhr88
force-pushed
the
refactor-load3d-2
branch
from
61da21c to
8ed52a5
Compare
This was referenced Oct 2, 2025
jtydhr88
added a commit
that referenced
this pull request
Oct 2, 2025
## 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)
jtydhr88
force-pushed
the
refactor-load3d-2
branch
from
8ed52a5 to
13c8a5b
Compare
christian-byrne
pushed a commit
that referenced
this pull request
Oct 6, 2025
…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
pushed a commit
that referenced
this pull request
Oct 6, 2025
## 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)
arjansingh
pushed a commit
that referenced
this pull request
Oct 7, 2025
…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)
arjansingh
pushed a commit
that referenced
this pull request
Oct 7, 2025
## 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)
jtydhr88
force-pushed
the
refactor-load3d-2
branch
from
7bff100 to
8fb9751
Compare
🔧 Auto-fixes AppliedThis PR has been automatically updated to fix linting and formatting issues.
Changes made:
|
christian-byrne
requested changes
Oct 8, 2025
| } | ||
| } | ||
|
|
||
| const restoreConfigurationsFromNode = async (node: LGraphNode) => { |
There was a problem hiding this comment.
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.
Comment on lines
+168
to
+178
| 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) | ||
| } | ||
| } |
There was a problem hiding this comment.
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
1 task
jtydhr88
force-pushed
the
refactor-load3d-2
branch
from
5587e4f to
2577068
Compare
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 3.26 MB (baseline 3.33 MB) • 🟢 -63 kBMain entry bundles and manifests
Status: 2 added / 2 removed Graph Workspace — 724 kB (baseline 724 kB) • 🔴 +41 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 8.18 kB (baseline 8.14 kB) • 🔴 +37 BTop-level views, pages, and routed surfaces
Status: 1 added / 1 removed Panels & Settings — 295 kB (baseline 294 kB) • 🔴 +222 BConfiguration panels, inspectors, and settings screens
Status: 6 added / 6 removed UI Components — 12.3 kB (baseline 12.3 kB) • 🔴 +37 BReusable component library chunks
Status: 1 added / 1 removed Data & Services — 10 kB (baseline 10 kB) • ⚪ 0 BStores, services, APIs, and repositories
Status: 2 added / 2 removed Utilities & Hooks — 1.07 kB (baseline 1.07 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Vendor & Third-Party — 5.32 MB (baseline 5.36 MB) • 🟢 -36.9 kBExternal libraries and shared vendor chunks
Status: 4 added / 4 removed Other — 2.55 MB (baseline 2.55 MB) • ⚪ 0 BBundles that do not match a named category
|
jtydhr88
force-pushed
the
refactor-load3d-2
branch
from
2577068 to
f099c15
Compare
jtydhr88
force-pushed
the
refactor-load3d-2
branch
3 times, most recently
from
51742aa to
2b79dfb
Compare
jtydhr88
force-pushed
the
refactor-load3d-2
branch
from
2b79dfb to
dc31a71
Compare
christian-byrne
approved these changes
Oct 31, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
management
PreviewManager.ts)
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