Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add load 3d node support #1492

Closed
wants to merge 20 commits into from
Closed

add load 3d node support #1492

wants to merge 20 commits into from

Conversation

jtydhr88
Copy link
Collaborator

@jtydhr88 jtydhr88 commented Nov 10, 2024 •
edited
Loading

Hi,
I implemented a node to load local 3d models and output IMAGE for furthur process, such as controlnet, as reference image.
A test workflow

load-3d (1).json

screenshot
image

Need comfyanonymous/ComfyUI#5564 as BE

@socket-security Socket Security
Copy link

socket-security bot commented Nov 10, 2024 •
edited
Loading

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@types/[email protected] None +6 2.56 MB types
npm/[email protected] None 0 27.5 MB mrdoob

🚮 Removed packages: npm/[email protected], npm/[email protected]

View full report↗︎

huchenlei
huchenlei reviewed Nov 10, 2024
View reviewed changes
Copy link
Member

@huchenlei huchenlei left a comment

Choose a reason for hiding this comment

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

Wow +27MB on package size. This threejs lib is huuuuge. If it is as simple as viewing 3D object without requiring fancy features like particles and shaders, can we use a more lightweight library instead? Or is there plan to add more features here?

@huchenlei
Copy link
Member

Update:

threejs is probably fine as the binary size of dist only goes from 3.55 MB to 4.36 MB.

huchenlei
huchenlei reviewed Nov 10, 2024
View reviewed changes
public/assets/sorted-custom-node-map.json Outdated
@@ -2599,5 +2598,6 @@
"Perlin Power Fractal Latent (PPF Noise)": 1,
"Cross-Hatch Power Fractal (PPF Noise)": 1,
"Linear Cross-Hatch Power Fractal (PPF Noise)": 1,
"CLIPTextEncodeControlnet": 1
"CLIPTextEncodeControlnet": 1,
"Load3D": 1
Copy link
Member

@huchenlei huchenlei Nov 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You don't need to update this list as it is automatically generated, and not supposed to be always up-to-date.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

@jtydhr88 jtydhr88 marked this pull request as draft November 11, 2024 04:32
@jtydhr88
Copy link
Collaborator Author

image
use comfy native components instead

@jtydhr88 jtydhr88 marked this pull request as ready for review November 12, 2024 03:05
huchenlei
huchenlei requested changes Nov 12, 2024
View reviewed changes
Copy link
Member

@huchenlei huchenlei left a comment

Choose a reason for hiding this comment

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

Please fix:

Error: src/extensions/core/load3d.ts(528,14): error TS2339: Property 'name' does not exist on type 'LGraphNode'.
Error: src/extensions/core/load3d.ts(540,14): error TS2339: Property 'load3d' does not exist on type 'LGraphNode'.
Error: src/extensions/core/load3d.ts(540,34): error TS2345: Argument of type 'LGraphNode' is not assignable to parameter of type 'objectOutputType<{ id: ZodUnion<[ZodNumber, ZodString]>; type: ZodString; pos: ZodUnion<[ZodEffects<ZodObject<{ 0: ZodNumber; 1: ZodNumber; }, "passthrough", ZodTypeAny, objectOutputType<...>, objectInputType<...>>, number[], objectInputType<...>>, ZodTuple<...>]>; ... 9 more ...; bgcolor: ZodOptional<...>; }, ZodTy...'.
  Type 'LGraphNode' is not assignable to type '{ color?: string; size?: number[] | [number, number, ...unknown[]]; mode?: number; id?: string | number; type?: string; pos?: number[] | [number, number, ...unknown[]]; flags?: { collapsed?: boolean; pinned?: boolean; allow_interaction?: boolean; horizontal?: boolean; skip_repeated_outputs?: boolean; } & { ...; }; ....'.
    Types of property 'size' are incompatible.
      Type 'Size' is not assignable to type 'number[] | [number, number, ...unknown[]]'.
        Type 'Float32Array' is not assignable to type 'number[] | [number, number, ...unknown[]]'.
          Type 'Float32Array' is not assignable to type '[number, number, ...unknown[]]'.
Error: src/extensions/core/load3d.ts(548,20): error TS2339: Property 'load3d' does not exist on type 'LGraphNode'.
Error: src/extensions/core/load3d.ts(549,18): error TS2339: Property 'load3d' does not exist on type 'LGraphNode'.
Error: src/extensions/core/load3d.ts(556,20): error TS2339: Property 'load3d' does not exist on type 'LGraphNode'.
Error: src/extensions/core/load3d.ts(557,18): error TS2339: Property 'load3d' does not exist on type 'LGraphNode'.
Error: src/extensions/core/load3d.ts(564,16): error TS2339: Property 'load3d' does not exist on type 'LGraphNode'.

You can run npm run typecheck locally to verify things are compilable.

@jtydhr88
Copy link
Collaborator Author

@huchenlei I don't think I should add any properties to LGraphNode, so I add ts-strict-ignore back for now

@jtydhr88 jtydhr88 requested a review from huchenlei November 12, 2024 20:13
huchenlei
huchenlei requested changes Nov 12, 2024
View reviewed changes
Copy link
Member

@huchenlei huchenlei left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please enable ts-strict. The errors are actually meaningful. Also current errors are from non-strict tsc. 😞

src/extensions/core/load3d.ts Outdated
if (h <= 300) h = 300
this.size = [w, h]

if (this.load3d) {
Copy link
Member

Choose a reason for hiding this comment

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

this ref is ambiguous here. Please replace it with node if you are trying to bind something to the LGraphNode object. With previous comment in mind, here you probably want to read from widget object instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

src/extensions/core/load3d.ts Outdated
container.classList.add('comfyui-load-3d')
containerWrapper.appendChild(container)

node.load3d = new Load3d(node, container)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you are supposed to bind the load3d to LGraphNode, as a node can have multiple widget of same type in theory. If a backend node specifies multiple Load3D widgets, this impl will cause issue. Let's keep it local to the widget.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@jtydhr88
Copy link
Collaborator Author

Still have a few things need to correct, I will fix today

@jtydhr88
Copy link
Collaborator Author

jtydhr88 commented Nov 14, 2024 •
edited
Loading

please help to run the tests @huchenlei

@jtydhr88 jtydhr88 requested a review from huchenlei November 16, 2024 01:39
@jtydhr88
Copy link
Collaborator Author

close it for now, I will send a new PR

@jtydhr88 jtydhr88 closed this Nov 16, 2024
@jtydhr88
Copy link
Collaborator Author

please review #1563 instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@huchenlei huchenlei Awaiting requested review from huchenlei

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jtydhr88 @huchenlei