[Bf-codereview] First set of fixes (issue 7193061)

[lukas.toenne at gmail.com]

>> * When doing "Make Group", there is a submenu with one item. That
>> seems
>> pointless, and I'm not even sure why there has do be a popup in the
>> first place.

> Items in this submenu depend on the node selection:
> - The usual "From Selected" is always present and calls the standard
> make_group operator (or rather a generalized "insert_selected"
function)
> - If active node is a group node, it shows additionally "Insert",
> meaning all other nodes will be inserted into that group.

> I realize this is too complicated on user level. The second option
could
> be a different operator as well, then the submenu is not needed. We're
> just running out of usable keymap items :S

> I'd also like to revisit the grouping/ungrouping operators to move the
> low-level hacks into a nice API function to move nodes between trees
or
> maybe just make exact copies (anim data is a problem though). That
would
> open the door for scripted grouping operators in the future.

Have now split this into two independent operators now, the make group
op works as before without submenu.
"Insert" operator does not have a keymap item for now, but can be
accessed from the node menu.


https://codereview.appspot.com/7099061/diff/1/release/scripts/startup/bl_operators/node.py#newcode193
> release/scripts/startup/bl_operators/node.py:193: node =
> self.create_node(context, item[len(node_type_prefix):])
> I think NODE_OT_add_search needs to be derived from NodeAddOperator
for this to
> work?

Fixed.

The search operator currently still does an explicit type check for
group nodes, i'd like to replace that with a callback later so nodes can
define a list of operator calls themselves.


https://codereview.appspot.com/7099061/diff/1/source/blender/nodes/shader/node_shader_tree.c#newcode70
> source/blender/nodes/shader/node_shader_tree.c:70: * This will
probably change
> in the future, then each engine will get its own poll function.
> We discussed this before, but I still think we should not do this at
the node
> tree level. It's useful to be able to have setups for multiple engines
in one
> node tree, and some node groups can contain nodes that work in
multiple engines.
> Maya/XSI/... also allow such setups.

> I think such a poll function should be implemented at the node level,
and then
> nodes that don't work in certain engines can be greyed out in the node
editor.

Ok, i still have my doubts if that is feasible due to the very different
renderer mechanisms, but i see why it would be desirable. Removed that
comment for now, the poll function works fine with the current
compatibility checks.


https://codereview.appspot.com/7099061/diff/1/source/blender/nodes/shader/node_shader_tree.c#newcode84
> source/blender/nodes/shader/node_shader_tree.c:84: if
(snode->shaderfrom ==
> SNODE_SHADER_OBJECT) {
> Is it a safe assumption that snode is in the context?

Yes, this function is a callback to define automatic node tree selection
from context (when pinning is disabled). It will only be called from a
valid node space context, if that should ever not work it would have to
be fixed at the caller level anyway, no need to do such checks in the
callback itself.

> Opening this file causes a crash with the patch applied.

http://projects.blender.org/tracker/download.php/9/498/33896/23575/pm_bug.blend

Fixed. do_versions for this branch is a real nightmare :S In this case
the do_versions code needs to add new nodes, which requires valid
typeinfo pointers, but those are not set in the usual do_versions code
yet, so it has to delay the whole thing to lib_verify_nodetree. Ugly and
confusing as hell, but no way around it ... I just hope we can remove
the forward compat requirement at some point to make it a little bit
easier and avoid future problems.


https://codereview.appspot.com/7099061/diff/30005/doc/python_api/examples/bpy.types.NodeTree.1.py#newcode23
> doc/python_api/examples/bpy.types.NodeTree.1.py:23: return
> context.scene.render.engine == 'CYCLES'
> The code snippet here is not an example of get_from_context

Oops, i started this bit of doc, but then couldn't come up with a good
example. Removed it for now, API examples can be added later.


https://codereview.appspot.com/7099061/diff/30005/intern/cycles/blender/blender_shader.cpp#newcode172
> intern/cycles/blender/blender_shader.cpp:172: * are subclasses of
another.
> I don't see how that's a problem, I wouldn't use XXX in comments
unless
> something is broken and needs to be fixed.

Yes, i was a bit worried about this when i added it and then never
checked back. Removed the comment.

We could also use the old integer ids to compare node types (now in
b_node.bl_static_type), but i'd like to avoid using them as much as
possible. Using string identifiers (b_node.bl_idname) is also possible,
just not as convenient because the compiler does not have a way of
detecting unknown identifiers.


https://codereview.appspot.com/7099061/diff/30005/intern/cycles/render/nodes.h#newcode197
> intern/cycles/render/nodes.h:197: class GroupInputNode : public
ShaderNode {
> These two class are not used, so can be removed?

Yes, done. Group input/output nodes are converted directly to ProxyNodes
(and subsequently removed).


https://codereview.appspot.com/7099061/diff/30005/release/scripts/startup/bl_ui/node.py#newcode23
> release/scripts/startup/bl_ui/node.py:23:
> Does this file have a purpose?

It was intended for standard py nodes, but currently we don't have any
of these (unlike socket types which are all python now). Removed.


http://codereview.appspot.com/7193061/