Closed davidmartinrius closed 11 months ago
Hello! Overall, quite good! However, for code extendability, a slightly different approach is required. I try to keep the code_generation_funnel as separated from the API as possible. I will give some specific things to look for using code review. This functionality is a great addition, I like that it does not break anything.
Also a question: is it possible to later make the code support strings for model selection? Like, could we have a parameter that could be either int or a string? If not, do you think it would be a reasonable thing to do, to have it as string from the beginning and later add support for model names? Not sure if anybody wants to add it (would certainly be welcome), but just would be nice to work around this potential backward-compatibility issue.
Well, if you wanted I can create a specific endpoint for this functionality like /depth/generate/video and do not use the code_generation_funnel, but implement a separated function for that endpoint. Or better, if you could explain what exactly would you want I could try to program it.
About this question "is it possible to later make the code support strings for model selection? "
I do not understand what do you refer. Could you explain with more details and examples, please? π€
Thank you!
Ah, I suppose you meant this: available_models = { 'dpt_beit_large_512': 1, #midas 3.1 'dpt_beit_large_384': 2, #midas 3.1 'dpt_large_384': 3, #midas 3.0 'dpt_hybrid_384': 4, #midas 3.0 'midas_v21': 5, 'midas_v21_small': 6, 'zoedepth_n': 7, #indoor 'zoedepth_k': 8, #outdoor 'zoedepth_nk': 9, }
To be available inside the API and pass a string of the model name or pass an integer that matches that model, it that right?
Like "model_type": "dpt_beit_large_512" OR "model_type": 1
?
So basically depthmap_api.py should "wrap around" the core and ask it to do stuff, and then transform this stuff for API-specific needs. But in this state of MR, its leaking a bit - core is "aware" of the API stuff happening.
Exactly. Sorry, did not sleep well tonight π₯Ά
| if you wanted I can create a specific endpoint for this functionality like /depth/generate/video Not sure... Full disclosure: i touched most of the original code in this project, but it was some time ago... Can't be very sure of everything right now, need to think a bit. Looking for maintainers :)
So basically depthmap_api.py should "wrap around" the core and ask it to do stuff, and then transform this stuff for API-specific needs. But in this state of MR, its leaking a bit - core is "aware" of the API stuff happening.
Yes, I am aware of that. I just followed the order of what was already programmed. It is clear that the API should wrap around the core and not be mixed into it.
Although I don't know the project enough to make such big changes. So I continued in the order in which things were already done.
I think restructuring the API and refactoring it requires another separate pull request and I don't know if I would be able to do it without very clear instructions on how you would want it.
I feel the same way, that a big task. Then just please try to make the core_generation_funnel not call the API specific code and then I think we can call it a day. Parameter names, optimal design decisions, et cetera can wait I suppose.
| if you wanted I can create a specific endpoint for this functionality like /depth/generate/video Not sure... Full disclosure: i touched most of the original code in this project, but it was some time ago... Can't be very sure of everything right now, need to think a bit. Looking for maintainers :)
Absoulutely! :D You have a much better vision than me. If you want to do it, it will be appreciated.
I have tried to separate the code from lines 345 to 353 into a function outside of the core_generation_funnel. Although I haven't found a clean way to do it. In any case, I need variables set in the core_funnel to process the video later. So, the only way I found is to yield that variables instead of calling run_makevideo_api. And in another part of the code call run_makevideo_api. But I think is not a good approach..
Please, could you suggest me to abstract that part into another function, so that the core doesn't get mixed up with the API tasks?
Thank you!
You might be able to use a similar method to run_makevideo
in common_ui.py
. Maybe you could use the if line 336 inp[go.GEN_INPAINTED_MESH]:
to generate a mesh from the core funnel and then run_makevideo
afterwards in the API code. It does feel like inpainted mesh generation should be extracted into a separate function core_generation_funnel
(but this seems like a separate task). Is that the key issue, the inpainted mesh generation requiring variables from the core_funnel?
It does also feel like the second part of run_makevideo_api
could call run_makevideo
directly to reduce code repetition. I might be missing the difference.
Great work!
Ok, I'll do it this way. As soon as programmed I'll let you know. Thanks!
I made several changes.
So, the core is not mixed with the api anymore when generating videos from the api.
What do you think about this changes?
Thank you!
David Martin Rius
Hello again :)
I did not really get in to all the details of the code, but overall (at least architecturally), it looks very, very good π I will give it some more attention and then merge.
Indeed, very good code! Merging with next to no modifications. There are some security risks from exposing this functionality in API, but since we never advertised the API as something that can be made accessible from the internet, this is ok.
Thank you so much for contributing this code β€οΈ I am happy that now this project can be more useful for people. I would be glad to collaborate with you more, choose you to create more code for this project π Feel free to let me know if something does not work right.
Indeed, very good code! Merging with next to no modifications. There are some security risks from exposing this functionality in API, but since we never advertised the API as something that can be made accessible from the internet, this is ok.
Thank you so much for contributing this code β€οΈ I am happy that now this project can be more useful for people. I would be glad to collaborate with you more, choose you to create more code for this project π Feel free to let me know if something does not work right.
Hi @semjon00 !!
I am very glad to contribute. Thank you so much for merging the code.
Now I will create a new endpoint to https://github.com/mix1009/sdwebuiapi in this way, this plugin will be easier to use via API for any user. (For both endpoints available in the API)
On the other hand, I am trying to make the mesh generation to work with pytorch instead numpy to accelerate the mesh generation process. Actually some mesh generation steps already use pytorch, but not in a optimal way and do not use pytorch everywhere. (I refer to the code inside inpaint/mesh.py, inpaint/mesh_tools.py, inpaint/bilateral_filtering.py, etc) And because of that the mesh generation takes too much time. It uses the cpu because of numpy. So, my idea is to migrate to pytorch tensors. Is not an easy task and it requires to modify the code in blocks and very carefully. Maybe there are other reasons that affect to the performance, but I still do not know.
Do you know the critical points when generating a 3D mesh? I mean that ones that specially slow down the process and could use the gpu to speed up the process.
If you had to do it where would you start?
Thank you!
David Martin Rius
If you had to do it where would you start?
Honestly, I am not even exactly sure what magic happens over there. @thygate is the person who added this into the script - I think he borrowed this code from somewhere else - he knows how it works better than me. The proper way to change the code would be to find the upstream and contribute there (granted, hopefully that repository is still maintained), and then tweak this repository to use the newest upstream version.
I think that code may come from https://github.com/vt-vl-lab/3d-photo-inpainting and also facebookresearch. Unfortunately, the last updates are from 3 years ago. So, this is the most updated repository altough it is almost a copy&paste of that project if I am not wrong.
Maybe @thygate has an idea on how to do it.
Thank you!
Right, I remember now. Indeed, then the most reasonable thing to do is to just patch it here. After the fact I/we might try to find other "forks" and MR the changes there.
Hello!
How are you doing? π My name is David Martin Rius and I added a new functionality to create a video from an image with the API.
It does not affect the current extension behaviour. It is just an enhancement.
The endpoint is the same, /depth/generate I added a new option "RUN_MAKEVIDEO_API" in common_constants.py (available when calling /depth/get_options)
If this code is ok I will create a new endpoint in https://github.com/mix1009/sdwebuiapi to integrate it. If you don't know what is this other project, it is by now the most advanced and open source API for automatic1111.
This object "video_parameters" has this properties:
Important:
By now the output filename will have an extra underscore. For example if passed /my/folder/video.mp4 the ouput will be /my/folder/video.mp4_
_The property "mesh_fifilename" is optional, Is not needed. Also can be None or a path of a .obj file. If you already have an .obj file it will create a video much faster. Creating a mesh is the lowest part of the process, so I recommend using it if you are rendering multiple videos.
On the other hand, if any of this properties is required and not passed is controlled by an exception inside run_makevideo_api function and will tell you what is missing. The function run_makevideo_api can be found in scr/core.py
You can use this snippet example to create a video from the API:
Thank you! Enjoy it! π
David Martin Rius