Skip to content
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

Maya: Rendersettings unify logic towards single library #3880

Closed

Conversation

BigRoy
Copy link
Collaborator

@BigRoy BigRoy commented Sep 17, 2022

Brief description

This tries to reduce the scattered hardcoded values for renderer attribute names, types and tries to clean up the same things on the RenderSettings class. I've also refactored the usage of it across the code.

Description

  • Delays getting image prefixes per renderer instead of loading project settings three times on import of lib_rendersettings
  • Move over missing image prefixes values that existed on validator but not in lib_rendersettings
  • Move over AOV char logic to lib_rendersettings
  • Readability cosmetic tweaks on some lines which did more complex formatting than needed.
  • Removed lib.RENDER_ATTRS in favor of the now added RenderSettings.get_padding_attr (other logic of lib.RENDER_ATTRS was unused)

Additional info

Relates to #3878, #3879 and #3821 since they touch a similar area of the codebase.
Also relates to #3873 which also touches an aspect of rendering in Maya.

Testing notes:

  1. Test creation, validating and publishing of Render instances.
  2. Ensure "Reset render settings" works as expected.
  • Maya 2023 (Py3)
  • Maya 2020 (Py2)
  • Redshift Renderer
  • V-Ray Renderer
  • Arnold Renderer
  • Renderman Renderer
  • Maya Hardware 2.0
  • Perform tests with multilayer on and off (make sure image prefix validations and repair work as intended and are clear to the artist)

@BigRoy BigRoy requested a review from a user September 17, 2022 13:14
@BigRoy
Copy link
Collaborator Author

BigRoy commented Sep 18, 2022

@mkolar @antirotor what do we think of adding this BigRoy#7 to RenderSettings as well?

EDIT: Done with 90667f3

@antirotor
Copy link
Member

@mkolar @antirotor what do we think of adding this BigRoy#7 to RenderSettings as well?

Yes please!

Maya: RenderSettings class easy access to its project settings
Copy link
Member

@tokejepsen tokejepsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have Merge AOV disabled and remove the token , the validate/repair never gets it back. This is for Maya and Arnold.

Tested in Maya 2020 successfully with #4795 merged.

@BigRoy
Copy link
Collaborator Author

BigRoy commented Apr 6, 2023

If you have Merge AOV disabled and remove the token , the validate/repair never gets it back. This is for Maya and Arnold.

Could you elaborate the preferred behavior. The question is what "repair" should do or allow? If I understand correctly it should revert the merge AOV option back to what the settings dictate, correct? It's not up to the scene or artist to deviate from the merge AOV option, yes?

Currently the validation DOES allow to deviate merge AOVs settings + the image prefix as well. Which maybe makes it weird if it then forces the value on repair if it invalidates based on another setting?

The validator does require however the token to match with the merge AOV setting, so have ` if merge AOV is disabled, and not have if it's enabled.

Anyway, I've now made it so that it also repairs the merge AOV setting. @tokejepsen does that solve it for you?

@BigRoy BigRoy requested a review from tokejepsen April 6, 2023 12:20
@tokejepsen
Copy link
Member

Could you elaborate the preferred behavior. The question is what "repair" should do or allow?

I think the repair should fix the file name prefix according to what state the Merge AOVs is in.

Merge AOVs enabled > no <RenderPass> token.
Merge AOVs disabled > need <RenderPass> token.

@tokejepsen
Copy link
Member

Also Maya Hardware 2.0 works.

@BigRoy
Copy link
Collaborator Author

BigRoy commented Apr 6, 2023

I think the repair should fix the file name prefix according to what state the Merge AOVs is in.

Merge AOVs enabled > no token.
Merge AOVs disabled > need token.

What should happen if the default prefix in settings does NOT have a token, should we append it to the end? Should we append _<RenderPass>?

@tokejepsen
Copy link
Member

What should happen if the default prefix in settings does NOT have a token, should we append it to the end? Should we append _?

Hmm, had some thoughts about optional tokens but since this setting is project based, admin should be in control of this. Lets keep as it now. Nice one!

Copy link
Member

@tokejepsen tokejepsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good and partially tested for the renderers I can.

@BigRoy
Copy link
Collaborator Author

BigRoy commented Apr 12, 2023

@LiborBatek could you test again whether in the meantime your issue mentioned here has been resolved?

There have been changes in code (4 months ago) by @moonyuet here which I'm not sure whether they do what it intends to do. Since it seems off that first it's trying to set redshiftOptions.primaryGIEngine and redshiftOptions.secondaryGIEngine based on some conditions but in the end it still ALWAYS sets it to the value int(rs_p_engine) and int(rs_s_engine). Seems like a bug @moonyuet ?

Anyway, the code seems related to the issue you described @LiborBatek so I suspect your particular issue has been resolved.

@moonyuet
Copy link
Member

moonyuet commented Apr 13, 2023

@LiborBatek could you test again whether in the meantime your issue mentioned here has been resolved?

There have been changes in code (4 months ago) by @moonyuet here which I'm not sure whether they do what it intends to do. Since it seems off that first it's trying to set redshiftOptions.primaryGIEngine and redshiftOptions.secondaryGIEngine based on some conditions but in the end it still ALWAYS sets it to the value int(rs_p_engine) and int(rs_s_engine). Seems like a bug @moonyuet ?

Anyway, the code seems related to the issue you described @LiborBatek so I suspect your particular issue has been resolved.

maybe needs a double check on the particular setting, but it got tested and resolved in the other ticket which is merged in December 2022, while @LiborBatek found this on November 2022.The related PR is #4087

Copy link
Member

@LiborBatek LiborBatek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did test in Vray and got error when triggered Repair on Render Settings (I have left intentionally empty File Name Prefix empty)

image

However when tried to Publish it afterwards straight away ....it worked then (so I guess the Repair worked tho)

image

@Minkiu Minkiu assigned BigRoy and unassigned LiborBatek Jun 30, 2023
@tokejepsen
Copy link
Member

@BigRoy could you resolve the conflicts?

@tokejepsen tokejepsen marked this pull request as draft July 25, 2023 07:30
@LiborBatek
Copy link
Member

@BigRoy maybe silly question, how this PR adhere to the new publisher in maya? all good?!

@BigRoy
Copy link
Collaborator Author

BigRoy commented Oct 20, 2023

maybe silly question, how this PR adhere to the new publisher in maya? all good?!

I don't expect it to be functional momentarily - it's a very old PR that I'm not sure how to best merge logically nowadays. I think we might have it merged in our colorbleed branch and along the way have synced that branch with new publisher - etc. but likely that's seen decent refactoring to make it work.

However, some of the cleanup is still relevant - I'll close this PR and see if I can set up a new one with some of the cleanup in here that is still relevant.

@BigRoy BigRoy closed this Oct 20, 2023
@ynbot ynbot added this to the next-patch milestone Oct 20, 2023
BigRoy added a commit to BigRoy/OpenPype that referenced this pull request Oct 20, 2023
…ng_attr` method

- Partial cleanup extracted from ynput#3880
kalisp pushed a commit that referenced this pull request Nov 15, 2023
…ng_attr` method (#5801)

- Partial cleanup extracted from #3880
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

8 participants