Skip to content
Closed
Show file tree
Hide file tree
Changes from all 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
32 changes: 20 additions & 12 deletions src/composables/useLoad3d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -178,14 +178,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 +204,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
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
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
:allow-upload="allowUpload"
:upload-folder="uploadFolder"
:is-asset-mode="isAssetMode"
:upload-subfolder="uploadSubfolder"
:default-layout-mode="defaultLayoutMode"
/>
<WidgetWithControl
Expand Down Expand Up @@ -58,9 +59,13 @@ const specDescriptor = computed<{
kind: AssetKind
allowUpload: boolean
folder: ResultItemType | undefined
subfolder?: string
}>(() => {
const isLoad3DMesh =
props.nodeType === 'Load3D' && props.widget.name === 'model_file'
Comment on lines +64 to +65
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Consider extracting magic strings to constants.

The hardcoded 'Load3D' and 'model_file' strings work correctly but could become maintenance concerns if these identifiers change or are referenced elsewhere. Consider extracting them to named constants for better maintainability and searchability.

🔎 Suggested improvement
+const LOAD_3D_NODE_TYPE = 'Load3D'
+const MODEL_FILE_WIDGET_NAME = 'model_file'
+
 const specDescriptor = computed<{
   kind: AssetKind
   allowUpload: boolean
   folder: ResultItemType | undefined
   subfolder?: string
 }>(() => {
   const isLoad3DMesh =
-    props.nodeType === 'Load3D' && props.widget.name === 'model_file'
+    props.nodeType === LOAD_3D_NODE_TYPE && props.widget.name === MODEL_FILE_WIDGET_NAME
📝 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
const isLoad3DMesh =
props.nodeType === 'Load3D' && props.widget.name === 'model_file'
const LOAD_3D_NODE_TYPE = 'Load3D'
const MODEL_FILE_WIDGET_NAME = 'model_file'
const specDescriptor = computed<{
kind: AssetKind
allowUpload: boolean
folder: ResultItemType | undefined
subfolder?: string
}>(() => {
const isLoad3DMesh =
props.nodeType === LOAD_3D_NODE_TYPE &&
props.widget.name === MODEL_FILE_WIDGET_NAME
🤖 Prompt for AI Agents
In @src/renderer/extensions/vueNodes/widgets/components/WidgetSelect.vue around
lines 64-65, Extract the magic strings used in the isLoad3DMesh expression into
named constants (e.g., LOAD3D_NODE_TYPE and MODEL_FILE_WIDGET) and replace the
inline literals in the check (currently using props.nodeType === 'Load3D' and
props.widget.name === 'model_file') with those constants; define the constants
near the top of WidgetSelect.vue (or in a shared constants module if used
elsewhere) and update any other occurrences to use the new constant names to
improve maintainability and searchability.


const spec = comboSpec.value
if (!spec) {
if (!spec && !isLoad3DMesh) {
return {
kind: 'unknown',
allowUpload: false,
Expand All @@ -74,7 +79,7 @@ const specDescriptor = computed<{
video_upload,
image_folder,
audio_upload
} = spec
} = spec || {}

let kind: AssetKind = 'unknown'
if (video_upload) {
Expand All @@ -83,18 +88,27 @@ const specDescriptor = computed<{
kind = 'image'
} else if (audio_upload) {
kind = 'audio'
} else if (isLoad3DMesh) {
kind = 'mesh'
}

// TODO: add support for models (checkpoints, VAE, LoRAs, etc.) -- get widgetType from spec

const allowUpload =
image_upload === true ||
animated_image_upload === true ||
video_upload === true ||
audio_upload === true
audio_upload === true ||
isLoad3DMesh

const subfolder = isLoad3DMesh ? '3d' : undefined
const folder = isLoad3DMesh ? 'input' : image_folder

return {
kind,
allowUpload,
folder: image_folder
folder,
subfolder
}
})

Expand All @@ -120,7 +134,9 @@ const allowUpload = computed(() => specDescriptor.value.allowUpload)
const uploadFolder = computed<ResultItemType>(() => {
return specDescriptor.value.folder ?? 'input'
})
const uploadSubfolder = computed(() => specDescriptor.value.subfolder)
const defaultLayoutMode = computed<LayoutMode>(() => {
if (assetKind.value === 'mesh') return 'list'
return isAssetMode.value ? 'list' : 'grid'
})
</script>
Loading