nortikin / sverchok

Sverchok
http://nortikin.github.io/sverchok/
GNU General Public License v3.0
2.25k stars 234 forks source link

implementing groups #785

Closed zeffii closed 8 years ago

zeffii commented 8 years ago

For my own sanity i felt it was necessary to have a closer look at the mindfuck that is CustomGroup Nodes :) . I was especially interested in reusing as much of the PyNode api as possible, but I think @ly29 already concluded that also this requires us to pretty much roll our own.

I started a branch https://github.com/nortikin/sverchok/tree/group_experiment which hopes to get away from the use of the Frame and instead be more like the ShaderNodeTree with a back button and an overlay to show the parent NodeTree.


This is merely an exploration i'm not married to any of this code

here a list of things that need to be taken care of (feel free to interject / edit) in no order of importance.


Design Reference

lukas_t made his own node group for object_nodes: https://developer.blender.org/diffusion/B/browse/object_nodes/release/scripts/nodes/group_nodes.py

zeffii commented 8 years ago

I don't know if i'll have time to do anythig with this, I hope to reuse some of @ly29 's code (which looks fine) seen as it appears we'll really need to code all the bits, and then add retrigger updates for the nested node group.

image

ly29 commented 8 years ago

Yeah. my code ran in to issues with all the possibilities of editing. The UI issues also became annoying. If anything seems off in that code it probably is.

zeffii commented 8 years ago

i'll take a long stare at it, and if possible implement something we can elaborate on. I'm not yet convinced that it's a dead end :)

ly29 commented 8 years ago

I don't think it is unworkable. I just wanted to do the Redux project, for which I later didn't have time. Sadly, but I still want scratch that itch.

ly29 commented 8 years ago

That said, this is a small scale project which I might have the time to play around with a bit.

zeffii commented 8 years ago

yeah, understanding the plumbing of this can be useful for redux or whatever :)

zeffii commented 8 years ago

something weird is happening.. in space_node.py


class NODE_UL_interface_sockets(bpy.types.UIList):
    def draw_item(self, context, layout, data, item, icon, active_data, active_propname, index):
        socket = item
        color = socket.draw_color(context)

I think draw_item is called for each new socket generated by the Group Input / Output nodes. Surprisingly socket doesn't have a draw_color attribute. In our node_tree.py it shows our sockets do all have this attribute, so perhaps some information is being stripped?

zeffii commented 8 years ago

printing the socket info..

            print(dir(socket))
            print(socket.bl_socket_idname)
            print(socket.identifier)
            print(socket.name)
            print(socket.rna_type)

['__doc__', '__module__', '__slots__', 'bl_rna', 'bl_socket_idname', 'identifier', 'is_output', 'name', 'rna_type']
StringsSocket
Input_1
Start
<bpy_struct, Struct("NodeSocketInterface")>

shows this is indeed the case...

zeffii commented 8 years ago

It looks like the Group Input node is both a node and a part of this_nodegroup.inputs, it seems that the sv_node_group_in would need to inherit from multiple places... arghhh! :)

zeffii commented 8 years ago

image

but only by modifying node_tree.py, which is not an option for sverchok..

class NODE_UL_interface_sockets(bpy.types.UIList):
    def draw_item(self, context, layout, data, item, icon, active_data, active_propname, index):
        socket = item

        if hasattr(socket, 'draw_color'):
            color = socket.draw_color(context)
        else:
            color = {
                "StringsSocket": (0.6, 1.0, 0.6, 1.0),
                "VerticesSocket": (0.9, 0.6, 0.2, 1.0),
                "MatrixSocket":  (.2, .8, .8, 1.0)
            }.get(socket.bl_socket_idname, (0.2, 0.2, 0.2, 1.0))
ly29 commented 8 years ago

Okay I have looked a my own code a bit as well as yours some small issues to resolve pops up.

Where to store the sv groups, I choose a specific node group tree type to mimic the cycles behavior where the nodes groups are hidden and keep the actual editing via the interface only.

Also looking at space_node.py the socket used there are of type NodeSocketInterface which has draw_color(self, context) but ordinary NodeSocket has draw_color(self, context, node). The reason for this change of signature escapes between the different classes escapes me right now. Also the need for different classes...

Also, some comments never hurts, took a while to understand my thinking in group.py

ly29 commented 8 years ago

Basically the behavior of editing needs to be well defined.

My implementation uses the SverchGroupTree tree type

Looking back I am quite happy with my mixin class StoreSockets, almost a bit smug. Needs some tweaking of course.

Some other things I did makes me cringe, for example the sv_init change is less clear to me now. Also how node groups actually runs and how they copy data back and forth seems less than ideal.

ly29 commented 8 years ago

So we need to find out how our sockets can be converted into interface sockets...

ly29 commented 8 years ago

I guess we would have to create matching NodeSocketInterface classes like NodeSocketInterfaceVectorVelocity etc.

How does the matching mechanism for creating NodeSocketInterfaceInt from NodeSocketInt look?

zeffii commented 8 years ago

So we need to find out how our sockets can be converted into interface sockets...

this . and there's no documentation that i'm aware of. not even hints... just the C code if you can make sense of it.

Where to store the sv groups, I choose a specific node group tree type to mimic the cycles behavior where the nodes groups are hidden and keep the actual editing via the interface only.

yeah, not cluttering the node tree list with groups. Tho this wasn't a concern while exploring.


There's a good reason to ask LukasToenne about this, because we're getting close enough to "know what we don't know" :) and if there was a way to reuse some of the existing pynodes NodeSocket interface stuff that would make our life easier and anyone interested in similar.

I guess we would have to create matching NodeSocketInterface classes like NodeSocketInterfaceVectorVelocity etc. How does the matching mechanism for creating NodeSocketInterfaceInt from NodeSocketInt look?

I haven't spent a lot of time trying to figure this out, but blender github repo by dfelinto allows for convenient online searching

zeffii commented 8 years ago

I think even if the NodeCustomGroup can't be usefully subclassed that the concepts you offered in group.py can be extended to replicate all features

... just a bit of heartburn to have to re-implement everything.

ly29 commented 8 years ago

Yes, I vaguely remember that it might not be finished, however it is worth asking just keep it in the todo or work out a solution. And I might very well be wrong. I looked into the c code but couldn't quite find the magic.

zeffii commented 8 years ago

when ShaderNodeTree nodes are added to a Custom group, upon inspection like:

bpy.data.node_groups['NodeGroup'].inputs[0].rna_type.name
>>> 'Color Node Socket Interface'

but when I add a sv node / socket to a nodegroup input, it just says

>>> 'Node Socket Template'
ly29 commented 8 years ago

So the NodeSocketInterface* are the input and outputs for the NodeGroup, when we create the node group we could set them I guess.

ng.inputs.new("StringsSocketInterface", "x'")

It is starting to make sense now perhaps.

zeffii commented 8 years ago

Design Reference

lukas_t made his own node group for object_nodes: https://developer.blender.org/diffusion/B/browse/object_nodes/release/scripts/nodes/group_nodes.py

ly29 commented 8 years ago

nice to have a reference

zeffii commented 8 years ago

nice use of dynamic class's type() :)

ly29 commented 8 years ago

As a gist for easier viewing for me right now. https://gist.github.com/ly29/fc31153e9a0fecdd5703d0306d40bb31

Will take some time to wrap my head around that

ly29 commented 8 years ago

Now I think we have all the knowledge we need. It looks fairly clear now. Sure a lot of issues will crop up...

I will be away over the weekend.

zeffii commented 8 years ago

okidoki... i'm going to mess around with this..

image

zeffii commented 8 years ago

@ly29 copied you process function from the group node almost verbatim :)

image

ly29 commented 8 years ago

Very nice looking. That '''process''' is almost sane even though it kind of breaks the separation of where thing should be happening...

zeffii commented 8 years ago

I do like your approach to having the interface directly on the Input and Output nodes, instead of the Panel, but I wanted to see the least amount of code (not withstanding the fact that the process of In and Out are virtually the same except for self.inputs and self.outputs and using links[0].from_socket vs links[0].to_socket ) i felt it was too early to write dynamic code.. but maybe it makes sense to do exactly that already.

ly29 commented 8 years ago

The group inputs don't need process, but update because it only dealing with UI issues.

zeffii commented 8 years ago

Oh i see what you mean now, yeah process i'll move back into the parent Node.. it may even solve the red color of the nodes inside the group..

ly29 commented 8 years ago

They shouldn't be running while being edited I think. Mmm.

Also the issue of groups within groups. Sanity checks to avoid infinite recursion could be a good idea even though it should the max recursion depth pretty quickly in most cases...

ly29 commented 8 years ago

Having the editing directly on the node feels logical even if we have make it a bit bigger.

zeffii commented 8 years ago

maybe we can call these node_groups something unique to help disambiguate bpy.data_node_groups. -- i was thinking sub_groups or nested_groups. (altho the distinction may be unimportant to non scripters)

yeah a superset seems logical like non of the view nodes or wifi.

regarding the I/O nodes, we can either set a default width to show all operators nicely, or make the UI respond to width resizes and display only sockets if it's minimally wide and when resized it can display the move / delete operators. no haste with that :)

ly29 commented 8 years ago

Node groups is an bad name that I guess we are stuck with now. How about node functions? I always think about them as function and also brings across the side effects clearly. Wifi within a group could be fine, but better think about restricting them. Also as much I dislike wifi cross layout would be nice. However that has the side effect of imposing an order among different layouts. (For later anyway) But restrictions are more of usability issue which isn't urgent.

enzyme69 commented 8 years ago

How about Node Sverchok Sub-Network? Is this for "grouping of bunch of network to reuse as a single node"?


On 30 July 2016 at 19:07, ly29 notifications@github.com wrote:

Node groups is an bad name that I guess we are stuck with now. How about node functions? I always think about them as function and also brings across the side effects clearly. Wifi within a group could be fine, but better think about restricting them. Also as much I dislike wifi cross layout would be nice. However that has the side effect of imposing an order among different layouts. (For later anyway) But restrictions are more of usability issue which isn't urgent.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/nortikin/sverchok/issues/785#issuecomment-236354651, or mute the thread https://github.com/notifications/unsubscribe-auth/ADxQL4ej605xNcJSuzo5sjR4Asu211vgks5qaxQ8gaJpZM4JVSJQ .

zeffii commented 8 years ago

yeah anything but node_groups.

zeffii commented 8 years ago

Hi @enzyme69 I'd like to keep the names short to help us talk about them easier.

zeffii commented 8 years ago

there seems to be a problem with duplicate socket names,

ly29 commented 8 years ago

Use socket.identifier instead :)

ly29 commented 8 years ago

Or index

zeffii commented 8 years ago

what I should have said was:

while it is possible to connect multiple sockets with the same name to the group's inputs, they will be added just fine, but this will fail..

        for socket in self.inputs:
            if socket.is_linked:
                data = socket.sv_get(deepcopy=False)
                in_node.outputs[socket.name].sv_set(data)

so yeah we can use index there, probably makes sense.

but just like with functions we shouldn't really be allowing multiple default keyworded parameters to have the same name like def some_function(x=x, y=y, x=x):

ly29 commented 8 years ago

I agree, I think it is a somewhat strange design decision, but the names of sockets are more like labels than the name function arguments. For Sverchok I think there are other places it will fail if the socket names aren't unique

ly29 commented 8 years ago

Data lookup should work since it uses the socket.identifier so. It might break somewhere else but I think it should work.

zeffii commented 8 years ago

image

ly29 commented 8 years ago

Organizing the node groups and naming them is very useful. Also worth looking into the possibility of auto migrating old node groups to new ones.

zeffii commented 8 years ago

will need to learn about UIList :) I think this is exactly what we want.. time to park some links

ly29 commented 8 years ago

The reference document is quite good https://www.blender.org/api/blender_python_api_2_70_5/bpy.types.UIList.html

zeffii commented 8 years ago

We won't need both, some decision has to be made between the interface directly on the I/O nodes, or in the panel. I'm starting to lean even more towards directly on the node UI as with your original implementation.

zeffii commented 8 years ago

hmmm, but this doesn't seem so heavy : https://gist.github.com/zeffii/cf86ac59f954403ab81b81f428f84e3a#file-node_groups-py-L123-L179

the rest of the code does seem quite elaborate, like a tank.... (but heavy?... )

ly29 commented 8 years ago

I really like editing directly on the node, going to the panel feels a bit complicated, however it is the blender way...