ynput / OpenPype

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

Maya: Look Shading Engine Naming validator repair action not working #2263

Open antirotor opened 2 years ago

antirotor commented 2 years ago

Bug

When publishing look with incorrect naming validator Look Shading Engine Naming will trigger ok, but its repair action fails on:

Preparing <class 'openpype.action.RepairAction'>..
Action prepared.
// pyblish.pyblish.plugin.Action : Finding failed instances.. // 
// Error: pyblish.plugin : Traceback (most recent call last):
  File "E:\projects\openpype\3x\.venv\lib\site-packages\pyblish\plugin.py", line 522, in __explicit_process
    runner(*args)
  File "E:\projects\openpype\3x\openpype\action.py", line 65, in process
    plugin.repair(instance)
  File "E:\projects\openpype\3x\openpype\hosts\maya\plugins\publish\validate_look_shading_group.py", line 58, in repair
RuntimeError: Cannot rename a read only node.

image

Problem is that it is trying to rename initialShadingGroup that is read only. So if artist forget to apply shader so there is part of the model with default shader, this will fail and it is not very clear why.

[cuID:OP-2032]

mkolar commented 2 years ago

Actually there is another validator that should flag, that you cannot publish InitialShadingGroup as part of the look. If artist tries to run this repair on invalid look in general then it fail. The question is of course how to make it more intuitive

mkolar commented 2 years ago

I'd say maybe joining multiple validator to a single on that can be a bit more complex, might be better.

BigRoy commented 2 years ago

I'd say maybe joining multiple validator to a single on that can be a bit more complex, might be better.

I personally would avoid that - I feel it's much more trivial to keep each doing tiny things instead of having monster validators. Easier to maintain, to test, to isolate and set optional/inactive if you don't like the particular validation, etc.

In this case I'd actually just have the repair action log the error together with the node name it was trying to rename - just so that at least it's clear which node it was unable to rename due to being read only.