Cleanup: various cleanups in Cycles kernel #120714

Weizhen Huang · 2024-04-16T19:17:09+02:00

Weizhen Huang commented

2024-04-16 19:17:09 +02:00

mostly about unused parameters and code duplication. See individual commits for details.

Weizhen Huang added the

Module

Render & Cycles

label 2024-04-16 19:17:10 +02:00

Weizhen Huang added 1 commit 2024-04-16 19:17:19 +02:00

Cleanup: remove unused function parameters e6cfcfbd02

This is not a complete list of all the unused parameters in kernel, but
those I touch often, so I am more confident that it's safe to delete them.

Weizhen Huang force-pushed cycles-cleanup from 6439f33848 to 981734fe3e

2024-04-16 19:26:54 +02:00

Compare

Weizhen Huang reviewed 2024-04-16 19:33:28 +02:00

intern/cycles/kernel/light/sample.h Outdated

						
				@ -422,6 +420,13 @@ ccl_device_inline float light_sample_mis_weight_forward_surface(KernelGlobals kg

				                                                                const uint32_t path_flag,

				                                                                const ccl_private ShaderData *sd)

				{

				  const bool has_mis = (sd->type & PRIMITIVE_TRIANGLE) && !(path_flag & PATH_RAY_MIS_SKIP) &&

Weizhen Huang commented

2024-04-16 19:33:28 +02:00

I removed the # ifdef __HAIR__ case here. It should be fine to check (sd->type & PRIMITIVE_TRIANGLE) even when __HAIR__ is not defined?

I removed the `# ifdef __HAIR__` case here. It should be fine to check `(sd->type & PRIMITIVE_TRIANGLE)` even when `__HAIR__` is not defined?

Sergey Sharybin commented

2024-04-17 11:01:18 +02:00

Functionality-wise I think it is fine, but it could have a performance impact for feature-optimized Cycles kernels, as it is probably an extra fetch and extra comparison.

weizhen marked this conversation as resolved

Weizhen Huang commented

2024-04-16 19:35:32 +02:00

@blender-bot build +gpu

Weizhen Huang requested review from Sergey Sharybin 2024-04-16 21:17:50 +02:00

Weizhen Huang requested review from Brecht Van Lommel 2024-04-16 21:17:59 +02:00

Sergey Sharybin reviewed 2024-04-17 11:02:46 +02:00

intern/cycles/kernel/integrator/shade_surface.h Outdated

						
				@ -346,2 +330,2 @@

				      bsdf_eval_mul(&bsdf_eval, mis_weight);

				    }

				    const float mis_weight = light_sample_mis_weight_nee(kg, ls.pdf, bsdf_pdf);

				    bsdf_eval_mul(&bsdf_eval, light_eval / ls.pdf * mis_weight);

Sergey Sharybin commented

2024-04-17 10:57:50 +02:00

This seems to change behavior when direct_light_sampling_type is set to DIRECT_LIGHT_SAMPLING_FORWARD and the light shader is not using MIS. Not sure whether I am missing something, or whether it is in fact an expected change. If that is a change and it is expected, then it should be presented as something else than a refactor.

This seems to change behavior when `direct_light_sampling_type` is set to `DIRECT_LIGHT_SAMPLING_FORWARD` and the light shader is not using MIS. Not sure whether I am missing something, or whether it is in fact an expected change. If that is a change and it is expected, then it should be presented as something else than a refactor.

Weizhen Huang commented

2024-04-17 11:30:04 +02:00

Ah, this is my oversight.

In main the logic of direct_light_sampling_type is that whichever option you set, it should converge to the same result. So even if you choose forward sampling and the light actually can't be forward sampled, the MIS weight would still be treated as 1.0f. I think this a more meaningful behavior than what I do here, because direct_light_sampling_type is usually used to check whether all three options give the same result.

I will revert to the old behavior.

Ah, this is my oversight. In main the logic of `direct_light_sampling_type` is that whichever option you set, it should converge to the same result. So even if you choose forward sampling and the light actually can't be forward sampled, the MIS weight would still be treated as 1.0f. I think this a more meaningful behavior than what I do here, because `direct_light_sampling_type` is usually used to check whether all three options give the same result. I will revert to the old behavior.

weizhen marked this conversation as resolved

intern/cycles/kernel/integrator/shade_volume.h Outdated

						
				@ -871,10 +866,7 @@ ccl_device_forceinline void integrate_volume_direct_light(

				  INTEGRATOR_STATE_WRITE(shadow_state, shadow_path, throughput) = throughput_phase;

				  /* Write Light-group, +1 as light-group is int but we need to encode into a uint8_t. */

				  INTEGRATOR_STATE_WRITE(