It's a lot of places to change to add a new maths function isn't it! 😆

In D10591#267657, @Charlie Jolly (charlie) wrote:

It's a lot of places to change to add a new maths function isn't it! 😆

Oh yes, it is (It is a change for five Editors if you count Cycles)! But thanks to VS Code and the Keyword I had to look for, I found them. :)

Generally looks fine, but got some remarks anyway:

• Please apply clang-format to your changes. You can do that by running the make format command or by setting up your editor to apply clang-format automatically when you save a file.
• Comments should start with a capital letter and end with a dot.
• Why do some of the implementations handle more cases than others?

In D10591#267833, @Jacques Lucke (JacquesLucke) wrote:

Generally looks fine, but got some remarks anyway:

• Please apply clang-format to your changes. You can do that by running the make format command or by setting up your editor to apply clang-format automatically when you save a file.

How do I set up VS Code to apply clang-format automatically? That would help me very much :)

Have a look at https://wiki.blender.org/wiki/Tools/ClangFormat#IDE_Integration.

In D10591#267833, @Jacques Lucke (JacquesLucke) wrote:

• Why do some of the implementations handle more cases than others?

Oh, I guess I forgot to add that for the ones that handle less cases. Could you highlight the code sections that handle less cases than the others?

Well, just from looking at the code it seems like MathRootOperation::executePixelSampled than e.g. the code in svm_math_util.h.
Maybe you can just add a new safe_root function and use that in the cases where it is possible?

In D10591#267849, @Jacques Lucke (JacquesLucke) wrote:

Well, just from looking at the code it seems like MathRootOperation::executePixelSampled than e.g. the code in svm_math_util.h.
Maybe you can just add a new safe_root function and use that in the cases where it is possible?

"MathRootOperation::executePixelSampled" / "node_texture_math.c" --> It looks like that's Code left from the code I reused, and I think the second big else statement can be safely removed, since the root of a negative number is not defined (at least not with real numbers)

Maybe you can just add a new safe_root function and use that in the cases where it is possible?

I thought of that, but I didn't find the file where e.g. safe_sqrt() is defined. I'll try to find it, a function like this would be a good idea.

There inconsistency between the different implementations. Please implement all of them as follows:

(b > 0.0) ? safe_pow(a, 1.0 / b) : 0.0

Where safe_pow is exactly the function used by the Power operation. To avoid code duplication with the Power operation, add safe_pow functions where they don't already exist.

Note also that a should not be required to be positive, negative numbers are fine for cube roots.

@Brecht Van Lommel (brecht) Ok, thank you for your feedback. Should I use a safe_root() function, like @Jacques Lucke (JacquesLucke) suggested, or is that unnecessary?

I think that's unnecessary, just safe_pow should be enough.

@Brecht Van Lommel (brecht) Ok, then I'll do it without a save_root() function

Note also that a should not be required to be positive, negative numbers are fine for cube roots.

@Brecht Van Lommel (brecht) The problem with that is, that it's only possible if the index is an odd number. And the different power functions (glsl pow(), osl pow(), pow() and safe_powf()) all return a different solution for e.g. cubic root of -27:

• glsl pow() [Eevee] returns -3
• pow() [Compositor, Texture Nodes] returns NaN
• safe_powf() and osl pow() [Cycles, Geometry Nodes] return 0

Also, from a user perspective, it might be confusing changing the index for a negative number when the result is 0 almost everywhere, except for an odd number as index.
In order to get that working, osl would have to change its pow() function, so I guess we're limited to positive numbers only. ¯\_(ツ)_/¯

Please let me know if you know a better way to make negative cubic (, ...) roots possible. Thanks!

It seems you've found an inconsistency indeed! I did a quick summary of the pow functions which this relies upon. Since this hasn't been fixed it is unlikely that negative numbers have been used much.

GLSL
https://www.khronos.org/registry/OpenGL-Refpages/gl4/html/pow.xhtml
pow returns the value of x raised to the y power, i.e. xy. The result is undefined if x<0 or if x=0 and y≤0.

OSL
https://github.com/AcademySoftwareFoundation/OpenShadingLanguage/blob/master/src/doc/osl-languagespec.pdf
type pow (type x, float y)
Computes x^y. This function will return 0 for “undefined” operations, such as pow(-1,0.5).

In D10591#268852, @Charlie Jolly (charlie) wrote:

It seems you've found an inconsistency indeed! I did a quick summary of the pow functions which this relies upon. Since this hasn't been fixed it is unlikely that negative numbers have been used much.

Area	Pow Function	Value check
Cycles	safe_powf	Checks values then calls compatible_powf which uses fmod
OSL	pow	Calls native pow function
Eeevee GLSL	math_power	Checks values then calls compatible_pow which uses mod
NOD Math	safe_powf	Checks values then calls powf
Compositor and Texture Nodes	Custom function	Checks values then calls powf

GLSL
https://www.khronos.org/registry/OpenGL-Refpages/gl4/html/pow.xhtml
pow returns the value of x raised to the y power, i.e. xy. The result is undefined if x<0 or if x=0 and y≤0.

OSL
https://github.com/AcademySoftwareFoundation/OpenShadingLanguage/blob/master/src/doc/osl-languagespec.pdf
type pow (type x, float y)
Computes x^y. This function will return 0 for “undefined” operations, such as pow(-1,0.5).

Now, the question is, what should we do about it? A cheap workaround would be to make negative numbers positive, then take the root and make it negative again, but this couldn't be done without creating at least two root() functions. That would require more work. The other possibility would be to say "negative roots are undefined" and require a to be positive as well (as it's done here). What do you think would be better?

The other possibility would be to say "negative roots are undefined" and require a to be positive as well (as it's done here).

I think do this then the pow and negative number issue can be addressed in a new patch. At least for the patch make it consistent as to how root works.

In D10591#268854, @Charlie Jolly (charlie) wrote:

I think do this then the pow and negative number issue can be addressed in a new patch. At least for the patch make it consistent as to how root works.

So I guess I have to leave that as it is and wait for the reviewers feedback now...

The reviewer would need to approve/commit the patch anyway so I'm sure they'll add their thoughts at that time.