Skip to content

build: SG-45024 - Support C++20 - #1399

Open
cedrik-fuoco-adsk wants to merge 11 commits into
AcademySoftwareFoundation:mainfrom
cedrik-fuoco-adsk:SG-45024-cpp20
Open

build: SG-45024 - Support C++20#1399
cedrik-fuoco-adsk wants to merge 11 commits into
AcademySoftwareFoundation:mainfrom
cedrik-fuoco-adsk:SG-45024-cpp20

Conversation

@cedrik-fuoco-adsk

@cedrik-fuoco-adsk cedrik-fuoco-adsk commented Sep 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

SG-45024 - Support C++20

Linked issues

none

Summarize your change.

  • Setting CPP version based on the VFX platform year.
  • Ignore warning from PySide6. A lot of warning was logged on MacOS for PySide6.
  • Stop using eigen for Imath with custom code to cover the functionality

OpenRV-pub PR: shotgunsoftware/openrv-pub#22

Eigen will be re move from OpenRV-pub for both CY2025 and CY2026.

Describe the reason for the change.

Fix issues while compiling OpenRV with C++20 while staying backwards compatible with C++17.

Describe what you have tested and on which operating system.

MacOS and Linux
Testing Windows compilation with CI

cedrik-fuoco-adsk and others added 8 commits September 3, 2026 12:34
@cedrik-fuoco-adsk
Set C++ 20 as the default
6cea320 
Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
@cedrik-fuoco-adsk
Rename requires to requires_ since this is a reserved keyword in C++17
b70288f 
Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
@cedrik-fuoco-adsk
build: Drop vendored Eigen dependency from MuMathLinear
6eb43da 
MuMathLinear was the only consumer of the Eigen headers in src/pub, and
used them for six operations: 3x3 and 4x4 multiply and inverse, plus a
generic inverse and transpose over arbitrarily sized fixed arrays.

Use Imath for the fixed size paths. It is already a hard dependency and
was already on this target's include path. gjInverse() is used rather
than inverse() because it makes no assumption about matrix convention --
RV stores translation in the last column, while Imath's affine fast path
assumes the last row.

The generic ?fixed_array overloads have no Imath equivalent, so they now
use Gauss-Jordan elimination with partial pivoting.

Two behavior changes:

- Singular matrices return the identity instead of Eigen's inf/NaN, now
  consistent across all four inverse functions.
- The generic overloads verify their argument is a square two dimensional
  array. Previously size(1) was read unconditionally, which indexes
  dimensions() out of bounds for a one dimensional argument, and Eigen's
  dynamic inverse() on a non-square map relied on an assert compiled out
  under NDEBUG. This also fixes transpose_mXX mapping its output with the
  input's dimensions.

Verified with mu-interp that a non-affine projection matrix, an
RV-convention translation matrix and a general 3x3 each round-trip to
exact identity through inverse(), that A * B remains the A-times-B
product rather than its transpose, and that the generic 5x5 inverse and
transpose are unchanged.

Removing the vendored copy is a separate change in openrv-pub.

Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
@cedrik-fuoco-adsk
build: silence -Wcast-function-type-mismatch in shiboken6 generated b…
69f7390 
…indings

Patch ShibokenHelpers.cmake during PySide6 preparation to add
-Wno-error=cast-function-type-mismatch and
-Wno-cast-function-type-mismatch alongside the existing GNU warning
flags. The generated CPython-style PyMethodDef casts otherwise
produce thousands of Clang warnings.

Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
@cedrik-fuoco-adsk
build: only silence -Wcast-function-type-mismatch for Clang in shiboken6
f0fa2fc 
GCC does not recognize the Clang-specific flag, so add it only in the
Clang branch of ShibokenHelpers.cmake rather than unconditionally.

Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
@cedrik-fuoco-adsk @claude
style: reflow code left mangled by the requires -> requires_ rename
df652f8 
Commit 0d53c20 renamed the `requires` members to `requires_`, but the
clang-format run of that commit had been done while the identifier was
still `requires`, which clang-format treats as the C++20 requires-clause
keyword. The result was badly wrapped expressions such as:

    entry.
        requires_
    = mode.
          requires_;

Now that the identifier is no longer a keyword, clang-format collapses
these back to normal statements. Applying it here means the affected
files stop failing the clang-format pre-commit hook for anyone who
touches them.

No functional change: this commit is the output of the pinned
clang-format hook and nothing else.

Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
@cedrik-fuoco-adsk
update openrv-sub, revert later
fa0c383 
Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
@cedrik-fuoco-adsk
build: default C++ standard follows VFX platform (C++20 for CY2026+, …
7a3e1d5 
…C++17 otherwise)

Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
@cedrik-fuoco-adsk cedrik-fuoco-adsk changed the title SG-45024 - Support C++20 build: SG-45024 - Support C++20 Sep 3, 2026
@cedrik-fuoco-adsk
cedrik-fuoco-adsk marked this pull request as ready for review September 4, 2026 13:59
bernie-laberge
bernie-laberge reviewed Sep 4, 2026
View reviewed changes
Comment thread cmake/defaults/rv_options.cmake Outdated

SET(RV_CPP_STANDARD
"${_RV_CPP_STANDARD}"
CACHE STRING "RV's general C++ coding standard"

@bernie-laberge bernie-laberge Sep 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is the only important finding from our AI friend. Adding FORCE at the end would save us some trouble I think:
cmake/defaults/rv_options.cmake:82 — RV_CPP_STANDARD is set as a cache var without FORCE, unlike RV_VFX_PLATFORM right above it. Existing build trees reconfigured to a newer VFX platform won't actually pick up C++20.

bernie-laberge
bernie-laberge approved these changes Sep 4, 2026
View reviewed changes

@bernie-laberge bernie-laberge left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM thanks !
The only comment that we might address

  1. cmake/defaults/rv_options.cmake:82 — RV_CPP_STANDARD is set as a cache var without FORCE, unlike RV_VFX_PLATFORM right above it. Existing build trees reconfigured to a newer VFX platform won't actually pick up C++20.

@cedrik-fuoco-adsk
build: force RV_CPP_STANDARD to follow the VFX platform on every conf…
f0afa07 
…igure

The C++ standard is derived from RV_VFX_PLATFORM, not user-set, so mark
the cache entry with FORCE so reconfiguring an existing build tree to a
newer platform updates the standard instead of keeping the first-configured
value. Also restrict the cache STRINGS to the derived standard.

Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
eloisebrosseau
eloisebrosseau reviewed Sep 9, 2026
View reviewed changes
Comment thread src/lib/app/RvPackage/RvPackage/PackageManager.h Outdated
Comment thread src/build/make_pyside6.py Outdated
Comment thread src/build/make_pyside6.py Outdated
Comment thread cmake/defaults/rv_options.cmake Outdated
eloisebrosseau
eloisebrosseau reviewed Sep 9, 2026
View reviewed changes
Comment thread src/lib/mu/MuMathLinear/MathLinearModule.cpp
Comment thread src/lib/mu/MuMathLinear/MathLinearModule.cpp Outdated
Comment thread src/lib/mu/MuMathLinear/MathLinearModule.cpp Outdated
Comment thread src/lib/mu/MuMathLinear/MathLinearModule.cpp Outdated
@cedrik-fuoco-adsk
rename to requiresList, MATCHES
ee75264 
Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
@cedrik-fuoco-adsk
refactor(MathLinear): use std::copy_n and Matrix::getValue for matrix…
e9ab99a 
… load/store

Replace memcpy and direct ImathMatrix::x access with std::copy_n via
getValue() when converting to/from Imath matrices, and remove the now
unused <cstring> include. Also use std::abs instead of fabsf, expand a
few single-line control statements with braces, and rename local
variables for clarity.

Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
@cedrik-fuoco-adsk

cedrik-fuoco-adsk commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

All comments should be covered!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eloisebrosseau eloisebrosseau eloisebrosseau approved these changes
@bernie-laberge bernie-laberge bernie-laberge approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cedrik-fuoco-adsk @eloisebrosseau @bernie-laberge