-
-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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 constexpr in default math functions where possible #7662
Conversation
It doesn't seem super wise to replace fabsf() and fmodf() in our context without an in-depth explanation and proof that this is going to be equivalent on many fronts? Do you actually need e.g. ImFabsf() at compile-time? |
I have looked at https://github.com/kfish/impop/blob/master/include/impop_color.h to get a little more context at your use case. My slight issue with turning everything Which function do you think you actually need? |
Hi @ocornut, thanks for looking into this. I understand that we can't guarantee correct fmodf and fabs etc. implementations on all platforms. The trivial implementations I provided might not handle errors the same way as the standard functions, for example for fmodf, "If a range error occurs due to underflow, the correct result (after rounding) is returned." There will be constexpr versions of these in C++23 (and using eg. std::fmodf not C99 fmodf) so anyone who wants constexpr versions would be better off using those in future anyway. As my use case is fairly trivial, just looking at color values, I'll keep these definitions in impop, along with the constexpr color conversion and interpolation functions etc. Thanks also for looking at impop, Some of it is pretty application-specific but bits and pieces could probably be merged up eventually. The app config is fairly generic but uses imgui internals; similarly the canvas can probably be replaced with the work rect and the datepicker (which is just a wrapper around ImPlot::ShowDatePicker()) could be useful to expose in implot at some point. Anyway, happy to close this PR as is. |
I don’t mind merging some of this if helpful, unsure why closing? My question was mostly aiming to understand which functions you needed. |
Ok no problem, I'll re-open. No rush, happy to help clean up whichever bits you find useful |
Hello, I reevaluated this and decided against it for now. I think would imply we encourage people to use our internal math stuff as a mini math library, which may be too much responsibility to carry on. My test minimum target compiler VS 2015 also isn't happy with all of it: static constexpr float ImLinearSweep(float current, float target, float speed) { if (current < target) return ImMin(current + speed, target); if (current > target) return ImMax(current - speed, target); return current; } error C3259: 'constexpr' functions can only have one return statement I'd prefer to close for now. |
This allows some basic math, palette generation etc. to be done in constructors or at compile-time.
imgui_internal.h: Add constexpr (replace "static inline" with "static constexpr") in math functions. This also implements ImFabs and ImFmod explicitly as fabsf and fmodf are not necessarily constexpr. Other non-constexpr functions (anything involving transcendentals, sqrt etc) remain "static inline".