search

3Dickulus / FragM

Derived from https://github.com/Syntopia/Fragmentarium/
GNU General Public License v3.0
349 stars 30 forks source link

[RFC] refactor Complex.frag to support both float and double #67

Closed claudeha closed 4 years ago

claudeha commented 5 years ago

At the moment, when USE_DOUBLE is defined, the complex functions (eg cMul()) are defined only for double precision.

Proposal: refactor Complex.frag so that it includes a DoubleMath.frag for the trig etc and a ComplexBase.frag twice (once for float and once for double with different VEC2 defines etc).

It should be backward compatible, just exposing additional overloaded float versions of the complex functions even when USE_DOUBLE is defined.

3Dickulus commented 5 years ago

I suppose if the compiler doesn't choke on it and it doesn't create much of a binary footprint it sounds great, Complex.frag is almost entirely your doing anyways (great work btw) so you can take it in any direction you like, I'll always support improvements :-D

as long as we don't have to change every occurrence of vec with VEC in all frags :-p or all previous frags that used Complex.frag break now :-(

Proposal Rebuttal:

2 new files DoubleMath.frag DoubleComplex.frag keep the name of Complex.frag the same

define USE_DOUBLE at the top of DoubleMath.frag so if no #include DoubleMath.frag in loaded working frag then included Complex.frag is float only, if it is included then double types enabled as well, this should make earlier frags happy too. Complex.frag can detect USE_DOUBLE and include the DoubleComplex.frag

edit: not really a rebuttal more like agreement

claudeha commented 5 years ago

User frags should work as before. If USE_DOUBLE is defined, the VEC etc will be double as before, otherwise float. User frags can choose whether to use vec (single) dvec (double) or VEC (highest available precision).

Re the rebuttal, I don't want to have to write all the complex functions twice, which is why I want to do something like:

#define VEC2 vec2
#include "ComplexBase.frag"
#ifdef USE_DOUBLE
#undef VEC2
#define VEC2 dvec2
#include "ComplexBase.frag"
#endif
3Dickulus commented 5 years ago

Can the name remain Complex.frag and #define USE_DOUBLE in DoubleMath.frag so it is enabled when DoubleMath.frag is included? I think that would be ideal.

claudeha commented 5 years ago

Sure, but for backwards compatibility with user frags, I think it'd be better for Complex.frag to do something like:

#ifdef USE_DOUBLE
#include "DoubleMath.frag"
#endif

But perhaps it would be better to drop needing USE_DOUBLE at all, and just check for #if __VERSION__ >= 400? I don't know if there are any implementations out there that support double and don't support OpenGL 4, I guess we deal with that when a bug report arrives...

3Dickulus commented 5 years ago

that's a good point, maybe FragM should not care what the user wants and it will simply make it available if it's supported, if it doesn't get used then there is no impact on binary size after optimization by the compiler, and the user doesn't have to wonder "how and where do I enable double types"

#if __VERSION__ >= 400
#include "DoubleMath.frag"
#endif

...in Complex.frag sounds good

3Dickulus commented 4 years ago

bump... any progress with further testing?

I think this would be more applicable on the gl4dev branch and #included by default in new FragMv3 fragment code, a legacy branch \< GL4 and a modern branch > GL4

claudeha commented 4 years ago

Not had a chance to look at it recently, sorry.

Issues remaining to be investigated: