-
Notifications
You must be signed in to change notification settings - Fork 161
Large refactor #278
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
Large refactor #278
Conversation
|
@graemeniedermayer @thygate I would kindly ask you to keep an eye on this, as this will create totally massive merge conflicts if main branch receives significant changes while this is in the works. |
01f30e8 to
aaed517
Compare
|
@thygate
|
Large refactor part, may be broken Also indent everything with spaces, add/remove line breaks and add some TODOs
Large refactor part, may be broken
…_depthmap Large refactor part, may be broken
aaed517 to
daab32b
Compare
Large refactor part, may be broken
daab32b to
c414acd
Compare
Large refactor part, may be broken
|
@thygate
|
|
@thygate |
|
(C) Yes! The show depthmap is a leftover from the very first version, it serves no purpose. Always showing everything that was generated would be the most intuitive I agree. |
|
(B) The clip depth (far/near) function was something I implemented quickly and never really properly tested tbh .. |
|
(A) No effect for all seems like the correct approach to me, this option (invert depth) was meant for raw depthmap output only since some external software requires it. It should have no effect on any of the options except clip and renormalize. So all seem good to me. |
Large refactor part, may be broken Also fix downloading models
Large refactor part, may be broken Also restore "wrong mode" bug workaround
|
@thygate Thank you! |
Large refactor part, may be broken
|
(D) net_w and net_h are assigned, but never used; net_width and net_height are actually used. I guess it should work differently... Do I get it right that every model has its original (suggested) net size, but sometimes it is beneficial to override it? How do you think: maybe the net size should be a dropdown with values: "default" (match model), "match image" and "custom", where custom shows two sliders: for width and height? Or simply "match model size" and "match input size"? |
|
(D) Yes correct, net_w and net_h were the initial model sizes, at the start the sliders would mirror this initial model size, but this changed at one point and I never removed them. Drop down would be fine, but not all models support all sizes, and tweaking the size manually can be beneficial to tune for detail. I would prefer to keep the option to manually set size. |
Large refactor part, may be broken
Large refactor part, may be broken
f35d5ae to
b8c72a9
Compare
b8c72a9 to
88aa86f
Compare
|
Alright! I am tempted to call it a 3556223 Two Gradio interfaces that we currently have (script and tab) were improved a bit and moved to a separate file. These interfaces no longer rely on positioned arguments (I always got scared to touch them in any non-trivial way) - GradioComponentBundle now conveniently packs them and sends them in an easy package. eb82cd4 Supplying custom depthmaps had a quirky logic, where run_depthmap would try to find the files itself. A more general logic was introduced: depthmap for any image may be provided (and so generating is not required). This reduces the number of edge cases, which is always good. [More edge cases = harder to figure our and keep in mind all the different ways in which stuff could break; less entangled = less complicated = easier to navigate and edit.] Also, not trying to load/save files helps in case we want to add new interfaces in the future. c414acd Tweaked code to reduce duplication (Don't-repeat-yourself). Funnily enough, the line count got bigger. Welp, at least it is now easier to change save parameters. f65d8a7 Loading and storing models in memory is best done outside of the big function - this is quite a separate task from everything else that the big function did. This approach makes the model-lifetime logic much more clear, thus allowing to modify it with less fear. It would be cool to implement a timer (or a callback?) that would unload depth and pix2pix models and reload SD model when the Depth tab is unselected - this would help us win a couple of seconds from every consecutive run with the same model. There are still things to refactor (some of them are marked in the code itself), but I think it is a good idea to stop for now - endless refactoring starts to wear me down a bit. Improving the code further is still needed, this can be done while implementing new features. For example, before adding a CLI interface (a suggested feature), core should be disentangled from webui and gradio stuff. I did some simple testing, but for such a behemoth of a MR... We could expect that every little thing that could break, broke. Even after the testing we probably will see a temporary influx of bugs. This is the price for having less bugs in the long run (especially the weird bugs). @thygate I would be very grateful if you would go trough all the changed code and notify me of places that are important to improve before this is merged. If there is anything potentially scary, it'd be better fixed before the merge. I kindly ask you to give this MR a good test - it is highly likely that there is a regression or two lurking within the changes, that perhaps may be not so obvious to see. |
|
For starters, thanks for all this work !! I'm doing some quick tests to see if anything is broken .. I can only generate once using leres, every attempt afterwards fails until i change the model .. edit: same for all midas models, only exception is zoedepth models. |
|
@thygate Oops, a logic error, forgot to reload the models. Now should be fixed. |
9ed8ac5 to
cc55be2
Compare
|
@graemeniedermayer Oh! This sounds like a good idea. #282 |
|
When using "match net size" and leres I get this error when using leres and boost, seems to be for most models and boost : |
cc55be2 to
447b7af
Compare
|
@thygate Sorry, I forgot to test unloading 😬 New version hopefully fixes |
|
@thygate Can reproduce the net size issue in the main branch, too. This happens if image has a weird size. Do you think the code should simply round the net dimensions upwards to the next multiple of 32? (quite trivial, changed that already) |
|
Hehe, no worries, I'm very grateful for all the work you've put in this! Ah and yes, I see it now, (187) vs (188), my bad, I was a bit too fast with that report, sorry. Rounding the dimensions upward to the next multiple of 32 is a great idea and will solve this problem too. |
|
😊 |
be1986a to
336bcea
Compare
|
I'm testing it out! It's looking good. The only bug I found was also on the main branch. Zoe depth and pre_depth_background_removal don't seem to work together. This stems from RGB vs RGBA. It's a little tricky. I think the best solution would be to explicitly convert to RGB in the batched_background_removal function, but the alpha channel is used later to create masks so I'll need to think about this a little bit. |
|
Found a problem with the script mode. |
336bcea to
1889b76
Compare
|
Remove background tested. |
Does not work! Imports updated in the next commit.
* Reload model before generation, if it is offloaded to CPU * Load model if boost got selected * Do not try to offload pix2pix * Net dimensions are multiple of 32 regardless of match size * Change the default net size to default net size of the default model * Fixed script mode * UI fixes
1889b76 to
3fa9185
Compare
|
Both simple and inpainted meshes seem to generate fine. Videos, too. |
|
I've taken some time off (lots of stuff), but now I am back. I would like to get it merged - @thygate could you please do the last checks you have in mind and then Create a merge commit? |
|
Whoops, there has been an issue with this I did not find up until now: if model offloading is enabled, it will be offloaded only once, and then won't offload. Now fixed. |
|
And now we wait... Issues are bound to come, hopefully nothing that will be hiding for months like that one time I messed up a feature for 2+ months |
|
I checked all features and everything seems to be working for me, if anything is to pop up, I assume it will be small stuff, or problems with packages .. I went over all your commits and read all your comments, thanks again for the thorough job of refactoring the hot mess that it was. I did notice that just having some other extensions installed can change the torch hub checkpoint path, so the zoedepth models were redownloaded. |
It is much easier to work with! So if there's issues they might be easier to find. Also hopefully the update for sdxl 1.0 doesn't break anything. |
Status (2023-07-18): needs merging
This is expected to be semi-broken. My plan currently is to refactor it more, and only then do a good test to see what is broken (and fix it). Still would appreciate nods into the right directions - what broke and needs fixing, what could be improved in the refactor, what else is worth to refactor.The changed line count is inflated by changing tabs to spaces. Some editors (e.g. Pycharm 2023.1) will show whitespace edits differently than style and significant edits. Also, I moved code to different files.