Skip to content
Closed
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 43 additions & 12 deletions src/composables/useLoad3d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,29 @@ export const useLoad3d = (nodeOrRef: MaybeRef<LGraphNode | null>) => {
}

handleEvents('add')

const meshWidget = node.widgets?.find((w) => w.name === 'model_file')
if (meshWidget) {
const originalCallback = meshWidget.callback
meshWidget.callback = (v: any, ...args: any[]) => {
originalCallback?.(v, ...args)
const meshUrl = getMeshUrl(v)
if (meshUrl) {
loading.value = true
loadingMessage.value = t('load3d.loadingModel')
load3d
?.loadModel(meshUrl)
.catch((error) => {
console.error('Failed to reload model:', error)
useToastStore().addAlert(t('toastMessages.failedToLoadModel'))
})
.finally(() => {
loading.value = false
loadingMessage.value = ''
})
}
}
}
} catch (error) {
console.error('Error initializing Load3d:', error)
useToastStore().addAlert(t('toastMessages.failedToInitializeLoad3d'))
Expand Down Expand Up @@ -178,14 +201,14 @@ export const useLoad3d = (nodeOrRef: MaybeRef<LGraphNode | null>) => {
lightConfig.value = savedLightConfig
}

const modelWidget = node.widgets?.find((w) => w.name === 'model_file')
if (modelWidget?.value) {
const modelUrl = getModelUrl(modelWidget.value as string)
if (modelUrl) {
const meshWidget = node.widgets?.find((w) => w.name === 'model_file')
if (meshWidget?.value) {
const meshUrl = getMeshUrl(meshWidget.value as string)
if (meshUrl) {
loading.value = true
loadingMessage.value = t('load3d.reloadingModel')
try {
await load3d.loadModel(modelUrl)
await load3d.loadModel(meshUrl)

if (cameraStateToRestore) {
await nextTick()
Expand All @@ -204,24 +227,32 @@ export const useLoad3d = (nodeOrRef: MaybeRef<LGraphNode | null>) => {
}
}

const getModelUrl = (modelPath: string): string | null => {
if (!modelPath) return null
const getMeshUrl = (meshPath: string): string | null => {
if (!meshPath) return null

try {
if (modelPath.startsWith('http')) {
return modelPath
if (meshPath.startsWith('http')) {
return meshPath
}

const [subfolder, filename] = Load3dUtils.splitFilePath(modelPath)
let cleanPath = meshPath
let forcedType: 'output' | 'input' | undefined

if (meshPath.trim().endsWith('[output]')) {
cleanPath = meshPath.replace(/\s*\[output\]$/, '')
forcedType = 'output'
}

const [subfolder, filename] = Load3dUtils.splitFilePath(cleanPath)
return api.apiURL(
Load3dUtils.getResourceURL(
subfolder,
filename,
isPreview.value ? 'output' : 'input'
forcedType ?? (isPreview.value ? 'output' : 'input')
)
)
} catch (error) {
console.error('Failed to construct model URL:', error)
console.error('Error getting model URL:', error)
return null
}
}
Expand Down
40 changes: 20 additions & 20 deletions src/extensions/core/load3d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ const inputSpecPreview3D: CustomInputSpec = {
async function handleModelUpload(files: FileList, node: any) {
if (!files?.length) return

const modelWidget = node.widgets?.find(
const meshWidget = node.widgets?.find(
(w: any) => w.name === 'model_file'
) as IStringWidget

Expand All @@ -53,7 +53,7 @@ async function handleModelUpload(files: FileList, node: any) {
return
}

const modelUrl = api.apiURL(
const meshUrl = api.apiURL(
Load3dUtils.getResourceURL(
...Load3dUtils.splitFilePath(uploadPath),
'input'
Expand All @@ -62,18 +62,18 @@ async function handleModelUpload(files: FileList, node: any) {

useLoad3d(node).waitForLoad3d((load3d) => {
try {
load3d.loadModel(modelUrl)
load3d.loadModel(meshUrl)
} catch (error) {
useToastStore().addAlert(t('toastMessages.failedToLoadModel'))
}
})

if (uploadPath && modelWidget) {
if (!modelWidget.options?.values?.includes(uploadPath)) {
modelWidget.options?.values?.push(uploadPath)
if (uploadPath && meshWidget) {
if (!meshWidget.options?.values?.includes(uploadPath)) {
meshWidget.options?.values?.push(uploadPath)
}

modelWidget.value = uploadPath
meshWidget.value = uploadPath
}
} catch (error) {
console.error('Model upload failed:', error)
Expand Down Expand Up @@ -285,9 +285,9 @@ useExtensionService().registerExtension({
load3d.clearModel()
})

const modelWidget = node.widgets?.find((w) => w.name === 'model_file')
if (modelWidget) {
modelWidget.value = ''
const meshWidget = node.widgets?.find((w) => w.name === 'model_file')
if (meshWidget) {
meshWidget.value = ''
}
})

Expand Down Expand Up @@ -335,15 +335,15 @@ useExtensionService().registerExtension({

const config = new Load3DConfiguration(load3d, node.properties)

const modelWidget = node.widgets?.find((w) => w.name === 'model_file')
const meshWidget = node.widgets?.find((w) => w.name === 'model_file')
const width = node.widgets?.find((w) => w.name === 'width')
const height = node.widgets?.find((w) => w.name === 'height')
const sceneWidget = node.widgets?.find((w) => w.name === 'image')

if (modelWidget && width && height && sceneWidget) {
if (meshWidget && width && height && sceneWidget) {
const settings = {
loadFolder: 'input',
modelWidget: modelWidget,
meshWidget: meshWidget,
cameraState: cameraState,
width: width,
height: height
Expand Down Expand Up @@ -464,20 +464,20 @@ useExtensionService().registerExtension({
useLoad3d(node).waitForLoad3d((load3d) => {
const config = new Load3DConfiguration(load3d, node.properties)

const modelWidget = node.widgets?.find((w) => w.name === 'model_file')
const meshWidget = node.widgets?.find((w) => w.name === 'model_file')

if (modelWidget) {
if (meshWidget) {
const lastTimeModelFile = node.properties['Last Time Model File']

if (lastTimeModelFile) {
modelWidget.value = lastTimeModelFile
meshWidget.value = lastTimeModelFile

const cameraConfig = node.properties['Camera Config'] as any
const cameraState = cameraConfig?.state

const settings = {
loadFolder: 'output',
modelWidget: modelWidget,
meshWidget: meshWidget,
cameraState: cameraState
}

Expand All @@ -498,13 +498,13 @@ useExtensionService().registerExtension({
let cameraState = message.result[1]
let bgImagePath = message.result[2]

modelWidget.value = filePath.replaceAll('\\', '/')
meshWidget.value = filePath.replaceAll('\\', '/')

node.properties['Last Time Model File'] = modelWidget.value
node.properties['Last Time Model File'] = meshWidget.value

const settings = {
loadFolder: 'output',
modelWidget: modelWidget,
meshWidget: meshWidget,
cameraState: cameraState,
bgImagePath: bgImagePath
}
Expand Down
65 changes: 43 additions & 22 deletions src/extensions/core/load3d/Load3DConfiguration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { api } from '@/scripts/api'

type Load3DConfigurationSettings = {
loadFolder: string
modelWidget: IBaseWidget
meshWidget: IBaseWidget
cameraState?: CameraState
width?: IBaseWidget
height?: IBaseWidget
Expand All @@ -29,16 +29,37 @@ class Load3DConfiguration {
) {}

configureForSaveMesh(loadFolder: 'input' | 'output', filePath: string) {
this.setupModelHandlingForSaveMesh(filePath, loadFolder)
this.setupMeshHandlingForSaveMesh(filePath, loadFolder)
this.setupDefaultProperties()
}

configure(setting: Load3DConfigurationSettings) {
this.setupModelHandling(
setting.modelWidget,
this.setupMeshHandling(
setting.meshWidget,
setting.loadFolder,
setting.cameraState
)

if (setting.meshWidget.options?.values) {
let values = setting.meshWidget.options.values as string[]

try {
const stored = localStorage.getItem('Comfy.Load3D.HiddenFiles')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Extract magic constants for maintainability.

The hard-coded localStorage key 'Comfy.Load3D.HiddenFiles' and the magic number 12 should be extracted as named constants at the class or module level for better maintainability and consistency.

🔎 Proposed refactor

Add constants at the top of the class:

class Load3DConfiguration {
  private static readonly HIDDEN_FILES_STORAGE_KEY = 'Comfy.Load3D.HiddenFiles'
  private static readonly MAX_MESH_OPTIONS = 12

  // ... rest of the class

Then use them in the configure method:

     try {
-      const stored = localStorage.getItem('Comfy.Load3D.HiddenFiles')
+      const stored = localStorage.getItem(Load3DConfiguration.HIDDEN_FILES_STORAGE_KEY)
       const hiddenFiles = stored ? JSON.parse(stored) : []
       if (hiddenFiles.length > 0) {
         values = values.filter((v) => !hiddenFiles.includes(v))
       }
     } catch (e) {
       console.error('Failed to read hidden files from localStorage', e)
     }

-    if (values.length > 12) {
-      values = values.slice(0, 12)
+    if (values.length > Load3DConfiguration.MAX_MESH_OPTIONS) {
+      values = values.slice(0, Load3DConfiguration.MAX_MESH_OPTIONS)
     }

Also applies to: 56-56

🤖 Prompt for AI Agents
In @src/extensions/core/load3d/Load3DConfiguration.ts around line 47, Extract
the magic values by adding named constants (e.g., HIDDEN_FILES_STORAGE_KEY and
MAX_MESH_OPTIONS) at the class or module level and replace the inline string
'Comfy.Load3D.HiddenFiles' and the numeric literal 12 with those constants;
update usages in Load3DConfiguration.configure (and any other occurrences in
this file) to reference Load3DConfiguration.HIDDEN_FILES_STORAGE_KEY and
Load3DConfiguration.MAX_MESH_OPTIONS (or module-scoped equivalents) for
maintainability and consistency.

const hiddenFiles = stored ? JSON.parse(stored) : []
if (hiddenFiles.length > 0) {
values = values.filter((v) => !hiddenFiles.includes(v))
}
} catch (e) {
console.error('Failed to read hidden files from localStorage', e)
}

if (values.length > 12) {
values = values.slice(0, 12)
}

setting.meshWidget.options.values = values
}
Comment on lines +43 to +61

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Add type guard before casting to string[].

The code casts setting.meshWidget.options.values to string[] without verifying that it's actually an array of strings. This could cause runtime errors if the values contain non-string elements.

🔎 Proposed fix with type guard
 if (setting.meshWidget.options?.values) {
-  let values = setting.meshWidget.options.values as string[]
+  const rawValues = setting.meshWidget.options.values
+  if (!Array.isArray(rawValues)) {
+    return
+  }
+  let values = rawValues.filter((v): v is string => typeof v === 'string')
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (setting.meshWidget.options?.values) {
let values = setting.meshWidget.options.values as string[]
try {
const stored = localStorage.getItem('Comfy.Load3D.HiddenFiles')
const hiddenFiles = stored ? JSON.parse(stored) : []
if (hiddenFiles.length > 0) {
values = values.filter((v) => !hiddenFiles.includes(v))
}
} catch (e) {
console.error('Failed to read hidden files from localStorage', e)
}
if (values.length > 12) {
values = values.slice(0, 12)
}
setting.meshWidget.options.values = values
}
if (setting.meshWidget.options?.values) {
const rawValues = setting.meshWidget.options.values
if (!Array.isArray(rawValues)) {
return
}
let values = rawValues.filter((v): v is string => typeof v === 'string')
try {
const stored = localStorage.getItem('Comfy.Load3D.HiddenFiles')
const hiddenFiles = stored ? JSON.parse(stored) : []
if (hiddenFiles.length > 0) {
values = values.filter((v) => !hiddenFiles.includes(v))
}
} catch (e) {
console.error('Failed to read hidden files from localStorage', e)
}
if (values.length > 12) {
values = values.slice(0, 12)
}
setting.meshWidget.options.values = values
}
🤖 Prompt for AI Agents
In @src/extensions/core/load3d/Load3DConfiguration.ts around lines 43-61, The
code casts setting.meshWidget.options.values to string[] without verifying its
shape; add a type guard to ensure it's an array of strings before using it
(e.g., check Array.isArray(setting.meshWidget.options.values) and filter
elements with typeof v === 'string'), fallback to an empty array if the guard
fails, then continue using the validated string[] for the localStorage
filtering/slicing and reassign back to setting.meshWidget.options.values.

🧹 Nitpick | 🔵 Trivial

Consider using immutable operations for filtering and slicing.

The current implementation mutates the values variable through reassignment. While this works, using immutable patterns would be more aligned with functional programming best practices and coding guidelines.

🔎 Proposed refactor using immutability
 if (setting.meshWidget.options?.values) {
-  let values = setting.meshWidget.options.values as string[]
+  const rawValues = setting.meshWidget.options.values
+  if (!Array.isArray(rawValues)) {
+    return
+  }
+  
+  const validValues = rawValues.filter((v): v is string => typeof v === 'string')

   try {
     const stored = localStorage.getItem('Comfy.Load3D.HiddenFiles')
     const hiddenFiles = stored ? JSON.parse(stored) : []
-    if (hiddenFiles.length > 0) {
-      values = values.filter((v) => !hiddenFiles.includes(v))
-    }
+    const filteredValues = hiddenFiles.length > 0
+      ? validValues.filter((v) => !hiddenFiles.includes(v))
+      : validValues
+    
+    const limitedValues = filteredValues.length > 12
+      ? filteredValues.slice(0, 12)
+      : filteredValues
+    
+    setting.meshWidget.options.values = limitedValues
   } catch (e) {
     console.error('Failed to read hidden files from localStorage', e)
+    setting.meshWidget.options.values = validValues.slice(0, 12)
   }
-
-  if (values.length > 12) {
-    values = values.slice(0, 12)
-  }
-
-  setting.meshWidget.options.values = values
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (setting.meshWidget.options?.values) {
let values = setting.meshWidget.options.values as string[]
try {
const stored = localStorage.getItem('Comfy.Load3D.HiddenFiles')
const hiddenFiles = stored ? JSON.parse(stored) : []
if (hiddenFiles.length > 0) {
values = values.filter((v) => !hiddenFiles.includes(v))
}
} catch (e) {
console.error('Failed to read hidden files from localStorage', e)
}
if (values.length > 12) {
values = values.slice(0, 12)
}
setting.meshWidget.options.values = values
}
if (setting.meshWidget.options?.values) {
const rawValues = setting.meshWidget.options.values
if (!Array.isArray(rawValues)) {
return
}
const validValues = rawValues.filter(
(v): v is string => typeof v === 'string'
)
try {
const stored = localStorage.getItem('Comfy.Load3D.HiddenFiles')
const hiddenFiles = stored ? JSON.parse(stored) : []
const filteredValues = hiddenFiles.length > 0
? validValues.filter((v) => !hiddenFiles.includes(v))
: validValues
const limitedValues = filteredValues.length > 12
? filteredValues.slice(0, 12)
: filteredValues
setting.meshWidget.options.values = limitedValues
} catch (e) {
console.error('Failed to read hidden files from localStorage', e)
setting.meshWidget.options.values = validValues.slice(0, 12)
}
}
🤖 Prompt for AI Agents
In @src/extensions/core/load3d/Load3DConfiguration.ts around lines 43-61, The
code mutates the local variable values via reassignment; change to an immutable
flow: keep the original array (e.g., originalValues =
setting.meshWidget.options.values as string[]), then compute a new array by
applying filter based on parsed hiddenFiles and then applying slice(0, 12) as
needed, and finally assign that new array to setting.meshWidget.options.values;
reference the symbol setting.meshWidget.options.values and the localStorage key
'Comfy.Load3D.HiddenFiles' when locating the logic to refactor.


this.setupTargetSize(setting.width, setting.height)
this.setupDefaultProperties(setting.bgImagePath)
}
Expand All @@ -57,46 +78,46 @@ class Load3DConfiguration {
}
}

private setupModelHandlingForSaveMesh(filePath: string, loadFolder: string) {
const onModelWidgetUpdate = this.createModelUpdateHandler(loadFolder)
private setupMeshHandlingForSaveMesh(filePath: string, loadFolder: string) {
const onMeshWidgetUpdate = this.createMeshUpdateHandler(loadFolder)

if (filePath) {
onModelWidgetUpdate(filePath)
onMeshWidgetUpdate(filePath)
}
}

private setupModelHandling(
modelWidget: IBaseWidget,
private setupMeshHandling(
meshWidget: IBaseWidget,
loadFolder: string,
cameraState?: CameraState
) {
const onModelWidgetUpdate = this.createModelUpdateHandler(
const onMeshWidgetUpdate = this.createMeshUpdateHandler(
loadFolder,
cameraState
)
if (modelWidget.value) {
onModelWidgetUpdate(modelWidget.value)
if (meshWidget.value) {
onMeshWidgetUpdate(meshWidget.value)
}

const originalCallback = modelWidget.callback
const originalCallback = meshWidget.callback

let currentValue = modelWidget.value
Object.defineProperty(modelWidget, 'value', {
let currentValue = meshWidget.value
Object.defineProperty(meshWidget, 'value', {
get() {
return currentValue
},
set(newValue) {
currentValue = newValue
if (modelWidget.callback && newValue !== undefined && newValue !== '') {
modelWidget.callback(newValue)
if (meshWidget.callback && newValue !== undefined && newValue !== '') {
meshWidget.callback(newValue)
}
},
enumerable: true,
configurable: true
})

modelWidget.callback = (value: string | number | boolean | object) => {
onModelWidgetUpdate(value)
meshWidget.callback = (value: string | number | boolean | object) => {
onMeshWidgetUpdate(value)

if (originalCallback) {
originalCallback(value)
Expand Down Expand Up @@ -194,7 +215,7 @@ class Load3DConfiguration {
this.load3d.setMaterialMode(config.materialMode)
}

private createModelUpdateHandler(
private createMeshUpdateHandler(
loadFolder: string,
cameraState?: CameraState
) {
Expand All @@ -206,14 +227,14 @@ class Load3DConfiguration {

this.setResourceFolder(filename)

const modelUrl = api.apiURL(
const meshUrl = api.apiURL(
Load3dUtils.getResourceURL(
...Load3dUtils.splitFilePath(filename),
loadFolder
)
)

await this.load3d.loadModel(modelUrl, filename)
await this.load3d.loadModel(meshUrl, filename)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add error handling for loadModel call.

The loadModel call on line 237 is awaited but not wrapped in error handling. If the model fails to load (e.g., network error, invalid file format, file not found), the error will propagate uncaught. Since this function is used as a callback handler, unhandled errors could lead to silent failures or uncaught promise rejections.

🔎 Proposed fix with error handling
-      await this.load3d.loadModel(meshUrl, filename)
+      try {
+        await this.load3d.loadModel(meshUrl, filename)
+      } catch (error) {
+        console.error('Failed to load 3D mesh:', error)
+        return
+      }
🤖 Prompt for AI Agents
In @src/extensions/core/load3d/Load3DConfiguration.ts around line 237, The
awaited call to this.load3d.loadModel(meshUrl, filename) can throw and is
currently unhandled; wrap the call in a try/catch inside the callback handler
where loadModel is invoked, catch any errors from this.load3d.loadModel(meshUrl,
filename), log or report the error (including meshUrl and filename) via the
existing logger or error handler, and then either return/propagate an
appropriate error result or rethrow after logging so callers don’t get an
unhandled rejection.


const modelConfig = this.loadModelConfig()
this.applyModelConfig(modelConfig)
Expand Down
Loading