Page MenuHome

Fix for T69776
AcceptedPublic

Authored by Patrick Bender (ichbinkeinreh) on Dec 18 2019, 8:23 PM.

Details

Summary

This diff fixes https://developer.blender.org/T69776 by inlining the floorfrac function in the noise shaders.
It seems that the issue was present because the glsl compilers of the graphic card vendors could not inline the floorfrac function and therefore some limit in the gpu triggers the issue.

Diff Detail

Event Timeline

Germano Cavalcante (mano-wii) requested changes to this revision.EditedDec 19 2019, 1:11 AM

Nice to see you could find the cause of the problem!

But I have concerns about this solution.
floorfrac is no longer used, so it should be removed;
But first I would try other alternatives (as this one would complicate understanding of the code).

Have you tried using preprocessing directives?
Something like this:

#define floorfrac(v, v_floor, v_fract)\
  float _##v##_floor = floor(v);\
  v_floor = int(_##v##_floor);\
  v_fract = v - _##v##_floor

Or changes in the function like:

float floorfrac(float x, out int i)
{
  i = int(x);
  return fract(x);
}

If you decide to try changing the function, I suggest checking the peformanse.

@Clément Foucault (fclem) can have better ideas.

This revision now requires changes to proceed.Dec 19 2019, 1:11 AM

On second thought.
I believe the readability and performance of the code would be even better if the function was removed and in place int() and fract() were used.

int X = int(x);
float fx = fract(x);

I remember that fract was implementation dependent and lacked some precision in some cases.

I would prefer the MACRO style. Do note that multiline macros crash/break with some GPU/drivers. So use /* clang-format off */ for that.

I believe this is just moving the issue a bit further. You will end up hitting the limit anyway (which is a driver bug).

Changed floorfrac to be a macro.

Also tried to find if i can hit a new limit and trigger the bug again. The maximum i could try was 9 4D noise nodes attached to a bump node and 1 4D noise node as color. On my machine that did not result in any visible errors.

Without much option and unable to test, I think the patch is ok.
But I would still prefer an inline option without macro, but simplifying the previous solution.

I remember that fract was implementation dependent and lacked some precision in some cases.

If this is the case, we could do our own fract function:

float fract_safe(float x)
{
  return x - floor(x);
}

And with that function, it would look like this:

int X = int(x);
float fx = fract_safe(x);
This revision is now accepted and ready to land.Dec 19 2019, 9:57 PM

@Germano Cavalcante (mano-wii) I think your code is not completely right. Instead of:

int X = int(x);
float fx = fract_safe(x);

It should be:

int X = floor_to_int(x);
float fx = fract_safe(x);

I tried that solution too after you mentioned it. On my machine it renders 5 3D noise nodes fine. However i was able to trigger the original error again with 10 4D noise nodes. Whereas with the macro solution also 10 4D noise nodes work fine on my machine.

@Germano Cavalcante (mano-wii) Sorry for the question, but this is my first contribution to blender. Do i have to anything now to get the patch merged into the master branch? Or just beeing patient until someone with write permission merges it?

I'll wait to see if anyone has anything to add, then I can commit it.

Clément Foucault (fclem) requested changes to this revision.Dec 22 2019, 1:59 AM
Clément Foucault (fclem) added inline comments.
source/blender/gpu/shaders/material/gpu_shader_material_math_util.glsl
52

what I meant with multiline macros is the use of \

You must write the whole macro in one line.

#define FLOORFRAC(x, X, x_floor, x_fract) x_floor = floor(x);X = int(x_floor); x_fract = x - x_floor;

rename X to x_int

This revision now requires changes to proceed.Dec 22 2019, 1:59 AM

FLORRFRAC macro is now in one line. Renamed X to x_int in FLOORFRAC macro.

Patrick Bender (ichbinkeinreh) marked an inline comment as done.Fri, Dec 27, 7:36 PM
This revision is now accepted and ready to land.Wed, Jan 15, 9:32 PM

Before committing, I would like to propose these changes.
But since many compilers behave differently, I'm afraid you might fail some of them.

  • Remove floorfrac from gpu_shader_material_math_util.glsl
  • Avoid creating x_int when apparently unused
  • gpu_shader_material_math_util_library is no longer needed in gpu_shader_material_noise
  • Remove accidentally added changes
  • Revert "Python: disable environment variables by default"

Added "clang-format on" and" clang-format off". FLORRFRAC macro is now again in one line

Previous patch had an error. Added "clang-format on" and" clang-format off". FLORRFRAC macro is now again in one line