ynput / OpenPype

OpenPype has been surpassed by AYON and is now read only.
https://ayon.ynput.io
MIT License
287 stars 129 forks source link

Maya: Refactor Maya API #1306

Open antirotor opened 3 years ago

antirotor commented 3 years ago

Problem

Maya integration is complex. Main problem with it is that right now most of the code is in one file hosts/maya/api/lib.py. This file is ~2700 lines of code long and including function from looks management to generic dialog popup functions. This should be split into module lib/__init__.py with different packages grouping specific functionality. This way it will be more maintainable and testable.

[cuID:OP-1186]

antirotor commented 3 years ago

I would add to this that during that refactor, we should cleanup legacy code and switch to better handling of render setup. It seems we are hitting legacy api limits.

tokejepsen commented 3 years ago

To make it faster to collect render layers, maybe look at not relaying on switching layers and query the attributes instead; https://gist.github.com/davidlatwe/e33b942967f18f4b61f085e3effe86c7#file-rendersetup_util-py-L67

BigRoy commented 1 year ago

Has this been resolved since? :) Or are there still major pain points to be refactored? If so, it would be good to have a Task list here of bullet points to address.

BigRoy commented 1 year ago

@iLLiCiTiT getting back to my previous comment

Has this been resolved since? :) Or are there still major pain points to be refactored? If so, it would be good to have a Task list here of bullet points to address.

Should we close this?

iLLiCiTiT commented 1 year ago

This is on @antirotor :)

BigRoy commented 1 year ago

@antirotor please confirm what you'd like to do with this.

antirotor commented 1 year ago

My idea was to split it into more files to make it more managable even if they will at the end link back to the lib. The file is huge with mixed functions to work with attributes, alembic, transforms, menu, colorspaces, xgen, render layers and even though the functions there are mostly documented with docstrings, it is difficult to add/use something from there without spending time deciphering what's there. And it would be easier to add unit and functional tests.

tokejepsen commented 1 year ago

My main issue with the lib is that unless you know what is available, you end up writing duplicate code while developing, then trying to refactor to use the lib code. Not sure whether that is a valid way to work, but would assume being able to utilize the lib while developing would be faster than refactor afterwards. Have not got a solution though to how you would introduce knowledge to newcomers about what it available in the lib.

antirotor commented 1 year ago

My main issue with the lib is that unless you know what is available, you end up writing duplicate code while developing, then trying to refactor to use the lib code. Not sure whether that is a valid way to work, but would assume being able to utilize the lib while developing would be faster than refactor afterwards. Have not got a solution though to how you would introduce knowledge to newcomers about what it available in the lib.

That is actually why I've created this issue. I think that splitting it into clearly defined pieces will help a bit - for example: you need to use function that will query something in render layers, first you look into lib_renderlayer.py (or whatever) and if it is not there, you know you need to write something new. No need to crawl over one huge file. I understand that it might add slight overhead from the other side - but it is much easier to refer to one file that bits and pieces scattered over one huge,

BigRoy commented 1 year ago

That is actually why I've created this issue. I think that splitting it into clearly defined pieces will help a bit - for example: you need to use function that will query something in render layers, first you look into lib_renderlayer.py (or whatever) and if it is not there, you know you need to write something new.

I actually don't think we'll get benefit in that at all. For what it's worth I've maybe had a 5% success rate going into openpype/lib and knowing which file I should be looking in.

I think if we end up separating to files we better start defining what ends up in which file. I can definitely see value in separating out clear pieces related to a specific functionality, for example this "lookdev" region in lib.py can be moved into a separate file, but then what's the name lib_looks.py, lib_lookdev.py, lib_shaders.py? Or is it just look.py?

There's definitely worth in cleaning this up, but I don't expect it'll provide that much clarity on knowing what's there in the codebase without doing a small hunt yourself unless there's documentation for a dev to go through.

antirotor commented 1 year ago

That's because now it is all over the place. I think the clearest and most "pythonic" way would be to move things related to specific workflows to separate files - like what you've mentioned with looks. looks.py is pretty obvious. The same with attributes.py, render_setup.py and so on. We'll need to keep lib for the generic functions but anything that is used exclusively for specific type of workflow should be imho in separate file. And yes, ideally with file-level docstring.

BigRoy commented 1 year ago

Let's give it a go. Anyone up for taking this on? Who eats refactors for breakfast?