GraphEdit/GraphNode Overhaul
#108099
GraphEdit/GraphNode Overhaul
#108099
Conversation
themes, layout, mouse interaction, connections, signals, etc
scene/gui/graph_edit.h
Outdated
| void move_connections(const Ref<GraphPort> p_from_port, const Ref<GraphPort> p_to_port); | ||
| bool is_connected(const Ref<GraphPort> p_first_port, const Ref<GraphPort> p_second_port); | ||
| int get_connection_count(const Ref<GraphPort> p_port); | ||
| GraphNode *get_connection_target(const Ref<GraphPort> p_port); |
There was a problem hiding this comment.
We should not assume that a GraphPort has a single target. For example in my plug-in that heavily relies on this contract, a data output port can be connected to N targets while only control flow output ports can be connected to a single target.
So I think this method, as it's designed, introduces nuanced behavior we should probably avoid. I would say if we truly wanted to keep this method, it should be changed to:
- Return multiple targets by default, that way it satisfies both cases I referred to above.
- It should return a collection of
GraphPort, not the nodes. The nodes can be inferred from the ports.
There was a problem hiding this comment.
The method get_connections_by_port already serves this purpose, returning a TypedArray of all GraphConnections on a given port. Target ports can be accessed from this by calling GraphConnection.get_other(), but I'll keep in mind that getting a collection of target ports all at once is a useful API.
You're right that this method should be changed, but currently, it's only used in GraphNode for keyboard navigation, so fixing this will wait until I rework the keyboard accessibility features of GraphNode and GraphPort.
There was a problem hiding this comment.
I will say other than this, I am really stoked at seeing these new APIs. We've had to build a lot of this as part of our own GraphNode implementations, so seeing this pulled up into the base implementation really helps to reduce a technical debt. And when you pair that with the idea of decoupling slot/index from ports and leaving that as an internal detail -- this is a huge improvement IMO; so definitely looking forward to this!
There was a problem hiding this comment.
Still need to rework keyboard nav, but I've added a new method get_connected_ports to GraphEdit, which returns an array of all ports connected to a given port. Should cover this use case!
…aphnode-revision # Conflicts: # editor/shader/visual_shader_editor_plugin.cpp # editor/themes/editor_theme_manager.cpp # scene/gui/graph_edit.cpp # scene/gui/graph_node.cpp # scene/gui/graph_node.h
visual shaders are almost back to working as intended!! :D
yo dawg i heard you like fixes, so we put fixes in your fixes so you can fix while you fix
this commit marks a tipping point: i've begun rewriting VNML to rely on this PR! VNML was the project that justified this change to begin with, and now, changes will be made in tandem with testing to speed up bug-squashing and feature/API improvements
new method for GraphEdit that returns all connections to ports that match a filter direction on a node. also comes with similar methods on GraphNode and GraphNodeIndexed
mostly ease of use/shortcut methods on GraphNode and GraphPort, as well as get_first_connection_by_port on GraphEdit to shortcut get_connections_by_port()[0]
this time, it's some ways to get all ports on a node that match a filter direction
Implements godotengine/godot-proposals#12207 - read the description there for more details.
As this is a substantial breaking change (and my first real contribution), I'm making this a draft PR to get early feedback and make progress more visible.
Notably, this implementation does not overhaul
VisualShaderorVisualShaderNode. I believe a future PR that overhauls the visual shader system in parallel would be a good idea, but for this PR, changes will be relegated toVisualShaderNodePluginand seek to maintain compatibility as much as possible.TODO:
so manyregressions and bugsGraphNodeIndexed