Fix #103977: Cycles handling ray visibility inconsistent for forward and nee #107747

Sebastian Herholz · 2023-05-08T17:53:23+02:00

Sebastian Herholz commented

2023-05-08 17:53:23 +02:00

Reopening the PR to fix the ray visibility inconsistency on light sources (#103977)

Brecht Van Lommel was assigned by Sebastian Herholz

2023-05-08 17:53:23 +02:00

Sergey Sharybin was assigned by Sebastian Herholz

2023-05-08 17:53:23 +02:00

Sebastian Herholz added 1 commit 2023-05-08 17:53:33 +02:00

edbc5a941b Fix T103977: Cycles handling ray visibility inconsistent for forward and nee

Sebastian Herholz commented

2023-05-08 17:55:18 +02:00

@brecht to reduce the additional per state memory consumption I was thinking of using RGBE compressed values.
http://www.graphics.cornell.edu/online/formats/rgbe/

What do you think?

@brecht to reduce the additional per state memory consumption I was thinking of using RGBE compressed values. http://www.graphics.cornell.edu/online/formats/rgbe/ What do you think?

Sebastian Herholz added 1 commit 2023-05-08 21:26:23 +02:00

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

c7bbd800c1 Cycles: Adding SpectrumRGBE type to store a compact form of BSDFEval

Sebastian Herholz commented

2023-05-08 21:28:23 +02:00

Added SpectrumRGBE support, that stores a float3 into an int32 using 8bit RGB and a shared exponent.
No visual difference between using float3.

Added SpectrumRGBE support, that stores a float3 into an int32 using 8bit RGB and a shared exponent. No visual difference between using float3.

Sergey Sharybin changed title from ~~Fix T103977: Cycles handling ray visibility inconsistent for forward and nee~~ to Fix #103977: Cycles handling ray visibility inconsistent for forward and nee

2023-05-09 09:01:07 +02:00

Sergey Sharybin commented

2023-05-09 09:05:09 +02:00

@blender-bot package

Blender Bot commented

2023-05-09 09:05:12 +02:00

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR107747) when ready.

Sebastian Herholz added 1 commit 2023-05-09 09:26:39 +02:00

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

6696aa979b Cycles: SpectrumRGBE: oneAPI float3 fix

Sergey Sharybin requested changes 2023-05-09 09:30:47 +02:00

Sergey Sharybin left a comment

Did not deeply check on the algorithm/math yet, was trying to get the patch to testable state. There is some technical feedback so far.

Would need to have a closer look to see if we can probably lower the memory impact further.

Please also check the output of the buildbot: https://builder.blender.org/admin/#/builders/136/builds/1170

Did not deeply check on the algorithm/math yet, was trying to get the patch to testable state. There is some technical feedback so far. Would need to have a closer look to see if we can probably lower the memory impact further. Please also check the output of the buildbot: https://builder.blender.org/admin/#/builders/136/builds/1170

intern/cycles/kernel/light/visibility.h

						
				@ -0,0 +11,4 @@

				ccl_device_inline SpectrumRGBE SpectrumToSpectrumRGBE(const Spectrum &spec)

				{

				  SpectrumRGBE specRGBE;

				  char *specRGBEPtr = (char *)&specRGBE;

Sergey Sharybin commented

2023-05-09 09:14:37 +02:00

Can we define SpectrumRGBE as

struct SpectrumRGBE {
  uint8_t r, g, b, e;
};

and avoid such memory casts, and casts between signed/unsigned char?

This also seems to a bit arbitrary place to have such utility functions. Within the current file structure it seems types_spectrum.h is the most fitted place for it.

Can we define `SpectrumRGBE ` as ``` struct SpectrumRGBE { uint8_t r, g, b, e; }; ``` and avoid such memory casts, and casts between signed/unsigned `char`? This also seems to a bit arbitrary place to have such utility functions. Within the current file structure it seems `types_spectrum.h` is the most fitted place for it.

Sebastian Herholz commented

2023-05-09 09:49:14 +02:00

sure no problem.

Sebastian Herholz commented

2023-05-09 10:05:31 +02:00

@Sergey done.
I am not sure if you can compress the data even more.
The positive side on this approach is that we can realize handling scalar ray types light interactions straight forward instead of only binary ones.

@Sergey done. I am not sure if you can compress the data even more. The positive side on this approach is that we can realize handling scalar ray types light interactions straight forward instead of only binary ones.

Sebastian Herholz commented

2023-05-09 10:13:28 +02:00

Another thing worth considering is moving the conversion methods to a different place (not into visibility.h).
Do you have a good suggestion for it?

Another thing worth considering is moving the conversion methods to a different place (not into `visibility.h`). Do you have a good suggestion for it?

Sergey Sharybin commented

2023-09-11 16:11:57 +02:00

The immediate thought is kernel/util/color.h. There is already spectrum_to_rgb so intuitively feels like spectrum_to_spectrum_rgbe could fit there as well.

The immediate thought is `kernel/util/color.h`. There is already `spectrum_to_rgb` so intuitively feels like `spectrum_to_spectrum_rgbe` could fit there as well.

Sebastian Herholz commented

2023-05-09 09:47:40 +02:00

@blender-bot package

Blender Bot commented

2023-05-09 09:47:43 +02:00

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR107747) when ready.

Sebastian Herholz added 1 commit 2023-05-09 10:03:56 +02:00

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

7a4f223c47 Cycles: SpectrumRGBE moved from char* to uint8_t

Sebastian Herholz commented

2023-05-09 10:07:07 +02:00

@blender-bot package

Blender Bot commented

2023-05-09 10:07:10 +02:00

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR107747) when ready.

Sebastian Herholz added 1 commit 2023-05-09 13:54:36 +02:00

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

c0edb57875 Cycles: SpectrumRGBE potential Metal fix

Sebastian Herholz commented

2023-05-09 13:55:18 +02:00

@blender-bot package

Blender Bot commented

2023-05-09 13:55:21 +02:00

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR107747) when ready.

Sebastian Herholz added 1 commit 2023-05-09 14:33:10 +02:00

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

f1bce9b0a9 Cycles: SpectrumRGBE potential Metal fix part 2

Sebastian Herholz commented

2023-05-09 14:33:23 +02:00

@blender-bot package

Blender Bot commented

2023-05-09 14:33:26 +02:00

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR107747) when ready.

Brecht Van Lommel commented

2023-05-09 15:50:36 +02:00

If this can solve the problem without serious downsides it will be great, but I think this needs to be checked more closely.

What is the performance impact of this on CPU and GPU? I would not be surprised if there is a measurable performance impact, but maybe it's not so bad.
8 bits is not a lot, I expect this will lead to noticeable banding and precision loss in some cases. Especially when combined with compositing. For example one a large plane, if one color channel is fixed, and another has a gradient from 0 to 1, then imagine that will show up as banding in the render buffer?

If this can solve the problem without serious downsides it will be great, but I think this needs to be checked more closely. * What is the performance impact of this on CPU and GPU? I would not be surprised if there is a measurable performance impact, but maybe it's not so bad. * 8 bits is not a lot, I expect this will lead to noticeable banding and precision loss in some cases. Especially when combined with compositing. For example one a large plane, if one color channel is fixed, and another has a gradient from 0 to 1, then imagine that will show up as banding in the render buffer?

Sebastian-Herholz commented

2023-05-09 19:40:50 +02:00

@brecht I will do some deeper evaluation and report the results later.
On my first tests on my RTX2070 I did not measure any performance difference.

I think if there is one, it will only happen on the GPU, and then it will be mainly caused by the bigger state.
Additional computing is only performed when I light source with visibility flags it hit, and in all the standard scenes, this feature is not used so often (e.g., barbershop scene (2 light sources) and Dark Interior (also 2 light sources)).

About RGBE, I am quite surprised that it works so well. In my test scene, I couldn't even see a difference in the EXR images when I made a diff (float3 vs RGBE) and pulled up the exposure. I will post the images later.
I think one reason why the compression works well, in this case, is that we are not working with really high dynamic range values, mainly BSDF_evals/pdfs.
These values only get large if the BSDF has a large value and the PDF is suuuper low.

I will try to set up a scene like the one you described.

@brecht I will do some deeper evaluation and report the results later. On my first tests on my RTX2070 I did not measure any performance difference. I think if there is one, it will only happen on the GPU, and then it will be mainly caused by the bigger state. Additional computing is only performed when I light source with visibility flags it hit, and in all the standard scenes, this feature is not used so often (e.g., barbershop scene (2 light sources) and Dark Interior (also 2 light sources)). About RGBE, I am quite surprised that it works so well. In my test scene, I couldn't even see a difference in the EXR images when I made a diff (float3 vs RGBE) and pulled up the exposure. I will post the images later. I think one reason why the compression works well, in this case, is that we are not working with really high dynamic range values, mainly BSDF_evals/pdfs. These values only get large if the BSDF has a large value and the PDF is suuuper low. I will try to set up a scene like the one you described.

Weizhen Huang reviewed 2023-07-17 09:10:40 +02:00

intern/cycles/kernel/integrator/surface_shader.h

						
				@ -246,3 +246,3 @@

				        if (bsdf_pdf != 0.0f) {

				          bsdf_eval_accum(result_eval, sc->type, eval * sc->weight);

				          if (!_surface_shader_exclude(sc->type, light_shader_flags)) {

Weizhen Huang commented

2023-07-17 09:10:40 +02:00

I wonder, shouldn't the function surface_shader_bsdf_eval_pdfs() just below it follow the same logic and be corrected too?

I wonder, shouldn't the function `surface_shader_bsdf_eval_pdfs()` just below it follow the same logic and be corrected too?

Sergey Sharybin commented

2023-09-11 16:13:26 +02:00

@Sebastian-Herholz Any progress with the benchmarking and testing? Apart from performance there are two extra topics which I am wondering here:

The state size increase on high-end cards
The "future-proofness" of the solution: is it how we'd want to solve the problem when Cycles goes spectral?

@Sebastian-Herholz Any progress with the benchmarking and testing? Apart from performance there are two extra topics which I am wondering here: * The state size increase on high-end cards * The "future-proofness" of the solution: is it how we'd want to solve the problem when Cycles goes spectral?

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

Details

This pull request has changes conflicting with the target branch.

intern/cycles/kernel/integrator/shade_background.h
intern/cycles/kernel/integrator/shade_light.h
intern/cycles/kernel/integrator/shade_surface.h
intern/cycles/kernel/integrator/surface_shader.h

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u light-visibility-fix:sherholz-light-visibility-fix

git checkout sherholz-light-visibility-fix

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 #103977: Cycles handling ray visibility inconsistent for forward and nee #107747

Checkout