search

LMMS / lmms

Cross-platform music production software
https://lmms.io
GNU General Public License v2.0
8.18k stars 1.01k forks source link

Use templates for common geometric constants #7558

Open rubiefawn opened 1 month ago

rubiefawn commented 1 month ago

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:

messmerd commented 1 month ago

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.

rubiefawn commented 1 month ago

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

messmerd commented 1 month ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 3 weeks ago

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 commented 2 weeks ago

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!