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

Open
Sebastian Herholz wants to merge 6 commits from sherholz/blender:light-visibility-fix into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
5 changed files with 88 additions and 22 deletions
Showing only changes of commit c7bbd800c1 - Show all commits

View File

@ -430,7 +430,8 @@ ccl_device_forceinline int integrate_surface_bsdf_bssrdf_bounce(
/* Update throughput. */
const Spectrum bsdf_weight = bsdf_eval_sum(&bsdf_eval) / bsdf_pdf;
INTEGRATOR_STATE_WRITE(state, path, throughput) *= bsdf_weight;
INTEGRATOR_STATE_WRITE(state, path, scatter_eval) = bsdf_eval;
const BsdfEvalRGBE bsdf_eval_RGBE = BsdfEvalToBsdfEvalRGBE(bsdf_eval);
INTEGRATOR_STATE_WRITE(state, path, scatter_eval) = bsdf_eval_RGBE;
if (kernel_data.kernel_features & KERNEL_FEATURE_LIGHT_PASSES) {
if (INTEGRATOR_STATE(state, path, bounce) == 0) {

View File

@ -48,7 +48,7 @@ KERNEL_STRUCT_MEMBER(path, float, min_ray_pdf, KERNEL_FEATURE_PATH_TRACING)
KERNEL_STRUCT_MEMBER(path, float, continuation_probability, KERNEL_FEATURE_PATH_TRACING)
/* Throughput. */
KERNEL_STRUCT_MEMBER(path, PackedSpectrum, throughput, KERNEL_FEATURE_PATH_TRACING)
KERNEL_STRUCT_MEMBER(path, BsdfEval, scatter_eval, KERNEL_FEATURE_PATH_TRACING)
KERNEL_STRUCT_MEMBER(path, BsdfEvalRGBE, scatter_eval, KERNEL_FEATURE_PATH_TRACING)
/* Factor to multiple with throughput to get remove any guiding PDFS.
* Such throughput without guiding PDFS is used for Russian roulette termination. */
KERNEL_STRUCT_MEMBER(path, float, unguided_throughput, KERNEL_FEATURE_PATH_GUIDING)

View File

@ -5,30 +5,87 @@
CCL_NAMESPACE_BEGIN
ccl_device_inline void light_visibility_correction(KernelGlobals kg,
IntegratorState state,
const int shader,
Spectrum* light_visibility)
// float EGB to RGBE conversion as described by Greg Ward
// http://www.graphics.cornell.edu/online/formats/rgbe/
ccl_device_inline SpectrumRGBE SpectrumToSpectrumRGBE(const Spectrum &spec)
{
Spectrum visible_components = zero_spectrum();
const BsdfEval scatter_eval = INTEGRATOR_STATE(state, path, scatter_eval);
Spectrum transmission = scatter_eval.sum - (scatter_eval.glossy + scatter_eval.diffuse);
if (!(shader & SHADER_EXCLUDE_DIFFUSE))
{
visible_components += scatter_eval.diffuse;
}
SpectrumRGBE specRGBE;
char *specRGBEPtr = (char *)&specRGBE;
Review

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.

sure no problem.

sure no problem.

@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.

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?
Review

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.
if (!(shader & SHADER_EXCLUDE_GLOSSY))
{
visible_components += scatter_eval.glossy;
}
float v;
int e;
v = max(spec[0], max(spec[1], spec[2]));
if (v < 1e-32f) {
specRGBEPtr[0] = specRGBEPtr[1] = specRGBEPtr[2] = specRGBEPtr[3] = 0;
}
else {
v = frexp(v, &e) * 256.0f / v;
specRGBEPtr[0] = (unsigned char)(spec[0] * v);
specRGBEPtr[1] = (unsigned char)(spec[1] * v);
specRGBEPtr[2] = (unsigned char)(spec[2] * v);
specRGBEPtr[3] = (unsigned char)(e + 128);
}
return specRGBE;
}
if (!(shader & SHADER_EXCLUDE_TRANSMIT))
{
visible_components += transmission;
}
ccl_device_inline Spectrum SpectrumRGBEToSpectrum(const SpectrumRGBE &specRGBE)
{
Spectrum spec;
SpectrumRGBE specRGBE_ = specRGBE;
char *specRGBEPtr = (char *)&specRGBE_;
if (specRGBEPtr[3]) {
const float f = ldexp(1.0f, specRGBEPtr[3] - (int)(128 + 8));
spec[0] = specRGBEPtr[0] * f;
spec[1] = specRGBEPtr[1] * f;
spec[2] = specRGBEPtr[2] * f;
}
else {
spec[0] = spec[1] = spec[2] = 0.0f;
}
return spec;
}
*light_visibility = safe_divide(visible_components , scatter_eval.sum);
ccl_device_inline BsdfEvalRGBE BsdfEvalToBsdfEvalRGBE(const BsdfEval &bsdfEval)
{
BsdfEvalRGBE bsdfEvalRGBE;
bsdfEvalRGBE.diffuse = SpectrumToSpectrumRGBE(bsdfEval.diffuse);
bsdfEvalRGBE.glossy = SpectrumToSpectrumRGBE(bsdfEval.glossy);
bsdfEvalRGBE.sum = SpectrumToSpectrumRGBE(bsdfEval.sum);
return bsdfEvalRGBE;
}
ccl_device_inline BsdfEval BsdfEvalRGBEToBsdfEval(const BsdfEvalRGBE &bsdfEvalRGBE)
{
BsdfEval bsdfEval;
bsdfEval.diffuse = SpectrumRGBEToSpectrum(bsdfEvalRGBE.diffuse);
bsdfEval.glossy = SpectrumRGBEToSpectrum(bsdfEvalRGBE.glossy);
bsdfEval.sum = SpectrumRGBEToSpectrum(bsdfEvalRGBE.sum);
return bsdfEval;
}
ccl_device_inline void light_visibility_correction(KernelGlobals kg,
IntegratorState state,
const int shader,
Spectrum *light_visibility)
{
Spectrum visible_components = zero_spectrum();
const BsdfEval scatter_eval = BsdfEvalRGBEToBsdfEval(
INTEGRATOR_STATE(state, path, scatter_eval));
Spectrum transmission = scatter_eval.sum - (scatter_eval.glossy + scatter_eval.diffuse);
if (!(shader & SHADER_EXCLUDE_DIFFUSE)) {
visible_components += scatter_eval.diffuse;
}
if (!(shader & SHADER_EXCLUDE_GLOSSY)) {
visible_components += scatter_eval.glossy;
}
if (!(shader & SHADER_EXCLUDE_TRANSMIT)) {
visible_components += transmission;
}
*light_visibility = safe_divide(visible_components, scatter_eval.sum);
}
CCL_NAMESPACE_END

View File

@ -443,6 +443,12 @@ typedef struct BsdfEval {
Spectrum sum;
} BsdfEval;
typedef struct BsdfEvalRGBE {
SpectrumRGBE diffuse;
SpectrumRGBE glossy;
SpectrumRGBE sum;
} BsdfEvalRGBE;
/* Closure Filter */
typedef enum FilterClosures {

View File

@ -13,9 +13,11 @@ CCL_NAMESPACE_BEGIN
#define SPECTRUM_CHANNELS 3
#define SPECTRUM_DATA_TYPE float3
#define PACKED_SPECTRUM_DATA_TYPE packed_float3
#define SPECTRUM_RGBE_DATATYPE int32_t
using Spectrum = SPECTRUM_DATA_TYPE;
using PackedSpectrum = PACKED_SPECTRUM_DATA_TYPE;
using SpectrumRGBE = SPECTRUM_RGBE_DATATYPE;
#define make_spectrum(f) CONCAT(make_, SPECTRUM_DATA_TYPE(f))
#define load_spectrum(f) CONCAT(load_, SPECTRUM_DATA_TYPE(f))