Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Node groups instead of monads #3319

Closed
Durman opened this issue Jun 5, 2020 · 59 comments · Fixed by #4152
Closed

Node groups instead of monads #3319

Durman opened this issue Jun 5, 2020 · 59 comments · Fixed by #4152
Assignees
Labels
core Architectural problems Proposal 💡 Would be nice to have
Milestone

Comments

@Durman
Copy link
Collaborator

Durman commented Jun 5, 2020

Monads are very complex and difficult to support. They does not use dedicated for creating node groups Blender tools. Last Sorcar updates proves that this tools can be used.

2020-06-05_08-31-13

Here is the code of node group node.
And here is the code of creating node group operator. It is nearly all code I guess which is needed for handling node groups.
How had seen the code of monads know that this code is much simpler.

What I don't like in current implementation in Sorcar is that sockets in node group node do not have parameters. They should be plugged to other nodes for managing node group node.

@aachman98 Did not you try to use this class NodeSocketInterface for creating sockets of node group trees? Probably it could solve the problem.

I Had tried it but faced with Blender crash. Here is bug report but it has low priority and probably will be fixing for years.

@Durman Durman added the Proposal 💡 Would be nice to have label Jun 5, 2020
@zeffii
Copy link
Collaborator

zeffii commented Jun 5, 2020

i welcome a simpler implementation of node groups in Sverchok. It must be possible.

@aachman98
Copy link
Contributor

aachman98 commented Jun 5, 2020

--EDIT 2--

Here is the code of node group node.
And here is the code of creating node group operator. It is nearly all code I guess which is needed for handling node groups.

The current method is incomplete, and is somewhat unstable (frankly the whole of Sorcar is, at the moment xD). It relies on copying selected nodes to the new group and making the connections with the default NodeGroupInput & NodeGroupOutput classes.

What I don't like in current implementation in Sorcar is that sockets in node group node do not have parameters. They should be plugged to other nodes for managing node group node.

Will be improving this soon, now that NodeSocketInterface derived custom classes are working. Each interface class will have its own property to which the values would be copied before evaluating the nodetree. Those properties can be drawn in the layout of "Node Group" node inputs.

@aachman98 Did not you try to use this class NodeSocketInterface for creating sockets of node group trees? Probably it could solve the problem.

Yes, I am in the process of implementing it. Earlier I was searching for a way to link the socket classes to the node interface. So I took another shot at the interface base class and, found out that it was the other way around. The interface class needs to register with the socket type.
image
Thanks!

@Durman
Copy link
Collaborator Author

Durman commented Jun 6, 2020

Yes, next code looks working, but how to draw properties on a node group node still is the issue.

class ScNodeSocketInterfaceNumber(bpy.types.NodeSocketInterfaceStandard):
    bl_idname = "ScNodeSocketInterfaceNumber"
    bl_socket_idname = "ScNodeSocketNumber"
    bl_label = "Number"
    color = (0.0, 1.0, 0.0, 1.0)

    default_value: FloatProperty()
    default_type: StringProperty(default="NUMBER")

    def draw_color(self, context):
        return self.color

    def draw(self, context, layout):
        layout.prop(self, 'default_value')

2020-06-06_11-46-28

@aachman98
Copy link
Contributor

Created a new property of same type as default_defualt in the socket class. Now it can be drawn and used to update nodetree. You can also draw default_value, but in case of Sorcar, it doesn't have an update function.
image
image

One problem with this is that the prop data is fixed. For example, you can't default change value, min/max, soft_min/max, etc unless using standard sockets. Also, the integer & float types have different bpy.props but are bundled as one (ScNodeSocketNumber class).

@Durman
Copy link
Collaborator Author

Durman commented Jun 6, 2020

The problem is which property should be shown by socket. I guess that in Sorcar as in Sverchok a socket shows property of a node where it was initialized. This property can't be used in case of node groups because several node groups can have different parameters for one socket.

The solution can be to reconsider using properties. So all default values should be kept in socket classes. But this can increase number of sockets types and in case of Sverchok it is quite significant changes which will touch very many parts of code.

@aachman98 Thanks for help. I will think probably simpler solution exists.
In your case you can keep two properties in Number socket and bool switch between them.

@zeffii
Copy link
Collaborator

zeffii commented Jun 6, 2020

much of the complexity in ScriptNode and Monads came from the inability to dynamically set the min/max of Float and IntProperties.

@aachman98
Copy link
Contributor

The problem is which property should be shown by socket.

I added an extra property in all socket classes which is of the same type as default_prop:
image

Then, that property can be drawn in the special case of node groups:
image

@aachman98
Copy link
Contributor

much of the complexity in ScriptNode and Monads came from the inability to dynamically set the min/max of Float and IntProperties.

Would it help if you simply changed the parent class of all the Sverchok sockets to ScNodeSocket...? They already have dynamic min/max functionalities along with default values.

@Durman
Copy link
Collaborator Author

Durman commented Jun 6, 2020

In Sverchok we have String socket which can be float array, int array, edges array, polygons array and how know what else it can be because it some sort of generic socket type and in this case it is problematic to determine which default property should be used.

@zeffii
Copy link
Collaborator

zeffii commented Jun 6, 2020

using set/get is not ui friendly, we do however use those in A Number and a few other places. This is a problem that could be resolved relatively easily by blender devs.

@zeffii
Copy link
Collaborator

zeffii commented Jun 6, 2020

ideally, if my socket has a limit, i don't want to let the user think that when they slide beyond that value that it will stick. they only realize after letting go of the slider than it jumped back to the value allowed by set/get . i consider it a kludge. but we could do that. I am not in favour.

@zeffii
Copy link
Collaborator

zeffii commented Jun 6, 2020

but a special socket-class that can mimic all socket types, just for the Group Node, is a possibility i do like. it could ofcourse have dynamic min/max using set/get

@Durman
Copy link
Collaborator Author

Durman commented Jun 6, 2020

It impossible because sockets are created automatically by Blender and their type is determined by type of sockets which was connected to group input node.

@zeffii
Copy link
Collaborator

zeffii commented Jun 6, 2020

historic context. #785 :)

@Durman
Copy link
Collaborator Author

Durman commented Jun 6, 2020

Reroutes can't be connected to group input node. It is very good.

reroutes in groups

@aachman98
Copy link
Contributor

@zeffii i don't want to let the user think that when they slide beyond that value that it will stick

I get your point. Somehow, standard node sockets manage to set the slider limits, without letting users to go past them. Just tried it on a NodeSocketFloat in a material node group:
image
image
image

@zeffii
Copy link
Collaborator

zeffii commented Jun 6, 2020

pynodes is a cool abstraction, but in practice when we try to push the boundaries these issues are all undocumented (and in some parts incompletely wrapped ) and therefore a massive time sink.

@aachman98
Copy link
Contributor

I already have a half mind to replace all custom sockets in Sorcar with standard sockets ;)
Have you seen this: https://github.com/wonderworks-software/PyFlow ?
Wish they could integrate all of this in Blender someday... It's like visual scripting of UE4, Houdini and Blender combined!

@zeffii
Copy link
Collaborator

zeffii commented Jun 6, 2020

in some respects a lot of non-sense would be solved if we would fully define our own nodetree UI external to Blender maybe imGUI. I have been tempted on several occasions, but lack incentive.

@Durman
Copy link
Collaborator Author

Durman commented Jun 6, 2020

It is also interesting (node based) project: https://www.luna-lang.org/

@zeffii
Copy link
Collaborator

zeffii commented Jun 6, 2020

pyflow is really cool :)

@zeffii
Copy link
Collaborator

zeffii commented Jun 6, 2020

also yes!! luna looks spectacular.

@aachman98
Copy link
Contributor

Created all the interface classes with default properties. Working for now...
Screenshot from 2020-06-06 23-09-04
No problems with nested groups too. Sorcar uses object references (from scene) so there are some cases where I will have to restrict selection, but Sverchok should be fine. Updating the code asap.

Thanks for all the help!

@Durman
Copy link
Collaborator Author

Durman commented Jun 7, 2020

With properties it looks much cleaner, my congratulations.

If to think of it is strange how in Sverchok properties are bound to sockets.
A property belongs to nodes. But during sv_get() method we are calling socket from node and socket calling node back. It looks like that properties should belong to sockets not to nodes. But too much done.

@aachman98
Copy link
Contributor

aachman98 commented Jun 7, 2020

This shouldn't be a problem when creating node groups, as the link class contains from/to_socket as well as from/to_node data members. You can make the node interfaces in such a way that they link to the node rather than the sockets created in the "node group" node. It would look like a bypass where the output sockets would serve no purpose but to call sv_get() on the "group output" node. The "group input" node will call the same function on the inputs of the node actually calling the group.

EDIT: Just realised that it might look confusing at first. I'll try to make a small demo node to visualize this. Also, I have almost no experience in developing Sverchok nodes or know how they function internally, so it is possible that what I propose wouldn't work.

@aachman98
Copy link
Contributor

aachman98 commented Jul 5, 2020

image
Would probably need to add exception to sockets in case of "Group Input/Output" node. The nodes will get/set data in the same manner (sv_get & sv_set), but the "Node Group" node will need to call the sv_set of the sub-group (custom "Group Input" node, to be precise)
How many types of sockets are there in Sverchok?

EDIT:
There seems to be some code for UI already present for the group input/output type nodes:
image
And the sockets do draw correctly when not connected:
image

@zeffii
Copy link
Collaborator

zeffii commented Jul 5, 2020

@aachman98 this code is present for all nodes. , we could exclude if for nodes of type Group Input.

bpy.types.NODE_PT_active_node_generic.append(idname_draw)

def idname_draw(self, context):
    if not displaying_sverchok_nodes(context):
        return
    layout = self.layout
    node = context.active_node
    ntree = node.id_data
    if not node:
        return
    if node.bl_idname in {'NodeGroupInput', 'NodeGroupOutput'}:
        return
    ...

@Durman
Copy link
Collaborator Author

Durman commented Jul 6, 2020

All type of sockets can be found here.

sverchok/core/sockets.py

Lines 767 to 772 in a0231bd

classes = [
SvVerticesSocket, SvMatrixSocket, SvStringsSocket, SvFilePathSocket,
SvColorSocket, SvQuaternionSocket, SvDummySocket, SvSeparatorSocket,
SvTextSocket, SvObjectSocket, SvDictionarySocket, SvChameleonSocket,
SvSurfaceSocket, SvCurveSocket, SvScalarFieldSocket, SvVectorFieldSocket
]

For using node groups sockets should be fixed but this will be minor changes I guess. It looks like that main problem is fixing update system.

Current implementation could be split into two parts now. First is aware of evaluating main tree and second is responsible for evaluating the monads. I think such splitting should lead to redundant code. I would prefer to see update system which could handle any tree. In this case it would be possible to evaluate all net recursively.

@Durman
Copy link
Collaborator Author

Durman commented Sep 17, 2020

We have a chance to got official documentation about creating node groups.
https://developer.blender.org/T80827

@Durman
Copy link
Collaborator Author

Durman commented Oct 21, 2020

There is problem which I still not sure how to solve. The only role of update system at the moment is to call process method of outdated nodes in appropriate order. Nodes themselves get information from previous nodes. But the same group trees can be used several times and in this case process method of the same node can be called several times and socket.sv_set method just will erase preivous result.

I see two ways of solving the problem. It is possible to modify sv_get and sv_set methods so they take into account context of processed node. But the problem is that context is known only to update system. Node itself can't know to which group node its belong at the moment of processing.
Second solution could be that update system should give input data and store output data. Here problem is how to pass this data into process method.

@portnov
Copy link
Collaborator

portnov commented Oct 21, 2020

As far as I remember, we store contents of outputs in a dictionary, something like outputs[node_name][output_name]. We could modify this to store it like outputs[node_path][output_name], where node_path is a path to the node in the tree of node groups, like Group1/NestedGroup/NodeName.

@Durman
Copy link
Collaborator Author

Durman commented Oct 21, 2020

Probably then a node could has update_path (update_context?) property which should be changed before calling process of the node.

for node in outdated_nodes:
    node.update_path = base_group_node.node_id  # it looks enough to distinguish the context
    node.update()

Socket:
    def sv_get(socket):
        if socket.node.id_data.bl_idname == 'SvGroupTree'
            return sockets_data_dictionary[socket.other.node.update_path][socket.other.socket_id]

@portnov
Copy link
Collaborator

portnov commented Oct 21, 2020

It is possible too. But I was thinking about calculating the path to the node at the moment we write into sockets_data_dictionary. Not sure if it will take much time.

@Durman
Copy link
Collaborator Author

Durman commented Oct 21, 2020

How the path can be calculated at that place?

@portnov
Copy link
Collaborator

portnov commented Oct 21, 2020

don't we have a reference to the node there? Node has a reference to it's tree. Node group tree should have a reference to it's parent, I think...

@Durman
Copy link
Collaborator Author

Durman commented Oct 21, 2020

A node tree can have multiple parents (base group nodes) but the tree does not know from which parent, update system is calling a node.

@vicdoval
Copy link
Collaborator

I think that has been the main unsolved bug of the monads, if you find a way to fix it probably it could also applied to them

@Durman
Copy link
Collaborator Author

Durman commented Oct 22, 2020

I think if node groups will work we won't need to have monads.

@vicdoval
Copy link
Collaborator

I agree, but they will need "vectorize" and "loop" to replace them

@Durman
Copy link
Collaborator Author

Durman commented Oct 22, 2020

Goal of group nodes is to hide part of a tree under their interface. So tree could be splitted into simple parts and more complex trees could be builded. Loops is different conception which has nothing in common with node groups.

@vicdoval
Copy link
Collaborator

So you plan to add some other nodes to handle looping?
And what about the 'vectorize' i find both features super useful

@Durman
Copy link
Collaborator Author

Durman commented Oct 22, 2020

If I will add node groups I would like to delete monads. After that I could make refactoring of json importer again because monads are builded in that way that I had to use recursion during importing. Also after deleting monads refactoring of update system will be also doable. No other plans at the moment.

@portnov
Copy link
Collaborator

portnov commented Oct 22, 2020

We could think of several special "flow control" nodes... for example...

  • new socket type — "control". Most usual nodes do not have such sockets by default.
  • new node "Loop", with inputs: "Control", "LoopsCount"
  • special operator, that allows you to connect other (controlled) node to Control input of Loop node. Like select these two nodes and press a key (or a button in the N panel). This works only if controlled node has enough outputs to be "looped" (for example, if it has Verts/Edges/Faces inputs and Verts/Edges/Faces outputs), otherwise an error is shown. When this key is pressed,
    • matching inputs and outputs of controlled node are automatically hidden
    • new output socket "Control" is automatically added to the controlled node
    • this new "Control" output is connected to Control input of the "Loop" node.
    • In the Loop node, new input and output sockets are created — the same as were hidden from the controlled node.
  • then the user connects inputs / outputs of this Loop node to others, like he would work with controlled node.
  • process method of the Loop node is something like
controlled = self.inputs['Control'].linked_node
# put values from inputs of the Loop node to inputs of controlled node
for i in range(n):
    controlled.process()
    # put values from outputs of controlled node to it's inputs
# put values from outputs of controlled node to outputs of the Loop node

@Durman
Copy link
Collaborator Author

Durman commented Oct 22, 2020

I was thinking about something like this. In this examples sphere is extruded by 10 random selected masks and masks are creating with taking in account previous extrusions. And keep in mind that it is possible to create nested loops.
2020-10-22_20-00-34

@portnov
Copy link
Collaborator

portnov commented Oct 22, 2020

Not quite clear, how Input and Output Loop nodes will know they are paired? And where looping logic itself will live?

@Durman
Copy link
Collaborator Author

Durman commented Oct 23, 2020

It is possible to add property input or output nodes with name of another node or they can be connected via special sockets to each other. Logic of evaluation such things should be in update system or at least it should be aware of it and be able to pass responsibility of tree evaluation to those nodes.

@Durman
Copy link
Collaborator Author

Durman commented Oct 28, 2020

Node groups Hello world

circular array

@Durman
Copy link
Collaborator Author

Durman commented Nov 1, 2020

Monads are able to trigger update of several trees I think node groups should do the same.

update several trees

@Durman
Copy link
Collaborator Author

Durman commented Nov 7, 2020

Group nodes can become a tool of prototyping new nodes or probably can be presented in nodes menu by themselves.
Random color approach.
random color2
2020-11-07_23-33-27

@Durman
Copy link
Collaborator Author

Durman commented Nov 25, 2020

As I already was saying current sv_get and sv_set was not design with taking into account using group nodes (subtrees). It is possible to add context execution information to nodes (make them work properly) but such solution will be really fragile. It can work as temporal solution for now. In future I think those method should leave node object and join update system which possesses actual information how to use them properly. It shoul be really huge change. I'm actually not enthusiastic about fixing 600 nodes.

@nortikin
Copy link
Owner

As I already was saying current sv_get and sv_set was not design with taking into account using group nodes (subtrees). It is possible to add context execution information to nodes (make them work properly) but such solution will be really fragile. It can work as temporal solution for now. In future I think those method should leave node object and join update system which possesses actual information how to use them properly. It shoul be really huge change. I'm actually not enthusiastic about fixing 600 nodes.

What exactly it should be?
are there lines of code? Maybe i will have some time in december to go through all nodes. and maybe not olny with your issue.

@Durman
Copy link
Collaborator Author

Durman commented Nov 25, 2020

Proper moment to fixing what I'm talking about is somewhere far in the future. First things first. And we should have some plan first for such architectural changes so we could do this once and not return to the issue again. Probably we could create separate issue to discuss who and which changes would like to see.

@nortikin
Copy link
Owner

i'd like to handle messive changes if there are so.

@Durman Durman mentioned this issue Dec 1, 2020
1 task
@Durman Durman added this to the core milestone Mar 13, 2021
@Durman Durman mentioned this issue Jun 8, 2021
1 task
@Durman Durman added the core Architectural problems label Jul 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Architectural problems Proposal 💡 Would be nice to have
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants