OpenPype icon indicating copy to clipboard operation
OpenPype copied to clipboard

Maya: Refactor Maya API

Open antirotor opened this issue 4 years ago • 12 comments

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 avatar Apr 12 '21 09:04 antirotor

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.

antirotor avatar May 28 '21 14:05 antirotor

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

tokejepsen avatar May 28 '21 15:05 tokejepsen

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 avatar Jan 06 '23 12:01 BigRoy

@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?

BigRoy avatar Mar 11 '23 20:03 BigRoy

This is on @antirotor :)

iLLiCiTiT avatar Mar 13 '23 10:03 iLLiCiTiT

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

BigRoy avatar Mar 26 '23 18:03 BigRoy

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.

antirotor avatar Mar 27 '23 06:03 antirotor

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.

tokejepsen avatar Mar 27 '23 07:03 tokejepsen

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,

antirotor avatar Mar 27 '23 09:03 antirotor

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.

  • We'll just end up getting the issue that this namespaced commands exists in setdress.py but at the same time in lib.py (or another). I've no idea what to expect in a setdress.py but the majority of functions in there to me don't scream "setdress".
  • There's also render_setup_tools, lib_rendersetup and other lib_render** which from wording sound like they have so much overlap that I personally constantly forget which one I'd need. :) Maybe it helps if each of the files would at least have a top docstring of what it's about and what it's for?
  • Or what about having some empty file
  • Or maybe have a maya/api/shader_definition_editor.py in the maya/api which is actually a Tool but the command to start it is in maya/api/commands.py - weird maybe?
  • There's also this SHAPE_ATTRS definition which is unused but appears to be the same here and here

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.

BigRoy avatar Mar 27 '23 09:03 BigRoy

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.

antirotor avatar Mar 27 '23 09:03 antirotor

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

BigRoy avatar Mar 27 '23 09:03 BigRoy