Cycles X: tweak adaptive sampling threshold
ClosedPublic
Actions

Authored by Brecht Van Lommel (brecht) on Sep 3 2021, 8:08 PM.

Details

Reviewers

Sergey Sharybin (sergey)
William Leeson (leesonw)

Commits

rB4882c742b560: Cycles X: tweak adaptive sampling threshold

Summary

Fix incorrect handling of samples in adaptive sampling threshold. The previous formula relied on number of samples cancelling out, but this was not the case.
Base automatic min samples on the adaptive threshold instead of number of samples, since we now consider that the main setting to tweak.

Note this changes noise levels in existing renders and requires thresholds to be
tweaked again.

I could not find a significant upside or downsides in scenes when comparing equal
render times. Mainly what this seems to do is make different scenes behave more
consistently with the same noise threshold.

Diff Detail

Repository

rB Blender

Branch

sample-settings (branched from master)

Build Status

Buildable 16812
Build 16812: arc lint + arc unit

Event Timeline

Brecht Van Lommel (brecht) requested review of this revision.Sep 3 2021, 8:08 PM

Brecht Van Lommel (brecht) created this revision.

Harbormaster completed remote builds in B16812: Diff 41428.Sep 3 2021, 8:08 PM

Brecht Van Lommel (brecht) added a parent revision: D12384: Cycles X: sampling settings update.Sep 3 2021, 8:09 PM

HEYPictures (HEYPictures) awarded a token.Sep 3 2021, 11:19 PM

Alaska (Alaska) awarded a token.Sep 3 2021, 11:47 PM

Adam Friesen (ace_dragon) awarded a token.Sep 4 2021, 1:27 AM

Raimund Klink (Raimund58) awarded a token.Sep 5 2021, 8:02 PM

Suggested tweak to the comment wording. But trust to apply the tweak without extra review iteration.

intern/cycles/render/integrator.cpp
306–307	Referencing code state from somewhere in the past is always weird and puzzling. Such information is more suitable to the patch description/commit message. Alternative wording could be something: Arbitrary factor that makes the threshold more similar to the adaptive sampling termination which compared accumulated image buffers (without scaling the buffer down). Also gives arguably more intuitive values. Just disambiguate "before", make it clear from reading the comment without digging the history.

This revision is now accepted and ready to land.Sep 6 2021, 12:13 PM

Closed by commit rB4882c742b560: Cycles X: tweak adaptive sampling threshold (authored by Brecht Van Lommel (brecht)). · Explain WhySep 6 2021, 2:25 PM

This revision was automatically updated to reflect the committed changes.

Brecht Van Lommel (brecht) added a commit: rB4882c742b560: Cycles X: tweak adaptive sampling threshold.

You don't need the inv_sample as it would cancel out in the expression. Also the 0.0001 to stop a divide by zero seems relatively large compared to the error threshold of 0.001 it's only a factor of 10.

intern/cycles/kernel/kernel_adaptive_sampling.h
73–81	You don't need to divide by `inv_sample` as that term would cancel out at `error_difference/error_normalise` ignoring the `0.0001f`

In D12395#322967, @William Leeson (leesonw) wrote:

Also the 0.0001 to stop a divide by zero seems relatively large compared to the error threshold of 0.001 it's only a factor of 10.

Probably it can be lowered to 0.00001 but not much more due to numerical precision. 0.0001 might already be low enough that you can't reach that kind of threshold with a realistic number of samples for real world scenes.

intern/cycles/kernel/kernel_adaptive_sampling.h
73–81	I don't think it cancels out since it evaluates to `inv_sample / sqrtf(inv_sample)`? That factor is what made the threshold behavior unpredictable previously.