[VisualShader] Add reroute node and improve port drawing

[Geometror]

Detailed changes:

• Refactoring: Create subclass VSGraphNode for the VisualShader editor to allow implementing specific features without needing to modify GraphNode (also tests its API)
• Add border/rim to connection ports
• Add reroute node to VisualShader
  • Automatically adapts its type while ensuring valid connections
  • Minimal/modest design (one port)
  • Can be moved/selected via a move handle that fades in when the mouse is near

TODO:

• [x] Evaluate the design
• [ ] Find a way to fade in the move handle when the mouse is above the port hotzone (might be hard to do)
• [x] Allow setting the type of chained reroutes when valid
• [x] Improve docs

[fire]

Is this only valid for Visual Shaders is it a feature common to all graph nodes? Like should this be in the base class or in visual shaders.

[Geometror]

@fire Currently just for visual shader. This is rather simple to implement (the UI part) on the user side , so I don't think it's a good idea to expose it as a general node. Also, keeping it unexposed allows this to be merged sooner.

Edit: I might have misunderstood you. I think both VSGraphNode and VSRerouteNode could be renamed to EditorGraphNode/EditorGraphRerouteNode, moved to their own file and used in the other graph editors (e.g. animation blend tree), without exposing them. But I think that can be done in a subsequent PR.

[paddy-exe]

This is a really good change! Great work on this @Geometror!

I have a nitpick about the UX about the implementation: It would be quite cumbersome to add Reroute nodes how you show in the gif after a user wants to clean up a very complex graph. It would be great to reduce the friction to add a Reroute node inbetween a connection of two other nodes by e.g. double-clicking. That would be at least my view if I would use it later.

[CsloudX]

Can this be Godot Built-in Node(rename VSRerouteNode to GraphRerouteNode)? IMO, most user who use GraphEdit&GraphNode need this node(include myself).

[avedar]

This is a really good change! Great work on this @Geometror!

I have a nitpick about the UX about the implementation: It would be quite cumbersome to add Reroute nodes how you show in the gif after a user wants to clean up a very complex graph. It would be great to reduce the friction to add a Reroute node inbetween a connection of two other nodes by e.g. double-clicking. That would be at least my view if I would use it later.

If not double click it should at least be in the first context dialog (alongside Disconnect and Insert New Node)

I agree this looks great!

[Geometror]

@CsloudX I see your point, but to be honest for now I don't think this makes a lot of sense. Implementing this exact reroute node in GDScript (VSRerouteNode) is literally 40 lines of code with the benefit of full flexibility; the much more sophisticated part is integrating it in your editor. This was even possible before with restrictions that would have required some workarounds, but that shouldn't be the case anymore with this PR. There would be an option (I think) to move some of the code inside GraphEdit, expose a general GraphRerouteNode node and provide an API, but that would require way more time to implement (which I currently don't have). Keeping this simple and internal for now might get this merged in time for 4.3.

[Geometror]

Update:

• Subgraphs are now checked for correctness and chained reroute nodes adjust their types recursively.

https://github.com/godotengine/godot/assets/50084500/725cb075-3940-4ca2-9b78-56a238d0b990

https://github.com/godotengine/godot/assets/50084500/d8037afe-65af-4daf-bb99-89b514f42dd6

• Reroute nodes can now be added via the context menu (@paddy-exe double-click is also a good idea, but I think we can evaluate that later since we don't have any other double-click operations in the VS editor at the moment [I think])

https://github.com/godotengine/godot/assets/50084500/0f291253-6836-47b2-a4c8-c750f2b90753

[paddy-exe]

Reroute nodes can now be added via the context menu (@paddy-exe double-click is also a good idea, but I think we can evaluate that later since we don't have any other double-click operations in the VS editor at the moment [I think])

That seems fair enough. Having the option to add this node via Right-click menu is already easier then typing it into the Add Node dialogue

[Geometror]

@Calinou Thanks for testing! I changed the animation duration to 0.3s. Regarding unconnected reroute nodes: What should be the behavior in that case? To my mind its kind of expected when you have a "hanging" reroute node connected to a port that it behaves like an uninitialized value. Currently just the output variable is declared, without initializing it, e.g.: vec3 n_out14p0;

[QbieShay]

I think a disconnected reroute node should not count as a connection so the nodes can use their default value

[QbieShay]

Hey @Geometror !

I tested your PR, thank you for your work!

My comments

• Like i mentioned in the previous comment, i think a non-connected reroute should just let the node use its default values. I understand this may be complicated, so it can be left for later
• I think it's more common to click and drag a reroute than to pull extra connections out of it. After trying it, i think that creating a reroute and immediately moving it away is the most common usecase, i think moving a reroute node should be easier than pulling out a connection (since it's created from an existing connection)
• I struggle to select the reroute node - i think it should have a bigger round hotzone that mimicks the shape of the reroute: clicking the reroute node should select it

[Calinou]

Also, it might be worth highlighting a reroute node with no inputs in red (as it's generally not intended behavior to do this). This is especially the case if we can't fix the default value issue.

[Geometror]

Update:

• "Hanging" reroute nodes now use default values
• Fix: Dragging a connection backwards (input port of node to empty) and inserting a reroute node would result in an reroute node with a wrong port type
• Fix: Reroute nodes were broken for connections of type sampler (due to them working completely different)
• Selected reroute nodes now always show the move handle
• Reroute nodes now can be selected when clicking directly in the port hotzone (adresses all GraphNodes since it was a bug/unintuitive behavior in GraphEdit)

[KoBeWi]

If 2 reroutes are very close, the input will be wrongly picked by the port behind handle.

https://github.com/godotengine/godot/assets/2223172/5132c3f3-521b-444a-9381-bf6b3ab3ba98

New reroute connection will always go right, even if connected to a node to the left.

https://github.com/godotengine/godot/assets/2223172/aaf5f1df-301f-418e-b6f5-b955f600658b

And likely related to the above, you can't connect reroute to output port:

https://github.com/godotengine/godot/assets/2223172/9676e4df-a58a-4afb-b70e-957b59f1ba2b

Though I assume the above 2 are a limitation.

EDIT: Connecting empty reroute will not update its value type and it can actually break type of existing reroutes. Undo does not fix it.

https://github.com/godotengine/godot/assets/2223172/df4bfc36-9399-43d1-95a8-f1ef165240e6

[Geometror]

If 2 reroutes are very close, the input will be wrongly picked by the port behind handle.

This is a limitation of GraphEdit and quite hard to fix. You'd need to move the top one first. But I doubt you will ever have two reroute nodes directly below each other.

New reroute connection will always go right, even if connected to a node to the left. And likely related to the above, you can't connect reroute to output port:

This is technically also a limitation, but actually how the workflow was designed and consistent with other graph editors. Reroute nodes are expected to be connected from left to right.

Connecting empty reroute will not update its value type and it can actually break type of existing reroutes. Undo does not fix it.

Also a minor limitation, but also how it was designed to work (dangling/unconnected reroute chains are only an intermediate state). When you actually connect it to a valid output port of a normal node, the types of all reroute nodes are adjusted.

[Geometror]

Addressed all review comments and rebased.

[QbieShay]

LGTM!

[akien-mga]

Thanks!