-
Notifications
You must be signed in to change notification settings - Fork 129
fix the bug of failing to extract look when UDIMs format used in AiImage #3628
fix the bug of failing to extract look when UDIMs format used in AiImage #3628
Conversation
Task linked: OP-3531 Maya aiFile and UDIMs |
# set color space to Raw if its attribute is raw. | ||
if cmds.getAttr(color_space_attr) == "Raw": | ||
color_space = "Raw" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines of code actually wouldn't be needed because we only enter the else:
clause of the try.. except.. else
earlier when the try..
succeeds without a captured error. As such color_space = "Raw"
already.
However, I think we're solving this the wrong way anyway and we're "patching" an issue that originates elsewhere.
I have the feeling as well that we should instead be solving the issue as to why source
doesn't work with aiImage
to begin with. This actually sounds more like the resource isn't being collected correctly to begin with and might thus in the future run into other issues that might expect the resource itself to be collected correctly.
The question thus becomes why does a file
node with UDIM sequence get collected correctly as a resource and why does aiImage
with UDIM does not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually try removing the condition and just set else: color_space = "Raw"
before but I am not very certain of the condition being relevant to associate any data. But after you mention about the "wrong way" and issues which originates elsewhere, I'd prefer more concise solution for this issue instead of being cliche with both exception handling and conditon.
I agree that there might be running into other issues which is related to collecting resources if the resource isn't being collected correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might have misinterpreted but now the code is actually overriding color_space
whenever it wasn't "Raw" to begin with - the only example I described that would be equivalent is the one of:
# set color space to Raw if its attribute is raw.
if cmds.getAttr(color_space_attr) == "Raw":
color_space = "Raw"
If the attribute was set as "Raw" then color_space = "Raw"
was already the case.
The current PR state after 38c35a8 will break other things.
Anyway, it doesn't matter much since what we're looking to solve anyway is why files_metadata[source]["color_space"]
didn't have the source
entry to begin with.
We'll need to look into where the files_metadata[source]
entry is generated in the first place. And we'll need to make sure that the source is actually correctly added there already so that the original logic below would actually work as expected:
if files_metadata[source]["color_space"] == "Raw":
# set color space to raw if we linearized it
color_space = "Raw"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Roy. I took a look at how the files_metadata
is generated as I see it is implemented as a dictionary set with a normalized path with colors_space
inside it. It is generated through two loops(one is resource
iterating through resources
, the other is f
iterating through resources[files]
)
files_metadata = {}
for resource in resources:
# Preserve color space values (force value after filepath change)
# This will also trigger in the same order at end of context to
# ensure after context it's still the original value.
color_space = resource.get("color_space")
for f in resource["files"]:
files_metadata[os.path.normpath(f)] = {
"color_space": color_space}
And then I saw this one with a filepath
iterating through files_metadata
:
if do_maketx and files_metadata[filepath]["color_space"].lower() == "srgb": # noqa: E501
linearize = True
# set its file node to 'raw' as tx will be linearized
files_metadata[filepath]["color_space"] = "Raw"
I also take a look at where [source]
is generated
for resource in resources:
source = os.path.normpath(resource["source"])
if source not in destinations:
# Cache destination as source resource might be included
# multiple times
destinations[source] = self.resource_destination(
instance, source, do_maketx
)
I guess we need to figure out where resource["source"] from.
EDIT: the guy who reports the issue has mentioned that the UDIMs in AiImage is considered as unresolved path in Maya File Path Editor. I guess I can set up a if condition after else
to deal with this issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
color_space = "Raw" | ||
else: | ||
# if the files are unresolved | ||
if files_metadata[filepath]["color_space"] == "Raw": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say this is broken. Isn't this filepath
variable by pure luck the "last iteration" in the files_metadata
for loop and thus likely not the filepath for the resources
being iterated here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment. (I might have misread, but I couldn't see where filepath
was coming from that was correctly related to the resource. Do I need more coffee?)
I'd probably fix the else:
# if the files are unresolved from `source`
# assume color space from the first file of
# the resource
first_file = next(iter(resource.get("files", [])), None)
if not first_file:
# No files for this resource? Can this happen? Should this error?
continue
filepath = os.path.normpath(first_file) Of course consider that pseudocode. But at least that key then matches how it's generated in the |
Hi Roy. It could be me misreading some of the codes. I thought when f interating through resource["files"] would indicate some information inside files_metadata[os.path.normpath(f)] while I saw another iterations of filepaths iterating through files_metadata, i thought filepath somehow store a path information as it is an iteration from the files_metadata, which store the normalized filepaths when f iterating through resource["files"]. |
Thanks. By just looking at the code it does look like this would work. @m-u-r-p-h-y what do you think? I'm wondering whether refactoring it to this would be more readable for someone reading the code: # ensure color space is "Raw" if we 'linearized' it
# in the conversion process, like .tx or alike
metadata = files_metadata.get(source)
if not metadata:
# if metadata is unresolved from `source` use
# the metadata of first file of the resource
first_file = next(iter(resource.get("files", [])), None)
if not first_file:
continue
first_filepath = os.path.normpath(first_file)
metadata = files_metadata[first_filepath]
if metadata["color_space"] == "Raw":
# set color space to raw if we linearized it
color_space = "Raw" |
# get all the resolved files | ||
metadata = files_metadata.get(source) | ||
if metadata: | ||
metadata = files_metadata[source] | ||
if metadata["color_space"] == "Raw": | ||
# set color space to raw if we linearized it | ||
color_space = "Raw" | ||
else: | ||
# if the files are unresolved from `source` | ||
# assume color space from the first file of | ||
# the resource | ||
metadata = files_metadata.get(source) | ||
if not metadata: | ||
first_file = next(iter(resource.get( | ||
"files", [])), None) | ||
if not first_file: | ||
continue | ||
first_filepath = os.path.normpath(first_file) | ||
metadata = files_metadata[first_filepath] | ||
if metadata["color_space"] == "Raw": | ||
# set color space to raw if we linearized it | ||
color_space = "Raw" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# get all the resolved files | |
metadata = files_metadata.get(source) | |
if metadata: | |
metadata = files_metadata[source] | |
if metadata["color_space"] == "Raw": | |
# set color space to raw if we linearized it | |
color_space = "Raw" | |
else: | |
# if the files are unresolved from `source` | |
# assume color space from the first file of | |
# the resource | |
metadata = files_metadata.get(source) | |
if not metadata: | |
first_file = next(iter(resource.get( | |
"files", [])), None) | |
if not first_file: | |
continue | |
first_filepath = os.path.normpath(first_file) | |
metadata = files_metadata[first_filepath] | |
if metadata["color_space"] == "Raw": | |
# set color space to raw if we linearized it | |
color_space = "Raw" | |
metadata = files_metadata.get(source) | |
if not metadata: | |
# if the files are unresolved from `source` | |
# assume metadata from the first file of | |
# the resource | |
first_file = next(iter(resource.get( | |
"files", [])), None) | |
if not first_file: | |
continue | |
first_filepath = os.path.normpath(first_file) | |
metadata = files_metadata[first_filepath] | |
if metadata["color_space"] == "Raw": | |
# set color space to raw if we linearized it | |
color_space = "Raw" |
You seemed to have some duplicated code in there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. I will remove the duplicated line of metadata = files_metadata.get(source)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also the duplicated usage of this:
if metadata["color_space"] == "Raw":
# set color space to raw if we linearized it
color_space = "Raw"
My suggested code snippet above resolves that as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah i see. will fix it as soon as possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brief description
When the user added a texture with AiImage with UDIMs format and assign the look instance to the mesh with the texture, it will result to the KeyError when publishing the look.
Root cause
the system cannot find the texture path when passing through the if-condition code:
if files_metadata[source]["color_space"] == "Raw"
. This error contributes to the Maya File Path Editor, which maya cannot resolve the udim path in AiImage.Solution
Introduce exception handling to tell the system to reference the color_space attribute of the AiImage node itself to set up the color_space to raw specifically for the AiImage case.UPDATE: Get the resolved files from
resource.get("source")
and tell the system to do the condition offile_metadata[source]["color_space"] == "Raw"
to set color_space if the files are considered as resolved. If the files are considered as unresolved, the system would generally look at the filepath before self._process_texture(processing the texture)file_metadata[filepath]["color_space"] == "Raw"
and set the color spaceThis is generally not the best way to deal with the issue/bug . Some conversations with Autodesk regarding to the Maya File Path Editor may be needed to "solve" this.