kijai / ComfyUI-DynamiCrafterWrapper

Wrapper to use DynamiCrafter models in ComfyUI
Other
489 stars 15 forks source link

ToonCrafter is obfuscated by being implemented in this repo #46

Open FizzleDorf opened 1 month ago

FizzleDorf commented 1 month ago

Hey Kij! Thanks so much for the work you already put into the repo. My worry is that ToonCrafter isn't obvious that it's included in this repo from the title and especially not obvious when the repo is marked as a wrapper. This repo is probably going to be the first one that people see as it's following the standard naming convention in the title. I'll be migrating the comfy implement code over to that instead of this repo so things are just less confusing. I'll make sure you are properly attributed as you already put in so much into it. Let me know if there is any issues with that before I start machine gunning some PRs.

kijai commented 1 month ago

Hey,

I'm aware of that repo, and I personally find it bit annoying when wrappers "reserve" the name like that. I know I probably done that myself before, but have since changed my naming policy to better reflect what's a native implementation, and what is just a wrapper. And as the inference code is like 95% same as DynamiCrafter, I didn't want to make yet another repo.

Honestly the best approach would be to continue from this native implementation:

https://github.com/ExponentialML/ComfyUI_Native_DynamiCrafter

It just got abandoned before fully optimized, it is slower and more memory hungry than my wrapper, but ToonCrafter does somewhat work with it already, it's just missing the new encoder/decoder -functionality so quality suffers.

FizzleDorf commented 1 month ago

Totally see where you are coming from but the org running the ToonCrafter repo have a decent following in china (they have a site with native mandarin guides and workflows) and they give regular support to all their repos so they can be trusted to keep it alive.

It does get a little complicated now that I'm remembering Kosin wants to do some stuff with it as well (on top of everything else on his Todo list lol). I really wouldn't want someone to feel like their thunder is being stolen between you and the aigodlike org. I still think the repo name should indicate that tooncrafter is included so users definitely know what they are getting. I could fork the dynamiccrafter repo and rename it or ask the org to do it themselves so they can have an easier time. For now I am trying to juggle the naming so I'm not stepping on your toes either. Maybe we should get kosin and the org in on this conversation too and we can come up with a solution we will all be happy with ☺️ Kosin is flying home today so he will probably be tired so maybe tomorrow we can hear from him I'll link this issue to the other repo so the guys maintaining it know what's up.

kijai commented 1 month ago

I'm not sure Kos is aware of the already existing native repo, I do think best approach would be for someone like him to look into the part of the code I understand nothing about (attention etc.) and see what can be done. DynamiCrafter/ToonCrafter definitely has lot of potential and the demo code definitely isn't harnessing all of it.

This repo was never meant to be a long term solution, just doing what I can and mostly it's learning project like everything I've done so far. But it also doesn't make much sense to re-do what I've done with another wrapper starting from exact same point. Some sort of joint effort for truly native implementation would get my vote in the matter.

FizzleDorf commented 1 month ago

I don't think the repo intends on being a wrapper for long, just as a starting point for refactors. I can't speak for them though so I invited them here to communicate what would be the best approach. I should have brought this up at the summit so we could have had an outline how to handle redundant, duplicate or underutilized repos but maybe we can find a solution towards the joint effort I think everyone wants. Let's hear what the author has to say and don't let this stop you from developing it further in this repo, any step forward is great in my books.

Yorha4D commented 1 month ago

@FizzleDorf Hi brother, I noticed that you have reported this issue in multiple projects. In fact, in my country, a considerable number of people are unable to link to HF or Github. So from March 2023 until now, I have insisted on packaging and sharing some famous custom nodes with those who don't understand programming every month (including ComfyUI DynamiCrafterWrapper). Some of these nodes need to be connected to the network due to hf or other dependent code reasons, so in the past, we have also submitted code to some projects such as controlnet_aux and tripoSR to enable them to run completely locally. But recently we have found that due to some issues with hf( https://github.com/NVIDIA/NeMo/issues/2676 )Many developers are unwilling to develop these, so we try to directly migrate some projects In addition, our aigodlike project is a non-profit organization, and we do not believe that AI should be a tool for capitalists to exploit workers (such as cloud services), so just like in Blender, we work for it, that's all

Yorha4D commented 1 month ago

Regarding the standardization of custom nodes in ComfyUI, I remember there is a project called comfyregistry that should be ongoing. Perhaps it can be helpful for addressing issues between projects.Comfyregistry

FizzleDorf commented 1 month ago

Regarding the standardization of custom nodes in ComfyUI, I remember there is a project called comfyregistry that should be ongoing. Perhaps it can be helpful for addressing issues between projects.Comfyregistry

36 and #37 are already in the PRs for this repo so a lot of the setup is already there thanks to a good friend hao hao.

Yorha4D commented 1 month ago

Regarding the standardization of custom nodes in ComfyUI, I remember there is a project called comfyregistry that should be ongoing. Perhaps it can be helpful for addressing issues between projects.Comfyregistry

36 and #37 are already in the PRs for this repo so a lot of the setup is already there thanks to a good friend hao hao.

Yes, haohao, I also know him,The multi language custom nodes and clipboard custom nodes of ComfyUI that we have developed have also been standardized

To be honest, I just want ComfyUI to be more comfortable to use Because everything is for the use of Blender

https://github.com/kijai/ComfyUI-DynamiCrafterWrapper/assets/116185401/eb121970-34f0-47a8-b34d-5e7e22fe7c60