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

VisualScript Modules #45294

Conversation

swarnimarun
Copy link
Contributor

@swarnimarun swarnimarun commented Jan 18, 2021

I am not quite sure about its mergeability yet, my first rebase had issues because I tried rebasing with all the commits originally(even this one had issues likely due to formatting changes in the refactor PR before merge), and the current commit history needs clean up as well.

So I believe it needs a bit more of testing and fixes if there is an issue, also because of the changes to the Window GUI which might not need some of the old changes anymore. Will make it a proper PR as soon as I can say it's mergeable.

Created this so we can start reviewing.

Fixes: godotengine/godot-visual-script#16

@Calinou Calinou added this to the 4.0 milestone Jan 18, 2021
@swarnimarun swarnimarun force-pushed the master-vs-submodule-rebased branch from 1e55056 to 76fca75 Compare January 18, 2021 21:11
@swarnimarun swarnimarun changed the title VisualScript Submodules VisualScript Modules Jan 18, 2021
@swarnimarun swarnimarun force-pushed the master-vs-submodule-rebased branch from 76fca75 to 4c15d0d Compare January 18, 2021 21:25
@fire
Copy link
Member

fire commented Jan 19, 2021

Tell me when it's ready for review.

@fire
Copy link
Member

fire commented Feb 4, 2021

When you're ready, rebase and ping me.

@swarnimarun swarnimarun force-pushed the master-vs-submodule-rebased branch from 58f24b8 to 39b4031 Compare February 7, 2021 12:46
@swarnimarun swarnimarun marked this pull request as ready for review February 7, 2021 12:47
@swarnimarun
Copy link
Contributor Author

@fire ping.
Done. :)

Only things left were the breakpoints and type-guessing requiring some fixes. I had to delay it as I happened to get my build to crash, and had to do some bug hunting over the weekdays to figure it out. Everything seems fine now.
Though I totally don't expect it to be bug-free (though I hope it's only a few lol 🤞).
And I also have to push a PR or two for some older bugs and issues which I can do as soon as we get this one done. As I am expecting some of them to be fixed by default in this PR.

Also @akien-mga or @Calinou,
While we are at it, is there a way we can change Variant::is_null() method to have a better name, like is_object_null(). This one had cost me an entire weekend when I was initially patching stuff because I thought it will check if Variant is of type NIL.
If there is room for discussion I can create an issue thread or get on IRC, really found it way too annoying.

I don't think the Variant API is publicly exposed other than "maybe" GDNative IIRC.

@fire
Copy link
Member

fire commented Feb 8, 2021

Started review. Will take me some time.

  1. I noticed all the _process and _physics_process bindings are gone when clicking on the new functions.
  2. Dragging a function from the left bar crashes Godot.
  3. Creating a new nodes modes moves the new node far away.
    GIF 2021-02-07 7-50-38 PM
  4. Clicking to change the module name is strange. Why not match the normal way?
    image
  5. Hard to understand how to save modules externally

Here's the first round of review.

@fire
Copy link
Member

fire commented Feb 8, 2021

I apparently didn't compile correctly. Here's the second round.

  1. The new module nodes are put in the wrong place
    image
  2. Nil means any node
    image
  3. Once in module editing mode, no clear way of showing how to return.
  4. How do you save VS Modules? Loading appears to work.

@swarnimarun
Copy link
Contributor Author

swarnimarun commented Feb 8, 2021

"I noticed all the _process and _physics_process bindings are gone when clicking on the new functions."

Wait what...

"Dragging a function from the left bar crashes Godot."

I get a feeling I messed up something in the rebase.... Or it's a multi-window issue that I didn't fix.

On it.

Edit: Apparently I have not been using single-window mode in my recent demo projects. So seems like something got messed up.

@swarnimarun
Copy link
Contributor Author

image
Hmm...

Alright! let me do a clean build just in case.

@swarnimarun
Copy link
Contributor Author

test_module_pr
Clean, build also worked fine for me :(

Though, did notice I forgot to change/improve the placement code for nodes when inside module.

@swarnimarun
Copy link
Contributor Author

As for the popup on the module change button, it's changed because of changes to master, but yeah makes sense will make it more like how it used to be.

And yeah the save option does seem non-obvious.
image

Aside: Fixed Nil to Any and looking at what probable reason their might for the crash for dragging function to visual script graph.

@fire
Copy link
Member

fire commented Feb 8, 2021

Will wait for the push. Good night.

@fire
Copy link
Member

fire commented Feb 12, 2021

Some engine core changes were made, I asked for a review round on discord, but I need an updated branch.

@swarnimarun swarnimarun force-pushed the master-vs-submodule-rebased branch from 39b4031 to eb8c3fa Compare February 14, 2021 01:12
Visualscript modules are similar to scripts but they allow for storing
and loading parts of the visualscript aka snippet/sub-graph/module as a
resource that can be utilized in any other visualscript.

Added:
VisualScriptModule Class as Resource types for allowing handling
visualscript code snippets as resource.
VisualScriptModuleNodes for interfacing with the Modules in
visualscript.

Changed:
Creation of visualscript instances for making room for modules.
Adding checks in the visualscript editor for handling modules.
@swarnimarun swarnimarun force-pushed the master-vs-submodule-rebased branch from eb8c3fa to 052cf14 Compare February 14, 2021 01:43
@swarnimarun
Copy link
Contributor Author

This should fix most of the UX and things that were annoying, couldn't reproduce the function crash bug, but I did make some changes as per my best guesses(if it still happens mind throwing the crash report/log).

Though I did end up finding bugs and annoyances in the Engine itself, like on a Dual Monitor setup new subwindows open on the primary display and there is no way to override it from outside Window class as far as I can see. So popups are slightly broken on dual monitors. And some more similar stuff.
I will create an issue on GitHub and then see if I need to submit a patch for it or if there is already a patch in works.

Also, I think a simpler solution is to just have popups and dialogs be in the same Window. Will have to write some extra code in Window class for it, or create a subclass that extends the functionalities of the Window class.

@fire
Copy link
Member

fire commented Apr 28, 2021

@swarnimarun was unable to work on this due to personal reasons, so I'll see if I can update it.

@Gallilus
Copy link
Contributor

HI @swarnimarun @fire ,

I'm sorry to say,
I have been trying to test this build. It always crashes when trying to open or save.
Did not even get far enough to drag in one module in the script.

I did try to cherry-pick this after implementing PR #49749. The conflicts are minimal to fix and has similar issues.

@fire
Copy link
Member

fire commented Jun 22, 2021

Can you post the branch where you were able to compile? I can try debugging.

@akien-mga
Copy link
Member

Closing as we just removed VisualScript for 4.0, so this can no longer be evaluated as is.

The VisualScript module has been moved to https://github.com/godotengine/godot-visual-script with the aim that it might be ported to a GDExtension if there are interested contributors to do so. Further work to improve the features could be done on that repository too (so currently there isn't anyone dedicated to maintain this repository, it will depend again on whether some contributors step up to do so).

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.

Groups Nodes replace Functions in Visual Scripting
5 participants