Implement GraphFrame and integrate it in VisualShader

[Geometror]

This PR introduces GraphFrame, a new GraphElement designed for organizing node graphs, and integrates it into the VisualShader editor (can be split if desired). It aims to reimplement the functionality that was lost when comment nodes were removed. While it works a bit differently (very similar to Blender) it should be a lot more predictable and much less problematic compared to the old implementation of comment nodes.

Finally closes godotengine/godot-proposals#5271.

https://github.com/godotengine/godot/assets/50084500/d81c7cf9-6b3d-4d44-b9ae-07f7af88dc43

Detailed changes:

• Add GraphFrame
  • A special graph element to which you can attach other graph elements (nodes and frames). Its rect automatically adapts to the attached elements (enclosing), if autoshrink is enabled. Only attached are moved together with the frame.
  • Can be tinted in an arbitrary color
  • GraphEdit automatically sorts the frames topologically (nested frames are supported)
  • GraphEdit automatically puts frames behind the connection layer
    • This requires to make the connection layer a non-internal child again (reintroduces #85005) since there is no better solution at the moment to keep it between background nodes, e.g. frames, and connectable nodes (just modifying the z_index of each node type would mess up the input and is just confusing)
• VisualShader integration
  • Remove VisualShaderNodeComment and replace it with VisualShaderNodeFrame
  • Right click context menu: Detach selected nodes from their "parent" frame
  • To attach nodes to a frame simply drag and drop them onto it.
  • Context menu option to enable/disable autoshrink and tint color (as well as choose the tint color)

API

Note for users of GraphEdit: Integrating GraphFrame in your application is a bit more complicated that the old comment nodes (where you could just check a property and it "worked" out of the box), especially for Undo-Redo, but in return it should work more reliable and be a lot more useful.

• GraphEdit
  • Methods
    • attach_graph_element_to_frame(element, frame) : Attach a node to a frame (params are string names)
    • detach_graph_element_from_frame(element) : Detach a node from its frame (param is string name)
    • get_frame(element) : Get the frame a node is attached to
  • Signals
    • frame_size_changed (frame, new_size) : Emitted when the size of a frame changes
    • nodes_linked_to_frame_request(elements, frame) : Emitted when nodes are dropped onto a frame
• GraphElement
  • Signals
    • resize_end : Emitted after releasing the mouse button after resizing the node
• GraphFrame
  • Properties
    • title : The title of the frame
    • title_centered : Whether the title is centered (convenience property, the titlebar/header can be fully customized the same way as with GraphNode)
    • autoshrink_enabled : Whether the frame should automatically shrink to fit its contents (attached nodes/frames)
    • autoshrink_margin : The margin to add to the frame size on each side when autoshrink is enabled
    • drag_margin : The margin around the frame where it can be dragged
    • tint_color_enabled : Whether the frame should be tinted in a color
    • tint_color : The color to tint the frame in
  • Methods
    • get_titlebar_hbox : Get the HBoxContainer of the titlebar (useful for customizing the titlebar)
  • Signals
    • autoshrink_toggled : Emitted when the autoshrink property is toggled
• VisualShader
  • Methods
    • attach_node_to_frame(node, frame) : Attach a node to a frame
    • detach_node_from_frame(node) : Detach a node from its frame

TODO:

• [x] Theme cache
• [x] Docs
• [x] Address remaining TODO comments

Production edit: closes godotengine/godot-roadmap#13

[Yagich]

Amazing work! Can't wait to try it out.

I have a few questions:

1. Does it support the resizable property of GraphElement? If so, how does it behave with auto-shrink?
2. You say that #85005 is reintroduced, does that mean that to get the actual slot children we have to slice the get_children() call? That's quite a workflow change, but if there's no way around it I can live with it 🙂

I'll give it a spin tomorrow!

[Geometror]

Rebased and addressed all review comments.

@Yagich

1. Yes, it should. You can test it with the VS expression node which is resizable.
2. I assume you mean the connectable nodes (GraphNodes) with "slot children". Then yes, you would need to check whether the children are of type GraphElement or GraphNode (depending on whether you want the frame nodes too). Just stripping the first child from the children array would only work if you don't have any GraphFrames in there. What we could do however, is expose a method get_connection_layer_index so that you could just use that instead of type checking (but then again type checking is a bit safer and not that expensive).

[Yagich]

I've tested the PR and overall it works great! I've only been able to find one issue: if you try to attach a frame to itself, the application will hang. It's not unfixable in user code, but I wonder if it should just be a no-op if element and frame are the same when calling attach_graph_element_to_frame().

[Geometror]

@Yagich Thanks for testing. I made both attach_graph_element_to_frame() and detach_graph_element_from_frame() more robust.

[Geometror]

Rebased and adjusted the style a bit after #85017 has been merged.

[akien-mga]

Tested briefly, I'm not familiar with Visual Shader so I'm just trying some random stuff.

I created a Visual Shader, attached to a Sprite2D (icon.svg).

Pressed "Add Node...", dragged and dropped ColorConstant to the graph, I get those two errors:

ERROR: Condition "!get_viewport()->gui_is_dragging()" is true.
   at: set_drag_preview (./scene/gui/control.cpp:1965)
ERROR: Graph element is not attached to any frame.
   at: detach_graph_element_from_frame (./scene/gui/graph_edit.cpp:2269)

The frame error gets repeated every time I do an action, e.g. toggling visibility of the ColorConstant, or connecting it to Output > Color.

I then found how to add a frame via right click/Add Node, which auto-shrunk to tiny. I had to right click it to disable auto-shrink, so I could extend it to encompass three nodes I had.

Then it seemed only the Output node within the frame was actually attached to the frame. It's not clear to me how to attach nodes to a frame. I did a bunch of random single or double clicking on nodes, moving them a bit within the frame, and they somehow ended up being attached, though there's visible feedback about it.

The end result works, but as a first time user this wasn't very intuitive.

The frame description also can easily overlap with nodes:

This was a really brief test on laptop with touchpad, so hard mode :P Hope it's useful anyway :)

---

BTW, this is likely not for this PR, but do you plan to add support for duplicating frames with their contents, and/or saving frames with their contents as a reusable component? I'm not familiar with Visual Shader but this sounds like it could be quite useful.

[BlueCube3310]

Bug report: Duplicating a frame with elements inside of it will cause the elements inside to be attached to both the original and the duplicate.

[Geometror]

@akien-mga @BlueCube3310 Thanks for testing! All the described (technical) issues should be solved now.

Regarding the usability problems: I will experiment a bit with a cursor hint (when dragging a unattached node over a frame), a hint text for empty frames and maybe a small visual indicator that a node is attached (although that might be too much and if not likely part of a subsequent PR due to limited time).

I'm thinking about removing the description feature and instead add a true "comment" node which is basically a dragable label with a full transparent background (which would behave the same way as a normal node inside a frame, so you could place it freely). IMO this would be much more intuitive.

BTW, this is likely not for this PR, but do you plan to add support for duplicating frames with their contents, and/or saving frames with their contents as a reusable component? I'm not familiar with Visual Shader but this sounds like it could be quite useful.

Yes, I plan to do something similar (https://github.com/godotengine/godot-proposals/issues/7730) Although that is completely separate from graph frames, which are purely for visual organization.

[Chaosus]

Pressed "Add Node...", dragged and dropped ColorConstant to the graph, I get those two errors:

ERROR: Condition "!get_viewport()->gui_is_dragging()" is true. at: set_drag_preview (./scene/gui/control.cpp:1965)

@akien-mga The first message is related to old bug: https://github.com/godotengine/godot/issues/59622, should be fixed by https://github.com/godotengine/godot/pull/67531

[Geometror]

Small update:

• Removed the description feature
• Refactoring on the visual shader side
• Added a hint for empty frames:

[KoBeWi]

• [x] You can't undo adding a frame and trying to delete one results in a crash:

CrashHandlerException: Program crashed
Engine version: Godot Engine v4.3.dev.custom_build (5d08c2631cadf29d80ca23c7d537e03c3e5edcc5)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[0] VisualShaderNode::is_deletable (C:\godot_source\scene\resources\visual_shader.cpp:352)
[1] VisualShaderNode::is_deletable (C:\godot_source\scene\resources\visual_shader.cpp:352)
[2] VisualShaderEditor::_delete_nodes_request (C:\godot_source\editor\plugins\visual_shader_editor_plugin.cpp:4346)
[3] call_with_variant_args_helper<VisualShaderEditor,TypedArray<StringName> const &,0> (C:\godot_source\core\variant\binder_common.h:304)
[4] call_with_variant_args<VisualShaderEditor,TypedArray<StringName> const &> (C:\godot_source\core\variant\binder_common.h:419)
[5] CallableCustomMethodPointer<VisualShaderEditor,TypedArray<StringName> const &>::call (C:\godot_source\core\object\callable_method_pointer.h:104)
[6] Callable::callp (C:\godot_source\core\variant\callable.cpp:58)
[7] Object::emit_signalp (C:\godot_source\core\object\object.cpp:1202)
[8] Node::emit_signalp (C:\godot_source\scene\main\node.cpp:3904)
[9] Object::emit_signal<TypedArray<StringName> > (C:\godot_source\core\object\object.h:933)
[10] GraphEdit::gui_input (C:\godot_source\scene\gui\graph_edit.cpp:1972)
[11] Control::_call_gui_input (C:\godot_source\scene\gui\control.cpp:1800)
[12] Viewport::_gui_input_event (C:\godot_source\scene\main\viewport.cpp:2178)
[13] Viewport::push_input (C:\godot_source\scene\main\viewport.cpp:3229)
[14] Window::_window_input (C:\godot_source\scene\main\window.cpp:1627)
[15] call_with_variant_args_helper<Window,Ref<InputEvent> const &,0> (C:\godot_source\core\variant\binder_common.h:304)
[16] call_with_variant_args<Window,Ref<InputEvent> const &> (C:\godot_source\core\variant\binder_common.h:419)
[17] CallableCustomMethodPointer<Window,Ref<InputEvent> const &>::call (C:\godot_source\core\object\callable_method_pointer.h:104)
[18] Callable::callp (C:\godot_source\core\variant\callable.cpp:58)
[19] Callable::call<Ref<InputEvent> > (C:\godot_source\core\variant\variant.h:864)
[20] DisplayServerWindows::_dispatch_input_event (C:\godot_source\platform\windows\display_server_windows.cpp:3450)
[21] DisplayServerWindows::_dispatch_input_events (C:\godot_source\platform\windows\display_server_windows.cpp:3420)
[22] Input::_parse_input_event_impl (C:\godot_source\core\input\input.cpp:773)
[23] Input::flush_buffered_events (C:\godot_source\core\input\input.cpp:1044)
[24] DisplayServerWindows::process_events (C:\godot_source\platform\windows\display_server_windows.cpp:2969)
[25] OS_Windows::run (C:\godot_source\platform\windows\os_windows.cpp:1476)
[26] widechar_main (C:\godot_source\platform\windows\godot_windows.cpp:181)
[27] _main (C:\godot_source\platform\windows\godot_windows.cpp:206)
[28] main (C:\godot_source\platform\windows\godot_windows.cpp:220)
[29] WinMain (C:\godot_source\platform\windows\godot_windows.cpp:234)
[30] __scrt_common_main_seh (D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
[31] <couldn't map PC to fn name>
-- END OF BACKTRACE --

EDIT: Sometimes right-clicking the frame produces the same crash.

• [ ] Title editing popup awkwardly appears near the cursor and it's super small: It would be nice if it appeared over the frame's title. Also the text should be selected automatically.

• [x] Operations on frame, e.g. toggling tint color, will unfocus the frame, so doing another right-click will open default VisualShader menu (the frame ignores right-clicks unless it's selected first).

EDIT: Also trying to change tint color will spawn color picker outside screen...

https://github.com/godotengine/godot/assets/2223172/9c0f81e1-9e9c-414a-90c7-27c1a1c2014f

• [x] Actually I can't even detach a node, because right-clicking it opens the create node menu.

https://github.com/godotengine/godot/assets/2223172/9f353d8c-a343-4ecd-8ef6-c0caa78b5487

• [x] IMO Set Tint Color option should be below Set Title. Also possibly it could be hidden if tint is disabled.

• [x] Toggling auto-shrink with nodes inside results in glitches.

https://github.com/godotengine/godot/assets/2223172/fcddcbf3-b704-4353-9b28-686fbffabeba

• [x] Duplicating a frame does not duplicate it with the contained nodes. Not sure if intended, but users will likely expect otherwise.

• [x] Frame's color updates only after the popup is closed. It would be nice if it updated in real-time, but it's fine like now too.

• [x] When you add GraphFrame node to your scene, autoshrink_margin can't be changed.

• [x] Also the GraphNodes inside a frame can't be moved nor resized, making autoshrink_enabled effectively useless. Though this is only inside editor, maybe the intended usage is different (especially considering it works in VisualShader).

• [x] Enabling resizable in GraphFrame has no effect on the node.

[Geometror]

Ok, I might have fixed the crashes, but please recheck since I had a hard time reproducing.

Title editing popup awkwardly appears near the cursor and it's super small:

This is basically the old code I just readded in this case. I have plans to do it more intuitive, but I'd rather do that in a subsequent PR. For now I just made the text auto-select on opening the popup.

Actually I can't even detach a node, because right-clicking it opens the create node menu.

That one I can't reproduce :/

Duplicating a frame does not duplicate it with the contained nodes. Not sure if intended, but users will likely expect otherwise.

This is intended and follows the behavior present in other software, e.g. Blender. The frame node is just a spezial node for visual organization - attached nodes aren't children.

Also the GraphNodes inside a frame can't be moved nor resized, making autoshrink_enabled effectively useless. Though this is only inside editor, maybe the intended usage is different (especially considering it works in VisualShader).

What was your setup? Sounds like you added GraphNodes as children to a GraphFrame, which is not how it works (due to layering issues it's not possible this way -> connection lines). Nodes have to be attached to a frame using a GraphEdit method.

Enabling resizable in GraphFrame has no effect on the node.

I'm not sure what to do with the resizable property. It makes not much sense for frames, unless we would remove the autoshrink prop and use it as a replacement. So: resizable=false -> auto shrink, resizable=true -> no auto shrink, free resizing with certain restrictions.

I haven't fixed the glitches yet (when changing autoshrink), but I'm working on it.

[KoBeWi]

The crashes are now fixed, but I still have the problem with detaching. I'm on Windows if that matters.

What was your setup? Sounds like you added GraphNodes as children to a GraphFrame, which is not how it works (due to layering issues it's not possible this way -> connection lines). Nodes have to be attached to a frame using a GraphEdit method.

Yep, I just used it wrongly.

I'm not sure what to do with the resizable property. It makes not much sense for frames, unless we would remove the autoshrink prop and use it as a replacement.

You can hide it at least, using _validate_property().

[KoBeWi]

Changing autoshrink_margin with a node attached to a frame will not automatically redraw the frame.

https://github.com/godotengine/godot/assets/2223172/285e1e71-0fca-48e0-95c9-eb0a9a3d65ee

[lyuma]

@Geometror would it be possible to resolve the conflict with extension_api_validation / squash?

(I believe the conflict was caused by an urgent bugfix with Node::replace_by in #89992)

[Geometror]

Rebased and fixed a small resizing issue. Initially, I wanted to split keep autoshrink always enabled (and split the free resize functionality from this PR), but I think it works well enough for now. Imo this is ready to merge :)

[akien-mga]

Thanks!

[lyuma]

Remove VisualShaderNodeComment and replace it with VisualShaderNodeFrame

@Geometror Removing this part of our API is causing problems with some GDExtension plugins: it wasn't marked experimental, so this breaks all rust plugins since they hard-link to the whole API. See #90303

I think the easiest way to solve this is to add a stub class VisualShaderNodeComment : public VisualShaderNodeFrame so it will work just like the new VisualShaderNodeFrame. If there is a way to register the GDCLASS without it showing up in the resource picker, that would be best.

Even though it extends from VisualShaderNodeFrame, it might also be necessary to implement bind_compatibility_methods() for compat versions of set/get_title and set/get_description that call the real versions of those methods.

[Geometror]

@lyuma That's a good idea to derive from VisualShaderNodeFrame! I just opened #90797.