Issues with overloading by return type

[niello]

Hi! Just updated from ACL 1.3 to latest version with RTM 2.2.0. I have been using ACL math for my own code outside animation system, and there are a couple of issues which are noticeable after porting.

Case 1

const auto NearAxis = rtm::vector_set(m.m[0][2], m.m[1][2], m.m[2][2], 0.f);
const float InvNearLen = rtm::scalar_sqrt_reciprocal(rtm::vector_length_squared3(NearAxis));

produces following errors:

Error	C2668	'rtm::scalar_sqrt_reciprocal': ambiguous call to overloaded function
Warning	C4244	'initializing': conversion from 'double' to 'float', possible loss of data
Warning	C4244	'initializing': conversion from 'double' to 'const float', possible loss of data

Case 2

float MaxDim = std::max({ rtm::vector_get_x(BoxExtent), rtm::vector_get_y(BoxExtent), rtm::vector_get_z(BoxExtent) });
(BTW can I do horizontal max better with RTM?)
and
std::max(rtm::vector_distance3(_EyePos, Record.BoxCenter), 1.0f)
now fail to choose the same type for all arguments.

Case 2 is fixed trivially with std::max<float>(...). Case 1 requires more verbose fix:

const float NearAxisLengthSq = rtm::vector_length_squared3(NearAxis);
const float InvNearLen = rtm::scalar_sqrt_reciprocal(NearAxisLengthSq);

or

const float InvNearLen = rtm::scalar_sqrt_reciprocal(static_cast<float>(rtm::vector_length_squared3(NearAxis)));

Maybe you have an idea how to improve default behaviour? As far as I understand, overloading by return type is a feature that reduces necessity of explicit casts, but here it does the opposite thing.

MSVC 2022 17.7.6
Windows 11

[niello]

Have just met another Case 3:

rtm::vector4f a, b;
rtm::vector_mul(a, rtm::vector_dot3(b, a))

doesn't compile because of ambiguous call.

[nfrechette]

Thank you for the feedback!
I tend to be very explicit and verbose in the math code I write, and RTM somewhat reflects that philosophy. The main idea behind return type coercion is to keep the API lean and avoid near duplicate functions (e.g. vector_dot, vector_dot_as_scalar, vector_dot_as_vector, etc). But doing so come at the price of ambiguity at the call site in certain circumstances, as you've run into. In practice, I found that being explicit helps with long term maintenance and readability but that is largely a personal preference and it can definitely hinder quick prototyping etc. The intent behind this is to help fine tune code generation for optimal assembly where being explicit is often important.

That being said, I realize that the current state of things can be confusing and frustrating. My apologies!

There's a few things that we can do to improve things. The ambiguity tends to happen in 3 classes of usage:

1. Using auto to capture am ambiguous result such as auto foo = rtm::vector_zero()
   Here, there is little we can do. The ambiguity will propagate where the captured variable is used and we cannot tell the compiler which return value should be preferred (and it isn't possible for us to know ahead of time, statically in the header code, which return value should be preferred, it depends on the usage at the call site). I don't think we can do much to help with this. I would refrain from using auto in math code (I also hate it in general and keep its usage to a minimum if I can avoid it).

2. In non RTM functions such as std::min. This can happen with templated arguments where the code assumes that implicit coercion can be relied upon which isn't always the case. Here as well, for each argument, we would have to know which return type is preferred and only the call site can know this. Being explicit with a cast is the only way.

3. In overloaded functions such as rtm::vector_mul(vec4, float/vec4/scalar).
   Here, we can do better by introducing an overload that takes a template argument for the overloaded argument. This helper overload would have lower precedence (since it's templated) and would kick in when the call is ambiguous. The function can then simply attempt to explicitly coerce the argument by calling one of the other known overloads. e.g.

namespace rtm
{
	template<typename rhs_t>
	RTM_DISABLE_SECURITY_COOKIE_CHECK RTM_FORCE_INLINE vector4f RTM_SIMD_CALL vector_mul(vector4f_arg0 lhs, const rhs_t& rhs) RTM_NO_EXCEPT
	{
		return vector_mul(lhs, static_cast<scalarf>(rhs));
	}
}

This would resolve overload ambiguity while maintaining the API consistent. It would also allow user types to be used with RTM if they can coerce.

The only other alternative I can think of would be to mark some return coercion statements as being explicit only. This would require an explicit cast or direct initialization to use them which would be quite awkward I think.. but perhaps that can be worked around if we overload the operator= where possible. Sadly, that operator cannot be non-member though, and so wouldn't work for vector4f on most platforms. I wonder if we could introduce a helper type that implicitly coerces from our special return types to the desired output type, one that has lower precedence than the default implicit coercion path. I'll have to think on it and thinker a bit.

I'll see what I can do with this for v2.3 early next year. I'm currently busy finishing up the next release of ACL this month but I hope to quickly release an RTM update next year with a few improvements.

[niello]

Speaking of these usage classes:

1. I don't use auto in ambiguous cases with RTM and it would be strange for me to want overloading by return type working with auto. Nothing should (or can) be done with this, and it's OK.
2. I agree in general but maybe there is a way to improve float vs scalarf overload handling, at least when they are the same thing?
3. Looks like a great idea for me. And this way you can force the best option when there is a difference, and user hasn't to think of what overload should be used.

[nfrechette]

Hmm I'm not super happy with how this is turning out...

Implicit coercion does not allow chaining user-defined conversions: only one is attempted (as per the standard). That prevents me from using template logic to explicitly define a coercion priority (by inserting dummy types that coerce into one another).

Adding explicit coercion is ugly. It forces a different syntax and because it only works with direct initialization (can't use operator=). Working around it would require explicit casting which adds as many characters as typing some alternate function that doesn't rely on coercion...

I think if we have to change the syntax to have only one coercion to float that is automatic, I would rather add functions on the return structs to clarify rather than rely on explicit coercion (e.g. vector4f tmp = vector_dot(foo, bar).as_vector();). This would be a little nicer to read, perhaps.. I'm not sure. But then this is basically the same as vector_dot_as_vector(foo, bar) which I wanted to avoid in the first place.

[nfrechette]

I've given this quite a bit of thought over the past few weeks, and in the end I think some overloaded return types may have to go. While I personally love them and they work great for my programming style, I think that may be more of a niche. I'll break down my reasoning as follows:

• They don't compose too well with non-RTM stuff. As you've found out with the std::max, it quickly becomes a pain and it would require even more code changes to add a templated overload for stuff like vector_mul which have multiple variants to have one coerce to float.
• The ambiguous call error messages can be confusing to those not familiar with the API. It may not be immediately obvious why something like vector_dot is ambiguous.
• The shim struct that is returned is likely going to make documentation generation more complicated. I haven't gotten to it yet, but it'll obfuscate the intent to some degree.
• Most programmers are likely going to expect that basic arithmetic functions like vector_length3 return a float and may attempt to feed it to ambiguous things (e.g. matrix_mul, vector_mul, etc). It goes against common expectations.

To that end, I think the best course of action may be to introduce new functions with a suffix to clarify usage and intent:

• _as_scalar will be added when the return value is a scalar (e.g. vector_dot3_as_scalar)
• _as_vector will be added when the return value is a vector4 (e.g. vector_dot3_as_vector)
• No suffix will be used when the return value is a float/double (e.g. vector_dot3) as that is the most common use case IMO
• Existing coercion functions to scalar/vector4 will be deprecated, to be removed in a future release with an appropriate warning message

I think a more verbose suffix makes sense for clarity. I fear that something more compact like _s/_v may be harder to read for those less familiar with the API. In practice, the versions with suffix, while handy in hot code for optimal codegen, aren't as commonly used and aren't otherwise necessary.

However, some return based overloads will remain:

• Things like constants are unlikely to cause ambiguous calls (e.g. vector_zero)
• Matrix casts are unlikely to cause ambiguous calls (e.g. matrix_cast)

Functions that have a scalar based overload will return a scalar automatically without a suffix (e.g. scalarf scalar_sin(scalarf angle) vs float scalar_sin(float angle).

How does that sound? Let me know what you think. I intent to move forward with this and finish up the 2.3 release shortly (this is the last task for it).

[niello]

I think that's OK to have a function with a short name for the logically expected case and a function with postfix for an opposite case. I.e. it is expected for vector_dot3 to return a scalar, therefore vector_dot3 must return a scalar and vector_dot3_as_vector must return vector. Then the code will be very readable, without unnecessary clutter. And optimized cases will be immediately noticeable, because as soon as you see vector_dot3_as_vector you know it is there for a reason.

[nfrechette]

I'll deprecate what I can for this release. This is likely to generate warnings with ACL until its next release. To that end, I'm adding RTM_NO_DEPRECATION to allow disabling deprecation warnings.