Fix #125595: Cycles artifacts in overlapping volumes with different phase functions #125676

Weizhen Huang · 2024-07-30T18:28:39+02:00

Weizhen Huang commented

2024-07-30 18:28:39 +02:00

A phase function is normalized over the sphere, it is therefore
incorrect to sum two phase functions together when evaluating for NEE.
It should be a weighted sum with normalized weights, which, according to
volume_shader_phase_pick(), is sample_weight / sum_sample_weight.

Also corrects an error in volume_shader_phase_pick().

A phase function is normalized over the sphere, it is therefore incorrect to sum two phase functions together when evaluating for NEE. It should be a weighted sum with normalized weights, which, according to `volume_shader_phase_pick()`, is `sample_weight / sum_sample_weight`. Also corrects an error in `volume_shader_phase_pick()`.

Weizhen Huang added this to the Render & Cycles project 2024-07-30 18:28:54 +02:00

Weizhen Huang commented

2024-07-30 18:38:22 +02:00

Each location in an overlapping region is composed of x_1\% of volume 1 and x_2\% of volume 2, so the phase function at that point should be x_1\%p_1+x_2\%p_2 instead of p_1 + p_2.

The only thing I am not sure is if x\% corresponds to sample_weight / sum_sample_weight, because sample weight includes the color besides density. The proposed implementation at least makes NEE and forward path consistent, I believe we can check the "ground truth" behaviour another time, since we have plans to evaluate overlapping volumes separately anyway.

Each location in an overlapping region is composed of \(x_1\%\) of volume 1 and \(x_2\%\) of volume 2, so the phase function at that point should be \(x_1\%p_1+x_2\%p_2\) instead of \(p_1 + p_2\). The only thing I am not sure is if \(x\%\) corresponds to `sample_weight / sum_sample_weight`, because sample weight includes the color besides density. The proposed implementation at least makes NEE and forward path consistent, I believe we can check the "ground truth" behaviour another time, since we have plans to evaluate overlapping volumes separately anyway.

Weizhen Huang commented

2024-07-30 18:38:48 +02:00

The second commit is a refactor and can be reviewly separately. I will not squash the commits.

Weizhen Huang requested review from Lukas Stockner 2024-07-30 18:41:35 +02:00

Weizhen Huang requested review from Sergey Sharybin 2024-07-30 18:41:41 +02:00

Sergey Sharybin commented

2024-08-01 12:29:30 +02:00

@blender-bot build

Sergey Sharybin approved these changes 2024-08-01 12:32:02 +02:00

Sergey Sharybin left a comment

It seems fine.
I don't have insights on the x\% at this time, but as long as NEE and forward paths are consistent, we should be good for now.

For the bug fixes we put the Fix #<number> in the commit title, making it more clear when looking at one-liner git logs. See https://developer.blender.org/docs/handbook/guidelines/commit_messages/#bug-fixes

It seems fine. I don't have insights on the \(x\%\) at this time, but as long as NEE and forward paths are consistent, we should be good for now. For the bug fixes we put the `Fix #<number>` in the commit title, making it more clear when looking at one-liner git logs. See https://developer.blender.org/docs/handbook/guidelines/commit_messages/#bug-fixes

Weizhen Huang commented

2024-08-01 12:34:42 +02:00

For the bug fixes we put the Fix # in the commit title

I usually add # in the commit title, but this commit title is too long for that and I could not think of a good shorter title.

> For the bug fixes we put the Fix #<number> in the commit title I usually add #<number> in the commit title, but this commit title is too long for that and I could not think of a good shorter title.

Weizhen Huang changed title from ~~Fix: Cycles rendering overlapping volumes with different phase functions incorrectly~~ to Fix #125595: Cycles artifacts in overlapping volumes with different phase functions

2024-08-01 12:44:34 +02:00

Weizhen Huang force-pushed fix-overlapping-volume-with-different-phases from 59b508985f to cd81691098

2024-08-01 17:55:17 +02:00

Compare

~~Weizhen Huang referenced this pull request 2024-08-02 12:26:06 +02:00~~

Cycles: Improve the coverage of automated regression tests #123012

Lukas Stockner reviewed 2024-08-02 19:06:11 +02:00

intern/cycles/kernel/integrator/volume_shader.h

						
				@ -177,3 +179,1 @@

				      ccl_private const ShaderVolumeClosure *svc = &phases->closure[sampled];

				      sum += svc->sample_weight;

				    }

				    for (int i = 1; i < phases->num_closure; i++) {

Lukas Stockner commented

2024-08-02 19:05:54 +02:00

Is there a reason we do sampling here like this instead of the usual

total_pdf = 0
for each option:
  total_pdf += option.pdf
sum = 0
for each option:
  sum += option.pdf
  if sum >= rand:
    pick option
    pick_pdf = option.pdf / total_pdf
    rand = (rand - sum - option.pdf) / option.pdf

I feel like repeatedly rescaling the random number might lead to numerical issues, but I'm not sure so we could also just keep it for now.

Is there a reason we do sampling here like this instead of the usual ``` total_pdf = 0 for each option: total_pdf += option.pdf sum = 0 for each option: sum += option.pdf if sum >= rand: pick option pick_pdf = option.pdf / total_pdf rand = (rand - sum - option.pdf) / option.pdf ``` I feel like repeatedly rescaling the random number might lead to numerical issues, but I'm not sure so we could also just keep it for now.

Weizhen Huang commented

2024-08-02 19:45:47 +02:00

The original code is doing the stuff as the code block you pasted, my first commit didn't change that, if you think that's fine I'll push that first.

Although I didn't measure the performance, reservoir sampling is theoretically faster, and it only rescales for at most the amount of intersection - 1. I don't think people use lots of intersections with different phases, otherwise we would have long ago bug reports about this. Besides, light tree doesn't have problem with rescaling the random number yet.

The original code is doing the stuff as the code block you pasted, my first commit didn't change that, if you think that's fine I'll push that first. Although I didn't measure the performance, reservoir sampling is theoretically faster, and it only rescales for at most the amount of intersection - 1. I don't think people use lots of intersections with different phases, otherwise we would have long ago bug reports about this. Besides, light tree doesn't have problem with rescaling the random number yet.

Lukas Stockner commented

2024-08-04 22:19:19 +02:00

Fair enough, we can keep it for now. I had just never seen this style before.

LukasStockner marked this conversation as resolved

Lukas Stockner approved these changes 2024-08-04 22:19:39 +02:00

Weizhen Huang merged commit e981389bdd into main

2024-08-05 10:56:07 +02:00

Weizhen Huang deleted branch fix-overlapping-volume-with-different-phases

2024-08-05 10:56:11 +02:00

Weizhen Huang referenced this issue from a commit

2024-08-05 10:56:14 +02:00

Refactor: Cycles: use reservoir sampling to pick phase function in volume