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

upgrade maya-usd project to the C++14 standard #438

Merged
merged 3 commits into from
Apr 17, 2020

Conversation

mattyjams
Copy link
Contributor

Re-opening this from the original #395 pull request...

Conforming to VFX Reference Platform CY2018+ and following suit from core USD, this change sets the maya-usd project to use the C++14 standard if we're either building for Maya 2019 or later, or if we're building against USD 20.05 or later.

Core USD moved to C++14 with release 20.05 in this commit:
PixarAnimationStudios/OpenUSD@afc0d5e

Conforming to VFX Reference Platform CY2018+ and following suit from core USD,
this change sets the maya-usd project to use the C++14 standard if we're either
building for Maya 2019 or later, or if we're building against USD 20.05 or
later.

Core USD moved to C++14 with release 20.05 in this commit:
PixarAnimationStudios/OpenUSD@afc0d5e
@kxl-adsk kxl-adsk added the build Related to building maya-usd repository label Apr 16, 2020
Copy link
Contributor

@HamedSabri-adsk HamedSabri-adsk left a comment

Choose a reason for hiding this comment

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

Thank you @mattyjams. lgtm!

# building against USD 20.05 or later. Otherwise require C++11.
if ((MAYA_APP_VERSION VERSION_GREATER_EQUAL 2019) OR
((DEFINED USD_VERSION_NUM) AND
(USD_VERSION_NUM VERSION_GREATER_EQUAL 2005)))
Copy link
Contributor

@HamedSabri-adsk HamedSabri-adsk Apr 16, 2020

Choose a reason for hiding this comment

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

@mattyjams Just 1 minor thing, do we really need (DEFINED USD_VERSION_NUM)? Why do we need to check for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was a little wary of that myself, but we already had something similar lower in that file:

if (DEFINED USD_VERSION_NUM)

It looks like it's technically possible to make it out of FindUSD.cmake without having gotten a value for USD_VERSION_NUM:

math(EXPR USD_VERSION_NUM "${USD_MAJOR_VERSION} * 10000 + ${USD_MINOR_VERSION} * 100 + ${USD_PATCH_VERSION}")

Maybe that's the case for especially old releases of USD? I can't remember when that was added.

Copy link
Contributor

@HamedSabri-adsk HamedSabri-adsk Apr 16, 2020

Choose a reason for hiding this comment

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

Not a biggie at all but I believe it is safe to not check for this and the reason is that this variable always exist as part of logic that you showed me in FindUSD.cmake. This variable was introduced by Paul ( 848de8f ) back in October of last year.

The reason I brought was because for certain variables ( e.g UFE_PREVIEW_VERSION_NUM ), we do want to check because they simply don't exist when building against certain version of things.

See my comments here:

#412 (comment)

Cheers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, but since USD_VERSION_NUM is only set inside an if() block, it's technically possible for it not to be set:

if(USD_INCLUDE_DIR AND EXISTS "${USD_INCLUDE_DIR}/pxr/pxr.h")

It would admittedly have to be for the pretty bizarre reason that pxr/pxr.h was missing even though USD_INCLUDE_DIR is set, since the find_package_handle_standard_args() call validates USD_INCLUDE_DIR. The build is likely to fail in some spectacular way if that's the case.

I can add USD_VERSION and USD_VERSION_NUM as REQUIRED_VARs to find_package_handle_standard_args() and remove the defined() checks? It might be better anyway if we fail faster during CMake config, as that would hopefully be easier to diagnose than some incomprehensible compile failure later on.

Copy link
Contributor

@HamedSabri-adsk HamedSabri-adsk Apr 16, 2020

Choose a reason for hiding this comment

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

Cool, that makes sense to me. Good call!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. Done in bc4d81f and b7a7f20.

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome! Thank you again for quick respond.

These CMake variables are used in many places throughout the code base, so the
build is likely to break in ways that would be difficult to decipher if they
happen to be undefined. This change marks them as required so that the failure
occurs sooner and can be debugged more easily.
USD_VERSION_NUM is now a required variable from FindUSD.cmake, so there is no
longer a need to check whether it is defined before using it.
@kxl-adsk kxl-adsk merged commit fa1118e into Autodesk:dev Apr 17, 2020
@mattyjams mattyjams deleted the pr/upgrade_to_cpp14_standard branch April 17, 2020 22:27
ppt-adsk added a commit that referenced this pull request Sep 27, 2023
* Run mesh tests in mesh adapter mode.

* Added mesh adapter test capability, and refactored test case base classes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Related to building maya-usd repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants