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

Support compiling against imath #1829

Closed
wants to merge 3 commits into from

Conversation

dgovil
Copy link
Collaborator

@dgovil dgovil commented Apr 7, 2022

Description of Change(s)

This is an attempt to allow USD to build against the newer imath 3 libs as well as the older openexr 2 setup.
I've verified building this against imath 3.1.4 and openexr 2.4.1 and 2.5.7.
Note: I've only verified this with the alembic plugin enabled. I currently do not build with the OpenVDB or OIIO bits enabled, however they should work the same way. I

Fixes Issue(s)

-

  • I have submitted a signed Contributor License Agreement (I should be under the Apple CLA)

@dgovil dgovil changed the base branch from release to dev April 7, 2022 01:02
@dgovil dgovil changed the base branch from dev to release April 7, 2022 01:03
@jilliene
Copy link

Filed as internal issue #USD-7316

Copy link
Contributor

@tallytalwar tallytalwar left a comment

Choose a reason for hiding this comment

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

Hi @dgovil

Thanks for sending this PR in.

I am putting together some thoughts in this review (including pointers from an internal review at Pixar).

  • Could you split the logic for Imath library in its own Find module (FindImath.cmake)? And then incorporating debug build support, as noted by Nick?

  • OpenExr separates out imath from version 3.0 onwards. VFX Platform 2021 is when OpenEXR is bumped to version 3.1. However in the up coming 22.08 USD release we will be moving to VFX Platform 2020, which still has OpenEXR marked as version 2.4. Though your change handles the versions appropriately by providing fallback, we feel this change might be too late in the game for 22.08 release, but we might consider it post 22.08 release.

Thanks

@@ -33,6 +33,18 @@ DOC
"OpenEXR headers path"
)

find_path(IMATH_INCLUDE_DIR
Imath/half.h
Copy link
Contributor

Choose a reason for hiding this comment

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

@meshula noted in an internal review here at Pixar that the find module is missing debug libraries (which are named with a trailing _d). Because of this it won't be possible to debug in imath or exr on mac/linux and won't be able to link on windows with a debug build.

Nick suggested that OpenEXR 3 has got an OpenEXRConfig.cmake output file that takes care of this debug builds, but it'll be some time before we're able to take advantage of that, since vfx2021 is when openexr 3.1 is introduced.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay this should now support looking for Imath debug files if doing a debug build or if a variable is set. @meshula does this look okay to you now?

@dgovil
Copy link
Collaborator Author

dgovil commented Jun 30, 2022

That sounds reasonable. I'll try and get on those notes ASAP, though I assume since it won't make it for 22.8, that there's no immediate urgency to it. I'll try and have the changes soon

@dgovil
Copy link
Collaborator Author

dgovil commented Aug 5, 2022

Hi , I just wanted to respond here and say I'm still going to look into this, but we had a bunch of work come up recently. I'll hit this up after I'm back from SIGGRAPH and hope to see many of y'all there too.

@tallytalwar
Copy link
Contributor

No worries @dgovil. We have a couple of months before the next release :D.

@dgovil dgovil changed the base branch from release to dev August 22, 2022 22:54
@dgovil
Copy link
Collaborator Author

dgovil commented Aug 22, 2022

Just picking this up again now that SIGGRAPH is done and I've finally caught up and rested

@dgovil
Copy link
Collaborator Author

dgovil commented Aug 23, 2022

@tallytalwar The requested changes should be in now. let me know if anything stands out.

pixar-oss pushed a commit that referenced this pull request Sep 3, 2022
   This change is a revised version of github PR: #1829

   - Use Imath's config package (https://github.com/AcademySoftwareFoundation/Imath/blob/main/docs/PortingGuide2-3.md#openexrimath-3x-only) instead of introducing a new FindImath.cmake,
     this allows for use of modern cmake design.
   - Attemps to find Imath and if not found, fallbacks to find OpenEXR.
   - Note on using Alembic plugin: Either OpenEXR or Imath is required depending on which library is used by the Alembic library specified in ALEMBIC_DIR.
   - Updates pxrConfig.cmake.in to set Imath_DIR accordingly to find the associated import targets which were used for this USD build.

   Fixes #1591

(Internal change: 2247708)
pixar-oss pushed a commit that referenced this pull request Sep 3, 2022
…SD build or not.

This is a followup fix for: 2247708

Fixes #1829

(Internal change: 2247772)
@tallytalwar
Copy link
Contributor

tallytalwar commented Sep 3, 2022

Thanks @dgovil, a modified version of this 2818017 was pushed to dev. Closing the PR in favor of that.

@tallytalwar tallytalwar closed this Sep 3, 2022
@jonahjnewton
Copy link

jonahjnewton commented Feb 29, 2024

Noting here that pxr/usd/plugin/sdrOsl/CMakeLists.txt also needs a similar fix to build with Imath @tallytalwar .

(Filed as issue #2977)

#pxr/usd/plugin/sdrOsl/CMakeLists.txt

set(PXR_PREFIX pxr/usd)
set(PXR_PACKAGE sdrOsl)

if(NOT PXR_ENABLE_OSL_SUPPORT)
    return()
endif()

if (Imath_FOUND)
    set(__OSL_IMATH_LIBS "Imath::Imath")
else()
    set(__OSL_IMATH_INCLUDE ${OPENEXR_INCLUDE_DIRS})
endif()

pxr_plugin(sdrOsl
    LIBRARIES
        gf
        tf
        vt
        ar
        ndr
        sdr
        ${OSL_QUERY_LIBRARY}
        ${OIIO_LIBRARIES}
        ${__OSL_IMATH_LIBS}

    INCLUDE_DIRS
        ${__OSL_IMATH_INCLUDE}
        ${OSL_INCLUDE_DIR}
        ${OIIO_INCLUDE_DIRS}

    PUBLIC_CLASSES
        oslParser

    PRIVATE_HEADERS
        api.h

    PYTHON_CPPFILES
        moduleDeps.cpp

    PYMODULE_CPPFILES
        module.cpp
        wrapOslParser.cpp

    PYMODULE_FILES
        __init__.py

    RESOURCE_FILES
        plugInfo.json
)

pxr_test_scripts(
    testenv/testOslParser.py
)

pxr_install_test_dir(
    SRC testenv/testOslParser.testenv
    DEST testOslParser
)

pxr_register_test(testOslParser
    PYTHON
    COMMAND "${CMAKE_INSTALL_PREFIX}/tests/testOslParser"
    EXPECTED_RETURN_CODE 0
)

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.

4 participants