Fix #105553: do not copy "Roughness" value from the previous node #105596

Weizhen Huang · 2023-03-09T11:59:27+01:00

Weizhen Huang commented

2023-03-09 11:59:27 +01:00

"Roughness" value is not copied, but the link is still preserved, not sure if this is reasonable.

Weizhen Huang added 1 commit 2023-03-09 11:59:37 +01:00

5336a00353 Fix #105553 : do not copy "Roughness" value from the previous node

Weizhen Huang added this to the Render & Cycles project 2023-03-09 12:02:26 +01:00

Weizhen Huang added the

labels 2023-03-09 12:03:05 +01:00

Weizhen Huang requested review from Brecht Van Lommel 2023-03-09 12:03:28 +01:00

Weizhen Huang commented

2023-03-09 14:31:28 +01:00

Jacques was suggesting comparing the identifier instead of name. identifier is default to name if not specified: 3b900048f1/source/blender/nodes/NOD_node_declaration.hh (L729)
I didn't know this field exists, I think it is a good idea, if a new socket needs to be differentiated from others with the same name we can add an identifier to it. But not sure about the existing ones.

Jacques was suggesting comparing the `identifier` instead of `name`. `identifier` is default to `name` if not specified: https://projects.blender.org/blender/blender/src/commit/3b900048f1c5ec098adea09620399ef8266d723b/source/blender/nodes/NOD_node_declaration.hh#L729 I didn't know this field exists, I think it is a good idea, if a new socket needs to be differentiated from others with the same name we can add an identifier to it. But not sure about the existing ones.

Brecht Van Lommel requested changes 2023-03-09 18:26:12 +01:00

Brecht Van Lommel left a comment

The identifier should be valid on all existing shading nodes, as Cycles uses this for exporting shader nodes and does not look at the name.

Can you specifically add a test for the Diffuse BSDF and the Hair BSDFs to not copy the roughness to other nodes? Copying roughness between Glossy and Glass for example should be fine.

The identifier should be valid on all existing shading nodes, as Cycles uses this for exporting shader nodes and does not look at the name. Can you specifically add a test for the Diffuse BSDF and the Hair BSDFs to not copy the roughness to other nodes? Copying roughness between Glossy and Glass for example should be fine.

Iliya Katushenock commented

2023-03-09 18:27:45 +01:00

Still not sure if such selective exclusions even make sense if the name and type are the same...

Weizhen Huang commented

2023-03-09 18:35:50 +01:00

@mod_moder There are different roughness types, specified for half angle or outgoing direction for example, they are not quite the same thing. I'm actually more interested in another field called "Transmission", that also has different meanings in different context, it could be transmission between two interfaces or transmission through a thin object.
But if we could make that distinction in the identifier, I guess I can do without this patch, I just add identifiers to where they are needed.

@mod_moder There are different roughness types, specified for half angle or outgoing direction for example, they are not quite the same thing. I'm actually more interested in another field called "Transmission", that also has different meanings in different context, it could be transmission between two interfaces or transmission through a thin object. But if we could make that distinction in the identifier, I guess I can do without this patch, I just add identifiers to where they are needed.

Iliya Katushenock commented

2023-03-09 18:43:41 +01:00

@weizhen It would be better if you added a formalized description of this (for example, a set of references to node types) to make this kind of processing a post-processing stage of an already modified node.
It would raise far fewer questions about why this should happen.
Also, then it would be possible to somehow add a mask of sockets to indicate that they are specific to each node.

@weizhen It would be better if you added a formalized description of this (for example, a set of references to node types) to make this kind of processing a post-processing stage of an already modified node. It would raise far fewer questions about why this should happen. Also, then it would be possible to somehow add a mask of sockets to indicate that they are specific to each node.

Iliya Katushenock commented

2023-03-09 18:45:40 +01:00

There may also be a variant of the is_unreplaceable~ flag to declare a socket, which would mean that this node's socket cannot be replaced in another node. But it is better to approve with Jacques.

There may also be a variant of the `is_unreplaceable`~ flag to declare a socket, which would mean that this node's socket cannot be replaced in another node. But it is better to approve with Jacques.

Brecht Van Lommel commented

2023-03-09 18:50:43 +01:00

We considered making this a generic system but it seems a bit complicated for now and unclear if it's worth it. When it becomes clear this is needed for other cases we can make it into something more generic.

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Fix #105553: do not copy "Roughness" value from the previous node #105596

Checkout