-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Apple: Added HioImageIO as optional feature on macOS #3148
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
Apple: Added HioImageIO as optional feature on macOS #3148
Conversation
|
Filed as internal issue #USD-9827 ❗ Please make sure that a signed CLA has been submitted! |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Disregard the note above about missing a signed CLA; we do have your CLA on file, Alex. Sorry for any confusion! |
build_scripts/build_usd.py
Outdated
| if MacOS(): | ||
| group.add_argument("--imageio", dest="build_imageio", action="store_true", | ||
| default=False, | ||
| help="Build ImageIO plugin for USD (Uses ImageIO.framework). Overrides OpenImageIO") |
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.
"Overrides" is a little vague, since it could refer to runtime behavior. Instead, could we say "Disables building OpenImageIO" ?
7275e1b to
17ff229
Compare
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
c4c9a7f to
deb247e
Compare
|
I've updated the PR:
|
|
/AzurePipelines run |
tcauchois
left a comment
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.
Hey all,
I'm going to try to land this momentarily but we had a few requested changes to the CMake. If you're ok with the changes but would like me to make them I'm happy to do so as I pull it down. Alternatively, if you want me to hold on while you rev the PR let me know.
Thanks!
build_scripts/build_usd.py
Outdated
| and self.buildTests)) | ||
| and not embedded) | ||
| if MacOS(): | ||
| self.buildImageIO = args.build_imageio and not self.buildOIIO |
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.
Can we change this so that ImageIO and OpenImageIO don't turn each other off? i.e. get rid of the "and not self.buildOIIO" 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.
Sure, would you like me to document somewhere that they should ideally not be used together? Or just let Hio pick one itself at runtime?
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 ended up going with your suggestion below to make ImageIO take precedence.
build_scripts/build_usd.py
Outdated
| group.add_argument("--imageio", dest="build_imageio", action="store_true", | ||
| default=True, | ||
| help="Build the ImageIO.framework plugin for USD (default). " | ||
| "Will be turned off if the OpenImageIO plugin is enabled") |
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.
Comment update.
pxr/imaging/plugin/CMakeLists.txt
Outdated
|
|
||
| if (PXR_BUILD_OPENIMAGEIO_PLUGIN) | ||
| add_subdirectory(hioOiio) | ||
| elseif (APPLE AND PXR_BUILD_IMAGEIO_PLUGIN) |
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.
elseif -> if
| "HioImageIO_Image" : { | ||
| "bases": ["HioImage"], | ||
| "imageTypes": ["tif", "tiff", "zfile", "tx"], | ||
| "precedence": 10 |
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.
If we build both OpenImageIO and ImageIO, we choose the plugin to use based on precedence. Since hioOiio is "10", we could bump this to like "15" and then it would be used preferentially.
|
Either way works for me! I can take a pass in the morning so you don't have to. I think your changes all sound good to me and simplify things |
deb247e to
4f5357c
Compare
|
Okay, I've rebased on latest dev + addressed your changes. |
|
Two more documentation suggestions, which I can make inline or you can approve here. |
Co-authored-by: Tom <[email protected]>
Co-authored-by: Tom <[email protected]>
|
Thanks @tcauchois , I just accepted your changes. |
Added HioImageIO as optional feature to replace OIIO with ImageIO on macOS. Credit goes to Nicholas Blasingame. Closes PixarAnimationStudios#3148 (Internal change: 2368537) Co-authored-by: Dhruv Govil <[email protected]>
Description of Change(s)
Added HioImageIO as optional feature to replace OIIO with ImageIO on macOS. Credit goes to Nicholas Blasingame.
Fixes Issue(s)