Fix #120119: Area lamp artifacts in Cycles with light trees #120216

Sergey Sharybin · 2024-04-03T17:17:44+02:00

Sergey Sharybin commented

2024-04-03 17:17:44 +02:00

This is a regression in 4.1, caused by 36e603c430.

For unbiased MIS weight in light tree, we should use sd->N for
mis_origin_n, since sc->N is not available in NEE.

The change also makes it so we do not sample lights below sd->N even
when bump map correction is disabled. This diverges from the original
idea of giving full control to artists, but ensures the internal math
is happy.

This is a regression in 4.1, caused by 36e603c430. For unbiased MIS weight in light tree, we should use sd->N for mis_origin_n, since sc->N is not available in NEE. The change also makes it so we do not sample lights below sd->N even when bump map correction is disabled. This diverges from the original idea of giving full control to artists, but ensures the internal math is happy.

❤️ 3

Sergey Sharybin added 1 commit 2024-04-03 17:17:50 +02:00

Fix #120119 : Area lamp artifacts in Cycles with light trees 95457a4570

This is a regression in 4.1, caused by 36e603c430.

On a code side the issue was caused by a change in the mis_origin_n
which is a normal at a scatter point for light sampling.

Details about that specific change are a bit unclear. If not an
overseen mistake, it could have been part of idea that when no
bump map correction is used artists should have fuller control over
shading normal.

In any case, the change to this normal lead to some discrepancies
with shading/sampling/NEE code which uses sd->N and has no access
to closure.

In order to bring code back to consistent state this change reverts
the change to mis_origin_n. However it also does some extra things
to help with the original file used during development the bump map
correction option, as without doing anything else the file renders
with firefly-looking artifacts.

Those artifacts are due to normal going into the surface. While the
original thought was to let artists to have full control over the
normals and deal with such cases from shader, it seems that it
happens more often, could be hard to understand by artists what's
going on, and is also not compatible with features like light
trees.

Hence this change also does partial bump map correction when the
option is disabled: namely it forces evaluation to be 0 when normal
goes below the surface. This minimizes an impact on the files which
are currently relying on the dump map corrections being disabled.

Sergey Sharybin commented

2024-04-03 17:25:06 +02:00

A set of test files which I aim to add after landing this patch, to help catching similar issues in the future.

The light_tree_bump_map.blend is a smaller version of the file provided in the initial report #120119. its reference image is generated with the change in the code INTEGRATOR_STATE_WRITE(state, path, mis_origin_n) = sd->N; (which matches the state prior to the breaking change). This test file behaves exactly as it is expected with this PR applied.

The light_tree_bump_map_no_correction.blend is a smaller file derived from reef_shot3.blend which was used during the "Bump Map Correction" option project. The reference image for it is generated with the state of code prior to this PR. There are some changes in shading, especially when zoomed in so closely. Technically this test will fail with this PR applied, but I'd consider this acceptable trade-off (at least for now, without going too much deep into the normals story).

P.S. Surely when the tests will be added a new reference image will be generated. It is mainly to prepare files, and share them for the reviewers.

A set of test files which I aim to add after landing this patch, to help catching similar issues in the future. The `light_tree_bump_map.blend` is a smaller version of the file provided in the initial report #120119. its reference image is generated with the change in the code `INTEGRATOR_STATE_WRITE(state, path, mis_origin_n) = sd->N;` (which matches the state prior to the breaking change). This test file behaves exactly as it is expected with this PR applied. The `light_tree_bump_map_no_correction.blend` is a smaller file derived from `reef_shot3.blend` which was used during the "Bump Map Correction" option project. The reference image for it is generated with the state of code prior to this PR. There are some changes in shading, especially when zoomed in so closely. Technically this test will fail with this PR applied, but I'd consider this acceptable trade-off (at least for now, without going too much deep into the normals story). P.S. Surely when the tests will be added a new reference image will be generated. It is mainly to prepare files, and share them for the reviewers.

light_tree_bump_map_no_correction.blend

1.1 MiB

light_tree_bump_map_no_correction.png

22 KiB

light_tree_bump_map.blend

958 KiB

light_tree_bump_map.png

16 KiB

Sergey Sharybin commented

2024-04-03 17:28:01 +02:00

And one more thing I wanted to share is the demonstration of this change on the bigger picture of the original file reef_shot3.blend.

Bump Map Corrections Enabled	Before this PR	After this PR

0-enabled.png

1.2 MiB

2-after.png

1.2 MiB

1-before.png

1.2 MiB

Sergey Sharybin requested review from Weizhen Huang 2024-04-03 17:28:12 +02:00

Sergey Sharybin requested review from Lukas Stockner 2024-04-03 17:28:17 +02:00

Sergey Sharybin commented

2024-04-03 17:50:30 +02:00

I forgot to mention. In terms of performance impact, there is no change on M2, both CPU and GPU.
It is what one would intuitively expect, since it's just moving some lines around, but is always good to verify such things, especially on GPU.

I forgot to mention. In terms of performance impact, there is no change on M2, both CPU and GPU. It is what one would intuitively expect, since it's just moving some lines around, but is always good to verify such things, especially on GPU.

Weizhen Huang reviewed 2024-04-04 15:04:12 +02:00

Weizhen Huang left a comment

I believe changing sc->N to sd->N is the correct fix, as this normal must be consistent for both forward tracing and NEE, and in NEE sc->N is not available.

As for the bump_shadowing_term() fix, I’m not very sure. Even without light tree, the probability of picking a light with NEE is zero when the normal is below the surface due to the shadow ray intersection check; allowing the light to be picked in forward path tracing only adds to bias, it’s just more obvious with the MIS weight of light tree, therefore the firefly. This is also the case for specular surfaces, therefore, while this patch fixes the firefly for diffuse surfaces with bump mapping, I wonder would it be better to check if (dot(sd->N, *wo) < 0) in more general cases. Attached is a scene that also needs if (dot(sd->N, *wo) < 0) check, otherwise there are fireflies.

But probably it is better to add incremental changes like this. We should gather a bunch of test files like these to test the shadow terminator and revisit the topic at some point. For example is this shift_cos_in() function still needed, why we use ensure_valid_specular_reflection() for glossy surfaces and bump_shadowing_term() for diffuse surfaces, etc.

I expect the final commit message to be shorter than the current description for clarity. Mainly two points need to be stated:

For unbiased MIS weight in light tree, sd->N need to be consistently used in forward path tracing and NEE, since sc->N is not available in NEE.
the probability of sampling a light with NEE is zero when it’s below sd->N, therefore we also need to prevent such lights from being sampled in forward path tracing.

The current description can stay in the PR.

I believe changing `sc->N` to `sd->N` is the correct fix, as this normal must be consistent for both forward tracing and NEE, and in NEE `sc->N` is not available. As for the `bump_shadowing_term()` fix, I’m not very sure. Even without light tree, the probability of picking a light with NEE is zero when the normal is below the surface due to the shadow ray intersection check; allowing the light to be picked in forward path tracing only adds to bias, it’s just more obvious with the MIS weight of light tree, therefore the firefly. This is also the case for specular surfaces, therefore, while this patch fixes the firefly for diffuse surfaces with bump mapping, I wonder would it be better to check `if (dot(sd->N, *wo) < 0)` in more general cases. Attached is a scene that also needs `if (dot(sd->N, *wo) < 0)` check, otherwise there are fireflies. But probably it is better to add incremental changes like this. We should gather a bunch of test files like these to test the shadow terminator and revisit the topic at some point. For example is this `shift_cos_in()` function still needed, why we use `ensure_valid_specular_reflection()` for glossy surfaces and `bump_shadowing_term()` for diffuse surfaces, etc. I expect the final commit message to be shorter than the current description for clarity. Mainly two points need to be stated: 1. For unbiased MIS weight in light tree, `sd->N` need to be consistently used in forward path tracing and NEE, since `sc->N` is not available in NEE. 2. the probability of sampling a light with NEE is zero when it’s below `sd->N`, therefore we also need to prevent such lights from being sampled in forward path tracing. The current description can stay in the PR.

rough_glossy.blend

986 KiB

intern/cycles/kernel/closure/bsdf.h

						
				@ -81,0 +84,4 @@

				  /* When bump map correction is not used do not do smoothing, let the normal to be the one which

				   * artists desires it to be via shader configuration.

				   * Effectively, when bump map correction is not used only scale shading to 0 when the normal

				   * points inside of the mesh, which is not well supported by functionality like light trees. */

Weizhen Huang commented

2024-04-04 14:58:00 +02:00

First, the g < 0 condition is not checking whether a shading normal points inside the mesh, but whether the incoming light is below the geometry normal.
Second, to say it is not well supported by light tree is vague and inaccurate, better to say it is not supported by NEE.

First, the `g < 0` condition is not checking whether a shading normal points inside the mesh, but whether the incoming light is below the geometry normal. Second, to say it is not well supported by light tree is vague and inaccurate, better to say it is not supported by NEE.

weizhen marked this conversation as resolved

Sergey Sharybin added 1 commit 2024-04-04 16:54:29 +02:00

Simplify wording 1f77300113

Less words more gooder eh.

Weizhen Huang approved these changes 2024-04-04 16:59:04 +02:00

Lukas Stockner approved these changes 2024-04-05 02:01:41 +02:00

Lukas Stockner left a comment

Yes, I also agree that going with sd->N is correct here. It's unfortunate, but well, non-physical tricks only get you so far. Arguably we have this issue with some other features as well (such as the classic "object is visible to diffuse/glossy rays, but not shadow rays" topic), but normal maps in particular feel like something so basic that they need to just work without any MIS-relayed traps.

As for the dot(sd->N, *wo) topic: Yes, thanks for bringing that up. This is something I keep noticing when working on closures - it doesn't really seem consistent whether we check against sc->N, sd->N and/or sd->Ng, sometimes even between sample and eval functions in the same closure if I remember correctly. We should probably document this somewhere and/or enforce it centrally outside of the closure-specific functions. Personally, I'd prefer to be conservative and only accept rays that are in the correct hemisphere w.r.t. all normals, but that might cause visible artifacts.

For now, I think this fix should be okay, the finer details can be discussed later.

Yes, I also agree that going with `sd->N` is correct here. It's unfortunate, but well, non-physical tricks only get you so far. Arguably we have this issue with some other features as well (such as the classic "object is visible to diffuse/glossy rays, but not shadow rays" topic), but normal maps in particular feel like something so basic that they need to just work without any MIS-relayed traps. As for the `dot(sd->N, *wo)` topic: Yes, thanks for bringing that up. This is something I keep noticing when working on closures - it doesn't really seem consistent whether we check against `sc->N`, `sd->N` and/or `sd->Ng`, sometimes even between sample and eval functions in the same closure if I remember correctly. We should probably document this somewhere and/or enforce it centrally outside of the closure-specific functions. Personally, I'd prefer to be conservative and only accept rays that are in the correct hemisphere w.r.t. all normals, but that might cause visible artifacts. For now, I think this fix should be okay, the finer details can be discussed later.

Sergey Sharybin merged commit f6bff96ac5 into main

2024-04-05 10:11:46 +02:00

Sergey Sharybin deleted branch fix_120119

2024-04-05 10:11:48 +02:00

Sergey Sharybin referenced this issue from a commit

2024-04-05 10:11:50 +02:00

Fix #120119: Area lamp artifacts in Cycles with light trees

Thomas Dinges referenced this issue from a commit

2024-04-09 16:11:47 +02:00

Fix #120119: Area lamp artifacts in Cycles with light trees

Brecht Van Lommel referenced this issue from a commit

2024-06-26 21:15:00 +02:00