Fix T51023 by adding a separate (internal) clamp node
Needs ReviewPublic
Actions

Authored by Lukas Stockner (lukasstockner97) on Mar 22 2017, 2:37 AM.

Details

Reviewers

Sergey Sharybin (sergey)
Mai Lavelle (maiself)

Summary

The problem with T51023 is that the Mix node wasn't bypassed since its clamp was enabled. But, if it was possible to bypass the Mix, but not the Clamp, one of the images could be ignored.

So, this patch adds two Clamp nodes to Cycles - one for values and one for colors. They're NOT exposed to the user - instead, when a Mix/Math node with clampling enabled is synced, Cycles turns it into a Mix/Math node plugged into a Clamp node.

This makes the code cleaner - the Mix/Math nodes just compile to one SVM instruction, the constant folder can be simplified (since clamping isn't a weird special case anymore) and situations like the one in T51023 are optimized correctly.

Diff Detail

Repository: rB Blender

Event Timeline

Lukas Stockner (lukasstockner97) created this revision.Mar 22 2017, 2:37 AM

Did you do any performance check, especially with OpenCL split kernel? This will be an extra loop iteration for CUDA/CPU and will be extra registers and if-else chains for the split kernel.

I'm also quite skeptical about adding dedicated SVM nodes for such specific thing. If the performance is not an issue here, we can boil this down to two math nodes. Alternatively we can add an extra math node operation. Just an ideas to prevent having too much "opcodes" of our SVM.

P.S. The report you've linked is more like a feature request actually.

intern/cycles/render/nodes.cpp
3820	Indentation as: /* tab // tab /function_name(some_argument, / tab // tab // spaces / some_other_argument, / tab // tab // spaces */ some_other_argument); to properly indent argument to the "parent scope".
intern/cycles/render/nodes.h
676	Indentation.
686	Indentation.

Thomas Dinges (dingto) added a subscriber: Thomas Dinges (dingto).Mar 22 2017, 2:29 PM

Here's a simpler fix, to still execute the full mix node but avoid evaluating the other inputs. Any objections to doing this instead?

diff --git a/intern/cycles/render/constant_fold.cpp b/intern/cycles/render/constant_fold.cpp
index 2569d9e..97c4347 100644
--- a/intern/cycles/render/constant_fold.cpp
+++ b/intern/cycles/render/constant_fold.cpp
@@ -160,6 +160,15 @@ bool ConstantFolder::try_bypass_or_make_constant(ShaderInput *input, bool clamp)
                bypass(input->link);
                return true;
        }
+       else {
+               /* disconnect other inputs if we can't fully bypass due to clamp */
+               foreach(ShaderInput *other, node->inputs) {
+                       if(other != input && other->link) {
+                               graph->disconnect(other);
+                       }
+               }
+               return false;
+       }

        return false;
 }