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: Render settings validation attribute check tweak logging #3821

Conversation

BigRoy
Copy link
Collaborator

@BigRoy BigRoy commented Sep 9, 2022

Brief description

If settings was previously set up with an attribute name that was NOT {node_type}.{attr} like e.g. foobar without a dot the logic would've made it into foobar as node_name and would have tried to getAttr node. which seems odd. Now the logic will warn the user about the invalid settings.

I also believe the previous isinstance list check would have never proceeded to detect no nodes found since cmds.ls() always returns a list, even when no matching nodes detected.

It's mostly logging related cosmetics.

Additional Info

This PR is about:

project_settings/maya/publish/ValidateRenderSettings/arnold_render_attributes
project_settings/maya/publish/ValidateRenderSettings/vray_render_attributes
project_settings/maya/publish/ValidateRenderSettings/redshift_render_attributes
project_settings/maya/publish/ValidateRenderSettings/renderman_render_attributes

These take a {node_type}.{attribute_name} and a value

Testing notes:

  1. Submit renders for different renderers with different attribute value check implemented in settings.
  2. Test with invalid attr names - you should be warned.. but publish should not error hard.

@mkolar mkolar added the type: bug Something isn't working label Sep 12, 2022
@m-u-r-p-h-y
Copy link
Member

We are not sure we understand exactly the testing procedure here. But I encountered an error while testing render settings

image

[['enableProgressiveRender', '1'], ['motion_blur_enable', '0']]
# Error: No object matches name: enableProgressiveRender
# Traceback (most recent call last):
#   File "D:\REPO\OpenPype\openpype\hosts\maya\api\menu.py", line 131, in <lambda>
#     command=lambda *args: lib_rendersettings.RenderSettings().set_default_renderer_settings()    # noqa
#   File "D:\REPO\OpenPype\openpype\hosts\maya\api\lib_rendersettings.py", line 88, in set_default_renderer_settings
#     self._set_arnold_settings(width, height)
#   File "D:\REPO\OpenPype\openpype\hosts\maya\api\lib_rendersettings.py", line 141, in _set_arnold_settings
#     self._additional_attribs_setter(additional_options)
#   File "D:\REPO\OpenPype\openpype\hosts\maya\api\lib_rendersettings.py", line 236, in _additional_attribs_setter
#     if (cmds.getAttr(str(attribute), type=True)) == "long":
# ValueError: No object matches name: enableProgressiveRender # 

(as a side note, could it be possible to make the input key text box wider as the attributes name can get quite long and it makes it unreadable in the settings. or should I create a separate issue for it?)

@BigRoy
Copy link
Collaborator Author

BigRoy commented Sep 16, 2022

Thank you! I'm not in the office Today so sorry for the short reply but this looks like I didn't fix it correctly yet.

@BigRoy
Copy link
Collaborator Author

BigRoy commented Sep 17, 2022

Ha, that's funny @m-u-r-p-h-y - I have taken another look.

I think that's actually an other area of the code base, that's the Render Settings settings. This PR relates to the Validate Render Settings logic.

This PR is about:

project_settings/maya/publish/ValidateRenderSettings/arnold_render_attributes
project_settings/maya/publish/ValidateRenderSettings/vray_render_attributes
project_settings/maya/publish/ValidateRenderSettings/redshift_render_attributes
project_settings/maya/publish/ValidateRenderSettings/renderman_render_attributes

These take a {node_type}.{attribute_name} and a value

You are referring to an issue with:

project_settings/maya/RenderSettings/arnold_renderer/additional_options
project_settings/maya/RenderSettings/vray_renderer/additional_options
project_settings/maya/RenderSettings/redshift_renderer/additional_options

These take a {node_name}.{attribute_name} and a value

So that label in the documentation for the additional options in Render Settings I believe is wrong.

afbeelding

It shouldn't be just the attribute's name but it should include the node name as well.
So instead of:

Add additional options - put attribute and value, like AASamples

It would be:

Add additional options - put attribute and value, like defaultArnoldSettings.AASamples

I also feel that the render settings logic should log a warning if the {node_name}.{attribute_name} isn't found instead of an actual error stopping the setting of the render settings.

I'd be happy to rework that logic too - but I'm not sure if it makes sense to add into this PR.


(as a side note, could it be possible to make the input key text box wider as the attributes name can get quite long and it makes it unreadable in the settings. or should I create a separate issue for it?)

Totally agree. Would be nice if that Settings UX could get fixed for that.

@LiborBatek
Copy link
Member

LiborBatek commented Sep 29, 2022

I have tested several attributes like
defaultArnoldRenderOptions.AASamples and it works (OP>Set Render Settings - sets it accordingly) also I can confirm that this
formulation should be used as hint:

"Add additional options - put attribute name and value, like defaultArnoldRenderOptions.AASamples 4 - I can see putting even the example value as very useful to user to understand it

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.

Working ok!
Small note: the hint message in Admin/Project section should be adjusted-see my comment.

@BigRoy
Copy link
Collaborator Author

BigRoy commented Sep 29, 2022

Small note: the hint message in Admin/Project section should be adjusted-see my comment.

Should be fixed now with 484e317

afbeelding

(as a side note, could it be possible to make the input key text box wider as the attributes name can get quite long and it makes it unreadable in the settings. or should I create a separate issue for it?)

Like @m-u-r-p-h-y mentioned - this is still the case. The attribute field is a bit smaller than you'd like.

@LiborBatek
Copy link
Member

I can confirm that the "hint msg" in OP Project settings is updated and fine...

@mkolar mkolar merged commit 7bc645e into ynput:develop Oct 5, 2022
@BigRoy BigRoy deleted the maya_validate_render_settings_attribute_check_logging branch March 20, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants