Replace =delete with SFINAE for invalid Vec integer methods

[chinmaychahar]

Summary

Fixes #559

Replace = delete template specializations with SFINAE using the existing IMATH_ENABLE_IF macro for length(), normalize(), normalizeExc(), normalizeNonNull(), normalized(), normalizedExc(), and normalizedNonNull() in Vec2, Vec3, and Vec4.

Removes the entire Specializations for VecN<short>, VecN<int> block including the @cond Doxygen_Suppress wrapper which will no longer be needed when SFINAE makes the methods invisible to both the compiler and Doxygen for non-applicable types.

Status

Draft PR. I will iterate based on feedback and open questions in #559

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: chinmaychahar / name: chinmaychahar (33e84ff)

[lgritz]

I think it's probably unwise to rely on either std::is_floating_point or std::is_integral. We should probably define our own extensible type trait that will capture what we really intend and be user-extensible and future-proof.

[chinmaychahar]

Thanks @lgritz, I added an is_float_like that defaults to std::is_floating_point with a specialization for half. Let me know if this is along the lines of what you had in mind

[chinmaychahar]

Could a reviewer please take a look at this? Wouldn't want it go stale

[cary-ilm]

Apologies for the delay, I've been consumed with other issues. And thanks for you continued efforts! I'm happy to work through the issues.

The implementation needs to support Imath::Vec3<Foo> declarations for any externally-declared type Foo, not just half. The library needs to provide a way for the user to give a specialization for their own class. It may be rare, but it shouldn't be made impossible.

I think something like this is the right direction:

In ImathTypeTraits.h:

#if (IMATH_CPLUSPLUS_VERSION >= 17)
using std::void_t;
#else
template <typename...> using void_t = void;
#endif

/// True if ``T`` supports the arithmetic operations used by ``Vec::length()``.
template <typename T, typename = void> struct imath_supports_vec_length : std::false_type
{};

template <typename T>
struct imath_supports_vec_length<
    T,
    void_t<
        decltype (std::declval<T> () * std::declval<T> ()),
        decltype (std::declval<T> () + std::declval<T> ()),
        decltype (std::declval<T> () / std::declval<T> ()),
        decltype (std::declval<T> () < std::declval<T> ()),
        decltype (T (0)),
        decltype (T (2)),
        decltype (std::numeric_limits<T>::min ()),
        decltype (std::sqrt (std::declval<T> ())),
        decltype (std::abs (std::declval<T> ()))>> : std::true_type
{};

/// True for scalar types suitable for ``Vec`` length/normalize.
template <typename T>
struct imath_is_floating_point
    : std::integral_constant<
          bool,
          imath_supports_vec_length<T>::value && !std::is_integral<T>::value>
{};

and then in ImathVec.h:

#define IMATH_IF_FLOATING_POINT                                                  \
    template <                                                                 \
        typename U = T,                                                          \
        typename std::enable_if<                                                 \
            imath_is_floating_point<U>::value,                                  \
            int>::type = 0>

#define IMATH_IF_FLOATING_POINT_IMPL                                          \
    template <                                                                \
        typename U,                                                           \
        typename std::enable_if<                                                \
            imath_is_floating_point<U>::value,                                \
            int>::type>

and then later:

    IMATH_IF_FLOATING_POINT
    IMATH_HOSTDEVICE T length () const IMATH_NOEXCEPT;

and finally:

template <class T>
IMATH_IF_FLOATING_POINT_IMPL
IMATH_HOSTDEVICE inline T
Vec2<T>::length () const IMATH_NOEXCEPT
{
    T length2 = dot (*this);

    if (IMATH_UNLIKELY (length2 < T (2) * std::numeric_limits<T>::min ()))
        return lengthTiny ();

    return std::sqrt (length2);
}

Also note that this needs to work with C++17 through C++26, and unfortunately some of these trait template change signature, so it's a little tricky.

It would also be good to add CI jobs that build with the various C++ standards.

Also, be sure to configure your fork of the repo to run the CI on your local branch. That way, you can confirm it works before pushing to the PR. When I find myself in a situation like this where I have to make substantial changes to an already-submitted PR, I often create a second branch, work there until I'm happy, then squash the commits and cherry-pick them over to the branch for the PR. That avoids a lot of noise on the PR.

[lgritz]

template <typename T>
struct imath_supports_vec_length<
    T,
    void_t<
        decltype (std::declval<T> () * std::declval<T> ()),
        decltype (std::declval<T> () + std::declval<T> ()),
        decltype (std::declval<T> () / std::declval<T> ()),
        decltype (std::declval<T> () < std::declval<T> ()),
        decltype (T (0)),
        decltype (T (2)),
        decltype (std::numeric_limits<T>::min ()),
        decltype (std::sqrt (std::declval<T> ())),
        decltype (std::abs (std::declval<T> ()))>> : std::true_type
{};

What the heck does that mean?

[chinmaychahar]

Thanks @cary-ilm. Before I rework, @lgritz raised readability concerns about the void_t probe. Want to ask what approach to be aligned on?

[chinmaychahar]

hey both, any updates on this?

[cary-ilm]

My apologies again for letting this drop. This almost working. I copied your branch and made a few extensions to resolve the remaining issues, you can inspect it here: https://github.com/cary-ilm/Imath/tree/refs/heads/sfinae-vec-integer-methods

This addresses several issues:

• Add () around (is_float_like<S>::value) in definitions in ImathVec.h. The IMATH_ENABLE_IF macro places () around __VA_ARGS__ in enable_if_t<>. Since the parentheses appear in the method declaration (via IMATHENABLE_IF), they must also appear in the definition, so the two match exactly. This resolves the CI failure above.
• I added tests for SFINAE declarations of length(), normalize(), etc, static_assert statements that validate that the methods appear for float-like types and do not appear for non-float-like types. I also added a test to validate that the Vec classes can be instantiated with custom class objects, CustomFLoat and CustomNonFloat, provided that they are
  accompanied by a specialization of is_float_like<>.
• PyBindImathVec.h needs to use lambas for length(), normalize(), etc, since these methods are templates now, so an ordinary method won't work.

If you'd like feel free to cherry pick my commits there into your branch and push them to the PR. That would require adding my branch to your repo as a remote. Or simply download the files and copy/paste them into your branch, that's fine, too.

There is another unrelated outstanding issue with the pybind11 bindings that is causing the CI to fail, affecting all PRs, not just yours. I have a fix that I'll submit shortly, which you can also include.

Thanks again for the contribution and your patience.

[chinmaychahar]

Thanks a lot @cary-ilm, really appreciate you taking the time to work through the remaining issues. I've cherry-picked your three commits and rebased onto latest main

[chinmaychahar]

I ran the full CI on my fork first before pushing here and everything's green. Let me know if there's anything else you'd like changed.

[lgritz]

LGTM.

[cary-ilm]

Everything looks in order, thanks!