-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
iOS rendering changes #2263
iOS rendering changes #2263
Conversation
Filed as internal issue #USD-7999 |
build_scripts/build_usd.py
Outdated
@@ -726,24 +780,31 @@ def InstallBoost_Helper(context, force, buildArgs): | |||
dontExtract=dontExtract)): | |||
bootstrap = "bootstrap.bat" if Windows() else "./bootstrap.sh" | |||
|
|||
if context.targetIos: |
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 no link dependency on boost from iOS, because there's no support for python bindings to USD on iOS, right?
Can we simplify things by merely unarchiving boost, and pointing the iOS build at the headers, and calling it a day? In the unlikely event that one of the boost headers needs configuration for iOS, maybe we can just patch that header. But to the best of my recollection, even that shouldn't be necessary.
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'll give that a go
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.
That worked really well! Thank you so much!
build_scripts/build_usd.py
Outdated
extraJPEGArgs.append('-DCMAKE_SYSTEM_PROCESSOR=aarch64') | ||
extraJPEGArgs.append("-DENABLE_STATIC=TRUE") | ||
|
||
# Replace test and utility executables with static libraries to avoid issues with code signing. |
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.
Instead of fixing tj for them, can we instead suppress the creation of test and utility executables for tj, as they are unrelated to building and deploying USD. It makes sense to me to upstream fixes to tj overall.
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.
Disabled openimagiio for the time being - OpenEXR needs it.
build_scripts/build_usd.py
Outdated
@@ -1114,6 +1271,11 @@ def InstallTIFF(context, force, buildArgs): | |||
[("add_subdirectory(tools)", "# add_subdirectory(tools)"), | |||
("add_subdirectory(test)", "# add_subdirectory(test)")]) | |||
|
|||
if context.targetIos: |
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.
tbh, my opinion is that we should skip contrib on all platforms as people don't look to the USD project to build libtiff for them unless they are simply satisfying requirements of the oiio build... Not requesting an update though, as it's an existing behavior that contrib gets roped in.
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.
In our initial submission we could skip supporting OpenImageIO, although it would be nice - are you aware if any of OpenImageIO's dependencies are optional?
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.
errrmmm, 99% of them are? There are a lot of vendor package inclusions, documentation generators, Python dependencies, and OpenShadingLanguage. But I don't think we have to take particular action on that on behalf of iOS. On Mac, OCIO uses pybind11, not boost python thank goodness, so we are clean on the OCIO + boost front, as far as I can tell.
build_scripts/build_usd.py
Outdated
@@ -1135,6 +1297,24 @@ def InstallTIFF(context, force, buildArgs): | |||
def InstallPNG(context, force, buildArgs): | |||
with CurrentWorkingDirectory(DownloadURL(PNG_URL, context, force)): | |||
macArgs = [] | |||
if context.targetIos: |
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 just skip all this on ios instead of fixing libtiff for them? I can't think of any reason we'd want to create these utilities.
build_scripts/build_usd.py
Outdated
@@ -1161,13 +1341,160 @@ def InstallPNG(context, force, buildArgs): | |||
OPENEXR_URL = "https://github.com/AcademySoftwareFoundation/openexr/archive/refs/tags/v2.5.2.zip" | |||
else: | |||
OPENEXR_URL = "https://github.com/AcademySoftwareFoundation/openexr/archive/refs/tags/v2.4.3.zip" | |||
|
|||
def updateOpenEXRIOS(context, srcDir): | |||
# IlmBase |
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.
let's fix OpenEXR upstream. It's okay to depend on a later version of OpenEXR specifically for iOS. I'm able to make commits in the OpenEXR repo, so I can stand by to help get these fixes landed as quickly as possible.
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.
Agree to that!
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.
Hm, OpenImageIO will not respect -DUSE_OpenEXR=OFF
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.
So for the time being I believe we won't include openimageio
https://github.com/OpenImageIO/oiio/blob/master/INSTALL.md
from my side, it's a reasonable step to only use jpgs and minimal textures to begin with and openimagiio can then be added later
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.
That sounds like a good intermediate step in order to get things up and running.
build_scripts/build_usd.py
Outdated
@@ -1265,6 +1604,17 @@ def InstallOpenVDB(context, force, buildArgs): | |||
'-DOPENVDB_BUILD_BINARIES=OFF', | |||
'-DOPENVDB_BUILD_UNITTESTS=OFF' | |||
] | |||
if context.targetIos: | |||
extraArgs.append('Boost_USE_STATIC_LIBS=ON') |
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 think the reason you are patching the boost generation code above is to support OpenVdb here. How important is the support of OpenVdb on iOS at this time? To expedite things and avoid having to support boost moving forward, would it be a reasonable approach to skip OpenVdb for now? I personally would rather not have boost on iOS more than I personal would want OpenVdb support on iOS.
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.
For the time being, I agree it's more important to get the core functionality in and OpenVDB can join later - for users, it may a bit surprising to be able to attempt to build something not included - would you find it OK if I made a matrix for supported features by platform, and then we can explicitly tell the user it can't use that feature? We are seeing such suprises for users for OpenColorio for example, where nothing tells the user they can't use OCIO 1.x on Metal, but it however is not possible
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.
That's a good suggestion. By matrix, I think you are meaning documentation, as opposed to a matrix of features in the code? Perhaps the best place for it is in the documentation here https://github.com/PixarAnimationStudios/USD/blob/dev/BUILDING.md#optional-components
- that would be a separate changelist and we could update it once these changes land.
We could also emit diagnostics, as part of this changelist, so that if someone specifies for example OCIO 1.x, it could explicitly inform them of the impossibility of it.
build_scripts/build_usd.py
Outdated
@@ -1306,6 +1659,24 @@ def InstallOpenImageIO(context, force, buildArgs): | |||
'-DOIIO_BUILD_TESTS=OFF', | |||
'-DUSE_PYTHON=OFF', | |||
'-DSTOP_ON_WARNING=OFF'] | |||
if context.targetIos: |
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.
Once again, we could use a different version of OIIO on iOS. Can I request that we upstream fixes to OIIO, as I am sure @lgritz is able to assist integration.
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 don't have a way to test on iOS myself, but I'm pretty sure that over the years, people have contributed patches to OIIO to fix the iOS build, so I'm presuming that at some point it worked (maybe still?).
I'm not an iOS developer (nor a MacOS developer per se, except in the trivial sense that I use it all day long but almost entirely pretend it's just a generic Unix toolchain), so I don't know -- is it possible to build on MacOS but cross-compile for iOS, or even emulate iOS? If those tools are available on GitHub Actions runners, I'd be happy to add (that is, accept a PR from somebody who knows what they're doing) another entry to the test matrix for OIIO's CI, wherein we do an emulated iOS build just to ensure that it keeps working and we don't break it with future changes.
if MacOS():
# OIIO 2.3.15 adds fixes for Apple Silicon cross compilation.
OIIO_URL = "https://github.com/OpenImageIO/oiio/archive/refs/tags/v2.3.15.0.zip"
else:
OIIO_URL = "https://github.com/OpenImageIO/oiio/archive/Release-2.1.16.0.zip"
Oy, I am definitely available to advise anybody who would like to bring this up to a modern version. The current series is 2.4, and 2.1.16 dates from 2020.
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.
Here's an example of github CI for iOS ~ I don't know if it's a useful reference for OIIO's automation ~ but it does work for me :) https://github.com/LabSound/LabSound/blob/main/.github/workflows/cmake-ios.yml
Since OpenImageIO isn't one of the vfxplatform specified versions, I can look into why it's set at 2.1.16.
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.
Agree to do it upstream and we can start with the core components!
third_party/IlmBase/eLut.h
Outdated
@@ -0,0 +1,71 @@ | |||
// |
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.
please see my comments above about upstreaming OpenEXR fixes
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.
Agree - In the "modifications" commit, I'll remove OpenEXR and OpenVDB for the time being
Thank you for refactoring this commit chain, it's looking much more tractable now :) |
Also, the comment says "dependent on 2035" but 2035 has been closed. I think the meaning is that this PR supersedes 2035? |
Hi @slingthor, I’m interested to try this PR on iOS. Do you have indications on how to link this library to an Xcode project? I tried to import the lib and while it's working overall, I'm facing an issue where the HdStorm renderer does not clear its buffer after each frame. I'm hoping that this might be because of the way I'm implementing it. |
A pleasure! For the time being I could remove the components from the whole chain that we won't include in the initial support for iOS. |
@slingthor is this change by @jonny-apple relevant to this change? https://github.com/PixarAnimationStudios/USD/pull/2274/files I assume it is independent. Also I notice Jon updated a versions matrix there, so if that's the one you are referring to, another column for iOS makes sense... |
45810e9
to
52da947
Compare
Back when this came about, we actually solved the same problem at a similar time in different places - I'll try to align mine with the version matrix |
Hey! You would need to create an application that can display a surface Storm has rendered on your iOS screen - https://developer.apple.com/documentation/metal/metal_sample_code_library/creating_a_3d_application_with_hydra_rendering this example does it for MacOS, but you could modify it to run on iOS. You might face challenges with:
|
Thank you for your reply! I actually did something really similar in my test, can you confirm that HdStorm is working properly on your side? Happy to help find the cause if it's not only an issue on my side. |
@Volutionn I think maybe you just need to clear the frame buffer at the start of the render? In my renderers I do something like this ~ renderPassDescriptor.colorAttachments[0].loadAction = mustClear ? MTLLoadAction.clear : MTLLoadAction.load where |
16154e7
to
d830d6d
Compare
Thank you @meshula for your reply. I tried adding this line but it has no impact on the output... I actually thought that the buffer clearing is managed by Hydra ? What's weird also is that the exact same code when built for macOS, so also with USD built for macOS, does work perfectly without the need of adding this line. And, as you can see in my screenshot, the smooth shading does not work, but it does work on macOS with the same implementation. Can those issues be related to a bigger issue with one of the libraries ? |
Oh apologies! I assumed it had to do with the texture filtering which also had sort of a matrix - these changes are independent |
2ff71d7
to
a176aff
Compare
a176aff
to
e723dbb
Compare
#2263 by @slingthor (Internal change: 2263923)
@meshula Hope the PR is looking good by now! I will be in the near future submitting directly to the dependencies to simplify the build scripts for the additional dependencies, but the build without them I hope is looking good now! |
e723dbb
to
4900ced
Compare
Description of Change(s)
Allows building the CMAKE project
Allows rendering with iOS
Recreating this PR as it was quite risky and the history was hard to maintain the way it was set up.
In case of getting the work in, All the way up to step 9 is mandatory.
It would be our hope to get all the way to 7B in the possible future for all the expected features for iOS to be in.