Add a combined version define in MaterialX C++

[pablode]

Currently, checking against the MaterialX version in C++ code can be code-heavy:

#if (MATERIALX_MAJOR_VERSION > 1) || \
    (MATERIALX_MAJOR_VERSION == 1 && MATERIALX_MINOR_VERSION > 38) || \
    (MATERIALX_MAJOR_VERSION == 1 && MATERIALX_MINOR_VERSION == 38 && MATERIALX_BUILD_VERSION > 8)
    // ...some new feature
#endif

I propose that a define such as MATERIALX_VERSION should be added, just like it's done in USD:
https://github.com/PixarAnimationStudios/OpenUSD/blob/0b18ad3f840c24eb25e16b795a5b0821cf05126e/cmake/defaults/Version.cmake#L29

The above check can then be simplified to

#if MATERIALX_VERSION > 103808
  // ...
#endif

[jstone-lucasfilm]

This is a good suggestion, @pablode, though I wonder if we can do better than the combined integer approach for preprocessor comparisons.

For context, here is the pattern we currently use to compare MaterialX versions in Python:

if (mx.getVersionIntegers() > (1, 38, 8))

And here is the equivalent comparison in C++:

if (mx::getVersionIntegers() > std::tuple<int, int, int>(1, 38, 8))

Could there potentially be an equivalent approach for preprocessor comparisons, or are combined integers truly our best bet?

[pablode]

Indeed, the combined integer could be constructed with a macro; see f.i. Vulkan’s VK_MAKE_VERSION.

A MaterialX equivalent would then look like this:

#if MATERIALX_VERSION > MATERIALX_MAKE_VERSION(1, 38, 8)

[ruiyang2021]

Hi can I try solving this one for Dev Days 2024?

[jstone-lucasfilm]

Definitely, and welcome @ruiyang2021!

[ruiyang2021]

Hi folks, I take a closer look today. I'm still a beginner learning from the code repo, may have some shallow understanding.

I see that tuples are compared position by position, it is great we can use ">" and "<" operators to compare the versions.

Then here's a doubt:
Seems to me the request would like to replace this const object in Util.cpp (and mx.getVersionIntegers())

const std::tuple<int, int, int> LIBRARY_VERSION_TUPLE(MATERIALX_MAJOR_VERSION,
                                                      MATERIALX_MINOR_VERSION,
                                                      MATERIALX_BUILD_VERSION);

with something maybe like this in Util.h (or the Vulkan way of combining)

#define MATERIALX_VERSION std::make_tuple(MATERIALX_MAJOR_VERSION, MATERIALX_MINOR_VERSION, MATERIALX_BUILD_VERSION)
#define MATERIALX_MAKE_VERSION(major, minor, build) std::make_tuple(int(major), int(minor), int(build))

It requires changes at a number of places. To be thorough, there is also a Document::getVersionIntegers(), which, may be good to convert to macro as well?
Also, I see a few places mentioning const is preferred over macro for reasons in term of scoping and debugging. Wondering if it is on point for our case. Can I double check if we want to proceed with this request?

[jstone-lucasfilm]

This is a great question, @ruiyang2021, and fortunately I think the request in this GitHub Issue is simpler than the full change you're considering.

For this issue, I believe the only new features that we would need are the following two features in the Library.h header of MaterialXCore:

1. A new preprocessor macro that combines three version integers into one. This would be based on the VK_MAKE_VERSION macro found at https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VK_MAKE_VERSION.html, and it could have a name along the lines of MATERIALX_GENERATE_INDEX. As in the Vulkan example, this macro would not need to call down to std::tuple or MaterialX C++ code, and could be entirely defined through preprocessor math.
2. A new preprocessor integer for the combined MaterialX version. This could have a name along the lines of MATERIALX_VERSION_INDEX and could be defined as MATERIALX_GENERATE_INDEX(MATERIALX_MAJOR_VERSION, MATERIALX_MINOR_VERSION, MATERIALX_BUILD_VERSION).

Then, when a developer needs to compare versions in their C++ code, they should be able to add a call like the following:

#if MATERIALX_VERSION_INDEX > MATERIALX_GENERATE_INDEX(1, 38, 8)

Let me know if that helps to clarify things, and feel free to ask additional questions!

[ruiyang2021]

Thanks for breaking it down for me @jstone-lucasfilm ! Sending a pr #2031

[jstone-lucasfilm]

Thanks for this original report, @pablode, as well as to @ruiyang2021 for the implementation in #2031!