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

OCIO DisplayViewTransform uses display name as colourspace if display_colorspace is <USE_DISPLAY_NAME> in OCIO config #3017

Conversation

jonahjnewton
Copy link

@jonahjnewton jonahjnewton commented Mar 26, 2024

OCIO DisplayViewTransform uses display name as colourspace if display_colorspace is <USE_DISPLAY_NAME> in OCIO config

Description of Change(s)

OCIO 2 allows View transforms in an OCIO config to set their display_colorspace to <USE_DISPLAY_NAME> to use the Display name to find the display colorspace. Usdview does not handle this, and therefore errors stating that it can't find a colorspace named <USE_DISPLAY_NAME>.

This change adds a simple check to use the display name as the colorspace in Usdview if the incoming colorspace is <USE_DISPLAY_NAME>.

Fixes Issue(s)

  • No existing issues that I can find
  • I have verified that all unit tests pass with the proposed changes
  • I have submitted a signed Contributor License Agreement

@jonahjnewton jonahjnewton changed the title OCIO DisplayViewTransform uses display transform if display_colorspace is <USE_DISPLAY_NAME> in OCIO config OCIO DisplayViewTransform uses display name as colourspace if display_colorspace is <USE_DISPLAY_NAME> in OCIO config Mar 26, 2024
@jesschimein
Copy link
Contributor

Filed as internal issue #USD-9485

@jesschimein
Copy link
Contributor

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tcauchois
Copy link
Contributor

tcauchois commented May 3, 2024

Hey Jonah,

My understanding from the OCIO folks is that <USE_DISPLAY_NAME> should be entirely an internal OCIO detail, and client code shouldn't need to worry about it at all. Can you give us some more information, like where the "<USE_DISPLAY_NAME>" is coming from (is it OCIO.GetCurrentConfig().getColorSpaces()?) and which error you're hitting exactly?

Thanks,
Tom

@jonahjnewton
Copy link
Author

jonahjnewton commented May 13, 2024

Hey Tom, I'm simply loading an OCIO config with a View that contains the <USE_DISPLAY_NAME> keyword, and then applying that view transform in the OCIO section of USDview. The error that comes up in the USDview terminal states that it can't find a colorspace named <USE_DISPLAY_NAME>, which I understand should be recognised by OCIO and then translated to the display colorspace.

The same OCIO install works in multiple other applications just fine, so I'm unsure why it would be happening only with USDview, but that's the issue I'm encountering.

@tcauchois
Copy link
Contributor

Got it, thanks. Could you paste the terminal output?

@jonahjnewton
Copy link
Author

I just get this line as the terminal output:
ERROR: Usdview encountered an error while rendering.DisplayViewTransform error. Cannot find source color space named '<USE_DISPLAY_NAME>'.

@tcauchois
Copy link
Contributor

The folks on OCIO slack suggested an alternate fix that I have written up locally, but if you could share some kind of minimal OCIO config for me to validate against that would be great.

@tcauchois
Copy link
Contributor

Or, if you'd like to try their proposal yourself: in appController.py, setOcioConfig, remove the line where we compute colorspace, and only pass "display" and "view" to setOcioSettings. Let me know how that works!

@jonahjnewton
Copy link
Author

Or, if you'd like to try their proposal yourself: in appController.py, setOcioConfig, remove the line where we compute colorspace, and only pass "display" and "view" to setOcioSettings. Let me know how that works!

Hey Tom, I just replicated this proposal on my side and it definitely works better than my fix. It updates the colorspaces more consistently, whereas I have found my fix can cause the display transform to be applied inconsistently.

Thanks for the update!

@tcauchois
Copy link
Contributor

tcauchois commented Jun 3, 2024

Awesome, thanks for the confirmation! I've checked in my fix and plan to take it instead of this PR, but once it lands please let me know if you have any further issues! And even though we're going with the other approach, thanks for the PR :). I'm happy to see the OCIO integration in a more robust place.

Once my fix lands in dev, it should get linked to this PR and the PR should be closed.

@jonahjnewton
Copy link
Author

Awesome, thanks for the confirmation! I've checked in my fix and plan to take it instead of this PR, but once it lands please let me know if you have any further issues! And even though we're going with the other approach, thanks for the PR :). I'm happy to see the OCIO integration in a more robust place.

Once my fix lands in dev, it should get linked to this PR and the PR should be closed.

Thanks a lot mate! Glad to see it fixed!

@pixar-oss pixar-oss closed this in 77eeacc Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants