Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup lmms_math.h #7382

Merged
merged 36 commits into from
Aug 28, 2024
Merged

Cleanup lmms_math.h #7382

merged 36 commits into from
Aug 28, 2024

Conversation

Rossmaxx
Copy link
Contributor

Cleaned up some legacy code from lmms_math.h

@JohannesLorenz JohannesLorenz removed their request for review July 16, 2024 19:28
include/lmms_math.h Outdated Show resolved Hide resolved
include/lmms_math.h Outdated Show resolved Hide resolved
include/lmms_math.h Outdated Show resolved Hide resolved
include/lmms_math.h Outdated Show resolved Hide resolved
include/lmms_math.h Outdated Show resolved Hide resolved
include/lmms_math.h Outdated Show resolved Hide resolved
Co-authored-by: saker <sakertooth@gmail.com>
include/lmms_math.h Outdated Show resolved Hide resolved
include/lmms_math.h Show resolved Hide resolved
@Rossmaxx
Copy link
Contributor Author

I would like to wait for #7380 before this PR.

include/lmms_math.h Outdated Show resolved Hide resolved
@Rossmaxx
Copy link
Contributor Author

Rossmaxx commented Aug 9, 2024

I have decided to clean up more mathematical stuff.

include/lmms_math.h Outdated Show resolved Hide resolved
include/lmms_math.h Outdated Show resolved Hide resolved
include/lmms_math.h Show resolved Hide resolved
include/lmms_math.h Outdated Show resolved Hide resolved
Copy link
Contributor

@sakertooth sakertooth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not much to add, Dom covered most of what needed to be said already.

include/interpolation.h Outdated Show resolved Hide resolved
@Rossmaxx
Copy link
Contributor Author

@DomClark we good for merge? I'm waiting for your nod.

@DomClark
Copy link
Member

Did you plan to use std::copysign? I thought you said that it looked okay to use (it does to me).

@Rossmaxx
Copy link
Contributor Author

No i said I'm not sure about the sign. There's chances negative values might be used. I don't wanna take much chances.

include/lmms_math.h Outdated Show resolved Hide resolved
@Rossmaxx Rossmaxx merged commit a992019 into LMMS:master Aug 28, 2024
11 checks passed
@Rossmaxx Rossmaxx deleted the cleanup-lmms-math branch August 28, 2024 07:19
@DomClark
Copy link
Member

DomClark commented Sep 1, 2024

There's chances negative values might be used.

There isn't - std::sqrt(std::abs(val)) and std::pow(std::abs(v), e) are guaranteed to be non-negative for all values of val, v, and e. You always get a non-negative value from std::abs, and powers and square roots of non-negative numbers are themselves non-negative. Conceptually, what this code does is strip the sign, perform the operation using the non-negative value, then restore the original sign. This is a typical use for std::copysign - as an example, I used it like this in Sf2Player here:

const auto panning = n->panning;
const auto factor = 1.f - std::abs(panning) / static_cast<float>(PanningRight);
const auto balance = std::copysign(
factor <= 0 ? maxBalance : std::min(-200.f * std::log10(factor), maxBalance),
panning
);

@Rossmaxx
Copy link
Contributor Author

Rossmaxx commented Sep 1, 2024

A bit late but thanks for the info. I'll keep it in mind for the future.

sakertooth added a commit to sakertooth/lmms that referenced this pull request Sep 4, 2024
* simplified fraction and absfraction functions

* removed unused fastSqrt() and fastPow()  
functions

* unused absMin() and absMax()

* move roundAt to math header

* Code review from saker

Co-authored-by: saker <sakertooth@gmail.com>

* use std::trunc()

* fixup after fixing merge conflicts

* remove unused fastFma and fastFmal functions.

* remove lmms_basics include, not needed

* use signedPowf from lmms_math in NES

* removed fastRand function, unused

* remove unused sinc function

* cleanup signedPowf

* code review

* further simplify random number math

* removed static from lmms_math file

---------

Co-authored-by: saker <sakertooth@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants