Make GraphNode connection slots independent from child nodes

nezvers commented 3 years ago

Describe the project you are working on

Color palette creation, generating gradients with curves for HSV and RGB. ColorPalettizer

Describe the problem or limitation you are having in your project

I really grow to hate working with GraphNode because of its requirements for having child count just to have connection slots. It is impacting the ease of GUI design and making it harder to have adjustable/dynamic slot counts. Also just to change properties for one side it needs to be adjusted also for the other side, set to its already existing settings. Child flags don't really work - horizontally it is forced fill and vertically fill and expand doesn't work (Reproduced with resizable GraphNode).

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Have separate input slots and separate output slots that are independent of the children count for the GraphNode. Have a separate property in GraphNode to set slot count for left size and right side or even make those slots as its own GUI class. Either way, it opens up options for visual configuration to align slots (all to the top/bottom, equal spread, center). That way it also will get out of way of GUI design of the GraphNode and making tools will become much easier.

Also looking at the existing API is it a connection or is it a slot or maybe port (looking at Theme properties)? Those connection ones are conceptually questionable methods and I could write a lot of questions about them and why they are there and not having the ability to change the connection's line color without re-connecting it. GraphNode I'm fighting with the ability to have a resizable connection slot count and that gives a problem with Nodes size and visible slots

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

To have it structured as if it already has:

HBoxContainer
    VboxContainer ->InputSlots
    PanelContainer or Container->ChildNodes
    VboxContainer ->OutputSlots

Method ideas:

void set_left_slot_count(count:int, alignment:Enum)
void set_left_slot( int idx, bool enable_left, int type_left, Color color_left, name:String)

The theme properties could have options to set size for impacting the spacing and alignment.

If this enhancement will not be used often, can it be worked around with a few lines of script?

It's not few lines and frankly, I've been contemplating making my own GraphNode but that would set me back in making the app.

Is there a reason why this should be core and not an add-on in the asset library?

GraphNode is a really powerful concept but has terrible usability outside the static design of visual scripting.

Jummit commented 3 years ago

Another solution would be to add a position parameter to set_port, but the signature is getting pretty long already.

maximkulkin commented 2 years ago

Any advances here? Can we get this moving? I really like idea of being able to specify left and right slots independently of each other and independently of child controls. Seems like nobody is working on it at the moment. If we agree on the implementation, I can work on it.

My proposal:

add left_slot_count and right_slot_count properties
add API to configure each left and right slot: set_left_slot(int idx, int type, Color color, int offset) (and same for right slots)
add setters to configure individual slot offsets: set_left_slot_offset(int idx, int offset) (and same for right slots)

Old "enabled" parameter is not relevant anymore as left and right slots are independent and it just does not make sense to disable a slot when left/right_slot_count can be decreased. Slots are positioned from bottom of graph node title according to offsets. Each offset specifies offset in pixels from previous slot and has reasonable (non-zero) default. If more sophisticated slot positioning is required, one can implement manual slot positioning by reacting to resize event.

YuriSizov commented 2 years ago

There is no progress here, neither work nor agreement that this is a positive change. While GraphEdit is exposed, its main function is to power our own visual tools, visual shaders and visual scripts. So changes to it need to be vetted with these maintainers first, and we'd need to agree that this is a manageable change for those parts of the engine.

As a personal note, this change would make it harder to make simple setups with graph nodes, because you'd have to manually control the position every time. In that sense, reliance on controls is more handy and easy to manage, but it comes at a cost of not being as versatile as some users may hope for their own tools.

A proper solution, as I see it somewhere down the road, is to have a base GraphNode class that would be totally manual, and then to have the current GraphNode class as its extension that automatically handles children. Kind of like we have TabBar and TabContainer that uses it and its own children to generate tabs without any manual user input.

But that's down the road. We have other pending work for GraphEdit that would directly conflict with a refactoring like that.

maximkulkin commented 2 years ago

Thank you Yuri for the update. Regarding making it harder for simple setups: I mentioned "reasonable defaults" for GraphNode slots, which should make this changes easy on simple setups. E.g. picking a default offset value which should space slots reasonably (e.g. 20 pixels apart), so user do not have to set them up manually.

Having a specialized subclasses also sounds like a good idea!

I understand that core devs are busy people and there are roadmaps and priorities, that's why I am offering help. I just need some kind of sign off on a proposal to start work.

m21-cerutti commented 8 months ago

I like the fact to have a specialised class like a SlotItem, even if it would not allow the versatility for all type of usages, and could lead to the same debate why we don't do MenuItem for example (mainly for performance reason, see https://github.com/godotengine/godot-proposals/issues/7987).

But it would be easier to deal with dynamic slots. We could add children that are not slots (like foldout section, description or other things), and respond to events like resize and show/hide dynamically would be out of the box with signals.

Maybe for the initial independency problem you could have with this system a hierarchy like

GraphNode
    @HBoxContainer -> TitleBox
        @Label -> Title
    VboxContainer -> For slots, user defined
         SlotItem (from HBoxContainer)
             @LeftSlot
             @Label (empty text by default)
             @RightSlot
    MarginContainer -> ChildNodesContainer, could calculate margin from SlotItem.get_chidren(true) etc
        ControlNode

And for the generic case :

GraphNode
    @HBoxContainer -> TitleBox
        @Label -> Title
    VboxContainer
        SlotItem
        ControlNode (Button to hide next slot for example)
        SlotItem

And we could have API to be less verbose for the set slot like

GraphNode:
get_slot_item(index:int) -> SlotItem
(add would be from Node API add_child or from the Editor, remove queue_free())

SlotItem:
set_slot_left(enable_port: bool, type: int, color: Color, custom_icon: Texture2D = null, draw_stylebox: bool = true)
set_slot_right(...)
get_slot_label_text() -> String
set_slot_label_text(text:String)

with of course a lot of GraphNode API (https://docs.godotengine.org/en/4.2/classes/class_graphnode.html) moved to SlotItem.

By redefining SlotItem, we can have custom slots, and by allowing management of slots by the user, we could have great capabilities for customisation.

However at the expense of performance. I think it would be minimal and we could have a caching mechanism if necessary (internally or user-side), but doubts remain about the implementation of get_slot_item and the mechanisms between GraphNode and SlotItem (it seems GraphNode would only handle a bunch of helpers with this change). Also open the door to drawing overlap for slots, but that would be explicit I suppose, like Node's hierarchy in general?

Like you say YuriSizov, would be down the road with maybe a GraphNode refactor with SlotItem, or even SlotContainer that would do the current behaviour to add slots on Control children (and hide SlotItem hierarchy with overlapping? Just an idea).

godotengine / godot-proposals