Cycles: Replace Sheen model in the Principled BSDF #109949

Lukas Stockner · 2023-07-11T02:25:43+02:00

Lukas Stockner commented

2023-07-11 02:25:43 +02:00

This replaces the Sheen model used in the Principled BSDF with the model from #108869 that is already used in the Sheen BSDF now.

The three notable differences are:

At full intensity (Sheen = 1.0), the new model is significantly stronger than the old one. For existing files, the intensity is adjusted to keep the overall look similar.
The Sheen Tint input is now a color input, instead of the previous blend factor between white and the base color.
There is now a Sheen roughness control, which can be used to tweak the look between velvet-like and dust-like.

This replaces the Sheen model used in the Principled BSDF with the model from #108869 that is already used in the Sheen BSDF now. The three notable differences are: - At full intensity (Sheen = 1.0), the new model is significantly stronger than the old one. For existing files, the intensity is adjusted to keep the overall look similar. - The Sheen Tint input is now a color input, instead of the previous blend factor between white and the base color. - There is now a Sheen roughness control, which can be used to tweak the look between velvet-like and dust-like.

Lukas Stockner added the

Module

Render & Cycles

label 2023-07-11 02:25:43 +02:00

Lukas Stockner force-pushed sheen-principled from 222f4e5ba3 to 9b7ee1ffac

2023-07-24 16:10:28 +02:00

Compare

Lukas Stockner changed title from ~~WIP: Replace Sheen model in the Principled BSDF~~ to Replace Sheen model in the Principled BSDF

2023-07-24 16:10:52 +02:00

Lukas Stockner added this to the Render & Cycles project 2023-07-24 16:11:01 +02:00

Lukas Stockner requested review from Brecht Van Lommel 2023-07-24 16:11:07 +02:00

Brecht Van Lommel approved these changes 2023-07-25 15:46:58 +02:00

Brecht Van Lommel left a comment

Looks good besides minor comment that does not need re-review.

source/blender/blenloader/intern/versioning_400.cc Outdated

						
				@ -299,0 +301,4 @@

				static void version_principled_bsdf_sheen(bNodeTree *ntree)

				{

				  auto check_node = [](const bNode *node) {

				    return (node != nullptr) && (node->type == SH_NODE_BSDF_PRINCIPLED) &&

Brecht Van Lommel commented

2023-07-25 15:45:44 +02:00

Why does this check node != nullptr, but square_roughness_node_insert does not?

Maybe move that check to version_update_node_input.

Why does this check `node != nullptr`, but `square_roughness_node_insert` does not? Maybe move that check to `version_update_node_input`.

Iliya Katushenock commented

2023-07-25 15:53:17 +02:00

Might be used version_node_id, version_node_socket_name, version_node_input_socket_name, node_tree_relink_with_socket_id_map too

Might be used `version_node_id`, `version_node_socket_name`, `version_node_input_socket_name`, `node_tree_relink_with_socket_id_map` too

Lukas Stockner commented

2023-07-27 01:33:10 +02:00

Might be used version_node_id, version_node_socket_name, version_node_input_socket_name, node_tree_relink_with_socket_id_map too

These look useful in general, but in this case there's some additional conversion going on (the type of the socket changes, as well as the default value in case of old files), so there would need to be custom logic anyways.

> Might be used `version_node_id`, `version_node_socket_name`, `version_node_input_socket_name`, `node_tree_relink_with_socket_id_map` too These look useful in general, but in this case there's some additional conversion going on (the type of the socket changes, as well as the default value in case of old files), so there would need to be custom logic anyways.

Lukas Stockner commented

2023-07-27 01:39:10 +02:00

Why does this check node != nullptr, but square_roughness_node_insert does not?

For some reason tests/render/reports/T48790.blend contains a link with tonode==NULL. But yeah, it's better to handle it in version_update_node_input.

> Why does this check `node != nullptr`, but `square_roughness_node_insert` does not? For some reason `tests/render/reports/T48790.blend` contains a link with `tonode==NULL`. But yeah, it's better to handle it in `version_update_node_input`.

Iliya Katushenock commented

2023-07-27 01:48:50 +02:00

These look useful in general, but in this case there's some additional conversion going on (the type of the socket changes, as well as the default value in case of old files), so there would need to be custom logic anyways.

Changing the socket type and its value, is this the area of runtime declarations...?

> These look useful in general, but in this case there's some additional conversion going on (the type of the socket changes, as well as the default value in case of old files), so there would need to be custom logic anyways. Changing the socket type and its value, is this the area of runtime declarations...?

Lukas Stockner commented

2023-07-27 02:06:23 +02:00

Changing the socket type and its value, is this the area of runtime declarations...?

Not sure about the details, but I don't think so, the type of the socket is fixed - it's just a one-time change from older to newer versions.

> Changing the socket type and its value, is this the area of runtime declarations...? Not sure about the details, but I don't think so, the type of the socket is fixed - it's just a one-time change from older to newer versions.