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

Replace macros / constants in examples (but with what?) #5641

Open
bakercp opened this issue May 25, 2017 · 11 comments
Open

Replace macros / constants in examples (but with what?) #5641

bakercp opened this issue May 25, 2017 · 11 comments

Comments

@bakercp
Copy link
Member

bakercp commented May 25, 2017

Related to #5624, but I believe we need to decide what the correct solution is on the user side for our PI, TWO_PI, constants as the feedback for something like glm::pi<float> indicates a need for something simpler.

See also #5163 and #1007 for previous discussions.

@dimitre
Copy link
Member

dimitre commented May 25, 2022

I'm up to use glm constants and tau instead of TWO_PI :)

@ofTheo
Copy link
Member

ofTheo commented May 25, 2022

@bakercp the PR looks good to me - going to leave one comment in there - but otherwise good to merge.
I think it is also okay to use glm::pi internally.

@ofTheo
Copy link
Member

ofTheo commented May 26, 2022

@ofZach

I think the PR could get merged and we could work on a better user facing solution? ( I don't think #defines for PI etc in a global namespace are tenable ).

float ofPi(); Could be an obvious one? which would just return glm::pi<float>()

@ofZach
Copy link
Contributor

ofZach commented May 26, 2022

sure -- it def feels like we could use a friendly user facing option -- for sure this

glm::pi<float>()

is going to make a lot of beginner oriented code feel a lot less friendly.
I also think this will break alot of old code so we should think about the friendliest solution

@ofZach
Copy link
Contributor

ofZach commented May 26, 2022

one quick thought, I don't know if this reasonable, could we keep these constants in the ofApp class / ofApp namespace as consts, constexpressions etc -- so example and older sample code can stay as is? Then elsewhere, say in another class, you could write ofApp::PI ? The goal would be to not have code break as much as possible and keep things simple. I don't know if this is simpler, or even could work, but just thinking out loud. happy to take a look at this...

@ofTheo
Copy link
Member

ofTheo commented May 26, 2022

Jumping back to the namespace thoughts

Constants in the of namespace
of::Pi
of::HalfPi
of::TwoPi

More nested
of::Math::Pi
of::Math::HalfPi
of::Math::TwoPi
of::Math::Lerp(src, dst, 0.2)

Unity style ( note this is sort of weird, but would mimic the Unity approach, also not sure this can be done in C++ in a non hackey way )
of::Math.Pi
of::Math.TwoPi
of::Math.Lerp(src, dst, 0.2)

@ofZach Either way I think it is safer to just keep the current defines/macros for now ( with a deprecation warning ), but switch over to using the newer api for OF core and examples?

The ofApp solution is clever but it wouldn't account for classes in people's projects or addons and probably better to have an of::Pi ( or whatever we choose ) and warn about PI instead of having ofApp::PI which doesn't fix all use cases and also sort of splits the API in a weird way.

Anyway, it is a great excuse to circle back to the namespace discussion 🙂

@roymacdonald
Copy link
Contributor

my 2 cents, In terms of friendliness PI wins above all. But then, of::Pi or of::PI feels the closest. if using a namespace you can even have in the ofApp.cpp something like using namespace of and you would still able to use PI

@ofTheo
Copy link
Member

ofTheo commented Jun 2, 2022

Good point @roymacdonald !

Wondering though if of::PI prevents us from holding onto the macros at the same time?
ie: will of::PI and PI clash if we don't remove the #define PI at the same time as adding of::PI and doing using namespace of; ?

If we want to keep PI around for a bit ( to avoid breaking legacy code ), it might make sense to not mirror the old API but do it the way we want:

ie: of::Pi could exist at the same time as PI.

Will try this out though and do a PR for people to test with.

@oxillo
Copy link
Contributor

oxillo commented Jun 3, 2022

C++20 define std::numbers::pi (float,double and long double). It does not have 2pi.

can we have something more like of::numbers::pi ?
So we can have something like :

#if __cplusplus >= 202002L
  using namespace std::numbers;
#else
  using namespace of::numbers;
#endif

perimeter = 2 * pi * radius;

And that does not prevent using namespace of; elsewhere

@dimitre
Copy link
Member

dimitre commented Mar 12, 2023

now we are using namespace of for of::filesystem
maybe it is a good place to have constants too

@dimitre
Copy link
Member

dimitre commented Mar 22, 2023

This PR is meant to minimize the use of Math constants in the core #7413
so some of them can be removed in the future

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

No branches or pull requests

6 participants