Open saki7 opened 8 years ago
Thanks @saki7 - I think these have stuck around for too long. I know the PI definitions have caused issues with other libraries in the past.
Maybe we can deprecate somehow or remove them but allow for them to be reenabled for backwards compatibility?
ie:
#define OF_NEED_LEGACY_MACROS
CLAMP I don't think people use that much as we have ofClamp.
ABS I think most people just use the abs and fabs functions.
DEG_TO_RAD / RAD_TO_DEG
is used quite a bit.
as are the MIN and MAX functions
Thanks for the reply.
#define OF_NEED_LEGACY_MACROS
looks fine to me.
I think the core files must use the newer (suggested) versions and we should limit the legacy macros usage to the external files (e.g. addons).
Note that it is required to carefully replace the ABS
macro to the corresponding std::
functions since it is currently (mistakenly) used for both integer and float variables.
just to add that we have ofDegToRad() and ofRadToDeg which i think most people use instead of the macros and that glm which should be in the core soon includes glm::pi() but we could also have our own version as an inlined function.
and as @saki7 says MIN and MAX can be replaced with std::min and max so after a period of deprecation we can remove all of this macros without almost need for any replacement that is not there yet
I just pushed a PR that addresses this and removes all of the uses of our macros from the core, replacing them with glm::*, std::min/max, etc.
I think it's good to replace these but I am a little worried about tons of code breaking by just removing these rather than depreciating them.... I know I personally use the constants a lot in my code and examples. Mostly I use PI and DEG_TO_RAD / RAD_TO_DEG and MIN MAX... I didn't even know there were functions for rad to degrees :)
also, I am all for using GLM where possible but I feel like this :
glm::pi<float>()
is not as beginner friendly as
PI
I wonder if there's some clever wrapping we can do to make it a little beginner friendly? for example: I know we don't have namespace, but something like
of::PI
seems less of a leap than PI -> glm::pi<float>()
....
anyway, I am all for transitioning away from these but I wonder if there's a gentle way to do it.
or alternatively, instead of just putting them all in a
block, still allowing them but adding deprecation warning at the usage level if OF_USE_LEGACY_MACROS is not defined.... it would not break old code but throw a bunch of messags like use std::min vs MIN , etc.... at least for one release or so.
Yeah, I think just doing these without a deprecation release would be a nightmare. The PR for now was just too take the first step to removing everything from the core. The next step in my mind is to add deprecation warnings and define OF_USE_LEGACY_MACROS before release so they are all still available with warnings.
Same goes for GLM and other big changes. Communicate upcoming changes with warnings, then follow up with removal in a release or two.
Just a note that I just found an earlier issue related to this:
https://github.com/openframeworks/openFrameworks/issues/1007
There is some relevant discussion there.
+1 for OF_USE_LEGACY_MACROS
and deprecation warnings. but what after these are removed?
As @ofZach says,
glm::pi
() is not as beginner friendly as PI
What about OF_PI instead of PI? I guess that in this case it is better to have a preprocessor directive rather than an OF function to wrap glm::pi<float>()
Do we have any consensus on this?
@arturoc ?
ping @arturoc @ofTheo
this can now be removed. but it needs some decision of how to deal with examples. if using of::something or converting to glm:: or std::
@dimitre let's leave this open and I'll tackle the last thing you mentioned.
I think we could have of::pi which can be later assigned to use std::numbers::pi;
These are definetely evil:
Suggestions
PI
constexpr
value withM_PI
.TWO_PI
M_TWO_PI
FOUR_PI
HALF_PI
DEG_TO_RAD
constexpr
function.RAD_TO_DEG
constexpr
function.MIN
std::min()
.MAX
std::max()
.CLAMP
constexpr
function.ABS
std::abs
.std::fabs
too.