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.

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

Branch trellis update #423

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

iiiCpu
Copy link

@iiiCpu iiiCpu commented Jan 12, 2025

I had a problem with original Trellis pipeline on Windows 11 (wasted like 6 hours to make it work with no effect). So, I replaced it with Microsoft's one, which now works like charm. For me, I had to launch microsoft/TRELLIS app.py, let it build all it needed to build, copy all microsoft/TRELLIS/trellis contents into ComfyUI-3D-Pack/Gen_3D_Modules/TRELLIS/trellis.

Also, I had to move some mesh postprocess code from trellis.postprocessing_utils into Trellis_Structured_3D_Latents_Models.save_gs as I wanted to get MESH instead of just saved file name.

I also changed Save_3D* nodes to act more like standard Save_Image node: to add index to a file name instead of replacing it. And I also made option to create entire folder, it's useful to save different formats or preview into the same place.

…880b130ff (2024.12.27 19:37:55)

Updated ComfyUI-3D-Pack\nodes.py to match general flow. Pulled out some hidden parameters of TRELLIS sampler to UI.
…e creates new file every time instead of rewriting it.

New output format option to select between [glb, ply, obj] instead of reading the file name extension.
New "create_folder" option to create new directory for every output and put all files into the same place.
@YanWenKun
Copy link
Contributor

Great job!
A small question, maybe trellis_ folder is unnecessary?

Updated `Trellis_Structured_3D_Latents_Models` so, if provided with batch of images, it will process in `run_multi_image` pipe.
Cleanup in `nodes.py`.
@iiiCpu
Copy link
Author

iiiCpu commented Jan 21, 2025

A small question, maybe trellis_ folder is unnecessary?

True.

I also made a little clean up in nodes.py.

@YanWenKun
Copy link
Contributor

YanWenKun commented Jan 21, 2025

I just tested this PR and found some files that would be better left unchanged:

  1. renderers/gaussian_render.py

    3D-Pack uses diff-gaussian-rasterization from ashawkey, which is incompatible with the version used by the TRELLIS official repo. Other parts of 3D-Pack are using this package as well, so it's better to stay with previous implementation.

  2. modules/attention/__init__.py

  3. modules/sparse/__init__.py

    3D-Pack uses xformers by default to ensure maximum compatibility. Replacing it with flash_attn would require providing additional options for users to choose from.

  4. representations/mesh/flexicubes/examples

    I believe these example files are not needed in this use case.

In addition, modifying the Save 3D Mesh node will break all existing workflows. I suggest creating a new node and including it in a separate pull request.

@iiiCpu
Copy link
Author

iiiCpu commented Jan 21, 2025

  1. Agreed.

  2. I simply replaced Trellis from official Microsoft repo, without much thinking. I don't know much about xformers and flash_attn compatibility. Seems like I have some homework to do.

  3. Agreed.

p.s. Gosh, markdown sometimes make me feel so miserable.

@MrForExample
Copy link
Owner

MrForExample commented Jan 22, 2025

@iiiCpu Hi, thank you for your contribution, I'll take a closer look later this week
In the meantime, can you please remove some unnecessary changes and change it back to xformers, because xformers actually supports flash_attn (Although probably not v3, but in time it should), thanks again in advance :)
Have a good day 👍

Copy link
Owner

@MrForExample MrForExample left a comment

Choose a reason for hiding this comment

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

Please clean up your change before I merge them

Copy link
Owner

Choose a reason for hiding this comment

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

What's the reason to remove comfy progressbar?

Copy link
Owner

Choose a reason for hiding this comment

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

Please remove some unnecessary changes and change it back to xformers, because xformers actually supports flash_attn

Copy link
Owner

Choose a reason for hiding this comment

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

Where in the code did you used the newly added function run_multi_image?

Copy link
Owner

Choose a reason for hiding this comment

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

Those change were mode to support xformer, please revert it back

Copy link
Owner

Choose a reason for hiding this comment

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

Please remove all the unnecessary files from flexicubes, this project is already big enough in size

Copy link
Owner

Choose a reason for hiding this comment

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

Please double check all the library you used, for example, kaolin notoriously is hard to install on Windows, thus I didn't use it in this project and replaced all the functions with customized function

Copy link
Owner

Choose a reason for hiding this comment

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

Can you please merge trelles_ with trelles, is hard to tell what is changed in trelles_, minimal change when ever possible please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants