Fix VisualScriptEditor after namespaces

[mhilbrunner]

New iteration on a fix for the VisualScript editor after the move to namespaces.
As both VisualScriptEditor classes need to be exposed to GDScript, this renames the small one to VisualScriptCustomNodes, but it still is bound as the VisualScriptEditor singleton like it previously was. This seems to fix the issues.

The vs_bind namespace is removed, as its then no longer needed.

As added in the code comment previously, we may want to refactor these two classes, but still is a fix until then.

Properly fixes #51894, after #51627 broke it
Also reverts #51916, as both classes need to be exposed

[fire]

Posted in the visual script discord for review.

https://discord.gg/bfR6by5xEs

[fire]

Can confirm these operations work:

1. Can spawn new node
2. Left sidebar showing functions and variables work
3. Dragging a new node can now open the auto complete.

[fire]

I checked basic operations.

I wonder if the proxy name suffix is the standard pattern in godot. Not a blocker issue.

Solves the issues posted.

[mhilbrunner]

It could probably be named VisualScriptEditorCustomNodes or similar, as that is what it manages, and nothing else.

The fix would be the same though, as long as they have different names, not the same, everything should work again like before the change.

Thanks for the review and passing it on!

[fire]

Naming the class to be exactly what it does sounds like a good idea.

[mhilbrunner]

Need to fix the docs for this.

[aaronfranke]

VisualScriptCustomNodes would make sense to me.

[mhilbrunner]

Renamed to VisualScriptCustomNodes and fixed docs. This is good to go :)

[mhilbrunner]

Renamed the singleton variable. Eventually, we should consider renaming the exposed singleton from VisualScriptEditor to VisualScriptCustomNodes as well. But not sure who needs to discuss this - anyway, this should fix the current issues with VS.

[Gallilus]

I do not understand why there is a new reference to VisualScriptEditor!

So after some figuring out, I think I understand.
There is an engine singlton named _VisualScriptEditor that is exposed to the users with the name VisualScriptEditor.

• It exposes 2 functions, add_custom_node and remove_custom_node the rest is not accessible to Engine users.
  Did you remove the underscore to comply with new practices?
  This gives a conflict and you want to rename this underscore_VisualScriptEditor.
  And then we have 2 VisualScriptEditor and VisualScriptCustomNodes?

I'm sorry if I am missing something here.

PS: I think the name 'VisualScriptCustomNodes' is misleading as you want to reference the editor not a list of nodes or a specific script.
😕

[mhilbrunner]

Don't be sorry, its confusing.

Previously, there was the VisualScriptEditor, that is exposed as VisualScriptEditor to users - thats the proper editor.
Also, there was a _VisualScriptEditor, that was exposed as _VisualScriptEditor but also as a singleton named VisualScriptEditor, no underscore. And yeah, it got only those 3 methods you mention.

We got rid of all underscores in class names, because in all other cases, the underscored classes were just proxy classes (e.g. _OS was just exposing functions in OS to GDScript/users, but OS was never itself exposed).

In this case, however, this doesn't work: if we get rid of the underscore, there are now two VisualScriptEditors in GDScript.

With this change, the proper VisualScriptEditor is still VisualScriptEditor, and the second one (previously _VisualScriptEditor) that just has 3 methods to manage custom nodes is called VisualScriptCustomNodes. The latter is still exposed as a singleton called VisualScriptEditor however, like it previously was, so you can still do VisualScriptEditor.add_custom_node() in code.

This all is highly confusing, I don't particularly like the implementation of these two classes, and IMO we should see about refactoring this.

But in the end, for users nothing changes with this - VisualScriptEditor.add_custom_node() works as it previously did, the only thing that changes is a few places in the docs where the class type was previously _VisualScriptEditor and is now VisualScriptCustomNodes.

[Gallilus]

So why have VisualScriptCustomNodes at all?
It looks to be excess.

[mhilbrunner]

So why have VisualScriptCustomNodes at all?
It looks to be excess.

I agree and it seems like it, thats outside the scope of this fix however. We should see about moving it into VisualScriptEditor proper (although there may be valid reasons its not part of that) or have a general VisualScript singleton to have such configurations, not sure.

[mhilbrunner]

Merging this now so the Visual Script Editor works again in master. However, discussing a refactor/removal of that class is still worthwhile.