This part adds support for feature passes - both on the host and in the rendering kernel.
For that, the UI components are required, but aren't actually doing anything noticeable yet.
Depends on D2590.
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
- Build Status
Buildable 625 Build 625: arc lint + arc unit
Event Timeline
| intern/cycles/blender/addon/ui.py | ||
|---|---|---|
| 616–624 | What are the situations where you only want to denoise a subset of components? It's not clear to me what is useful about a render with some components denoised and some not. | |
| intern/cycles/kernel/kernel_passes.h | ||
| 119–120 | A comment about or getting rid of these magic numbers would be good. | |
| 168–174 | Can we make a bsdf_roughness() function in closures/bsdf.h for this? | |
| 177 | I guess this explains why N was moved out of the specific BSDFs. | |
| intern/cycles/kernel/kernel_path.h | ||
| 752 | I think this should be either done before subsurface scattering, or a call to this function should be added in kernel_path_subsurface_scatter()? Otherwise SSS doesn't contribute to the denoising features as far as I can tell. | |
| intern/cycles/kernel/kernel_path_branched.h | ||
| 77–87 | Can we only run this when denoising is enabled? | |
| intern/cycles/kernel/kernel_path_state.h | ||
| 26 | If we only need this for shadow catch and denoising, might be good to only set this flag when one of those is in use? | |
| intern/cycles/kernel/kernel_path_surface.h | ||
| 159 | Indentation. | |
| 180 | sc->sample_weight / (sum_sample_weight * num_samples) saves one division. I have some doubts about the correctness of this denoising_feature_weight though, as far as I can tell it does not take into account Russian roulette boosting the contribution of some BSDFs relative to others. I'm also a bit wary of adding another fragile tracking of contribution weights. Would it be possible to use average(throughput) as the weights for denoising depth/normal/albedo, sum these weights, and then do the normalization right before writing out the passes? | |
| intern/cycles/kernel/kernel_types.h | ||
| 344 | This is unused. | |
| intern/cycles/render/buffers.cpp | ||
| 76–79 | Would prefer to have some #defines for these things to be shared with the kernel. It's also not entirely clear to me why these denoising passes can't be treated as regular passes here? | |
Comment Actions
Addressed most of the review.
The option to not denoise some components was requested several times in the initial versions of the patch, so I added it in the GSoC version. I'm not exactly sure for which scenes you'd use it, though.
As for the weight computation in branched path tracing: The problem is that any source of noise has to be avoided in the features, which is why I'm not exactly happy about using throughput for it. Even if the current solution isn't 100% correct, that's actually not even required - the least-squares fit is general enough that it can handle unnormalized weights etc. just fine.
Comment Actions
Looks good to me then.
I guess control over the components is useful as a last resort in case denoising fails for some reason, and being able to boost e.g. branched path tracing samples for some component while disabling denoising is a workaround. Hopefully it will not be needed eventually, but seems ok currently given how none of this denoising stuff is an exact science and some user control is important.
For the feature weights, I don't have a good sense for what works best in the final denoising algorithm. Let's keep it as is then, it can always be tweaked if needed.
Comment Actions
Moved the denoising properties into the Cycles addon and added an option to
store the denoising feature passes in the render result (only visible when
experimental features are enabled).
This makes it a lot easier to debug problems and check why some scenes don't
denoise well, and might be useful for animation denoising in the future
(since that requires multiple frames at once, it has to be separate anyways).
Comment Actions
Looks good to me.
Totally agreed these extra passes are great for developers and animation or post process denoising in the future.
Comment Actions
Python changes are not PEP8 compliant though.
Descriptions lines longer than 120 chars can be broken into:
descpription="Something something ... something "
"something something something",as that is allowed with strings that are inside the brackets (parameters). No need for backslashes.
The changes in engine.py:
If statements cannot be in one line (multiple statements in one colon E701)
Some lines are too long (>120 chars)
Multiples spaces between function parameters (E241 - multiples spaces after)
Python uses different style compared to C, C++ so aligning parameters between columns is a PEP8 warning.
Breaking the line will produce more lines but it can easily be aligned:
if srl.use_pass_z:
engine.register_pass(
scene, srl, "Depth",
1, "Z", 'VALUE'
)
if srl.use_pass_mist:
engine.register_pass(
scene, srl, "Mist",
1, "Z", 'VALUE'
)
if srl.use_pass_normal:
engine.register_pass(
scene, srl, "Normal",
3, "XYZ", 'VECTOR'
)That is valid PEP8. Depending on the preference the lines can be split differently.
Comment Actions
We're already breaking pep8 in these Python files many times then, which I guess would be good to fix but can also be done after for the whole file.
Comment Actions
I agree, but it's something to consider for future commits, as the files will drift away again with new features/changes added.