Some math functions now prefixed with std::

[dimitre]

1 - This one starts changing every one of this functions to std::

sin, cos, acos, atan2, atan, abs, ceil, floor, round, pow to std

this way we assure it is working with floats or double automatically. old C style versions always use double, so optimizations opportunities can be discarded from the compiler. Basically lots of floats are being casted to double and the results casted from double to float again

2 - Removing all macros like MAX or M_PI_2, etc from the core, and making an optional preprocessor directive to enable old macros if needed.

3 - replacing ofRadToDeg and ofDegToRad to glm::degrees / glm::radians. This one is almost the same, but some includes of ofMath.h can now be removed (they are removed in this PR) Advantage: glm::degrees / radians can be applied in vectors at once, so they are updated in two different places

4 - lastly and optional. a conditional OF_USE_LEGACY_MATH_MACROS to use legacy macros

[dimitre]

I think it is good to go!

[dimitre]

I think this one is good to go, but want to know more opinions on this: cc: @ofTheo @artificiel @2bbb @NickHardeman

[dimitre]

Yes, bigger than expected.

We can think better of examples. it was only a way of removing classic math macros, but this will accelerate discussions like ofMin, ofMax (good one) or pi constant for example.

I can duplicate the branch and remove examples, so we can merge the core sooner and discuss examples later.

[artificiel]

great work, I agree with @NickHardeman that internally std:: usage should be mandatory (and any type conversion should be considered a suspicious operation) (and NB if we're about cleaning up, all C-style casts should move to static_cast<T>()).

also, what about deprecating ofDegToRad & al.?

about the ofMin thing, I think it is an interesting discussion in regards to expected C++ literacy of end-users. looking at the long-term code maintenance considerations of reimplementing available functionality (which includes documentation) I would not recommend duplicating anything available in std or glm. Unless there is a good "life simplifying" macro-ish function, but in this case, while the hidden casting inside ofMin(whichever, whatever) makes things simple and generally works as expected, it's also risky. the correct way user-way would be to do auto m = std::min<float>(float, double);, and if one writes auto m = std::min(float, double) it's time to bring typing in the discussion (in that particular case it's unfortunate that the compiler error message is not more helpful). having that discussion takes more time than using ofMin() that 'just works', but it's definitely worth it.

moreover can you can use std::min on any type that has an '<' operator; you can do std::min({5,3,4,1}); the function is documented in cppreference etc; and it's an entry point into <algorithm>.

if we really want to provide ofMin(Ta, Tb), it should be templated to do the right thing most of the time (not cast down doubles, for example), so if Ta and Tb are the same, call std::min 'as is', and if not, figure out which type has the lowest std::numeric_limits::lowest() and use that type as the operation and return type. but it adds type uncertainty, and I'm not sure the maintenance effort is worth it vs empowering the user.

and/or upgrading the OF "default" type to double? so less mismatches with typing "3.14" and down casting becomes mostly moot (edge cases will still exist).

NB speaking of double, I personally work with Rust-inspired typedefs such as i32, i8, u32, f16, which makes things much more mentally clear than the old words (especially double — how braindead is that type name...long long also hurts a bit), without the visual weight of std::uint16_t — and C++23 introduces literals for floats like f32 https://en.cppreference.com/w/cpp/language/floating_literal

[artificiel]

also a quick question vs a PR that changes so many files: there are many PR's, many of which might not automatically merge anymore... how about auditing the current list of PR's, close the ones that have become irrelevant, discuss the pending ones, before applying such a wide change?

i am currently in a period with some available time and could do some of the auditing, but I would not embark on doing that without knowing it's part of a structured effort (i.e. need to know that the work will actually be applied).

[ofTheo]

@dimitre I think we'll need to keep the macros in the core for now as so many addons rely on it. so lets have OF_USE_LEGACY_MATH_MACROS enabled for now ( once this is merged ).

I think we can have it disabled by default in future releases, but let's have a couple of releases where the new usage is in the example but the old usage is preserved.

I think in the past I was in favor of doing something like of::math::PI as a replacement. ( glm::pi<float>() is kind of a clunky replacement ).

We could possibly do something like:

namespace of::math {
    // Global constant accessible in the program
    const float PI = glm::pi<float>();
    const float HALF_PI =  glm::pi<float>() * 0.5; 
    etc 
}

And have our own versions of min, max in a similar way, but I think most functions could rely on the std:: versions.

EDIT: apologies for closing and reopening.... fat fingers... :)

[dimitre]

@ofTheo We can keep glm::pi in core, as it doens't matter to be clunky and I can remove the examples from this PR

[dimitre]

@artificiel yes good to discuss ongoing PRs. fixing conflicts will be needed anyway merging in any order, and this PR have only function replacements

[ofTheo]

Thanks @dimitre - I think it's good that we update the examples too.

I think probably it's just the PI vs glm::<float>pi() thing to figure out ( and maybe that could be left as PI, unless we agree on a good approach ).

I think for now we could use std::min and std::max in the examples but in the way @NickHardeman mentioned?

[dimitre]

as a personal opinion I like exposing std::min, std::max, glm functions and I'm ok even with glm::pi() I'm influenced by the name openFrameworks, like the frameworks, opened

[dimitre]

@ofTheo now OF_USE_LEGACY_MATH_MACROS is defined

[ofTheo]

@dimitre oh yeah, I personally use glm::<float>pi() but I can see that being a mouthful for new users. I wish they had a glm::pi() that defaulted to float.

But yeah, maybe let's just go for it and use glm::<float>pi() in the examples - the macros will still be there for a while. :)

[dimitre]

Great! we can use glm::pi() in the examples and if we start using something similar in of namespace it will be easy to just replace them again in examples

[artificiel]

glm::pi<float>() works but as mentioned by @oxillo numbers are coming in C++20 so the future-proof "modernisation" would be to define of::numbers::pi (perhaps lifting https://github.com/llvm/llvm-project/blob/main/libcxx/include/numbers into ofNumbers?)

auto pi1 = glm::pi<float>();
//
auto pi2 = of::numbers::pi; 
//
auto pi3 = of::numbers::pi_v<float>; // NB default defined as a double; if you really need a float

[ofTheo]

oooh - I like that approach (edit) @artificiel

how about we do:

namespace of::numbers {
    const float pi = glm::pi<float>();
    etc 
}

for now and then later we can switch to std::numbers::pi when its available?

[artificiel]

yes, as long as the name follows the pattern of of::numbers::pi the implementation does not matter so much!

if your point is about being float by default they could be set to that, but that reinforces OF's assumptions of float, while a naked dotted decimal in C++ source is by default implicitly a double... "upgrading" the general OF behaviour to double makes it less likely to inadvertently end up with a mixture of types, and if you want to work in floats, you can cast down from a double (but it's an explicit gesture).

(I guess I'm thinking the "default plain text" of double does not match the "implicit expectation of OF" which is float.)

that being said if we're really keen on providing user-friendly "throw it anything" wrappers like ofMin() we could support float/double/whatever:

[edit to add: fell a bit in a template hole, but this should cover all arithmetic cases including the special case of size_t / uint64_t vs signed (does not play well will common_type), and still pass through any types that support < with the same type on both sides]

if we agree on this approach I will produce the ofMax version.

template<typename T>
T ofMin(const T & t1, const T & t2) const { return std::min(t1, t2); }

template<typename T, typename Q>
auto ofMin(const T & t, const Q & q) const {
    using CommonType = typename std::common_type<T, Q>::type;
    if constexpr ((std::is_same_v<T, uint64_t> or std::is_same_v<T, size_t>) && std::is_signed_v<Q>) {
        if (q < 0) {
            return q;
        } else {
            return std::min<Q>(static_cast<Q>(t), q);
        }
    } else if constexpr ((std::is_same_v<Q, uint64_t> or std::is_same_v<Q, size_t>) && std::is_signed_v<T>) {
        if (t < 0) {
            return t;
        } else {
            return std::min<T>(t, static_cast<T>(q));
        }
    } else if constexpr (std::is_signed_v<T> && std::is_unsigned_v<Q>) {
        if (t < 0 || q > static_cast<Q>(std::numeric_limits<T>::max())) {
            return static_cast<CommonType>(t);
        } else {
            return std::min(static_cast<CommonType>(t), static_cast<CommonType>(q));
        }
    } else if constexpr (std::is_signed_v<Q> && std::is_unsigned_v<T>){
        if (q < 0 || t > static_cast<T>(std::numeric_limits<Q>::max())) {
            return static_cast<CommonType>(q);
        } else {
            return std::min(static_cast<CommonType>(t), static_cast<CommonType>(q));
        }
    } else {
        return std::min(static_cast<CommonType>(t), static_cast<CommonType>(q));
    }
}

tests

assert(ofMin(size_t(1000), size_t(4)) == 4);
assert(ofMin(float(-1000.5), size_t(4)) == -1000.5f);
assert(ofMin(int(std::numeric_limits<int>::lowest()), int(4)) == std::numeric_limits<int>::lowest());
assert(ofMin(int64_t(std::numeric_limits<int64_t>::lowest()), uint16_t(3)) == std::numeric_limits<int64_t>::lowest());
assert(ofMin(float(3), int64_t(std::numeric_limits<int64_t>::lowest())) == std::numeric_limits<int64_t>::lowest());
assert(ofMin(int16_t(std::numeric_limits<int16_t>::lowest()), int64_t(222)) == std::numeric_limits<int16_t>::lowest());
assert(ofMin(int64_t(std::numeric_limits<int64_t>::lowest()), uint16_t(13)) == std::numeric_limits<int64_t>::lowest());
assert(ofMin(uint16_t(13), int64_t(std::numeric_limits<int64_t>::lowest())) == std::numeric_limits<int64_t>::lowest());
assert(ofMin(int64_t(std::numeric_limits<int64_t>::lowest()),uint64_t(3)) == std::numeric_limits<int64_t>::lowest());
assert(ofMin(uint64_t(3), int64_t(std::numeric_limits<int64_t>::lowest())) == std::numeric_limits<int64_t>::lowest());
assert(ofMin(int(-4), uint64_t(4)) == -4);
assert(ofMin(size_t(4), int(-4)) == -4);
assert(ofMin(uint16_t(20000), int16_t(10000)) == 10000);
assert(ofMin(uint16_t(20000), int16_t(-10000)) == -10000);
assert(ofMin(int16_t(10000), uint16_t(20000)) == 10000);
assert(ofMin(int16_t(-10000), uint16_t(20000)) == -10000);

[ofTheo]

@dimitre - I think lets merge this. we can do a seperate PR for of::numbers::pi etc - which should be easy to do.

@NickHardeman - it would be great to get your take on @artificiel's last comment about ofMin and ofMax - if it throws less errors than std::min / std::max it might be worth adding to the core.

[artificiel]

ofMin moved to #7940 of::numbers in #7941

[NickHardeman]

@artificiel yes! ofMin() solves the issue of passing in mixed arguments that provide an easier interface and avoids the std::min( 1.f, 2.0 ); ERROR that I think could be frustrating for beginners. And std::min() is still an option for people who want to use that.