Remove ABS in favor of Math::abs

[KoBeWi]

<-

[fire]

There's too many change to review, but they all look like copy and paste and the cicd passed.

[kleonc]

Is the same planned for MIN/MAX/CLAMP/SIGN? If no, why doing so for ABS but not for the others?

[akien-mga]

I would do it for all of them indeed, but that's something we ought to discuss to see if there's any reason not to do this change.

[KoBeWi]

Other functions don't seem to have Math equivalents.

[reduz]

I want to note that ABS works fine for integers, wheread Math::abs is sort of broken (only for int, there should be more integer types exposed or else you get ambiguous conversion).

[KoBeWi]

I added int8_t and int16_t, so all integer types are covered now.

[Calinou]

Should be good to merge after rebasing (and making sure new uses of ABS() are replaced after rebasing).

However, binary size is a bit larger compared to the previous commit (on a stripped Linux x86_64 debug editor build):

Before: 173,299,624 bytes
After: 173,306,856 bytes

The difference for release export templates is smaller:

Before: 67,925,056 bytes
After: 67,929,152 bytes

[Riteo]

Well, since #91283 popped up and discussion came again I wonder: what's the status of this PR? The last message talked about reassessing for 4.3 but now we're pretty ahead into that IMO :P

I'm still conflicted about this change I suggested, because of this inconsistency

IMO regarding that, considering that this is a pretty big codebase-wide change, I'd do one PR per method and do one of them at a time. Making a huge change at once would be a nightmare to review and handle, due to merge conflicts popping up constantly.

[akien-mga]

IMO regarding that, considering that this is a pretty big codebase-wide change, I'd do one PR per method and do one of them at a time. Making a huge change at once would be a nightmare to review and handle, due to merge conflicts popping up constantly.

Yeah of course, but the main question before doing one PR per method is whether we should do this change at all.

We should discuss the pros and cons of using methods vs macros for this. When I suggested this it made sense to me to consolidate all math operations in Math, but we then found that there are some drawbacks to using functions instead of plain pre-processor macros for those trivial operations.

[Riteo]

Yeah of course, but the main question before doing one PR per method is whether we should do this change at all.

Oh, I see.

I'm not enough of a C++ master to give a definite answer, some more expert feedback is definitely required.

That said, from my PoV, I'd be inclined to prefer a proper constexpr template method (as @Repiteo proposed), as they feel like a less finicky solution since, as far as I can tell, they should get inlined and optimized just like a macro, so I think that they'd compile to the same thing?

[Repiteo]

Just created #91324 as a minimal implementation of constexpr templates as math functions. It doesn't actually change any existing uses of the macros to the new implementations, so it should be a solid baseline for guaging any differences/issues with the existing macros.

[Ivorforce]

I'm in favour of this change!
I've just unknowingly opened the same PR (thanks @AThousandShips for linking!), with the minor detail different of using stdlib functions to aid the compiler (see comment).

(Edit: I've originally argued against macros here, but I totally missed it's been a template function for a while)

I'm still conflicted about this change I suggested, because of this inconsistency:

Is the same planned for MIN/MAX/CLAMP/SIGN? If no, why doing so for ABS but not for the others?

I think it's absolutely worth it to start moving away from macros for all these functions. Imo, it's not a new break in consistency because Math already has tons of functions (even abs). We can just tackle each remaining of the typedefs aliases one by one, in order to fully converge.

[akien-mga]

Let's go ahead with this.

Could you do a last rebase / check for remaining ABS and we'll merge this in priority?

[Ivorforce]

Suggested change

	static _ALWAYS_INLINE_ int64_t abs(int64_t g) { return ::abs(g); }
	static _ALWAYS_INLINE_ int64_t abs(int64_t g) { return ::labs(g); }

Careful, I'm not sure if abs(long) is guaranteed to exist: https://en.cppreference.com/w/c/numeric/math/abs

[akien-mga]

Yeah a big part of why we even have Math::abs instead of relying on stdlib is that abs overloads are/used to be ambiguous and would cause errors with MSVC (probably VS 2013 back then).

[KoBeWi]

Well... https://github.com/godotengine/godot/actions/runs/13946462884/job/39034795168?pr=69406
I'll go back to original code for int64_t.

[akien-mga]

It might need to be llabs(long long) for int64_t. I really despise short/int/long/long long types :D

[Ivorforce]

Yeah llabs may work. I think it's guaranteed to be int64_t? But yea, it would have been easier with C++ APIs rather than those old C headers...

[akien-mga]

Thanks!