-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat: Add custom OCIO config to job attachments when enabled #16
Conversation
571ff43
to
898252a
Compare
49afb3d
to
03bbcd5
Compare
d5f78e6
to
d35e0cb
Compare
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.
LGTM for a first implementation, I would create a follow up ticket regarding OCIO env vars, or fix it here.
|
||
asset_references.input_filenames.add(ocio_config_path) | ||
asset_references.input_directories.add(ocio_config_luts_dir) | ||
|
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.
LGTM if this adds all directories inside the path recursively
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.
Adding them as input directory adds the whole directory tree, yes.
"""True if the script is using a custom OCIO config""" | ||
return ( | ||
nuke.root().knob("colorManagement").value() == "OCIO" | ||
and nuke.root().knob("OCIO_config").value() == "custom" |
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's one tricky thing wih OCIO which is it also uses env vars to define library settings
https://opencolorio.readthedocs.io/en/latest/guides/using_ocio/using_ocio.html
Those will override the knob settings. Maybe there's a specific Nuke call to grab the OCIO env var it's using taking env vars into account
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.
Another approach is to just forward OCIO* env vars to the worker and let Nuke do its thing.
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 will file an internal ticket for forwarding OCIO variables
Also, I think you need to squash all the commits and make the comment there follow the "conventional commits" rules (as you did for the PR title) |
d023c97
to
bdfa4a7
Compare
Signed-off-by: Alex Gerveshi <15710060+agerveshi@users.noreply.github.com>
Signed-off-by: Alex Gerveshi <15710060+agerveshi@users.noreply.github.com>
Signed-off-by: Alex Gerveshi <15710060+agerveshi@users.noreply.github.com>
Signed-off-by: Alex Gerveshi <15710060+agerveshi@users.noreply.github.com>
Signed-off-by: Alex Gerveshi <15710060+agerveshi@users.noreply.github.com>
Signed-off-by: Alex Gerveshi <15710060+agerveshi@users.noreply.github.com>
Signed-off-by: Alex Gerveshi <15710060+agerveshi@users.noreply.github.com>
Signed-off-by: Alex Gerveshi <15710060+agerveshi@users.noreply.github.com>
Signed-off-by: Alex Gerveshi <15710060+agerveshi@users.noreply.github.com>
Signed-off-by: Alex Gerveshi <15710060+agerveshi@users.noreply.github.com>
Signed-off-by: Alex Gerveshi <15710060+agerveshi@users.noreply.github.com>
c895e66
to
ce6d393
Compare
{} | ||
search_path: | ||
- luts | ||
- /tmp/luts |
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 see that we're providing both of these as input directories for job attachments. Does Nuke apply the path mapping to these search paths automatically, or do we have to do some special processing?
Also, do any of the LUT file formats contain further references to additional files?
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 added both search paths to this config so we could test relative and absolute. In the relative case OCIO will search relative to the config file's directory. Is that what you mean by path mapping?
Good question about the LUT files, let me take a look.
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.
LUT files do not seem to contain references to other files.
|
||
asset_references.input_filenames.add(ocio_config_path) | ||
asset_references.input_directories.add(ocio_config_luts_dir) | ||
|
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.
Adding them as input directory adds the whole directory tree, yes.
What was the problem/requirement? (What/Why)
When auto-detecting job attachments, the script file from the root node and then all file knobs from the other nodes in the graph are considered. Additionally, a custom OCIO config file can be specified on the root node. When enabled, this config file and it's corresponding LUTs should also be added as job attachments.
What was the solution? (How)
When auto-detecting job attachments, determine if the script is configured to use a custom OCIO config. Load the config in to get the LUT search path. Add the config file and the LUTs directory as job attachments.
What is the impact of this change?
Users will be able to use custom OCIO configs without having to manually add the config file and LUTs directory as job attachments.
How was this change tested?
Did you run the "Job Bundle Output Tests"? If not, why not? If so, paste the test results here.
Yes, includes result of added test.
Was this change documented?
No
Is this a breaking change?
No, this is an additive change.