Cleanup: Cycles: remove SHARP distribution internally #109902

Weizhen Huang · 2023-07-10T12:17:17+02:00

Weizhen Huang commented

2023-07-10 12:17:17 +02:00

this option was already unselectable in the UI, and is treated as GGX
with zero roughness. Upon building the shader graph, we only convert a
closure to SHARP when option Filter Glossy is not used and the
roughness is below certain threshold. The benefit is that we can avoid
calling bsdf_eval() or return earlier in some cases, but the thresholds
vary across files.
This patch removes SHARP closures altogether, and checks if the
roughness value is below a global threshold BSDF_ROUGHNESS_THRESH
after blurring, in which case the flag SD_BSDF_HAS_EVAL is not set.
The global threshold is set to be 5e-7f because threshold smaller than
that seems to have caused problem in the past (c6aa0217ac). Also removes
a bunch of functions, variables and arguments that were only there
because we converted closures under certain conditions.

this option was already unselectable in the UI, and is treated as GGX with zero roughness. Upon building the shader graph, we only convert a closure to `SHARP` when option Filter Glossy is not used and the roughness is below certain threshold. The benefit is that we can avoid calling `bsdf_eval()` or return earlier in some cases, but the thresholds vary across files. This patch removes `SHARP` closures altogether, and checks if the roughness value is below a global threshold `BSDF_ROUGHNESS_THRESH` after blurring, in which case the flag `SD_BSDF_HAS_EVAL` is not set. The global threshold is set to be `5e-7f` because threshold smaller than that seems to have caused problem in the past (c6aa0217ac). Also removes a bunch of functions, variables and arguments that were only there because we converted closures under certain conditions.

Weizhen Huang added 1 commit 2023-07-10 12:17:32 +02:00

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

d172609e18 Cleanup: Cycles: remove `SHARP` distribution internally

this option was already unselectable in the UI, and is treated as GGX
with zero roughness. Upon building the shader graph, we only convert a
closure to `SHARP` when option Filter Glossy is not used and the
roughness is below certain threshold, but the thresholds vary across
files, and the only benefit is that we can avoid calling `bsdf_eval()`
in some cases.
This patch removes `SHARP` closures altogether, and checks if the
roughness value is below a global threshold `BSDF_ROUGHNESS_THRESH`
after blurring, in which case the flag `SD_BSDF_HAS_EVAL` is not set.
The global threshold is set to be `5e-7` because threshold smaller than
that seems to have caused problem in the past (c6aa0217ac). Also removes
a bunch of functions, variables and arguments that were only there
because we converted closures under certain conditions.

Weizhen Huang requested review from Lukas Stockner 2023-07-10 12:21:15 +02:00

Weizhen Huang requested review from Sergey Sharybin 2023-07-10 12:21:28 +02:00

Weizhen Huang requested review from Brecht Van Lommel 2023-07-10 12:21:36 +02:00

Brecht Van Lommel approved these changes 2023-07-10 12:42:29 +02:00

Sergey Sharybin commented

2023-07-10 14:04:43 +02:00

@blender-bot package

Blender Bot commented

2023-07-10 14:04:48 +02:00

Package build started. Download here when ready.

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

Sergey Sharybin approved these changes 2023-07-10 16:44:06 +02:00

Sergey Sharybin left a comment

Nice code reduction!

Weizhen Huang added 1 commit 2023-07-10 17:02:46 +02:00

b4cddda82e Cleanup: rename `BSDF_ROUGHNESS_THRESH` to `BSDF_ROUGHNESS_SQ_THRESH`

Lukas Stockner approved these changes 2023-07-11 02:13:35 +02:00

Lukas Stockner left a comment

Overall LGTM, just two comments.

intern/cycles/kernel/closure/bsdf_microfacet.h Outdated

						
				@ -851,1 +846,3 @@

				  return SD_BSDF | SD_BSDF_HAS_EVAL;

				  int flag = SD_BSDF;

				  if (bsdf->alpha_x * bsdf->alpha_y > BSDF_ROUGHNESS_SQ_THRESH) {

Lukas Stockner commented

2023-07-11 02:13:08 +02:00

This if block is repeated seven times, it might be worth adding something like

int bsdf_microfacet_flags(ccl_private MicrofacetBsdf *bsdf)
{
  if (bsdf->alpha_x * bsdf->alpha_y > BSDF_ROUGHNESS_SQ_THRESH) {
    return SD_BSDF | SD_BSDF_HAS_EVAL;
  }
  return SD_BSDF;
}

so that we can just do return bsdf_microfacet_flags(bsdf) or return bsdf_microfacet_flags(bsdf) | SD_BSDF_HAS_TRANSMISSION?

This `if` block is repeated seven times, it might be worth adding something like ``` int bsdf_microfacet_flags(ccl_private MicrofacetBsdf *bsdf) { if (bsdf->alpha_x * bsdf->alpha_y > BSDF_ROUGHNESS_SQ_THRESH) { return SD_BSDF | SD_BSDF_HAS_EVAL; } return SD_BSDF; } ``` so that we can just do `return bsdf_microfacet_flags(bsdf)` or `return bsdf_microfacet_flags(bsdf) | SD_BSDF_HAS_TRANSMISSION`?

weizhen marked this conversation as resolved

intern/cycles/kernel/osl/closures_template.h Outdated

						
				@ -28,15 +28,6 @@ OSL_CLOSURE_STRUCT_BEGIN(Translucent, translucent)

				  OSL_CLOSURE_STRUCT_MEMBER(Translucent, VECTOR, packed_float3, N, NULL)

				OSL_CLOSURE_STRUCT_END(Translucent, translucent)

				OSL_CLOSURE_STRUCT_BEGIN(Reflection, reflection)

Lukas Stockner commented

2023-07-11 02:06:54 +02:00

reflection() and refraction() are default builtin closures in OSL, so we probably want to keep this closure in the OSL interface. We can just implement it through bsdf_microfacet_ggx_setup() I guess.

`reflection()` and `refraction()` are default builtin closures in OSL, so we probably want to keep this closure in the OSL interface. We can just implement it through `bsdf_microfacet_ggx_setup()` I guess.

weizhen marked this conversation as resolved

Weizhen Huang added 3 commits 2023-07-12 12:32:18 +02:00

008513fd8b Reduce duplication of comparing roughness with thresh

1b37bb8c27 Add back osl reflection and refraction closures

056502f5a5 Merge branch 'main' into remove_sharp_distribution

Weizhen Huang merged commit 0b3efc9d8c into main

2023-07-12 12:36:37 +02:00

Weizhen Huang deleted branch remove_sharp_distribution

2023-07-12 12:36:39 +02:00

Weizhen Huang referenced this issue from a commit

2023-07-12 12:36:39 +02:00

Cleanup: Cycles: remove `SHARP` distribution internally