Use C++20 std::numbers, std::lerp() #7696
Conversation
Also replace the only use of lmms::numbers::inv_e with a local constexpr instead
They were only used in one or two places each
This is probably not needed for some of these files. I'll remove those later
Rest in peace lmms::numbers::tau, my beloved
A lot of the remaining constants in lmms_constants.h are specific to SaProcessor. If they are only used there, shouldn't they be in SaProcessor.h?
|
Almost all the remaining constants in Besides moving Edit: |
- And also move F_EPSILON into lmms_math.h - And also add an overload for fast_rand() to specify a higher and lower bound - And a bunch of other nonsense
As well as time travel to undo several foolish decisions and squash tiny commits together
rubiefawn
force-pushed
the
template-constants2
branch
from
a254925 to
d7a735b
Compare
include/lmms_constants.h
Outdated
| inline constexpr std::integral auto MaxScaleCount = 10; //!< number of scales per project | ||
| inline constexpr std::integral auto MaxKeymapCount = 10; //!< number of keyboard mappings per project |
There was a problem hiding this comment.
There's no need for concepts here
There was a problem hiding this comment.
In your opinion, when would concepts be needed? I'd like to better understand when I should or should not be using these; they seem like a reasonable default to me with my limited understanding of C++.
include/lmms_math.h
Outdated
| namespace | ||
| { | ||
|
|
||
| inline constexpr float F_EPSILON = 1.0e-10f; // 10^-10 | ||
|
|
||
| } |
There was a problem hiding this comment.
Anonymous namespaces shouldn't be in headers.
I think this would be fine just in the lmms namespace.
There was a problem hiding this comment.
Anonymous namespaces shouldn't be in headers.
Why is this? (Genuinely curious, I wouldn't know)
I think this would be fine just in the lmms namespace.
It's only used for approximatelyEqual() and roundAt() so I figured making it not visible outside this header would be appropriate. I can go ahead and move it back into lmms_constants.h if you believe it should be publicly available.
include/lmms_math.h
Outdated
| template<std::floating_point T = float> | ||
| inline T fast_rand(T range) noexcept |
There was a problem hiding this comment.
Would the = float prevent it from deducing the type from the argument?
I think I'd prefer the fastRand name for this function and its overloads, for consistency.
There was a problem hiding this comment.
= float just provides a default value to the template if none are manually provided, unless I am mistaken. (Edit: So yes, it does prevent inference, but it does allow double to be used if manually specified)
I'll go ahead and update the fast_rand()s to fastRand().
| template<std::floating_point T = float> | ||
| inline auto fastPow10f(T x) |
There was a problem hiding this comment.
Could this be inline auto fastPow10f(std::floating_point auto x) instead?
There was a problem hiding this comment.
The T is used to select the std::numbers::ln10_v<T> of the same type as the input parameter. Is there a way to do that while applying your suggestion?
| #include "interpolation.h" | ||
| #include "lmms_math.h" |
There was a problem hiding this comment.
SlicerT uses sign() on line 187:
if (sign(lastValue) != sign(singleChannel[i]))Prior to this PR, interpolation.h used to include lmms_math.h, so it was being indirectly included.
Co-authored-by: Dalton Messmer <[email protected]>
Co-authored-by: Dalton Messmer <[email protected]>
In #7558, I added
lmms::numbers; in this PR, I destroy it.<concepts>such asfloating_pointrather thantypenamefor templateslmms::numbersthat are also defined instd::numberswith thestd::numbersversionlmms::numbers::tauwith2 * std::numbers::pi? (I love τ dearly, but @messmerd has a point)lerp()fromlmms_math.handlinearInterpolate()frominterpolation.hwithstd::lerp()(this should have happened in Update math functions to C++ standard library #7685 but I didn't realizestd::lerp()existed)