Fix: Cycles: Rendering of the Principled BSDF when using adaptive kernel compilation #128003

Alaska · 2024-09-23T05:28:10+02:00

Alaska commented

2024-09-23 05:28:10 +02:00

Fixes a issue where the Principled BSDF would render incorrectly if
__SUBSURFACE__ is off. Which is common when using adaptive kernel
compilation (a unsupported Cycles feature).

Fixes a issue where the Principled BSDF would render incorrectly if `__SUBSURFACE__` is off. Which is common when using adaptive kernel compilation (a unsupported Cycles feature).

Alaska requested review from Sergey Sharybin 2024-09-23 05:28:37 +02:00

Alaska requested review from Lukas Stockner 2024-09-23 05:28:38 +02:00

Sergey Sharybin commented

2024-09-23 16:19:53 +02:00

That is a good find!

In a way it is kind of annoying to have such ifdef-ed offsets. I wonder if always doing read_node() will have a measurable impact on performance (as in, remove ifdef around the read_node). This is something we do in svm_node_closure_bsdf_skip() where we do 4 read_node() instead of changing offset manually. If we can avoid branching in preprocessor that'd be easier for read.

If not, maybe we can have skip_node(num_nodes_to_skip) to kind of hide the offset magic, and also use it in svm_node_closure_bsdf_skip ?

That is a good find! In a way it is kind of annoying to have such ifdef-ed offsets. I wonder if always doing `read_node()` will have a measurable impact on performance (as in, remove ifdef around the `read_node`). This is something we do in `svm_node_closure_bsdf_skip()` where we do 4 `read_node()` instead of changing offset manually. If we can avoid branching in preprocessor that'd be easier for read. If not, maybe we can have `skip_node(num_nodes_to_skip)` to kind of hide the offset magic, and also use it in `svm_node_closure_bsdf_skip` ?

Alaska commented

2024-09-23 17:30:05 +02:00

I'll take a look at the performance cost of always reading the node later today.

If the performance impact is meaningful, then I'll implement skip_nodes. Just checking, this is what you had in mind?

ccl_device_inline void skip_nodes(const int offset_amount, ccl_private int *offset) {
   *offset += offset_amount;
}

I'll take a look at the performance cost of always reading the node later today. If the performance impact is meaningful, then I'll implement `skip_nodes`. Just checking, this is what you had in mind? ```cpp ccl_device_inline void skip_nodes(const int offset_amount, ccl_private int *offset) { *offset += offset_amount; } ```

Alaska commented

2024-09-24 07:36:38 +02:00

I switched to just reading the node rather than having offset++ or skip_nodes() because it's cleaner.

I ran some benchmarks comparing offset++ to just reading the node and the results weren't consistent enough to convince me that there was any meaningful performance difference between reading the node and just increasing the offset.

What I mean by this was that in my testing, sometimes reading the node was faster, sometimes just increasing the offset was faster. And the difference in performance was within the margin for error and which was faster could flip when re-testing.

I switched to just reading the node rather than having `offset++` or `skip_nodes()` because it's cleaner. I ran some benchmarks comparing `offset++` to just reading the node and the results weren't consistent enough to convince me that there was any meaningful performance difference between reading the node and just increasing the offset. What I mean by this was that in my testing, sometimes reading the node was faster, sometimes just increasing the offset was faster. And the difference in performance was within the margin for error and which was faster could flip when re-testing.

Lukas Stockner approved these changes 2024-09-26 01:07:58 +02:00

Alaska force-pushed fix-adaptive-principled from e19f05dd2f to 7832265127

2024-10-03 10:28:35 +02:00

Compare

Alaska changed target branch from main to blender-v4.3-release

2024-10-03 10:28:40 +02:00

Sergey Sharybin approved these changes 2024-10-04 12:38:45 +02:00

Sergey Sharybin left a comment

Thanks for the update and benchmarking!

Sergey Sharybin merged commit 0709743c0c into blender-v4.3-release

2024-10-04 12:39:15 +02:00

Sergey Sharybin referenced this issue from a commit

2024-10-04 12:39:16 +02:00

Fix: Cycles: Rendering of the Principled BSDF when using adaptive kernel compilation

Sergey Sharybin deleted branch fix-adaptive-principled

2024-10-04 12:39:18 +02:00

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

Fix: Cycles: Rendering of the Principled BSDF when using adaptive kernel compilation #128003