Skip to content
This repository has been archived by the owner on Sep 20, 2024. It is now read-only.

fix the bug of failing to extract look when UDIMs format used in AiImage #3628

Merged
merged 18 commits into from
Aug 15, 2022

Conversation

moonyuet
Copy link
Member

@moonyuet moonyuet commented Aug 8, 2022

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.
image

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 of file_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 space
This 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.

@ynbot
Copy link
Contributor

ynbot commented Aug 8, 2022

Task linked: OP-3531 Maya aiFile and UDIMs

Comment on lines 437 to 439
# set color space to Raw if its attribute is raw.
if cmds.getAttr(color_space_attr) == "Raw":
color_space = "Raw"
Copy link
Collaborator

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.

Copy link
Member Author

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

Copy link
Collaborator

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"

Copy link
Member Author

@moonyuet moonyuet Aug 8, 2022

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

@moonyuet moonyuet requested a review from BigRoy August 8, 2022 14:20
Copy link
Member

@m-u-r-p-h-y m-u-r-p-h-y left a comment

Choose a reason for hiding this comment

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

it is publishing fine with <UDIM> token

image

color_space = "Raw"
else:
# if the files are unresolved
if files_metadata[filepath]["color_space"] == "Raw":
Copy link
Collaborator

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?

Copy link
Collaborator

@BigRoy BigRoy left a 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?)

@BigRoy
Copy link
Collaborator

BigRoy commented Aug 12, 2022

I'd probably fix the else clause like this:

                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 files_metadata just above here

@moonyuet
Copy link
Member Author

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"].
I think you are right. I should implement to find the unresolved path by creating an interator through resource.get["files"] and then normalize the path. This way would help to find the path in a more accurate and safe manner, while it wont break the codes. I have committed the branch with the updated code. Please have a look! Thanks a lot for helping me on this!

@BigRoy
Copy link
Collaborator

BigRoy commented Aug 12, 2022

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"

Comment on lines 432 to 453
# 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"
Copy link
Collaborator

@BigRoy BigRoy Aug 12, 2022

Choose a reason for hiding this comment

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

Suggested change
# 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.

Copy link
Member Author

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)

Copy link
Collaborator

@BigRoy BigRoy Aug 12, 2022

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.

Copy link
Member Author

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

@moonyuet moonyuet requested a review from BigRoy August 12, 2022 13:48
Copy link
Collaborator

@BigRoy BigRoy left a comment

Choose a reason for hiding this comment

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

⚠️ Haven't tested the code - but the code looks good to me.

Copy link
Member

@m-u-r-p-h-y m-u-r-p-h-y left a comment

Choose a reason for hiding this comment

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

tested again to make sure it is working correctly

now changed to Maya2023 and used <UDIM> and <udim> tokens and everything works as expected, including look assignment after publish.

image

@antirotor antirotor merged commit 0b0bc57 into ynput:develop Aug 15, 2022
@moonyuet moonyuet deleted the bugfix/OP-3531_Maya-aiImage-and-UDIM branch December 13, 2022 02:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants