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

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

Merged
merged 1 commit into from Sep 8, 2023

Conversation

Geometror
Copy link
Member

Split from #67152. Depends on #79308.
This PR contains the rework of GraphNode.

Detailed changes:

  • Split GraphNode into GraphElement and GraphNode
  • Renamed set/get_connection_input/output_x methods to set/get_port_input/output_x
  • Remove get_port_input/output_height as it was not necessary (basically returned the height of the slot of the given port which is the height of the child in that slot)
  • GraphNode: get_port_input/output_position returns no longer the position premultiplied with the scale
  • Revised custom port drawing (now achievable via overriding a function)
  • Flexibility improvements to GraphNode's titlebar
    • It now consists of a HBoxContainer with a Label (accessible via `get_titlebar_hbox
    • StyleBox for body (slot area) and title bar (both with selected variants)
    • Use "GraphNodeTitleLabel" theme variation for title labels, remove the title offset theme constants
  • Respective adjustments to the editor theme and default project theme
  • Rename lots of variables 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 custom port icon not being centered when it's bigger than the default icon
  • Fix and improve documentation with regard to port/connection/slot terminology
  • Fix comment style in various places
  • [can be extracted if desired] VisualShader: Fix error label increasing the minimum size in an ugly way since it autowrap isn't enabled

@Geometror Geometror added this to the 4.2 milestone Jul 11, 2023
@Geometror Geometror requested review from a team as code owners July 11, 2023 01:53
@Geometror Geometror force-pushed the rework-graphnode branch 2 times, most recently from 95a5183 to 809ecce Compare July 11, 2023 02:40
@YuriSizov YuriSizov self-requested a review July 11, 2023 08:29
@YuriSizov YuriSizov changed the title Rework GraphNode Clean up/refactor GraphNode and make it more flexible Jul 24, 2023
@Geometror
Copy link
Member Author

Rebased.

@YuriSizov
Copy link
Contributor

I'll review it this week, but for now I wanted to mention that grid toggle icons shouldn't be a part of this. They were added (though originally missing) in #79308.

@YuriSizov YuriSizov force-pushed the rework-graphnode branch 2 times, most recently from 404cdec to 9cc5663 Compare August 9, 2023 17:59
@YuriSizov
Copy link
Contributor

YuriSizov commented Aug 9, 2023

I fixed the icons and added some missing renames. I noticed some existing inconsistencies with closing/deleting, and unified them using the term closing, closable as it makes the most sense from the perspective of the graph itself. We can discuss this, but at least it's all consistent now and doesn't use both terms interchangeably. I also finalized and simplified some of the changes in the visual shader editor plugin, though it would've been better to never include anything other than necessary renames and whatnot in this PR.

From a quick look at visual shaders everything works fine. So I'll continue with my review.

PS. I think the names that we've picked for updated classes are good, but we can ask for more opinions.

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation and theme review.

Please when you make changes do not rebase against the master, otherwise it would be a hell to untangle what has been actually changed between force pushes. Alternatively, making it into separate commits for now is also fine.

scene/resources/default_theme/default_theme.cpp Outdated Show resolved Hide resolved
scene/resources/default_theme/default_theme.cpp Outdated Show resolved Hide resolved
scene/resources/default_theme/default_theme.cpp Outdated Show resolved Hide resolved
editor/editor_themes.cpp Outdated Show resolved Hide resolved
editor/editor_themes.cpp Outdated Show resolved Hide resolved
doc/classes/GraphNode.xml Outdated Show resolved Hide resolved
doc/classes/GraphNode.xml Outdated Show resolved Hide resolved
doc/classes/GraphNode.xml Outdated Show resolved Hide resolved
doc/classes/GraphNode.xml Outdated Show resolved Hide resolved
doc/classes/GraphNode.xml Outdated Show resolved Hide resolved
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review. As with GraphEdit it's still missing the manual theme cache, but I guess I'll add it after all this rework.

We should also add compatibility exceptions for the new renames and other public API changes, see #80168. I plan to follow this up with a PR that adds compatibility methods where possible, but for now adding exceptions should be fine.

scene/gui/graph_node.h Outdated Show resolved Hide resolved
scene/gui/graph_node.h Outdated Show resolved Hide resolved
scene/gui/graph_node.h Outdated Show resolved Hide resolved
scene/gui/graph_node.h Outdated Show resolved Hide resolved
scene/gui/graph_node.h Outdated Show resolved Hide resolved
scene/gui/graph_element.cpp Show resolved Hide resolved
scene/gui/graph_element.cpp Show resolved Hide resolved
scene/gui/graph_node.cpp Outdated Show resolved Hide resolved
scene/gui/graph_node.cpp Outdated Show resolved Hide resolved
scene/gui/graph_node.cpp Outdated Show resolved Hide resolved
@YuriSizov
Copy link
Contributor

I haven't checked the code too closely, but it seems mostly fine outside of outlined problems. This is to say that once those are addressed, I think we can go ahead with this, and use public testing to help us identify if there remain any typos or calculation mistakes.

What remains to test is using GraphEdit in a project, customizing it, extending elements and configuring the titlebar. I will do this tomorrow.

@YuriSizov
Copy link
Contributor

We should also remove the configuration warning with this PR:

image

@YuriSizov

This comment was marked as resolved.

@Geometror
Copy link
Member Author

Geometror commented Aug 31, 2023

@RodZill4
Thanks for your feedback!

I'd really like the "new comment node" to be able to contain any Control (something like a Window pinned to the graph canvas). Would be much more flexible and not limited to comments.

This will definitely be the case. You can probably implement a basic comment node right now (without fancy sorting or automatic resizing) using the new base class GraphElement.

ge3

More generally, I also have lots of requests about UI interactions with connections (being able to insert a node into a connection, select a connection to remove it etc.), mainly inspired by Blender. Not sure which ones actually require changes in Godot, though.

I have plans to construct an API around connections, but this has to wait for 4.3+.

@YuriSizov
Copy link
Contributor

This is a bit more verbose, but doing input checks in graph_element_resized feels not right, so I went with this approach.

I think singleton exists for this specific purpose, so you can check the state of the input at any point. So I don't have a problem with just copying the logic from GraphEdit::gui_input. But I'm not against setting a flag either. Just as long as we don't end up with many of them, making it's harder to understand the current state of the node (because many flags mean many permutations of them).

@YuriSizov
Copy link
Contributor

@Geometror This PR is still missing compatibility exceptions in the misc\extension_api_validation\4.1-stable.expected file. That's the only blocker. The snapping issue can be fixed with a follow up.

editor/editor_themes.cpp Outdated Show resolved Hide resolved
editor/editor_themes.cpp Outdated Show resolved Hide resolved
editor/editor_themes.cpp Outdated Show resolved Hide resolved
scene/theme/default_theme.cpp Outdated Show resolved Hide resolved
doc/classes/GraphEdit.xml Outdated Show resolved Hide resolved
doc/classes/GraphElement.xml Outdated Show resolved Hide resolved
doc/classes/GraphElement.xml Outdated Show resolved Hide resolved
doc/classes/GraphElement.xml Outdated Show resolved Hide resolved
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the grammar fixes above this LGTM

misc/extension_api_validation/4.1-stable.expected Outdated Show resolved Hide resolved
Split GraphNode into GraphElement and GraphNode, add custom
titlebar, and adjust theming.
@Geometror
Copy link
Member Author

Should be good to go now after it (hopefully) passes CI.

@YuriSizov The snapping was already fixed (when resizing).

@AThousandShips I just tried out the "Commit suggestion" feature for the first time. I hope I haven't spammed you with dozens of notifications :)

@AThousandShips
Copy link
Member

No worries! Next time try out the "Add suggestion to batch" option to do all in one commit

@akien-mga akien-mga merged commit 2167694 into godotengine:master Sep 8, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@YuriSizov
Copy link
Contributor

@YuriSizov The snapping was already fixed (when resizing).

I don't see any relevant changes and you haven't pushed anything before I asked the last time 👀

@YuriSizov
Copy link
Contributor

@Geometror Just to re-confirm, because I've just tested it on master, but there is no snapping when resizing. I think you haven't pushed the fixed that you've described before.

@nonchip
Copy link

nonchip commented Nov 17, 2023

We should also remove the configuration warning with this PR:

@YuriSizov btw that never happened, might be worth doing before the upcoming release?

@AThousandShips
Copy link
Member

AThousandShips commented Nov 18, 2023

GraphEdit is still experimental so I'd keep it until that changes, and see:

I would remove the configuration warning in a few weeks after making sure that everything works fine and we haven't overlooked any major design flaw.

In fact several changes have happened since and it's still in flux

@AThousandShips
Copy link
Member

Leaving it as experimental gives us room to tweak and fix, my suggestion is to leave it until 4.2.1 at the earliest to get some more field testing of it done

@YuriSizov
Copy link
Contributor

We should also remove the configuration warning with this PR:

@YuriSizov btw that never happened, might be worth doing before the upcoming release?

We decided to keep it marked as experimental until 4.3, but there shouldn't be major breaking changes. It's just to account for possible feedback from GraphEdit users, who aren't many.

@nonchip
Copy link

nonchip commented Nov 20, 2023

@YuriSizov ah yeah that kinda makes sense, the main issue i see with it is just that currently the most common feedback is "i'm scared to become a GraphEdit user, because it literally tells me to wait for an undetermined amount of time before i do so, when will the rework finally happen so i can start using it?"

@YuriSizov
Copy link
Contributor

Well then wait for the warning to disappear if you're uncertain :) We just want to give time existing users to update their tools and make sure nothing requires further breaking changes. Those people understand the current status.

@RodZill4
Copy link
Contributor

Updating Material Maker for Godot 4.2 has been pretty smooth on the GraphEdit/GraphNode side. Didn't notice any problem (and graph nodes are used A LOT). The only aspect I didn't dig into yet is converting comment nodes into GraphElements.

@Geometror
Copy link
Member Author

@RodZill4 Comment nodes are likely going to be difficult to implement just with GraphElement (at least how they used to work) due to the removal of dedicated logic in GraphEdit. Comment nodes were removed because the implementation was problematic and I couldn't come up with a better approach in time. I had a few prototypes which did almost work (but failed in certain cases). This has a high priority for me, but time is a scarce commodity and I'd like to merge the other GraphEdit related PRs first, since they might alter the implementation of GraphFrame (the dedicated comment node class).

@nagidev
Copy link
Contributor

nagidev commented Apr 21, 2024

@Geometror can you please take a look at this issue which was probably introduced due to this pull? I'm not familiar with the codebase, so I could do with some help understanding the GraphNode::_resort function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants