GraphEdit/GraphNode refactor [V2] #67152

Geometror · 2022-10-09T19:15:24Z

This PR implements godotengine/godot-proposals#5271 for the most part.
Supersedes #61414 and #61783.

Detailed changes

Restructuring/Code style

Split GraphNode into BaseGraphNode, GraphNode and GraphFrame (open for naming suggestions) due to:
- Maintainability: The GraphNode class got really large and complex, so splitting it up improves code navigation and readability
- Flexibility: Allows adding more features to the frame graph node or extend BaseGraphNode for a completely custom node without most of the predefined connection functionality (reference image backdrops, etc.)
- Performance/implementation complexity: For example, a comment node does not need to keep track of any connections/slots/ports
Rename lots of short variable names to be more descriptive (e.g. icofs -> icon_offset, c -> child)
Restructure the header and source files to follow a consistent scheme (i. a. order of member variable and method declarations, which were sometimes mixed)
Fix comment style in various places
GraphNode: Rename get_connection_input/output_* to get_port_input/output_*
GraphEdit: Rename get_zoom_hbox to get_menu_hbox
Add closable member variable to VisualShaderNode, before that show_close was used to save whether a GraphNode can be deleted (which is a bit hacky to begin with and show_close is gone)

Functional changes

GraphFrame
- Tint color
  - Added context menu options to VisualShader editor
- GraphFrame can be grabbed and on all sides, not just on the titlebar
  - drag_margin property
- title_centered property (for convenience, can be achieved via get_titlebar_hbox().get_child(0).horizontal_alignment = HORIZONTAL_ALIGNMENT_CENTER)
The order of frame nodes is now calculated similar to a 2D BVH: when a comment node encloses other comment nodes, the index of the enclosing node is kept below (subject to change, see "future work" below)
- The order is updated when resizing a comment node
The connections layer is now kept between the comment nodes and the normal graph
Removed the close button (and its theming) from GraphNode by default (can be added manually if needed or by default with the new titlebar if we really want to keep it)
Complete control over the titlebar of GraphFrame/GraphNode, e.g. custom controls (like a close or settings button, some status label) can be added to GraphFrame's/GraphNode's title bar via get_titlebar_hbox().add_child(...)
- by default the titlebar hbox now holds a Label control

Theming

StyleBox for body (slot area) and title bar (both with selected variants)
Use "GraphNodeTitleLabel" and "GraphFrameTitleLabel" theme variations for title labels, remove the title offset theme constants
Adjustments to the editor theme and default project theme

Fixes

Fix custom port icon not being centered when it's bigger than the default icon
VisualShader: Fix error label increasing the minimum size in an ugly way since it autowrap isn't enabled
Fix and improve documentation with regard to port/connection/slot terminology

Future work

In my view, there are still some things left to do, but I wanted to get this out now to receive some feedback.
These are some of the things that I will work on next or in the future, that might require some discussion (and more time). These may go in their own PRs or if desired/necessary, in this one.

Change the GraphFrame behavior, as it currently has some problems and isn't really nice to work with in some cases (also requires complex update logic under the hood)
- change it to work similar to Blender, where you can drop nodes in the frame (only then they are linked with the frame and move with it/extend its size, not just when you move the frame below them). Maybe actually utilize the scene tree for proper parent-child-relationship
- add an option to shrink the GraphFrame based on its enclosing nodes
Probably introduced with the change above, a layer index property for BaseGraphNode to improve/flesh out the concept of layers
Evaluate the changes in this PR, gather feedback to improve upon
Move the arranging logic out of GraphEdit in its own class (already over 500 LOC and it may get more complex)
(?) maybe move the whole graph stuff in a module
Provide the ability to minimize nodes
Find ways to reduce some code duplicates (mostly in GraphEdit)
Reevaluate the slot Stylebox

nyabinary · 2022-10-14T01:46:23Z

Why did the Linux build fail?

fire · 2022-10-20T14:20:28Z

I support this, and the work was started before the feature freeze.

QueenOfSquiggles · 2022-10-20T14:26:41Z

I'm super excited about these features. It'll make my development of Choreographer 1000% easier. Great job @Geometror ! Hoping these can get accepted & merged

nyabinary · 2022-10-20T16:50:22Z

I support this as well.

paddy-exe · 2022-10-22T10:48:07Z

About the GraphFrame behaviour: IMO Blender has this done really nicely where it automatically detects when you put in the nodes inside the frame and resized itself to fit in the node after the drag finishes. The other points like e.g. the minimizing nodes feature would be a nice to have for larger graphs or subgraphs but not sure how much work this would be to implement. Would these changes be compatibility breaking or could they still be implemented when your rework PR is merged?
Also, again thank you for your work on this! It's really great how the changes are making the graph node system even more useful!

joaopedrosgs · 2023-01-23T16:49:08Z

Any news on this?

fire · 2023-01-23T18:31:12Z

I would like this but Godot Engine 4.0 tasks have taken priority.

YuriSizov · 2023-01-24T13:48:17Z

This PR, while it has some breaking changes, will be a part of 4.1. We'll see if we can smooth out the experience for existing users, though this node is unlikely to be used in games. It's more of a tool for editor plugins and standalone tools. This is to say that in the worst case scenario, developers should still be capable of updating their tools to the new setup.

Zireael07 · 2023-01-24T14:17:11Z

I will chime in (tried using existing GraphEdit/Node for a tool) - this refactor is really needed

paddy-exe · 2023-04-08T23:34:56Z

When trying out this PR (again great work btw :D) I noticed that the Comment node is now called Frame. This change should also be applied to the Submenu Set Comment Title, Set Comment Description:

YuriSizov

I'm starting to look at the code, but have a few general notes.

GraphFrame is barely usable, and you also mention issues with it in your latest comments. My suggestion is to just drop it for now, remove that feature completely for the initial implementation. Its UX is not there yet, and it distracts you with layering issues. So let's get back to it in a follow-up.
- Some fallback code needs to be added for visual shaders though, so that comments information is preserved, even if you can't see or edit it.
I'm concerned about the naming scheme. IMO GraphControl doesn't make sense as the base type for GraphNode, since in Godot's terms Node is more basic than Control. So, I think it would be confusing with the concepts reversed. Perhaps GraphObject or GraphItem would be better as the base type?
I'd urge you to not add more features to this PR. As I mentioned in the proposal before, this should be really done in steps. Some things are easy to add alongside the general refactoring, but the more changes this includes, the harder it is to get to a mergeable state and review. I'd like to have the initial refactoring merged within the next two weeks, so we can have it in 4.1.

There are also a couple of review comments. I'll look into the implementation soon.

YuriSizov · 2023-05-17T12:16:48Z

scene/resources/default_theme/grid_toggle.svg

This SVG is missing width and height attributes alongside its viewBox.

Added it, but most of the icons in the default_theme folder don't have these attributes.

They should, otherwise they may not be rendered correctly.

YuriSizov · 2023-05-17T12:35:43Z

scene/gui/graph_frame.cpp

+	ClassDB::bind_method(D_METHOD("get_tint_color"), &GraphFrame::get_tint_color);
+
+	ADD_PROPERTY(PropertyInfo(Variant::STRING, "title"), "set_title", "get_title");
+	ADD_PROPERTY(PropertyInfo(Variant::BOOL, "title_centered"), "set_title_centered", "is_title_centered");


I think we can move the title string to the base type, because even if some custom node doesn't render the title in the conventional way, or at all, having one is still handy most of the time, so there is little reason for duplication.

title_centered should be removed, IMO. I'm not sure if there is a demand for it, and since we're refactoring everything, it's better to drop it and reintroduce later if there is an extra need for convenience.

YuriSizov · 2023-05-17T12:50:59Z

doc/classes/GraphNode.xml

@@ -21,145 +21,138 @@
 		</method>
 		<method name="clear_slot">
 			<return type="void" />
-			<param index="0" name="slot_index" type="int" />
+			<param index="0" name="slot_idx" type="int" />


IMO parameters should be named more verbosely, and this was already the case here. Changing it feels like going backwards. The port one renamed below should also be expanded.

Overall you aim to improve the naming and make it more explicit, and here it's the opposite for some reason.

YuriSizov

A bunch of comments on the API surface and on the changes in editor plugins.

YuriSizov · 2023-05-17T13:27:00Z

doc/classes/GraphEdit.xml

 		</member>
-		<member name="use_snap" type="bool" setter="set_use_snap" getter="is_using_snap" default="true">
+		<member name="use_snap" type="bool" setter="set_use_grid_snap" getter="is_using_grid_snap" default="false">


Why rename the methods but not the property? I think that a better name for both would be snapping_enabled, without any mention of the grid (since it's not related to the visual grid anymore).

doc/classes/GraphEdit.xml

doc/classes/GraphNode.xml

YuriSizov · 2023-05-17T14:12:03Z

editor/plugins/visual_shader_editor_plugin.cpp

+	// All nodes are closable except the output node.
+	if (p_id >= 2) {
+		vsnode->set_closable(true);
+		node->connect("close_request", callable_mp(editor, &VisualShaderEditor::_delete_node_request).bind(p_type, p_id), CONNECT_DEFERRED);


Does this work? You removed the button, and this signal is emitted by GraphControl::_close_requested(), but I don't see _close_requested being called anywhere? delete_node_request is handled on the graph level, so shortcuts should still work. But this seems to be no-op.

YuriSizov · 2023-05-17T14:13:11Z

editor/plugins/visual_shader_editor_plugin.cpp

@@ -466,19 +488,35 @@ void VisualShaderGraphPlugin::add_node(VisualShader::Type p_type, int p_id, bool
 	}

 	node->set_position_offset(visual_shader->get_node_position(p_type, p_id));
-	node->set_title(vsnode->get_caption());


We can keep the title if you follow the suggestion to put the method onto the base class.

YuriSizov · 2023-05-17T14:15:20Z

editor/plugins/visual_shader_editor_plugin.cpp

-					}
-					node->set_slot(i + port_offset, valid_left, port_left, type_color[port_left], true, VisualShaderNode::PORT_TYPE_SCALAR, vector_expanded_color[0]);
-					port_offset++;
+			graph_node->set_slot(idx, valid_left, port_left, type_color[port_left], valid_right, port_right, type_color[port_right]);


Don't we need a type check here in case it's not GraphNode?

editor/plugins/visual_shader_editor_plugin.cpp

YuriSizov

A few more notes from the main classes. So far I've ignored everything related to frames/comments, and I haven't looked through the updated GraphNode.

scene/gui/graph_control.cpp

YuriSizov · 2023-05-17T17:08:10Z

scene/gui/graph_control.cpp

+void GraphControl::_close_requested() {
+	// Send focus to parent.
+	get_parent_control()->grab_focus();
+	emit_signal(SNAME("close_request"));
+}


As mentioned before, this doesn't seem to be called anywhere.

YuriSizov · 2023-05-17T17:19:30Z

scene/gui/graph_edit.cpp

@@ -270,78 +273,78 @@ void GraphEdit::_update_scroll_offset() {
 	set_block_minimum_size_adjust(true);

 	for (int i = 0; i < get_child_count(); i++) {
-		GraphNode *gn = Object::cast_to<GraphNode>(get_child(i));
-		if (!gn) {
+		GraphControl *graph_node = Object::cast_to<GraphControl>(get_child(i));


The variable name is confusing as it refers to a different type.

scene/gui/graph_edit.cpp

YuriSizov · 2023-05-17T17:21:30Z

scene/gui/graph_edit.cpp

 	for (int i = 0; i < get_child_count(); i++) {
-		GraphNode *gn = Object::cast_to<GraphNode>(get_child(i));
-		if (!gn) {
+		GraphControl *graph_node = Object::cast_to<GraphControl>(get_child(i));


Same issue with the variable name. Probably a bunch of others around all the touched files.

scene/gui/graph_edit.cpp

scene/gui/graph_control.cpp

scene/gui/graph_control.h

YuriSizov · 2023-05-17T18:00:09Z

So, my overall impression so far is that we should split this into a few PRs (or at least a few commits, but with dedicated PRs we can merge them in steps, and not just review them easier).

First, we clean up GraphEdit, refactoring the code and renaming the methods and properties. Since at this point we still have the old GraphNode code, methods that deal with frames/comments should be adjusted compared to the current state of this PR. General code improvements should still be made, but the relevant logic would remain more or less the same. Due to renames, some this commit would touch animation and shader files as well.
- You can include snapping/grid and arranger improvements here as well.
Second, we do a clean up on GraphNode, similar to what we did to GraphEdit. This can be done alongside the first step, if desired, but the key point is that GraphNode remains structurally the same, no base class, no frame/comment code extraction.
- Here you would also remove the outdated code related to visual scripts.
Last, we do the overhaul of nodes, split GraphNode into two, and remove everything related to frames/comments to be added with a future PR.

I believe, that this way we can review, merge, and iterate quicker, as right now this PR contains A LOT of changes which are codestyle/cosmetic. And for the most part those changes seem independent from logical changes, so factoring them out into dedicated PR doesn't appear challenging to me.

@Geometror Would you be up for following with this plan? Does it sound sensible to you? If so, we could probably be done with this within a week. I'll be here to quickly review each step.

(snap->snapping, idx->index/slot_index, toggle_grid->grid_toggle for theme icon, minor code style fixes)

nyabinary · 2023-07-24T00:17:50Z

What the milestone for this?

YuriSizov · 2023-07-24T16:46:22Z

@nyabinary 4.2. This was split into several PRs to make the process easier for everyone. You can see them linked above your comment.

MewPurPur · 2023-09-08T08:42:26Z

Here are optimized/fixed versions for the new icons in this PR

GraphControl: <svg height="16" width="16" viewBox="0 0 16 16" xmlns="http://www.w3.org/2000/svg"><circle cx="8" cy="8" r="2" fill="#8eef97"/><circle cx="8" cy="8" r="5" stroke-width="2" stroke="#8eef97" fill="none"/></svg>

GraphEdit: <svg height="16" width="16" viewBox="0 0 16 16" xmlns="http://www.w3.org/2000/svg"><path d="M11 1a1 1 0 0 0-1 1v3a1 1 0 0 0 1 1h3a1 1 0 0 0 1-1V2a1 1 0 0 0-1-1zM6.732 5A2 2 0 0 1 7 6v1.117L9.268 6A2 2 0 0 1 9 5V3.883zM2 5a1 1 0 0 0-1 1v4a1 1 0 0 0 1 1h3a1 1 0 0 0 1-1V6a1 1 0 0 0-1-1zm5 3.883V10a2 2 0 0 1-.268 1L9 12.117V11a2 2 0 0 1 .268-1zM11 10a1 1 0 0 0-1 1v3a1 1 0 0 0 1 1h3a1 1 0 0 0 1-1v-3a1 1 0 0 0-1-1z" fill="#8eef97"/></svg>

GraphFrame: <svg height="16" width="16" viewBox="0 0 16 16" xmlns="http://www.w3.org/2000/svg"><path d="M3 1a2 2 0 0 0-2 2v10a2 2 0 0 0 2 2h10a2 2 0 0 0 2-2V3a2 2 0 0 0-2-2zm1.25 2h6.5A1.25 1.25 0 0 1 12 4.25V5a3 3 0 0 0 0 6v.75A1.25 1.25 0 0 1 10.75 13h-6.5A1.25 1.25 0 0 1 3 11.75v-7.5A1.25 1.25 0 0 1 4.25 3zM12 6a2 2 0 1 1 0 4 2 2 0 0 1 0-4z" fill="#8eef97"/></svg>

GraphNode: <svg height="16" width="16" viewBox="0 0 16 16" xmlns="http://www.w3.org/2000/svg"><path d="M5 2a2 2 0 0 0-2 2h10a2 2 0 0 0-2-2zM3 5v1a3 3 0 0 1 0 6 2 2 0 0 0 2 2h6a2 2 0 0 0 2-2 3 3 0 0 1 0-6V5zm0 2a2 2 0 0 0 0 4 2 2 0 0 0 0-4zm10 0a2 2 0 0 0 0 4 2 2 0 0 0 0-4z" fill="#8eef97"/></svg>

and GridToggle seems to already exist, so the PR adding it is probably a mistake.

YuriSizov · 2023-09-08T09:49:06Z

This PR is superseded by #79311 and its companions. Everything outlined here is now implemented. Feel free to open follow-up PRs to fix bugs and add new features and enhancements!

Geometror requested review from a team as code owners October 9, 2022 19:15

Geometror added needs testing breaks compat topic:gui topic:codestyle labels Oct 9, 2022

Geometror added this to the 4.0 milestone Oct 9, 2022

Calinou added the enhancement label Oct 9, 2022

Geometror force-pushed the graph-editor-refactor-v2 branch from fa0d694 to 6376dcc Compare October 10, 2022 18:46

Geometror force-pushed the graph-editor-refactor-v2 branch from 6376dcc to ff67573 Compare October 21, 2022 23:11

Geometror force-pushed the graph-editor-refactor-v2 branch 4 times, most recently from c376d1c to f5e21fb Compare October 30, 2022 03:41

Geometror force-pushed the graph-editor-refactor-v2 branch from f5e21fb to dc9a7a0 Compare November 25, 2022 18:17

Calinou modified the milestones: 4.0, 4.x Jan 23, 2023

akien-mga mentioned this pull request Jan 24, 2023

[WIP/Demo] GraphEdit/GraphNode refactor #61414

Closed

3 tasks

Geometror force-pushed the graph-editor-refactor-v2 branch 3 times, most recently from cc4b889 to e2bc01b Compare April 8, 2023 15:09

YuriSizov suggested changes May 17, 2023

View reviewed changes

YuriSizov reviewed May 17, 2023

View reviewed changes

YuriSizov modified the milestones: 4.x, 4.2 May 26, 2023

Geometror force-pushed the graph-editor-refactor-v2 branch 2 times, most recently from fb08d64 to 6c0d3e5 Compare July 7, 2023 14:38

Geometror added 6 commits July 7, 2023 16:51

Splitup GraphNode into several classes

6d8173e

Theming and titlebar flexibility improvements

7f48007

GraphEdit: Separate show grid from grid snap, small theme adjustments

6e230bc

Extract GraphEdit node arrangement

3239e3d

Remaining compat-breaking/consmetic renames and minor fixes

11efe7c

Renames and fixes addressing the review comments

1b4299f

(snap->snapping, idx->index/slot_index, toggle_grid->grid_toggle for theme icon, minor code style fixes)

Geometror force-pushed the graph-editor-refactor-v2 branch from 6c0d3e5 to 1b4299f Compare July 7, 2023 14:52

This was referenced Jul 10, 2023

Clean up/refactor GraphEdit #79308

Merged

Clean up/refactor GraphNode and make it more flexible #79311

Merged

Calinou mentioned this pull request Jul 18, 2023

Add an AnimationTreeCommentNode godotengine/godot-proposals#7313

Open

YuriSizov closed this Sep 8, 2023

YuriSizov added the archived label Sep 8, 2023

AThousandShips removed this from the 4.2 milestone Sep 8, 2023

AThousandShips removed the needs testing label Sep 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GraphEdit/GraphNode refactor [V2] #67152

GraphEdit/GraphNode refactor [V2] #67152

Geometror commented Oct 9, 2022

nyabinary commented Oct 14, 2022

fire commented Oct 20, 2022

QueenOfSquiggles commented Oct 20, 2022

nyabinary commented Oct 20, 2022

paddy-exe commented Oct 22, 2022

joaopedrosgs commented Jan 23, 2023

fire commented Jan 23, 2023

YuriSizov commented Jan 24, 2023

Zireael07 commented Jan 24, 2023

paddy-exe commented Apr 8, 2023

YuriSizov left a comment •

edited

Loading

YuriSizov May 17, 2023

Geometror May 18, 2023

YuriSizov May 19, 2023

YuriSizov May 17, 2023

YuriSizov May 17, 2023

YuriSizov left a comment

YuriSizov May 17, 2023

YuriSizov May 17, 2023

YuriSizov May 17, 2023

YuriSizov May 17, 2023

YuriSizov left a comment

YuriSizov May 17, 2023

YuriSizov May 17, 2023

YuriSizov May 17, 2023

YuriSizov commented May 17, 2023 •

edited

Loading

nyabinary commented Jul 24, 2023

YuriSizov commented Jul 24, 2023

MewPurPur commented Sep 8, 2023 •

edited

Loading

YuriSizov commented Sep 8, 2023

GraphEdit/GraphNode refactor [V2] #67152

GraphEdit/GraphNode refactor [V2] #67152

Conversation

Geometror commented Oct 9, 2022

Detailed changes

Restructuring/Code style

Functional changes

Theming

Fixes

Future work

nyabinary commented Oct 14, 2022

fire commented Oct 20, 2022

QueenOfSquiggles commented Oct 20, 2022

nyabinary commented Oct 20, 2022

paddy-exe commented Oct 22, 2022

joaopedrosgs commented Jan 23, 2023

fire commented Jan 23, 2023

YuriSizov commented Jan 24, 2023

Zireael07 commented Jan 24, 2023

paddy-exe commented Apr 8, 2023

YuriSizov left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YuriSizov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YuriSizov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YuriSizov commented May 17, 2023 • edited Loading

nyabinary commented Jul 24, 2023

YuriSizov commented Jul 24, 2023

MewPurPur commented Sep 8, 2023 • edited Loading

YuriSizov commented Sep 8, 2023

YuriSizov left a comment •

edited

Loading

YuriSizov commented May 17, 2023 •

edited

Loading

MewPurPur commented Sep 8, 2023 •

edited

Loading