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

Visual Script Graph Unification #29681

Open
wants to merge 2 commits into
base: master
from

Conversation

@swarnimarun
Copy link
Contributor

commented Jun 11, 2019

This is part of my GSOC project.
This is still heavily WIP and needs probably a lot of refactoring work.

Here's a quick showcase,

vs-showcase

cc: @fire @kidrigger

Bugs

  • Get Copy & Paste of Nodes working.
  • Ensure minimum collision of bounding boxes of functions on import of scripts from old versions.
  • Undo not working properly with the connection rescans.
  • Quick Create Functions shortcut (Ctrl + G)
  • Proper Mapping of Returns from functions.
  • Update function calls on function argument change or function rename.
  • Add simple warnings for all unintended use cases.
  • Test Undo edge cases().

Old bugs

  • NodePath edit with empty scene crashes the editor.
  • Make nodes Duplication work for connections as well.

Additional Changes (UI)

  • #29996 Better Type Detection
  • #29995 Improve search for Available Nodes
  • Add Port Swapping
@fire

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

Not placing it directly on top of another node is important, especially since we know the exact positions. I'll review more.

@swarnimarun

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

@fire Yep, I haven't added any UI changes at the moment just finishing the functionality for now.

@swarnimarun

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

It's in a proper working state now, just needs more testing to improve some more UI aspects if needed.
And also needs a clean up before it becomes ready for review.

vs-show

cc: @fire @kidrigger if any of you want certain specific changes do let me know.

@swarnimarun swarnimarun force-pushed the swarnimarun:vs-graph-unification branch from c13ce67 to f6c3ba5 Jun 14, 2019

@swarnimarun

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

A couple more gifs to show additions,
vs-group_datas
vs-grouping

@swarnimarun swarnimarun force-pushed the swarnimarun:vs-graph-unification branch from 022516f to e1df863 Jun 21, 2019

@fire

This comment has been minimized.

Copy link
Member

commented Jun 22, 2019

There are some clang format problems.

--- a/modules/visual_script/visual_script_editor.h	2019-06-21 20:29:57.059454622 +0000
+++ b/modules/visual_script/visual_script_editor.h	2019-06-21 20:30:04.981732182 +0000
@@ -193,7 +193,7 @@
 	void _end_node_move();
 	void _move_node(StringName func, int p_id, const Vector2 &p_to);
 
-	void _get_ends(int node, const List<VisualScript::SequenceConnection> &seqs, const Set<int> &selected, Set<int>& end_nodes);
+	void _get_ends(int node, const List<VisualScript::SequenceConnection> &seqs, const Set<int> &selected, Set<int> &end_nodes);
 
 	void _node_moved(Vector2 p_from, Vector2 p_to, int p_id);
 	void _remove_node(int p_id);
*** Aborting, please fix your commit(s) with 'git commit --amend' or 'git rebase -i <hash>'```
@swarnimarun

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2019

Yep somehow I managed to get my clang format settings messed up.. Fixed next PR should fix it all.

@swarnimarun swarnimarun force-pushed the swarnimarun:vs-graph-unification branch from db4e08b to 8b821ef Jun 22, 2019

@swarnimarun

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2019

Other relevant PRs: #29995 #29996

@fire

This comment has been minimized.

Copy link
Member

commented Jun 23, 2019

Travis is still failing.

@swarnimarun

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2019

Shadowing is a problem... :sigh:

@swarnimarun

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2019

Demo of the UI changes:
vs-swap

@swarnimarun swarnimarun force-pushed the swarnimarun:vs-graph-unification branch from 85bf4c9 to bb11323 Jun 25, 2019

@zohozer

This comment has been minimized.

Copy link

commented Jun 28, 2019

I’m curious if it is possible to implement a very useful feature from Bolt Unity into Godot Visual node system? I would really love to see the Flow Graph arrows like in Bolt, so to have a real visual feedback of what is happening at any time between the nodes.

@swarnimarun

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2019

@zohozer Not sure if it's possible at the moment as it would need Live Sync and also Live Editing support but will give it a look if it's easy enough no problems in adding more expressiveness :)

@fire

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

There are some formatting errors.

@swarnimarun swarnimarun force-pushed the swarnimarun:vs-graph-unification branch 4 times, most recently from b3564dd to 7353856 Jul 1, 2019

@swarnimarun swarnimarun marked this pull request as ready for review Jul 4, 2019

@swarnimarun

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2019

New Demo for Compose Array Node:
vs-list-nodes

Starting to move the Side Bar to the Top.(Can also be seen in the Gif)

@fire

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

I think we should prepare this pull request to be merged and make a separate pull request for vs group node.

@fire

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

Summary of meeting:

Discussed removing available nodes once #28523 is resolved.

Other In-Graph Editable Nodes are deferred because of the lack of time.

We discussed VS Custom Nodes and how we should be like gdscript and not handle defining nested functions.

@swarnimarun swarnimarun force-pushed the swarnimarun:vs-graph-unification branch 2 times, most recently from b4b0f72 to 1faa0e3 Jul 20, 2019

@swarnimarun

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2019

This PR is ready for review.

I just need to remove a few more of the TODOs I have left in it.

Things I am not sure about,

  • What should be the correct name for the base class for In-Graph Editable Nodes Class...(It's called VisualScriptLists atm)?
  • If a dialog would be better for creating functions? (IMO without the dialog it feels better)

cc: @fire @kidrigger @akien-mga @Chaosus

@fire

This comment has been minimized.

Copy link
Member

commented Jul 21, 2019

Can you show a video of both with a dialog and without a dialog for creating functions?

What should be the correct name for the base class for In-Graph Editable Nodes Class...(It's called VisualScriptLists atm)?

Can you describe more?

@Chaosus

This comment has been minimized.

Copy link
Member

commented Jul 21, 2019

image

The dialog default size seems to be very small compared to the amount content it provides - also the size is resetted every time the user call it

@swarnimarun

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2019

I was confused about that, will make it longer, probably also give it some more width and I thought resizing was better...
I guess it feels troublesome because it's too small...

@@ -103,6 +103,103 @@ class VisualScriptFunction : public VisualScriptNode {
VisualScriptFunction();
};

class VisualScriptLists : public VisualScriptNode {

This comment has been minimized.

Copy link
@swarnimarun

swarnimarun Jul 21, 2019

Author Contributor

I was talking about this @fire I am thinking about improving the name...

This comment has been minimized.

Copy link
@fire

fire Aug 11, 2019

Member

Did you think of any ideas?

@Chaosus

This comment has been minimized.

Copy link
Member

commented Jul 21, 2019

IMO when user clicks on the function in the list (or at least have a small button to do it) - the graph should set the focus on that function

@swarnimarun

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2019

@Chaosus you can do that with Ctrl + Click I didn't make it the default as functions need to be editable from the view...

@Chaosus

This comment has been minimized.

Copy link
Member

commented Jul 21, 2019

Yeah, of course - i mean you can add a button to do it (its not obvious to use CTRL)

@swarnimarun

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2019

Makes sense..

@swarnimarun

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2019

@fire The function dialog would basically allow for a way to setup function on creation.

It would look similar to
image

PS: I stole the image from Chaosus' PR

@fire

This comment has been minimized.

Copy link
Member

commented Jul 21, 2019

The sidebar for adding arguments is quite difficult to use, so this is an improvement.

@fire

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

Some warnings:

 2347 |   GraphNode *gn = Object::cast_to<GraphNode>(graph->get_child(i));
      |              ^~
modules/visual_script/visual_script_editor.cpp:2343:13: note: shadowed declaration is here
 2343 |  GraphNode *gn = Object::cast_to<GraphNode>(n);
      |             ^~```
@fire

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

Screen Shot 2019-07-29 at 4 52 43 AM

Adding arguments and then deleting the arguments leaves empty space.

@fire

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

Screen Shot 2019-07-29 at 4 55 03 AM

Is there a reason why there's ".." after add nodes?

@fire

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

Also the "Change Base Type:" is selectable, it shouldn't be.

@Chaosus

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

Is there a reason why there's ".." after add nodes?

I guess for compatibility with similar button in visual shaders. But there should be 3 dots after Add node (and better push this button at first place on the top bar)

Adding arguments and then deleting the arguments leaves empty space.

Yeah, to fix it simply add control->set_size(Size2(-1, -1)) on deletion

@swarnimarun

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

I missed some dots and just noticed the resizing problem.. :P

@swarnimarun swarnimarun force-pushed the swarnimarun:vs-graph-unification branch 2 times, most recently from 4550eb0 to 03c8b47 Aug 1, 2019

@fire

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

No string input bug.

image

Typing a character will make the search appear.

[Edited] I think this regresses functionality. So it makes visual script worse. It blocks the pr.

@swarnimarun swarnimarun force-pushed the swarnimarun:vs-graph-unification branch from 03c8b47 to f0cbd06 Aug 11, 2019

@swarnimarun

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

Screenshots of latest changes,
image
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.