[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:

• 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)
• Allow setting the type of chained reroutes when valid
• 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.

vsr_demo1.mp4

vsr_demo2.mp4

• 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])

vsr_demo3.mp4

[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

[Calinou]

Tested locally, it mostly works as expected. The main issue I noticed is that if you leave a reroute port connected with no input, it'll result in what I assume is an infinite or NaN value:

Testing project: test_vs_reroute.zip

Also, it might be worth speeding up the fade animation for the move icon that appears when you hover above the point's hotspot.

[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;

[KoBeWi]

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

godot.windows.editor.dev.x86_64_uLd8LWTkHU.mp4

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

hVOvJL2exG.mp4

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

godot.windows.editor.dev.x86_64_S0DpzD0dcQ.mp4

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.

godot.windows.editor.dev.x86_64_KfEo9cAxJl.mp4

[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!

[QbieShay]

LGTM! Thank you for this work!

[akien-mga]

Thanks!

[ArseniyMirniy]

Godot 4.3 is spamming with “VSRerouteNode size:” at launch for every reroute used in every shader. Is there any reason for it, or is it a bug?

[KoBeWi]

Bug, but seems already fixed in 4.4.