WIP: Fix #114780: Cycles: Importance sampling correction #115241

Amine Bensalem · 2023-11-21T16:11:52+01:00

Amine Bensalem commented

2023-11-21 16:11:52 +01:00

Principled Chiang hair longitudinal importance sampling correction,
according to the new pbrt fix here :
https://github.com/mmp/pbrt-v3/pull/256 ,
for issue : #114780.

Edit : Fix inverted scale tilt

300s old	300 s new (fixed scale tilt)	Difference

Principled Chiang hair longitudinal importance sampling correction, according to the new pbrt fix here : https://github.com/mmp/pbrt-v3/pull/256 , for issue : #114780. Edit : Fix inverted scale tilt | 300s old |300 s new (fixed scale tilt) | Difference| |---------------|--------------------------------------------|------------------| |![old](https://projects.blender.org/attachments/d8994a13-0da7-46c9-b7f4-3e6eab5be11e)|![new](https://projects.blender.org/attachments/7f825ae7-189d-4ba3-beaa-4425cf2d81aa)|![diff](https://projects.blender.org/attachments/997cae28-65d8-45d3-880a-3cfa893e652c)|

chiang_old_300s.bmp

42 MiB

chiang_new_300s_fixed_scale_tilt.bmp

42 MiB

chiang_final_diff_original_300s.bmp

42 MiB

hair_nodes-female_hair_styles.blend

51 MiB

Amine Bensalem added 1 commit 2023-11-21 16:12:09 +01:00

87d098abc0 Cycles: Fix #114780 : Importance sampling correction

Principled Chiang hair longitudinal importance sampling correction,
according to the new pbrt fix here :
https://github.com/mmp/pbrt-v3/pull/256

Iliya Katushenock changed title from ~~WIP:Cycles: Fix #114780 : Importance sampling correction~~ to WIP: Fix #114780: Cycles: Importance sampling correction

2023-11-21 16:18:06 +01:00

Fernando Alcala commented

2023-11-21 20:51:26 +01:00

First-time contributor

The Old image has the compositor enabled while the Corrected one does not.

Amine Bensalem commented

2023-11-21 21:02:44 +01:00

The Old image has the compositor enabled while the Corrected one does not.

Ah it's true . sorry , I don't know what results to expect yet , it's the first time I work with BSDFs . Still working on it , but I think the second image is not valid ... when changing alpha values to 0 , I don't see any difference with the second image...
Should expect the hair to be more "glossy" like , since light doesn't scatter as much , I guess ?

> The Old image has the compositor enabled while the Corrected one does not. Ah it's true . sorry , I don't know what results to expect yet , it's the first time I work with BSDFs . Still working on it , but I think the second image is not valid ... when changing alpha values to 0 , I don't see any difference with the second image... Should expect the hair to be more "glossy" like , since light doesn't scatter as much , I guess ?

Fernando Alcala commented

2023-11-21 22:10:35 +01:00

First-time contributor

There is a difference.
The fix seems to reduce color noise, improve noise for certain sections, as well as adding a bit of specularity to some areas (especially the bottom part of the image).
40sample old

There is a difference. The fix seems to reduce color noise, improve noise for certain sections, as well as adding a bit of specularity to some areas (especially the bottom part of the image). 40sample old ![40sam chiang old 4.0.0.png](/attachments/014cac00-a202-441e-b161-c6e056cb2ad2)

40sam chiang old 4.0.0.png

9.9 MiB

Amine Bensalem commented

2023-11-22 05:53:15 +01:00

can you pinpoint the differences on the image if that doesn't bother you ?
For example , I can see that there's a difference in colors , this is your version :

And the one I "corrected" :

I'm no 3D artist , but I can see mine has some kind of a greyish look... while yours has more pronounced colors .
I also cannot find the difference in specularity

can you pinpoint the differences on the image if that doesn't bother you ? For example , I can see that there's a difference in colors , this is your version : ![old chiang](/attachments/f9d324d6-6ec8-4b74-b8ca-1df4460eefaa) And the one I "corrected" : ![new chiang](/attachments/edd0cbbf-5635-4590-908e-e0229dc5c3e5) I'm no 3D artist , but I can see mine has some kind of a greyish look... while yours has more pronounced colors . I also cannot find the difference in specularity

image.png

4.4 MiB

image.png

4.5 MiB

image.png

4.5 MiB

Fernando Alcala commented

2023-11-22 06:19:12 +01:00

First-time contributor

Reduced color noise:

Enhanced specularity (cattle is not the best example to show cuticle tilt, sheep would most likely be better, or you could try increasing the tilt to something like 25 to see if large tilts highlight the difference), this little clump of hair and the loose children around it are brighter, indicating either enhanced or different specularity (in this specific section, the secondary reflection seems to benefit the most):

Edit, We could probably get a control reference between old and new by setting tilt to 0, since that code only(?) handles tilt.

Reduced color noise: ![reduced color noise.png](/attachments/7e19ddb2-67bc-4914-9b8a-e772af7e9fff) Enhanced specularity (cattle is not the best example to show cuticle tilt, sheep would most likely be better, or you could try increasing the tilt to something like 25 to see if large tilts highlight the difference), this little clump of hair and the loose children around it are brighter, indicating either enhanced or different specularity (in this specific section, the secondary reflection seems to benefit the most): ![specular.png](/attachments/fd6a1a33-f0f8-4e48-80de-e09bb33eae0f) Edit, We could probably get a control reference between old and new by setting tilt to 0, since that code only(?) handles tilt.

specular.png

1.1 MiB

reduced color noise.png

443 KiB

Amine Bensalem commented

2023-11-22 10:04:21 +01:00

I now see the difference for the noise , but for the specularity , not at all , is it that subtle on this kind of fur ?
Edit : Also , I think the noise reduction is due to the jpeg compression algorithm , need more tests

I now see the difference for the noise , but for the specularity , not at all , is it that subtle on this kind of fur ? Edit : Also , I think the noise reduction is due to the jpeg compression algorithm , need more tests

Fernando Alcala commented

2023-11-24 07:18:16 +01:00

First-time contributor

How did you make the difference image? Using the images you provide I get this

Also, could you send me/link me the build.

How did you make the `difference` image? Using the images you provide I get this ![differnece.jpg](/attachments/357cf70d-f84e-4c31-8305-3930aaa943d8) Also, could you send me/link me the build.

differnece.jpg

46 KiB

Amine Bensalem commented

2023-11-24 08:47:36 +01:00

How did you make the difference image? Using the images you provide I get this

Hi , I just used the compositor :

Also, could you send me/link me the build.

can you tell me how to do this ? one of the makefile file ?

Edit : rendered the two bases images into .hdr files , and made another diff , I think the compression messed up my other renders when I saved them into png or jpeg...

that line at the top , on the other hand is troublesome...

> How did you make the `difference` image? Using the images you provide I get this > ![differnece.jpg](/attachments/357cf70d-f84e-4c31-8305-3930aaa943d8) Hi , I just used the compositor : ![diffy](/attachments/4bee9628-9b4d-439e-a9f6-8e5a9a752a22) > Also, could you send me/link me the build. can you tell me how to do this ? one of the `makefile` file ? Edit : rendered the two bases images into .hdr files , and made another diff , I think the compression messed up my other renders when I saved them into png or jpeg... ![diff](/attachments/a86fd078-91e1-485d-a1a5-5214e5abd817) that line at the top , on the other hand is troublesome...

diff.png

16 MiB

image.png

2.0 MiB

Amine Bensalem force-pushed cycles-114780_chiang-hair from 21ee5c8859 to ea85357491

2023-11-27 10:02:57 +01:00

Compare

Holly Liu referenced this pull request

2024-02-12 19:57:53 +01:00

Improve Principled Hair Chiang importance sampling for tilt #114780

Amine Bensalem added 4 commits 2024-02-14 06:21:57 +01:00

d240c877fa Merge branch 'main' into cycles-114780_chiang-hair

6aee065fd3 Merge branch 'main' into cycles-114780_chiang-hair

a90d66543d Fix crash when alpha = 0

31a6ce82c8 Corrected TRRT+ for Mp in the sample function

Amine Bensalem changed title from ~~WIP: Fix #114780: Cycles: Importance sampling correction~~ to Fix #114780: Cycles: Importance sampling correction

2024-02-17 08:47:25 +01:00

Iliya Katushenock added this to the Render & Cycles project 2024-02-17 13:05:10 +01:00

Brecht Van Lommel requested review from Brecht Van Lommel 2024-02-20 16:52:53 +01:00

Brecht Van Lommel requested review from Weizhen Huang 2024-02-20 16:52:53 +01:00

Weizhen Huang requested changes 2024-03-18 13:13:13 +01:00

Weizhen Huang left a comment

Thank you! Sorry for the delay, somehow this PR totally slipped my attention.

The implementation looks mostly correct, however, it totally negated the behaviour of scale tilt, that's not what we want. You should add bsdf->alpha = -bsdf->alpha; in bsdf_hair_chiang_setup() (alternatively, preserve the +/- signs in hair_alpha_angles()), fix another issue I mentioned about the residual component, and update the reference images you posted in the PR. Then we can have a look how much this PR improves.

Thank you! Sorry for the delay, somehow this PR totally slipped my attention. The implementation looks mostly correct, however, it totally negated the behaviour of scale tilt, that's not what we want. You should add `bsdf->alpha = -bsdf->alpha;` in `bsdf_hair_chiang_setup()` (alternatively, preserve the +/- signs in `hair_alpha_angles()`), fix another issue I mentioned about the residual component, and update the reference images you posted in the PR. Then we can have a look how much this PR improves.

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

						
				@ -225,8 +227,8 @@ ccl_device_inline void hair_attenuation(

				}

				/* Given the tilt angle, generate the rotated theta_i for the different bounces. */

Weizhen Huang commented

2024-03-18 11:27:26 +01:00

also change theta_i to theta_o in the comment.
BTW the comment is quite misleading, considering it's not generating angles but sin_angles and cos_angles.
I would prefer rewriting it as:
/* Update sin_theta_o and cos_theta_o to account for scale tilt in each bounces. */

also change `theta_i` to `theta_o` in the comment. BTW the comment is quite misleading, considering it's not generating angles but sin_angles and cos_angles. I would prefer rewriting it as: `/* Update sin_theta_o and cos_theta_o to account for scale tilt in each bounces. */`

HamilcarR marked this conversation as resolved

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

						
				@ -349,2 +351,3 @@

				  const float3 local_O = make_float3(dot(sd->wi, X), dot(sd->wi, Y), dot(sd->wi, Z));

				  const float3 local_O = make_float3(

				      dot(sd->wi, X), dot(sd->wi, Y), dot(sd->wi, Z)); /* wo in pbrt */

Weizhen Huang commented

2024-03-18 11:27:29 +01:00

Put this comment above the code, add a period at the end of sentence

HamilcarR marked this conversation as resolved

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

						
				@ -394,3 +393,1 @@

				  float cos_theta_i = cos_from_sin(sin_theta_i);

				  float angles[6];

				  float angles[6]; /* contains sinThetaOp and cosThetaOp for each ray bounce */

Weizhen Huang commented

2024-03-18 11:33:07 +01:00

This comment does not explain what sinThetaOp and cosThetaOp are. Maybe:
/* Updated sin_theta_o and cos_theta_o accounting for scale tilt in each bounce. */
Remember to use proper English sentence, starting with a captical letter and ending with a full-stop: https://developer.blender.org/docs/handbook/guidelines/c_cpp/#comments
I would also prefer putting this comment above the variable.

This comment does not explain what `sinThetaOp` and `cosThetaOp` are. Maybe: `/* Updated sin_theta_o and cos_theta_o accounting for scale tilt in each bounce. */` Remember to use proper English sentence, starting with a captical letter and ending with a full-stop: https://developer.blender.org/docs/handbook/guidelines/c_cpp/#comments I would also prefer putting this comment above the variable.

HamilcarR marked this conversation as resolved

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

						
				@ -398,3 +398,1 @@

				    hair_alpha_angles(sin_theta_i, cos_theta_i, -bsdf->alpha, angles);

				    sin_theta_i = angles[2 * p];

				    cos_theta_i = angles[2 * p + 1];

				    sintheta_temp_o = angles[2 * p];

Weizhen Huang commented

2024-03-18 11:42:03 +01:00

I do not like this naming, because this variable has a concrete meaning and is therefore not temp, and it does not following the naming convention of the nearby code.
I suggest sin_theta_o_tilted

I do not like this naming, because this variable has a concrete meaning and is therefore not temp, and it does not following the naming convention of the nearby code. I suggest `sin_theta_o_tilted`

Amine Bensalem commented

2024-04-29 19:22:13 +02:00

agreed , this was poorly named.

HamilcarR marked this conversation as resolved

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

						
				@ -433,3 +435,3 @@

				  {

				    const float Mp = longitudinal_scattering(

				        sin_theta_i, cos_theta_i, sin_theta_o, cos_theta_o, 4.0f * bsdf->v);

				        sin_theta_i, cos_theta_i, sintheta_temp_o, costheta_temp_o, 4.0f * bsdf->v);

Weizhen Huang commented

2024-03-18 12:13:23 +01:00

In eval() we don't consider scale tilt for the residual component, we shouldn't change this behaviour here.

In `eval()` we don't consider scale tilt for the residual component, we shouldn't change this behaviour here.

HamilcarR marked this conversation as resolved

Amine Bensalem changed title from ~~Fix #114780: Cycles: Importance sampling correction~~ to WIP: Fix #114780: Cycles: Importance sampling correction

2024-04-25 17:54:07 +02:00

Amine Bensalem commented

2024-04-29 19:24:52 +02:00

Thank you! Sorry for the delay, somehow this PR totally slipped my attention.

The implementation looks mostly correct, however, it totally negated the behaviour of scale tilt, that's not what we want. You should add bsdf->alpha = -bsdf->alpha; in bsdf_hair_chiang_setup() (alternatively, preserve the +/- signs in hair_alpha_angles()), fix another issue I mentioned about the residual component, and update the reference images you posted in the PR. Then we can have a look how much this PR improves.

Thank you for your feedback.
I'm sorry myself for the delay , tough two last months at work.
there's just one question , concerning the residual component , from my understanding , this is the last "ray" coming out of the hair, but why isn't scale tilt applied at this level ?

> Thank you! Sorry for the delay, somehow this PR totally slipped my attention. > > The implementation looks mostly correct, however, it totally negated the behaviour of scale tilt, that's not what we want. You should add `bsdf->alpha = -bsdf->alpha;` in `bsdf_hair_chiang_setup()` (alternatively, preserve the +/- signs in `hair_alpha_angles()`), fix another issue I mentioned about the residual component, and update the reference images you posted in the PR. Then we can have a look how much this PR improves. Thank you for your feedback. I'm sorry myself for the delay , tough two last months at work. there's just one question , concerning the residual component , from my understanding , this is the last "ray" coming out of the hair, but why isn't scale tilt applied at this level ?

Weizhen Huang commented

2024-04-29 20:26:14 +02:00

why isn't scale tilt applied at this level ?

The residual term accumulates all the contribution after TRT, and is assumed to be diffuse, so the tilt doesn't make a difference.

> why isn't scale tilt applied at this level ? The residual term accumulates all the contribution after TRT, and is assumed to be diffuse, so the tilt doesn't make a difference.

👍 1

Amine Bensalem added 2 commits 2024-04-29 20:30:05 +02:00

6f4c5c0d23 Merge branch 'main' into cycles-114780_chiang-hair

edfceb0a67 Fixed inverted scale tilt

Amine Bensalem requested review from Weizhen Huang 2024-04-30 00:41:20 +02:00

This pull request is marked as a work in progress.

This branch is out-of-date with the base branch

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u cycles-114780_chiang-hair:HamilcarR-cycles-114780_chiang-hair

git checkout HamilcarR-cycles-114780_chiang-hair

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

WIP: Fix #114780: Cycles: Importance sampling correction #115241

Checkout