First set of fixes

Second issue for customnodes branch. First was uploaded by Brecht, so i cannot add new patch sets (https://codereview.appspot.com/7099061/).

On 2013/01/21 12:40:22, brechtvl wrote:
> Testing the patch, I found these issues:
> 
> * For Cycles in the material properties, it shows "No output node" in the
> Surface and Displacement panels.

Fixed. Issue here is that the 'type' property in nodes is deprecated, nodes are identified by strings that match the RNA type name. I changed the type property back to using the old enum identifiers now (same as bl_static_type property) to keep the ui scripts working, but in the long run this should be changed to use the bl_idname string instead (e.g. 'OUTPUT_MATERIAL' -> 'ShaderNodeOutputMaterial')

> * For color sockets, it now shows the color box aligned to the right of the node
> rather than the left. Is this intentional?  With e.g. a Diffuse BSDF node this
> looks quite confusing to me.

Fixed. Socket value drawing now uses a UILayout, which is much nicer than low-level uiDefBut functions but may require some tweaking to get as close as possible to the previous look.

> * 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.

> * When entering a group, there is a green background color shown. I'm not sure a
> different background color is needed, a good indication in the header is enough
> I think.

The main purpose of this background drawing is to give an indication of the tree "history" of the node editor, especially since we can now open unlimited nested node groups. It tries to mimick the old way of group "edit mode".

I had a simpler variant first that displayed a tree path string in the lower left cornder (e.g. "Material/NodeGroup/NodeGroup2"), but that was abstract and hard to parse quickly. I'm open to suggestions, but i think we need a good visualization of the successive node groups that have been opened.

> * Group Input/Ouptut nodes have a black header here, missing version patch for
> theming?

Yes, forgot about that. Added do_versions for the theme color in resources.c. Default for now is a slightly desaturated yellow, can be changed if UI mafia want to.

> * It seems that you can put Group Input/Output nodes in groups themselves. I
> guess that's not intenional.

Indeed, fixed so that input/output nodes stay in their original node tree, as if they were not selected.

> * When there are no nodes selected or use nodes is disabled, it doesn't show the
> grid lines. I think they should show still, I think it's a good visual cue for
> quickly seeing which editor type you're looking at. Other editors like graph or
> image do the same.

Agreed, added this back.

> * Cycles Render > open node editor > add new material, it prints
> "RNA_string_set: NodeSocketShader.value_property not found." in the console.

Fixed.

> * Cycles render > open node editor > add new material > press pin button > split
> node editor in two > crash.

Can't reproduce this. Maybe got fixed by changes to node space draw function...

> * Cycles render > open node editor > add new material > Diffuse BSDF color has 0
> alpha value instead of 1.

Fixed.

About cycles color sockets: These are generally RGB instead of RGBA. Currently there is still just one common NodeSocketColor type used throughout shader and compositor node (you can change alpha values for cycles nodes in trunk too). It would probably make sense to have 2 separate standard socket types for both RGB and RGBA color models. Socket types can be defined for specific tree type as well, there is no need to use the exact same socket definitions for the data types throughout all systems, so in the long run could have Cycles/BI sockets vs. compositor sockets for cleaner design.

> * Cycles render > open node editor > add new material > select Diffuse BSDF >
> make group > exit group. Group color socket has black rather than white color.

Fixed. Now a socket interface can initialize itself based on an existing socket when exposed (by connecting to the "virtual" extension socket, in trunk the equivalent was to drag a link outside the group bounding box). This copies the appropriate property settings such as current value, subtype, etc. as defaults for the group socket. After this initialization these values are independent from the internal socket and can be tweaked if necessary.

> * Cycles render > open node editor > add new material > pin > change to display
> compositing nodes:
>     layout.prop(snode_id.render, "use_free_unused_nodes", text="Free Unused")
> AttributeError: 'Material' object has no attribute 'render'

Fixed, pinning is now ignored when switch tree types or no node tree is active yet (i.e. "use_nodes" updates node trees once even when pinned).

> * Cycles render > open node editor > change to texture nodes > crash.

Fixed.

> * I think the pin icon should be to the right of the datablock selector as it is
> in the image editor.

Fixed.

[lukas.toenne1, 2013-01-28 09:42:50]

Forgot to add --send_mail to upload.py options, see mail before ...

[lukas.toenne1, 2013-02-02 14:14:46]

>> * 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_oper...
> 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/nod...
> 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/nod...
> 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...
> 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/blend...
> 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....
> 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_...
> 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.

[lukas.toenne1, 2013-02-07 09:12:03]

Merged trunk revisions up to r54370. Also includes an important fix to avoid
crashes when accessing node from API with undefined types, e.g. if a loading
nodes from .blend whose types are defined by a script that has not been run.
Undefined types can cause error reports and possibly py exceptions, but should
never crash Blender.