Page MenuHome

Shading: Refactor Math node and use dynamic inputs.
ClosedPublic

Authored by Omar Emara (OmarSquircleArt) on Aug 14 2019, 4:33 PM.

Details

Summary
  • Implement dynamic inputs. The second input is now unavailable in single operand math operators.
  • Rename inputs and outputs to be A, B, and Result.
  • Replace the Clamp option with a clamp node.
  • Clean up code and unify naming between Blender and Cycles.
  • Remove unused code.

Diff Detail

Repository
rB Blender
Branch
math-node (branched from master)
Build Status
Buildable 4476
Build 4476: arc lint + arc unit

Event Timeline

Omar Emara (OmarSquircleArt) planned changes to this revision.Aug 14 2019, 4:48 PM

Can you build WITH_GTESTS=ON and check the tests pass? There are currently build errors.
https://wiki.blender.org/wiki/Tools#Automated_Testing

@Brecht Van Lommel (brecht) Sorry, I didn't realize tests had to be enabled. Will fix that now.

  • Cycles: Fix tests.
  • Cycles Versioning: Update link iterator.

The following file includes a node tree with math nodes in different settings. This blend file was created before this patch so that it can be used to test backward compatibility.

Brecht Van Lommel (brecht) requested changes to this revision.Aug 16 2019, 2:53 PM

I don't agree with changing the input value names to A, B and Result. That's inconsistent with other nodes like Mix RGB and Vector Math. If we want to change the naming convention here it should be done everywhere, but I'm not sure one convention is really better than the other.

I'm also not sure the clamp option really needed to be removed. It's maybe a little odd to assume 0..1, but still quite convenient since many parameters need to be in the 0..1 range.

Implementation looks good otherwise.

source/blender/blenloader/intern/versioning_cycles.c
462

Just use {, or better, bump the subversion and use MAIN_VERSION_ATLEAST.

This revision now requires changes to proceed.Aug 16 2019, 2:53 PM

Keeping A and B in the tooltips still makes sense to me though.

I'm also not sure the clamp option really needed to be removed. It's maybe a little odd to assume 0..1, but still quite convenient since many parameters need to be in the 0..1 range.

Indeed. From user perspective losing the Clamp option would be quite inconvenient. I use it a lot, and having to use a node instead would suck. Can't really see a benefit from removing it either.

@Brecht Van Lommel (brecht) I also changed the socket names to A and B in the Vector Math node, and could do the same for the Mix RGB node. Sorry if I am wasting your time, but let me explain the reasoning behind this change:

  • The identifiers of the sockets will be unique by default. This has many advantages:
    • In C code, we can get sockets more readily by their unique identifiers using something like nodeFindSocket(node, SOCK_IN, "B"). If you look at the patch, you will see that this makes the code much more readable than before. Before we had to index the socket blindly in the listbase.
    • Socket names will be the same for Blender and Cycles. Before, Cycles used the names Value1 and Value2. Had we used A and B, the socket names would be consistent and the functions calls to get the sockets would be similar nodeFindSocket(node, SOCK_IN, "B") -> input("B").
    • The python path to a specific socket becomes more readable and easier to write. To me, inputs["B"] is better than inputs["Value_001"] or inputs[1].
    • You get the point, other areas of the code benefit from the distinction.
  • Tool tips become clearer.
  • Node labels can be more expressive, especially when the node is hidden. I plan to implement a better label function in a separate patch.
  • Animation Nodes uses this same naming convention and has a better label function. So this convention was tried by a lot of users already.

I can see the advantages. But this breaks Python API, also affects the compositor, and just in will cost us and add-on writes a bunch of time. I just rather not go down this rabbit hole.

If @Jacques Lucke (JacquesLucke) agrees this is important, you guys can make a design for better node socket naming guidelines. And then maybe the new node system can use it and existing node system can be changed to conform, in 2.82 or so.

@Brecht Van Lommel (brecht) Regarding the removal of the clamp option, let me explain my reasoning.

To quote the Node Design Guidelines

Inputs that have no reasonable values outside their range should be clamped. Nodes can't expect this to be ensured by the specified min/max range, as textures and other arbitrary node setups may result in values outside of the range.

To me, this means that if the clamp option is used as commonly as you say, then the nodes we have now don't follow the aforementioned guidelines. So if all inputs that expect values in the [0, 1] range clamp their input appropriately, the clamp option would be practically useless.

But putting the guidelines aside, I think that nodes work best when they follow the UNIX philosophy, that is, they should do one thing and only one thing. The Math node is the simplest node we have, we shouldn't clutter it with options that doesn't make sense in their context.

Finally, a Clamp node would be used correctly more often than a clamp option.

@Sebastian Koenig (sebastian_k) Can you screenshot some of the node setups that you used this option in?

If @Jacques Lucke (JacquesLucke) agrees this is important, you guys can make a design for better node socket naming guidelines. And then maybe the new node system can use it and existing node system can be changed to conform, in 2.82 or so.

While I think this is important, it is probably not important enough to break many addons now. It is probably better to wait a bit and make more incompatible changes at the same time later.

Personally, I don't really believe in the UNIX philosophy when it comes to user interface design. The UNIX command line is not a particularly user-friendly or discoverable interface.

I think it's important to make nodes easily composable, and functionality should not be duplicated carelessly. But I don't think nodes have to do only one thing, they can be higher level than that when it's convenient.

There are node inputs where a value outside the 0..1 range is usually what you want, but still allow outside the range for artistic purposes or less common use cases.

@Brecht Van Lommel (brecht) Just to make sure. Do you object to using A and B in the code base everywhere or are you talking about sockets only? In other words, can I use A and B internally in the Cycles/OSL/GLSL code?

Using A and B internally is fine with me.

Personally, I don't really believe in the UNIX philosophy when it comes to user interface design.

Unix philosophy and UI design are as alike as chocolate and fish ... they're not supposed to be on the same plate.

The UNIX command line is not a particularly user-friendly or discoverable interface.

Admittedly, when the command-line was invented, the options were kinda limited to text over teletype printers, but you have to admire the efforts of the budding CG artists of the day ... it is worth having a read on the various interpretations of the UNIX philosophy and noting that Don Norman makes the same critcism of the user interface. It is worth noting that the issues with the command-line generally fall into ridiculous names for common tasks, grep instead of search is a prime example of this.

Doing one thing and doing it well is only part of the UP story, it also mandates piping outputs to inputs, this is effectively what nodes do. The whole point of nodes is to build up more complicated operations using simple building blocks, as you say:

I think it's important to make nodes easily composable, and functionality should not be duplicated carelessly. But I don't think nodes have to do only one thing, they can be higher level than that when it's convenient.

and this is very true, but only if the convenience is of great benefit, the principled shaders are a fine example of this, offering a single node to model real materials when otherwise it would be a great effort to understand the mechanics of building nodes from scratch.

There are node inputs where a value outside the 0..1 range is usually what you want, but still allow outside the range for artistic purposes or less common use cases.

So perhaps the essence of this is to have a clamp node that has parameterized min and max values ... in DSP electronics this would be named band-pass filter.

I propose that there should be a class of nodes called filters, include lo, hi and band pass plus one special filter named "clamp" that implicitly filters in the 0 .. 1 range maintaining it's legacy meaning to blender veterans.

  • Revert socket renamming and removal of the clamp option.
  • Revert more renamming.
Omar Emara (OmarSquircleArt) marked an inline comment as done.Aug 17 2019, 1:30 PM
This revision is now accepted and ready to land.Aug 18 2019, 3:53 AM