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

Fusion: provide better logging for validate saver crash due type error #6082

Merged
115 changes: 63 additions & 52 deletions openpype/hosts/fusion/plugins/publish/validate_saver_resolution.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,55 +8,6 @@
from openpype.hosts.fusion.api import comp_lock_and_undo_chunk


def get_tool_resolution(tool, frame):
"""Return the 2D input resolution to a Fusion tool

If the current tool hasn't been rendered its input resolution
hasn't been saved. To combat this, add an expression in
the comments field to read the resolution

Args
tool (Fusion Tool): The tool to query input resolution
frame (int): The frame to query the resolution on.

Returns:
tuple: width, height as 2-tuple of integers

"""
comp = tool.Composition

# False undo removes the undo-stack from the undo list
with comp_lock_and_undo_chunk(comp, "Read resolution", False):
# Save old comment
old_comment = ""
has_expression = False
if tool["Comments"][frame] != "":
if tool["Comments"].GetExpression() is not None:
has_expression = True
old_comment = tool["Comments"].GetExpression()
tool["Comments"].SetExpression(None)
else:
old_comment = tool["Comments"][frame]
tool["Comments"][frame] = ""

# Get input width
tool["Comments"].SetExpression("self.Input.OriginalWidth")
width = int(tool["Comments"][frame])

# Get input height
tool["Comments"].SetExpression("self.Input.OriginalHeight")
height = int(tool["Comments"][frame])

# Reset old comment
tool["Comments"].SetExpression(None)
if has_expression:
tool["Comments"].SetExpression(old_comment)
else:
tool["Comments"][frame] = old_comment

return width, height


class ValidateSaverResolution(
pyblish.api.InstancePlugin, OptionalPyblishPluginMixin
):
Expand Down Expand Up @@ -87,19 +38,79 @@ def process(self, instance):

@classmethod
def get_invalid(cls, instance):
resolution = cls.get_resolution(instance)
saver = instance.data["tool"]
try:
resolution = cls.get_resolution(instance)
except PublishValidationError:
resolution = None
kalisp marked this conversation as resolved.
Show resolved Hide resolved
expected_resolution = cls.get_expected_resolution(instance)
if resolution != expected_resolution:
saver = instance.data["tool"]
return [saver]

@classmethod
def get_resolution(cls, instance):
saver = instance.data["tool"]
first_frame = instance.data["frameStartHandle"]
return get_tool_resolution(saver, frame=first_frame)
return cls.get_tool_resolution(saver, frame=first_frame)

@classmethod
def get_expected_resolution(cls, instance):
data = instance.data["assetEntity"]["data"]
return data["resolutionWidth"], data["resolutionHeight"]

@classmethod
def get_tool_resolution(cls, tool, frame):
"""Return the 2D input resolution to a Fusion tool

If the current tool hasn't been rendered its input resolution
hasn't been saved. To combat this, add an expression in
the comments field to read the resolution

Args
tool (Fusion Tool): The tool to query input resolution
frame (int): The frame to query the resolution on.

Returns:
tuple: width, height as 2-tuple of integers

"""
comp = tool.Composition

# False undo removes the undo-stack from the undo list
with comp_lock_and_undo_chunk(comp, "Read resolution", False):
# Save old comment
old_comment = ""
has_expression = False

if tool["Comments"][frame] not in ["", None]:
if tool["Comments"].GetExpression() is not None:
has_expression = True
old_comment = tool["Comments"].GetExpression()
tool["Comments"].SetExpression(None)
else:
old_comment = tool["Comments"][frame]
tool["Comments"][frame] = ""
Comment on lines +85 to +92
Copy link
Collaborator

@BigRoy BigRoy Jan 26, 2024

Choose a reason for hiding this comment

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

Here the comment field is being overwritten - IF for whatever reason the user had a value there it would now be lost if the error would get raised below, because the value would not get reverted.

Which could throw meaningful info from the scene away if the user used that comment field.

What ACTUALLY happens even is that the "expression" that is being set:

tool["Comments"].SetExpression("self.Input.OriginalWidth")

Will now persist after the validation and suddenly the user has an expression on the comment field that they don't know about or shouldn't have.

Basically we're altering the user's scene in a way without taking care that it's reverted, which we shouldn't be doing.

We should be doing this through a context manager so that whatever happens, the original state gets reverted - even on the error.

BUT, even better - I'd argue that if the ACTUAL issue is that it errors on having no input connections that we explicitly capture that case - so that if for whatever other reason the resolution can't be computed we still get an error (if e.g. the node network evaluates with errors, or there's a memory overflow issue, etc.

# Get input width
tool["Comments"].SetExpression("self.Input.OriginalWidth")
if tool["Comments"][frame] is None:
raise PublishValidationError(
"Cannot get resolution info for frame '{}'.\n\n "
"Please check that saver has connected input.".format(
frame
)
)
kalisp marked this conversation as resolved.
Show resolved Hide resolved

width = int(tool["Comments"][frame])

# Get input height
tool["Comments"].SetExpression("self.Input.OriginalHeight")
height = int(tool["Comments"][frame])

# Reset old comment
tool["Comments"].SetExpression(None)
if has_expression:
tool["Comments"].SetExpression(old_comment)
else:
tool["Comments"][frame] = old_comment

return width, height