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

Always write color space to usd texture prims #2548

Merged
merged 2 commits into from
Aug 26, 2022

Conversation

dgovil
Copy link
Collaborator

@dgovil dgovil commented Aug 16, 2022

This changes the behaviour of the texture prim writer to always write the colorSpace attribute, whereas currently they are elided if they match the default.
This is important for us, and other software vendors, who have software that needs to ingest data from a multitude of pipelines with different defaults. This means we currently have to apply heuristics to figure out the default for a given texture, but it would be most ideal to be able to just always write out the intended color space into the file.

I think this would be a beneficial default for Maya USD in general, because even in a film studio, we might share assets between shows that have different OCIO defaults, so it's beneficial to know that information up front as much as possible.

@seando-adsk seando-adsk added the import-export Related to Import and/or Export label Aug 19, 2022
Copy link
Collaborator

@JGamache-autodesk JGamache-autodesk left a comment

Choose a reason for hiding this comment

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

Goes against having sparse USD files, but is indeed necessary to prevent any confusion at this point in time.

@dgovil
Copy link
Collaborator Author

dgovil commented Aug 23, 2022

@JGamache-autodesk yeah I agree. The only other options I could think of are:

  • stage level color space metadata. It would be out of spec though.
  • use an env var to let people decide to write it only if it's not the default.

But this felt like the least complex solution in the interim

@JGamache-autodesk
Copy link
Collaborator

@dgovil I like your suggestion of using an env var to control the verbosity.

@dgovil
Copy link
Collaborator Author

dgovil commented Aug 23, 2022

Any preference for what it should be called and what the default behaviour should be?

IMHO the default should be "always write colorspace" because it's least surprising, even if it results in more data, with the environment variable toggling it back to its old behaviour.

That would also help us out when we're reading files that come from multiple locations/users etc to pick the right color settings.

@JGamache-autodesk
Copy link
Collaborator

Hi @dgovil, after consulting with some of our pipeline/color experts, it looks like USD would benefit from adding ACEScg in the list of valid values for the sourceColorSpace attribute, or simply not define a set of valid values. That will have to be discussed with Pixar so we can stop using metadata for color space, since this is not transported by Hydra.

In the meantime, can you please add a MAYAUSD_EXPORT_VERBOSE_COLORSPACE_METADATA Tf env var, with ON as the default to control the new behavior you implemented. Thanks!

@dgovil
Copy link
Collaborator Author

dgovil commented Aug 24, 2022

@JGamache-autodesk , added the environment variable to control the behaviour

Copy link
Collaborator

@JGamache-autodesk JGamache-autodesk left a comment

Choose a reason for hiding this comment

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

Thanks!

@JGamache-autodesk JGamache-autodesk added the ready-for-merge Development process is finished, PR is ready for merge label Aug 25, 2022
@JGamache-autodesk
Copy link
Collaborator

Preflight issue is known and on our side. Ready for merge.

@seando-adsk seando-adsk merged commit 9cc177d into Autodesk:dev Aug 26, 2022
JGamache-autodesk added a commit that referenced this pull request Aug 29, 2022
@dgovil dgovil deleted the always_write_colorspace branch September 3, 2022 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
import-export Related to Import and/or Export ready-for-merge Development process is finished, PR is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants