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

[WIP/Demo] GraphEdit/GraphNode refactor #61414

Closed
wants to merge 1 commit into from

Conversation

Geometror
Copy link
Member

@Geometror Geometror commented May 25, 2022

The main goal of this PR is to provide a discussion ground for some essential changes to improve the usability of GraphNode/GraphEdit for 4.0 which may break compatibility; hence it is marked as a draft. I know this PR is quite big, but it contains some interdependent changes, which I had to test together. It can be seen as a proof-of-concept PR to demonstrate some features as a whole (the code is a bit messy and there are still some bugs present). Any suggestions regarding design, structure or features are welcome.

There are still some things I have planned, primarily a customizable titlebar or at least a dedicated titlebar stylebox, but for now I just wanted to get this out and wait for feedback regarding the general direction/implementation.

Although some changes depend on each other we can still decide what could be split into separate PRs.

comment sorting

Detailed changes:

  • Split GraphNode into GraphNodeBase, GraphNode and GraphNodeComment (open for naming suggestions) due to:
    • Maintainability: The GraphNode class got really large and complex, so splitting it up improves code navigation and readability (removes some is_comment()-branching/ cleans up theme constants)
    • Flexibility: Allows adding more features to the frame/comment graph node or extend GraphNodeBase 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
  • Several comment node improvements:
    • Draw connection lines above comment nodes, but below normal graph nodes
    • Nested comment nodes are automatically sorted
    • Fix comment node moving nodes outside of its bounding box at different zoom levels
      Implemented in Fix enclosed GraphNodes calculation of GraphEdit comment nodes #61629
    • Implement tint color for comment nodes (useful for structuring the graph), can be toggled
      • Added to VisualShader to test the functionality, but can be split up
  • Improve connection bezier curve generation (small distances) [similar to 3.x behavior with some tweaks]
    Implemented in Improve Graphedit connection lines #61541
  • Improve node connection hot zones (approach similar to Automatic GraphEdit node connection hot zone height 2 #49722) Implemented in Improve and fix port hotzones of GraphNode #61515
  • Remove unused variables
  • Restructure the headers
    • Clearly separate class attributes and methods
    • Move enum and struct declarations
    • Group attributes/methods in a way that makes sense (I hope)
  • Adjust comment style for consistency
  • Fix some function parameters with missing p_ prefixes
  • Rename several variables to be more descriptive
  • Rename ConnCache of GraphNode to PortCache (and related variables) since it makes more sense
  • Rename connection_... to port_... to separate the terms port (a point to connect to) and connection (the actual link between two ports)

@Geometror Geometror added this to the 4.0 milestone May 25, 2022
@Geometror Geometror requested review from a team as code owners May 25, 2022 17:24
@Geometror Geometror marked this pull request as draft May 25, 2022 17:24
@Geometror Geometror force-pushed the refactor-graphnode branch from 5c8574e to df6f66b Compare May 25, 2022 17:28
@Geometror Geometror force-pushed the refactor-graphnode branch 2 times, most recently from e7b5bbb to dbda38c Compare May 25, 2022 18:06
@Geometror Geometror force-pushed the refactor-graphnode branch from dbda38c to cedf567 Compare May 25, 2022 19:34
@fire
Copy link
Member

fire commented May 25, 2022

Do you have any ideas where one would integrate sub grouping into this refactor? I'm currently using the terms from https://membraneframework.org/guide/v0.7/bins.html#content, but the terms needs to be translated to graph edit style. I honestly believe that subgraph needs to be fully integrated and not defined separately.

image

@Geometror
Copy link
Member Author

@fire Do you mean node groups like they are present in Blender? That could be implemented with a special GraphNodeGroup (inherited from GraphNode, since it needs inputs/outputs) node which holds a tree of graph nodes (of type GraphNodeBase). GraphEdit then needs to be modified to be able to switch to different "view roots", meaning you can set the view to a given GraphNodeGroup. Or you could just use another GraphEdit to display the subgraph.

@fire
Copy link
Member

fire commented May 26, 2022

ERROR: GraphEdit.xml: Unresolved method "GraphNode.get_connection_output_position".
1 error was found in the class reference XML. Please check the messages above.

Did you run godot --doctool?

@fire
Copy link
Member

fire commented May 26, 2022

Here's a graphic drawn in nomnoml.

image

[<GraphNodeBase> GraphNode |
  [<GraphNode> GraphNodeGroup | pad_input_0() | pad_output_0() |
    [<GraphNode> SourceGraphNode | 
    pad_output_0()] -> [<GraphNode> FilterGraphNode | pad_input_0() | pad_output_0() ]
    [<GraphNode> FilterGraphNode] -> [<GraphNode> SinkGraphNode | pad_input_0()  ]
  ]
]
[<GraphNodeBase> GraphNodeComment]

@fire
Copy link
Member

fire commented May 26, 2022

To make it easier to review I recommend breaking up the features:

  1. Split GraphNode into GraphNodeBase, GraphNode and GraphNodeComment (open for naming suggestions)
  2. Comment node improvements:
  3. Improve connection bezier curve generation (small distances)
  4. Improve node connection hot zones
  5. Code cleanup misc
  6. Adjust comment style for consistency
  7. Fix some function parameters with missing p_ prefixes (Maybe mixed with the previous or separately.)
  8. Rename several variables to be more descriptive (Maybe mixed with the previous or separately.)
  9. Rename ConnCache of GraphNode to PortCache (and related variables) since it makes more sense
  10. Rename connection_... to port_... to separate the terms port (a point to connect to) and connection (the actual link between two ports) This needs a proposal so all the stakeholders can review the design

Feel free to take this as a suggestion. You can divide it other ways.

@Geometror
Copy link
Member Author

Geometror commented May 26, 2022

The idea is to split this up when the general direction is approved.
Until then I plan to update this PR to include all suggestions and use it as a demonstration of all features working together.
When this process is finished and its ready for a detailed review, I could split up the PRs the following way: I think renaming, cleanups and splitting up GraphNode would go in one (since its a lot of work to separate them), but the connection bezier curve, hot zones and comment node improvements (+ maybe titlebar rework) can each go in their own.

@fire
Copy link
Member

fire commented May 26, 2022

These enhancements seems split-able.

  1. connection bezier curve
  2. hot zones
  3. comment node improvements
  4. (+ maybe titlebar rework)

I want to separate the GraphNode split-up from the rest of these enhancements to keep the review light.

@Geometror
Copy link
Member Author

Yes, that's what I meant, those you listed would each go in their own PR :) I think I will do 1-3 first, then the GraphNode split-up and after that the titlebar rework (since the implementation depends on whether the split-up is merged).

@fire-forge
Copy link
Contributor

This is great! I have an idea for a project that would use GraphEdit, so any improvements to it are very much appreciated :)

About the potential titlebar rework, I have a few suggestions:

  • Add an icon property to display an icon in the top-left corner.
  • Add an alignment property to control the alignment of the title text - left (next to icon), center, or right.

@fire
Copy link
Member

fire commented Jun 1, 2022

Two other prs were merged from the list. Yay!

@fire
Copy link
Member

fire commented Jun 2, 2022

1-3 is done.

Is it time for the GraphNode split-up?

@Geometror
Copy link
Member Author

I think I will do the comment node sorting/connection layering next, but I am not sure yet since there were some issues with my approach which I have to fix first.

@akien-mga
Copy link
Member

Superseded by #67152.

@akien-mga akien-mga closed this Jan 24, 2023
@aaronfranke aaronfranke removed this from the 4.x milestone Feb 1, 2023
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.

6 participants