Cycles: Fix invalid normals in various situations #114960

Alaska · 2023-11-16T05:57:36+01:00

Alaska commented

2023-11-16 05:57:36 +01:00

Fix issues related to NaN normals in some situations
by trying to detect when these cases might occur and just reverting
back to default normals.

As a side effect of these changes, OSL now behaves correctly when given
a non-normalized normal.

Fix issues related to NaN normals in some situations by trying to detect when these cases might occur and just reverting back to default normals. As a side effect of these changes, OSL now behaves correctly when given a non-normalized normal.

Alaska added 1 commit 2023-11-16 05:57:43 +01:00

Fix NaN in ensure_valid_specular_reflection()

buildbot/vexp-code-patch-coordinator Build done.

Details

665e6787b2

Alaska requested review from Brecht Van Lommel 2023-11-16 05:57:55 +01:00

Alaska requested review from Lukas Stockner 2023-11-16 05:58:03 +01:00

Alaska requested review from Sergey Sharybin 2023-11-16 05:58:11 +01:00

Alaska commented

2023-11-16 06:00:57 +01:00

This should probably be back ported to 3.3, 3.6, and 4.0.1 corrective release if it gets accepted.

Comment if you disagree.

This should probably be back ported to 3.3, 3.6, and 4.0.1 corrective release if it gets accepted. Comment if you disagree.

Sergey Sharybin commented

2023-11-16 09:17:49 +01:00

@blender-bot build

Sergey Sharybin reviewed 2023-11-16 16:55:36 +01:00

Sergey Sharybin left a comment

I think it is fine to avoid NaN like that.

One thing which I am not immediatelly sure, is whether using a non-normalized normal will return normalized normal from this function?

I think it is fine to avoid NaN like that. One thing which I am not immediatelly sure, is whether using a non-normalized normal will return normalized normal from this function?

Alaska commented

2023-11-16 20:48:19 +01:00

One thing which I am not immediatelly sure, is whether using a non-normalized normal will return normalized normal from this function?

I may be mis-understanding the question you're asking here.

From my understanding, if a non-normalized normal is passed to safe_normalize(), then it will be normalized.

The only thing that changes is a (0, 0, 0) vector will be remain (0, 0, 0) instead of turning into (Nan, NaN, NaN). (0, 0, 0) is not normalized, so that could lead to issues later on inside ensure_valid_specular_reflection(), or in other functions.

We could try other approaches like:

X = normalize(N - dot(N, Ng) * Ng)
if (!isnan_safe(X)) {
   X = one_float();
}

or

X = safe_normalize(N - dot(N, Ng) * Ng)
if (dot(X, X) < 0.9f) {
   X = one_float();
}

or something similar.

> One thing which I am not immediatelly sure, is whether using a non-normalized normal will return normalized normal from this function? I may be mis-understanding the question you're asking here. From my understanding, if a non-normalized normal is passed to safe_normalize(), then it will be normalized. The only thing that changes is a (0, 0, 0) vector will be remain (0, 0, 0) instead of turning into (Nan, NaN, NaN). (0, 0, 0) is not normalized, so that could lead to issues later on inside `ensure_valid_specular_reflection()`, or in other functions. We could try other approaches like: ``` X = normalize(N - dot(N, Ng) * Ng) if (!isnan_safe(X)) { X = one_float(); } ``` or ``` X = safe_normalize(N - dot(N, Ng) * Ng) if (dot(X, X) < 0.9f) { X = one_float(); } ``` or something similar.

Lukas Stockner commented

2023-11-18 19:44:42 +01:00

For zero normal inputs, the most straightforward fix would be to just return N as-is and skip the process. The entire algorithm is undefined for that case.

I'd go a step further and argue that we should always replace zeroed shading normals with Ng because they're obviously a mistake and are likely to break other things.

Also, looking at this, I'd expect numerical issues if N and Ng are almost equal, so that the unnormalized X is almost zero. We do check for exact equality in maybe_ensure_valid_specular_reflection, but with floating point numbers that might not be good enough.

Oh, and X = one_float3() in particular will cause other issues since it's not normalized.

For zero normal inputs, the most straightforward fix would be to just return `N` as-is and skip the process. The entire algorithm is undefined for that case. I'd go a step further and argue that we should always replace zeroed shading normals with `Ng` because they're obviously a mistake and are likely to break other things. Also, looking at this, I'd expect numerical issues if `N` and `Ng` are almost equal, so that the unnormalized X is almost zero. We do check for exact equality in `maybe_ensure_valid_specular_reflection`, but with floating point numbers that might not be good enough. Oh, and `X = one_float3()` in particular will cause other issues since it's not normalized.

Alaska changed title from ~~Cycles: Fix NaN in ensure_valid_specular_reflection()~~ to WIP: Cycles: Fix NaN in ensure_valid_specular_reflection()

2023-11-18 22:54:38 +01:00

Alaska commented

2023-11-18 23:07:51 +01:00

I'd go a step further and argue that we should always replace zeroed shading normals with Ng because they're obviously a mistake and are likely to break other things.

I assume when you say "we should always replaced zeroed shading normals with Ng", you mean for everything, not just maybe_ensure_valid_specular_reflection()?

Edit: The question that has been crossed out below is mostly irrelevant due to the recent commit.

~~If so, where do you think would be the best place to do this? For example:~~

~~Should I put the code in /intern/cycles/svm/closure.h, running the code on every Normal input?~~
~~Or should I put the check inside the functions like bsdf_sheen_setup?~~
~~Or should it go somewhere else?~~

> I'd go a step further and argue that we should always replace zeroed shading normals with `Ng` because they're obviously a mistake and are likely to break other things. I assume when you say "*we should always replaced zeroed shading normals with Ng*", you mean for everything, not just `maybe_ensure_valid_specular_reflection()`? Edit: The question that has been crossed out below is mostly irrelevant due to the recent commit. ~~If so, where do you think would be the best place to do this? For example:~~ - ~~Should I put the code in `/intern/cycles/svm/closure.h`, running the code on every Normal input?~~ - ~~Or should I put the check inside the functions like `bsdf_sheen_setup`?~~ - ~~Or should it go somewhere else?~~

Alaska added 2 commits 2023-11-22 09:28:57 +01:00

Merge branch 'main' into fix-valid-reflection-NaN a3b020a40b

Use default normals inplace of invalid 0, 0, 0 length normals 81b14b4bcb

Alaska changed title from ~~WIP: Cycles: Fix NaN in ensure_valid_specular_reflection()~~ to Cycles: Fix invalid normals in various locations

2023-11-22 09:32:38 +01:00

Alaska changed title from ~~Cycles: Fix invalid normals in various locations~~ to Cycles: Fix invalid normals in various situations

2023-11-22 09:35:57 +01:00

Alaska commented

2023-11-22 09:37:47 +01:00

With this pull request, there is a difference between how Cycles and EEVEE behaves when passed a 0, 0, 0 normal.

Cycles will return the default normals of the object.
EEVEE seems to use 1, 0, 0 normals

With this pull request, there is a difference between how Cycles and EEVEE behaves when passed a `0, 0, 0` normal. Cycles will return the default normals of the object. EEVEE seems to use `1, 0, 0` normals

Alaska added 1 commit 2023-11-22 13:01:59 +01:00

Fix coat normal 7cbadb082b

Lukas Stockner approved these changes 2023-11-23 22:54:51 +01:00

Lukas Stockner left a comment

Overall LGTM, just one typo.

Regarding the difference to EEVEE: I think that's fine - (1, 0, 0) doesn't make much sense, so I think it's fair to treat it as undefined behavior instead of an intentional (different) fallback.

Overall LGTM, just one typo. Regarding the difference to EEVEE: I think that's fine - `(1, 0, 0)` doesn't make much sense, so I think it's fair to treat it as undefined behavior instead of an intentional (different) fallback.

intern/cycles/kernel/closure/bsdf_util.h Outdated

						
				@ -132,1 +132,3 @@

				  const float3 X = normalize(N - dot(N, Ng) * Ng);

				  const float3 X = safe_normalize(N - dot(N, Ng) * Ng);

				  if (len(X) < 0.5f) {

				    /* Vector was not normalized by `safe_normlize()` function. This can cause rendering issues

Lukas Stockner commented

2023-11-23 22:51:34 +01:00

Nitpick: Typo in safe_normlize. Same in safe_normal.

Nitpick: Typo in `safe_normlize`. Same in `safe_normal`.

Lukas Stockner requested changes 2023-11-23 23:12:17 +01:00

intern/cycles/kernel/closure/bsdf_util.h Outdated

						
				@ -203,0 +208,4 @@

				ccl_device_inline float3 safe_normal(ccl_private ShaderData *sd, float3 N)

				{

				  N = safe_normalize(N);

				  if (len(N) < 0.5f) {

Lukas Stockner commented

2023-11-23 23:12:14 +01:00

Actually, we should use len_squared here - avoids a square root and does the job just as well.

Actually, we should use `len_squared` here - avoids a square root and does the job just as well.

Alaska added 3 commits 2023-11-23 23:48:12 +01:00

Merge branch 'main' into fix-valid-reflection-NaN f7f3aa319c

Merge branch 'main' into fix-valid-reflection-NaN 5e136e2a4e

Switch to len_squared and fix code comment f332ddeb37

Alaska requested review from Lukas Stockner 2023-11-23 23:50:24 +01:00

Lukas Stockner approved these changes 2023-11-24 00:30:03 +01:00

Lukas Stockner referenced this pull request

2023-11-24 00:34:25 +01:00

Fix #115206: Sheen renders incorrectly in certain situations #115286

Brecht Van Lommel requested changes 2023-11-27 19:17:24 +01:00

intern/cycles/kernel/closure/bsdf_util.h Outdated

						
				@ -200,6 +205,17 @@ ccl_device float3 maybe_ensure_valid_specular_reflection(ccl_private ShaderData

				  return ensure_valid_specular_reflection(sd->Ng, sd->wi, N);

				}

				ccl_device_inline float3 safe_normal(ccl_private ShaderData *sd, float3 N)

Brecht Van Lommel commented

2023-11-27 19:16:39 +01:00

It would be more efficient to have a function like this:

ccl_device_inline float3 safe_normalize_fallback(const float3 a, const float3 fallback)
{
  float t = len(a);
  return (t != 0.0f) ? a * (1.0f / t) : fallback;
}

That way we are not computing the length a second time.

It would be more efficient to have a function like this: ``` ccl_device_inline float3 safe_normalize_fallback(const float3 a, const float3 fallback) { float t = len(a); return (t != 0.0f) ? a * (1.0f / t) : fallback; } ``` That way we are not computing the length a second time.

Alaska added 3 commits 2023-11-28 03:54:24 +01:00

Merge branch 'main' into fix-valid-reflection-NaN 4c50e1c3f7

Use as suggested by Brecht 4a04f32c36

Make format 7472fd6c7c

Alaska added 1 commit 2023-11-28 10:34:48 +01:00

Switch to new system b89f000f1a

Alaska requested review from Brecht Van Lommel 2023-12-13 08:06:38 +01:00

Alaska added 1 commit 2023-12-13 08:06:50 +01:00

Merge branch 'main' into fix-valid-reflection-NaN

buildbot/vexp-code-patch-lint Build done.

Details

buildbot/vexp-code-patch-linux-x86_64 Build done.