Fix #104266: Metal AMD refractions #105967

Jason Fielder · 2023-03-21T18:53:57+01:00

Jason Fielder commented

2023-03-21 18:53:57 +01:00

Workaround for compiler issue on AMD
platforms resulting in the erroneous
discarding of valid rays in fragment
raytracing.

Authored by Apple: Michael Parkin-White

Workaround for compiler issue on AMD platforms resulting in the erroneous discarding of valid rays in fragment raytracing. Authored by Apple: Michael Parkin-White

Jason Fielder requested review from Jeroen Bakker 2023-03-21 18:54:53 +01:00

Jason Fielder requested review from Clément Foucault 2023-03-21 18:54:54 +01:00

Hans Goudey changed title from ~~Fix #104266: Metal AMD refractions.~~ to Fix #104266: Metal AMD refractions

2023-03-21 19:21:04 +01:00

Clément Foucault approved these changes 2023-03-21 20:48:06 +01:00

Clément Foucault left a comment

Nice find! I'm just adding suggestion to make the code clearer, but the patch is good.

source/blender/draw/engines/eevee/shaders/raytrace_lib.glsl Outdated

						
				@ -127,2 +127,4 @@

				  float lod_fac = saturate(fast_sqrt(params.roughness) * 2.0 - 0.4);

				#if defined(GPU_METAL) && defined(GPU_ATI)

				  bool hit_valid = true;

Clément Foucault commented

2023-03-21 20:34:18 +01:00

Suggest: Name it hit_failsafe and move it near bool hit = false;.

Suggest: Name it `hit_failsafe` and move it near `bool hit = false;`.

source/blender/draw/engines/eevee/shaders/raytrace_lib.glsl Outdated

						
				@ -146,0 +150,4 @@

				#if defined(GPU_METAL) && defined(GPU_ATI)

				    /* NOTE(Metal): Invalidate all future hits using immediate test to avoid side-effects from

				     * non-uniform control flow. This must only be used to invalidate while hit is false.  */

				    bool raytrace_fail_check =

Clément Foucault commented

2023-03-21 20:44:44 +01:00

I would replace it by bool hit_valid = !(discard_backface && prev_delta < 0.0) && (depth_sample != 1.0);
This at least doesn't confuse the reader as of why it is different from the same check at the bottom.

Also !hit operation at the end is always true at this point here so I removed it from the suggestion.

About the comment itself I don't see how this avoid non uniform control flow. So maybe note that this is to avoid a compiler bug.

I would also be tempted to make a METAL_REFRACTION_WORKAROUND define for these #if directive. This would make it clear all these checks are related to the same workaround.

I would replace it by `bool hit_valid = !(discard_backface && prev_delta < 0.0) && (depth_sample != 1.0);` This at least doesn't confuse the reader as of why it is different from the same check at the bottom. Also `!hit` operation at the end is always true at this point here so I removed it from the suggestion. About the comment itself I don't see how this avoid non uniform control flow. So maybe note that this is to avoid a compiler bug. I would also be tempted to make a `METAL_REFRACTION_WORKAROUND` define for these `#if` directive. This would make it clear all these checks are related to the same workaround.

Clément Foucault approved these changes 2023-03-22 09:51:27 +01:00

Clément Foucault force-pushed Fix_104266_MetalAMDRefraction_2 from e5d92615ad to 84e6cbb51e

2023-03-22 09:52:10 +01:00

Compare

Clément Foucault merged commit f73d1ddce4 into blender-v3.5-release

2023-03-22 09:57:40 +01:00

Clément Foucault referenced this issue from a commit

2023-03-22 09:57:41 +01:00

Fix #104266: Metal AMD refractions

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Fix #104266: Metal AMD refractions #105967