Cycles Refactor: Replace fixed Tangent input with custom input #119042

Alaska · 2024-03-04T04:55:28+01:00

Alaska commented

2024-03-04 04:55:28 +01:00

This replaces the fixed Tangent input in BsdfNode::compile
with a custom input.

This is done because very few nodes actually use the tangent input
and it would be better to have this slot avaliable for other inputs
on different nodes in the future.

There should be no user facing changes.

This replaces the fixed Tangent input in `BsdfNode::compile` with a custom input. This is done because very few nodes actually use the tangent input and it would be better to have this slot avaliable for other inputs on different nodes in the future. There should be no user facing changes.

Alaska added the

Module

Render & Cycles

label 2024-03-04 04:55:28 +01:00

Alaska added 1 commit 2024-03-04 04:55:36 +01:00

2a7939ddbd Cycles: Replace the fixed Tangent input with variable input

Alaska changed title from ~~Cycles: Replace the fixed Tangent input with custom input~~ to WIP: Cycles: Replace the fixed Tangent input with custom input

2024-03-04 04:55:48 +01:00

Alaska commented

2024-03-04 05:06:44 +01:00

@LukasStockner suggested this might be desirable during the review of #114499, but I noted I'd prefer this as a seperate pull request.

Alaska referenced this pull request

2024-03-04 05:24:49 +01:00

Shader: Add Roughness to Subsurface Scattering Node #114499

Alaska force-pushed tangent-to-param5 from 2a7939ddbd to 2d715d0a0a

2024-04-22 04:47:32 +02:00

Compare

Alaska changed title from ~~WIP: Cycles: Replace the fixed Tangent input with custom input~~ to Cycles: Replace the fixed Tangent input with custom input

2024-04-22 04:52:02 +02:00

Alaska requested review from Brecht Van Lommel 2024-04-22 04:52:10 +02:00

Alaska requested review from Lukas Stockner 2024-04-22 04:52:18 +02:00

Alaska requested review from Sergey Sharybin 2024-04-22 04:52:26 +02:00

Alaska force-pushed tangent-to-param5 from 2d715d0a0a to 7f215a0323

2024-04-22 05:00:46 +02:00

Compare

Brecht Van Lommel commented

2024-04-22 11:32:21 +02:00

@blender-bot build

Alaska added 1 commit 2024-04-22 13:58:24 +02:00

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

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

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

50110d22fc Reduce state usage with isotropic glossy bsdf

Brecht Van Lommel commented

2024-04-22 14:12:05 +02:00

@blender-bot build

Sergey Sharybin commented

2024-04-22 16:41:02 +02:00

Is there some actual practical example of nodes which plan to use the this slot?

I can't say undocumented and hard-to-see re-shuffling of numbered generic parameters like param5_offset, param3_offset, param4_offset is straightforward to follow and understand. There're plenty of cryptic aspects of Cycles already, to me it feels we should not make it even more cryptic. If there are plans to actually utilize the freed slot, maybe we can schedule a bit of time to work on a cleaner solution?

Is there some actual practical example of nodes which plan to use the this slot? I can't say undocumented and hard-to-see re-shuffling of numbered generic parameters like `param5_offset, param3_offset, param4_offset` is straightforward to follow and understand. There're plenty of cryptic aspects of Cycles already, to me it feels we should not make it even more cryptic. If there are plans to actually utilize the freed slot, maybe we can schedule a bit of time to work on a cleaner solution?

Brecht Van Lommel approved these changes 2024-04-22 16:43:16 +02:00

Dismissed

Lukas Stockner approved these changes 2024-04-23 01:10:18 +02:00

Dismissed

Lukas Stockner left a comment

I think this change is small enough to go through for now, but in general I agree with @Sergey that the parameter handling in its current form is a mess:

Some parameters are passed as int, some as uchar.
Some are always assigned, some only when not equal to a default, and some only when non-constant (with an extra 32-bit field to pass the constant value).
The ordering is often inconsistent and weird.

I don't think we can make this super-fancy because of the inherent limitations of the host-kernel interface, but it has to be possible to do it better than the current state. Even something small like a table of all SVM opcodes and their arguments might help.

I think this change is small enough to go through for now, but in general I agree with @Sergey that the parameter handling in its current form is a mess: - Some parameters are passed as `int`, some as `uchar`. - Some are always assigned, some only when not equal to a default, and some only when non-constant (with an extra 32-bit field to pass the constant value). - The ordering is often inconsistent and weird. I don't think we can make this super-fancy because of the inherent limitations of the host-kernel interface, but it has to be possible to do it better than the current state. Even something small like a table of all SVM opcodes and their arguments might help.

intern/cycles/scene/shader_nodes.cpp Outdated

						
				@ -2393,2 +2392,4 @@

				  closure = distribution;

				  ShaderInput *tangent = input("Tangent");

				  tangent = tangent->link || tangent->constant_folded_in ? tangent : NULL;

Lukas Stockner commented

2024-04-23 01:04:57 +02:00

I'd prefer to add a SVMCompiler::is_linked to check these conditions, seems a bit cleaner than repeating them here.

If not, I'd suggest at least wrapping the condition in parentheses.

Also, nitpick, but I think this should be nullptr in new code.

I'd prefer to add a `SVMCompiler::is_linked` to check these conditions, seems a bit cleaner than repeating them here. If not, I'd suggest at least wrapping the condition in parentheses. Also, nitpick, but I think this should be `nullptr` in new code.

Lukas Stockner requested changes 2024-04-23 01:10:31 +02:00

Lukas Stockner left a comment

Wrong button, sorry.

Alaska added 1 commit 2024-04-23 02:48:11 +02:00

26d9cda92c address Lukas comments

Alaska requested review from Lukas Stockner 2024-04-23 02:53:23 +02:00

Alaska commented

2024-04-23 02:55:34 +02:00

Is there some actual practical example of nodes which plan to use the this slot?

Lukas said we may want to make this change to make the addition of roughness to the Subsurface scattering node simpler: #114499

maybe we can schedule a bit of time to work on a cleaner solution?

What did you have in mind for a cleaner solution? Just making sure it's in numerical order (param3, param4, param5) and make all the fixes related to changing the order (Like this: 48ade96387)? Or something more complex?

> Is there some actual practical example of nodes which plan to use the this slot? Lukas said we may want to make this change to make the addition of roughness to the Subsurface scattering node simpler: #114499 > maybe we can schedule a bit of time to work on a cleaner solution? What did you have in mind for a cleaner solution? Just making sure it's in numerical order (param3, param4, param5) and make all the fixes related to changing the order (Like this: https://projects.blender.org/blender/blender/commit/48ade96387648fccbab8a71fdc511bd0bd50c0d1)? Or something more complex?

Alaska referenced this pull request

2024-04-23 05:33:38 +02:00

WIP: Clean version of replace fixed Tangent input with custom input #120957

Sergey Sharybin commented

2024-04-23 10:06:07 +02:00

What did you have in mind for a cleaner solution? Just making sure it's in numerical order (param3, param4, param5) and make all the fixes related to changing the order (Like this: 48ade96387)? Or something more complex?

Definitely nothing more complex, not for this step anyway. Something more complex would be better to be be handled separately.

Maybe for now it could be something simple as renaming param{1,2,3,4,5} to bsdf_{y,z}/data_{y,z,w}. Then it becomes more clear where things will be stored, even if they are currently expected to be provided out-of-canonical-order. Thoughts?

> What did you have in mind for a cleaner solution? Just making sure it's in numerical order (param3, param4, param5) and make all the fixes related to changing the order (Like this: 48ade96387)? Or something more complex? Definitely nothing more complex, not for this step anyway. Something more complex would be better to be be handled separately. Maybe for now it could be something simple as renaming `param{1,2,3,4,5}` to `bsdf_{y,z}`/`data_{y,z,w}`. Then it becomes more clear where things will be stored, even if they are currently expected to be provided out-of-canonical-order. Thoughts?

👍 1

Alaska added 3 commits 2024-04-23 10:52:28 +02:00

d0f2c53411 Re-arrange params to be numerical

7ed235690a Change param name to bsdf/data name

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

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

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

55c22bdc96 Merge branch 'main' into tangent-to-param5

Alaska changed title from ~~Cycles: Replace the fixed Tangent input with custom input~~ to Cycles Refactor: Replace fixed Tangent input with custom input

2024-04-23 10:53:11 +02:00

Brecht Van Lommel commented

2024-04-24 20:52:06 +02:00

@blender-bot build