jalberse / shimmer

Physically based rendering in Rust
Apache License 2.0
33 stars 0 forks source link

Fix use of named spectra for dielectric eta #70

Closed jalberse closed 6 months ago

jalberse commented 6 months ago

Rendering frame542, which uses a glass-bk7 spectrum for its eta, results in an extremely noisy surface. Rendering with the default eta (a const value of 1.5) results in proper-looking glass.

The issue could be in get_one_spectrum() called from DielectricMaterial::create(), which should filter down to PiecewiseLinearSpectrum::from_interleaved(). But we use these fine for e.g. the creation of copper conductors, so I'm wary of those being the problem. So perhaps the issue is in handling eta in DielectricMaterial::get_bxdf().

jalberse commented 6 months ago

frame542-b

jalberse commented 6 months ago

Attached a problematic render. The object should appear to be glass, which is DOES for default dielectric materials with constant eta.

Actually, the issue is likely in the handling of dispersion after terminate_secondary() is called. We might not respect the zeroed PDFs of the terminated secondary wavelengths somewhere?

jalberse commented 6 months ago

Investigating this led me to realize we didn't properly regularize in RgbFilm::add_sample() (we didn't check if G was the largest of the triplet, we checked b twice on accident).

That's unrelated to this, though.

Secondary wavelengths DO seem to get terminated properly. We don't do something dumb like continually modify the PDF of already-terminated-sampled-wavelengths (we do something else dumb).

Are we getting reasonable eta values?

jalberse commented 6 months ago

The sampled eta values are reasonable (near 1.5, as expected, matching GLASS_BK7_ETA_SAMPLES).

Further, the mf_distribution is effectively smooth, so DielectricBxDF::sample_f() is using the perfect specular dielectric BSDF model - the same as if the constant eta is used.

So I don't think the bug is within sample_f() or related to handling eta, but in the handling of the terminated wavelengths. This is the only difference I can see between our constant eta and our BK7 eta.

We do properly randomly select lambda[0] across the visible spectrum - I don't think there's bias there, which was my initial thought.

jalberse commented 6 months ago

Adding RGB samples from samples with terminated wavelengths looks to be correct.

It could be NaN poisoning. At a high depth (148), we're seeing eta_scale == +inf, so we're panicking at the calculation of rr_beta.

The bsdf sample eta is reasonable - 1.537. Are we accumulating to very large values for eta_scale at large depths, for many transmissive bounces?

jalberse commented 6 months ago

It doesn't always occur at high depth - eta_scale is a reasonable value (2.355 etc) for even very high depths for many paths.

edit (actually, large eta_scale is fine)

We do see cases where eta_scale accumulates to very large values. That seems correct, though we will want to handle the case where eta_scale accumulates to inf to avoid rr_beta becoming NaN.

Those only effect Russian roulette termination, however, and would (I think?) result in NOT terminating the path, which wouldn't result in NaN poisoining of the RGB values, I think.

jalberse commented 6 months ago

Added a check for INF eta_sample (and skipping russian roulette in that case, which makes sense for large rr_beta anyways).

This doesn't fix the resulting image as I expected.

jalberse commented 6 months ago

I believe the bug IS related to transmission (after all, that's what a different eta would cause...)

The specular reflections appear correct, visually.

I should make a scene that is just a sphere with BK-7 material. I'm curious if this is happening due a high depth and many transmissions, or if this is happening for all transmissions.

jalberse commented 6 months ago

Sow with a single sphere, things look correct.

Early termination of paths could lead to noise when transmission is involved. Maybe I am terminating too early?

We could make a test scene with many layered spheres.

I can also try disabling RR

jalberse commented 6 months ago

Early termination could also explain why I seem to be rendering some scenes faster than PBRT

jalberse commented 6 months ago

Well, disabling RR termination didn't fix the issue. There is some other issue with transmission

jalberse commented 6 months ago

Well, the issue does appear to be related to terminating secondary wavelengths - disabling that logic results in a more clear image (and the default constant spectra doesn't have the issue as well).

It does make sense - we effectively have fewer sampled wavelengths, so we would expect more noise.

Does the issue go away if I simply crank the spp extremely high? Am I incorrectly weighting samples? Am I taking less samples than I thought I was?

jalberse commented 6 months ago

shimmer-frame542-1000spp

It looks like increasing spp DID fix the issue - this is comparable to PBRT, and we get similar noise at 255spp as well (though there's 4 dark areas in our render, but that is an unrelated issue).

The RR fix might have actually worked, and I just didn't test with high enough SPP? I'll need to verify what happened.

jalberse commented 6 months ago

Related - this scene is boring without a skybox. If we want to show off dielectrics we should consider supporting skyboxes in infinite lights.