Page MenuHome

Node Editor: Highlight invalid closure and string links in red
Needs RevisionPublic

Authored by Lukas Stockner (lukasstockner97) on Sep 3 2016, 6:41 PM.

Details

Summary

Unlike the regular socket types (Color, Value...), the Closure and String types can't be auto-converted to or from any other type.
Therefore, connecting them with any other type is invalid - but that wasn't immediately obvious. So, this commit highlights these invalid links in red.

Diff Detail

Repository
rB Blender
Branch
invalid_link
Build Status
Buildable 141
Build 141: arc lint + arc unit

Event Timeline

Lukas Stockner (lukasstockner97) retitled this revision from to Node Editor: Highlight invalid closure and string links in red.Sep 3 2016, 6:41 PM

I like the idea of having them dashed such as in D1356.

I agree that we need a visual way to communicate invalid links, so a general +1 on the idea. However, this patch only applies to a pretty specific case, a proper solution would be more generic (tm) allowing custom compatibility rules. What if animation nodes Add-on would support linking shader and string sockets? ;) Obviously stupid example but I guess you see that these rules are different for each node tree type.

My suggestion would be to store some socket compatibility info in bNodeSocketType, this can be exposed in RNA somehow. I think it could even be another old-school bitfield (uint64_t would definitely be more than enough) where each bit represents the socket type of the according index. Using it would simply work like this:

bool node_link_sockets_compatible(const bNodeSocket *from, const bNodeSocket *to)
{
    /* in case compatibility_bits don't contain bit for own socket type */
    if (from->typeinfo == to->typeinfo) {
        return true;
    }

    if ((from->typeinfo->compatibility_bits & (1 << to->typeinfo->type)) &&
        (to->typeinfo->compatibility_bits & (1 << from->typeinfo->type)))
    {
        return true;
    }
    return false;
}

I only had a brief look at how custom sockets are defined via .py, afaics the bitfields could work for it too (in .py you would define a list of compatible socket types, RNA would set the according bits then). It may need some special attention to ensure socket type identifiers/indices stay unique, but this is doable.

Note that I'd also check for compatibility in node_link_find_socket, where you can set the NODE_LINK_VALID flag accordingly. This way we avoid doing this check for each link on each redraw.

Julian Eisel (Severin) requested changes to this revision.

I also agree with @Aaron Carlisle (Blendify) that dashed lines would work better for communicating invalid links (colors always bring the issue of color blindness), but this is not really an issue of this patch. Also depends on what alternative solution we find for D1356.

Requesting changes as I'm not so keen about applying this patch as is.

This revision now requires changes to proceed.Sep 3 2016, 8:24 PM

@Julian Eisel (Severin) does using a uint64_t bitfield mean that there cannot be more than 64 different socket types? I don't think that a limitation in the amount of socket types would be good. This might be enouph for material and other specialised node tree types. However that is not enouph for generic node tree types as Animation Nodes. I just counted, I already implemented 60 different types. And there will be more eventually, also because some other developers implement their own types for their specific needs.

Furthermore I implemented a system that finds invalid links, inserts a converter node if possible or removes the link: https://github.com/JacquesLucke/animation_nodes/blob/f1420c1df833b6132f55db3002b9f8bdfb7b7fb5/node_link_conversion.py#L8-L12
I'd also prefer 'marking' the link somehow instead of removing it.

What I'd like to see the most in Blender is a new enum property for Links. Something like draw_mode. Values could be DEFAULT, RED and DASHED. This can also be extended in the future when needed.

I don't want that Blender changes this property for custom node trees as this should be something each developer might want to implement himself.

Personally I also don't like that Blender automatically marks Links as invalid when there is a cycle. I know it is not a really a tree anymore then but Blender should let the developer decide what happens when there is a cycle. For me with animation nodes that is ok, but other developers let the user create cycles intentionally: https://youtu.be/45LGHMamp5Q?t=1m27s

Also you might think that it helps Pynodes developers when this is done automatically, so that they don't have to do it themselves. However everyone who actually develops a new node tree type will have to analyse the graph anyway, cycle detection is often easy to add to such algorithms.
(in Animation Nodes here: https://github.com/JacquesLucke/animation_nodes/blob/f1420c1df833b6132f55db3002b9f8bdfb7b7fb5/tree_info/network.py#L184-L186 and here: https://github.com/JacquesLucke/animation_nodes/blob/master/tree_info/forest_data.py#L107-L108)

In AN socket types already know which types they accept as input, eg here: https://github.com/JacquesLucke/animation_nodes/blob/f1420c1df833b6132f55db3002b9f8bdfb7b7fb5/sockets/float.py#L16
But there is also a generic type that accepts all input types, I would not want to write the name of each existing socket into the GenericSocket class. Furthermore I can imagine some types that only allow a specific subset of sockets as input, eg only list. This socket should not know about each all the list socket types that exist (also because maybe more will be registered at runtime dynamically).

What it all comes down to is the following proposal:

  1. Remove the is_valid property of the NodeLink type: https://www.blender.org/api/blender_python_api_2_77_0/bpy.types.NodeLink.html?highlight=nodelink#bpy.types.NodeLink.is_valid
  1. Add the draw_mode enum property. (maybe also draw_type to make it consistent with Object)
  1. Implement a callback that is called whenever a new link is made. This can be used by Blender to assign the node tree specific draw_mode to the new link. Maybe also create a python interface for that callback, but I don't need it (yet). Maybe this here can be reused by addon developers: https://www.blender.org/api/blender_python_api_2_77_0/bpy.types.Node.html#bpy.types.Node.insert_link
  1. Update the draw link c function to use the new draw_mode property.

[EDIT]
Ok, maybe the is_valid property does not have to be removed but it should not be modified by Blender for custom node trees.