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

Use templates for common geometric constants #7558

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rubiefawn
Copy link
Contributor

@rubiefawn rubiefawn commented Oct 23, 2024

lmms_constants.h has several common constants such as PI and E, with variants of each for float, double, and long double. Many of these are currently unused in the repo but are of potential use. This change replaces LD_PI, D_PI, and F_PI with Pi<T>, and so on.

Between a lot of plugins having their own #define PI 3.14... and the coding conventions stating "Global scope constants SHALL use UpperCamelCase", these new template constants use upper camel case instead of upper snake case.

Constants that are currently in use have not been removed for now, so this PR only touches lmms_constants.h. There are a few options:

  • Do nothing, keep existing code using these constants as-is (current solution)
  • Keep the constants, but mark them as [[deprecated]] to discourage new use
  • Update all files to use the templates instead and remove the now unused constants

@messmerd
Copy link
Member

You should use inline constexpr since the variables are in a header, though maybe it would be best to wait for C++20 (#7554) and GCC 10 where we'll get the <numbers> header.

Updated to account for `<numbers>` and C++20:
- Marked all `lmms_constants.h` constants with an exact equivalent in `<numbers>` as deprecated
- Removed all `lmms_constants.h` constants where no variant is currently in use
- Using `inline constexpr`
- Using `std::floating_point` concept instead of `typename`
@rubiefawn
Copy link
Contributor Author

Updated to account for <numbers> and C++20:

  • Marked all lmms_constants.h constants with an exact equivalent in <numbers> as deprecated
  • Removed all lmms_constants.h constants where no variant is currently in use
  • Using inline constexpr
  • Using std::floating_point concept instead of typename

@messmerd
Copy link
Member

messmerd commented Oct 23, 2024

This won't work. I suggested waiting for C++20 and the compiler upgrade since we are in the process of doing that now.

Currently we are using C++17 and GCC 9.3 for Linux and MinGW builds, so we can't use the <numbers> header or concepts yet.

If you wanted, maybe you could convert our mathematical constants to use the same names as those in <numbers>, but in the lmms::numbers namespace. Then once we upgrade to C++20, switching to <numbers> will be as easy as swapping out lmms::numbers::pi for std::numbers::pi. That may be quite a bit of work for you though, so it's up to you.

EDIT: The constants should probably be in a new lmms::numbers namespace to avoid collisions. It would basically just be a reimplementation of the parts of the numbers header that we use.

@rubiefawn rubiefawn marked this pull request as draft October 24, 2024 00:43
@rubiefawn
Copy link
Contributor Author

I'm aware that we don't have C++20 ready yet. I made the suggested changes (and more) in advance under the assumption that this PR was now dependent on #7554, based off of your suggestion. If this is not the case I can change it back, I'm good with whatever.

My original intention with this PR was small in scope; I just wanted to clean up lmms_constants.h. That being said, I love your suggestion for lmms::numbers, and if updating all usage of the constants is on the table, I'd love to explore that.

@messmerd
Copy link
Member

Ah gotcha. Sorry for the miscommunication. The C++20 support could happen tomorrow if people could review the PR, but upgrading our MinGW compiler and dependencies will be tricky and take some time.

I think the lmms::numbers idea would be good to implement. Function scope usages could use using namespace numbers; so that it doesn't need to be typed out fully as the clunkier numbers::pi or numbers::pi_v<float> in places where that would look better.
I'm not sure how big a task it would be, but it would probably be fairly easy with vscode or some other program to just search through src/, include/, and plugins/ and replace all.

@sakertooth
Copy link
Contributor

If I understand @messmerd correctly, I agree with him. We should wait for C++20, remove lmms_constants.h, and then just use the numeric library provided by the standard. The more code we can cut down and remove, I see as a win.

@rubiefawn
Copy link
Contributor Author

Just to clarify, there are several useful constants in lmms_constants.h that have no overlap with C++20 <numbers>. This PR isn't about removing that header entirely, it's just to clean it up. I agree with the "less is more" sentiment though.

On that note, I noticed panning_constants.h and panning.h and am wondering if those can be merged into one file (or even just moved to lmms_constants.h). Those could be considered further candidates for constant cleanup.

The current plan is to wait for #7554 before I make further changes. Input is welcome in the meantime!

@sakertooth
Copy link
Contributor

#7554 is here, so you can try and make the switch to the numbers header @rubiefawn (we have partial C++20 support, but I think we should have the numbers header at least).

On that note, I noticed panning_constants.h and panning.h and am wondering if those can be merged into one file (or even just moved to lmms_constants.h). Those could be considered further candidates for constant cleanup.

It's up to you really (or anyone else that wants to give it a go). I personally think it could go multiple ways with no apparent downside.

@messmerd
Copy link
Member

@sakertooth Nope, like most C++20 features, we're waiting for GCC 10.

See: https://en.cppreference.com/w/cpp/compiler_support/20
(Mathematical constants P0631R8)

@sakertooth
Copy link
Contributor

Hard to classify it as an upgrade then, but I guess it's a step in the right direction..

@messmerd
Copy link
Member

I'm using both class type NTTP and designated initializers in the pin connector now, so it's not nothing. Once I'm done with the pin connector, I'd like to upgrade to GCC 11 on Linux and MinGW 13 for the cross-compiled Windows builds. Then we'll have nearly all C++20 features including <numbers>.

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.

3 participants