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

Complete, consolidate, and optimize user-defined types (Script classes/Custom types) #22

Open
willnationsdev opened this issue Sep 5, 2019 · 6 comments

Comments

@willnationsdev
Copy link
Contributor

willnationsdev commented Sep 5, 2019

Describe the project you are working on:

I work on plugins and editor changes. Both involve creating editing tools for types within Godot's API and they currently support two different, highly divergent systems that don't both need to exist.

Describe how this feature / enhancement will help your project:

The engine's codebase splits user-defined types between Custom Types and Script Classes. The newer, superior Script Class system has not yet been fully implemented for all languages and therefore results in things needing to account for both user-defined "named" types. This further complicates any and all type-checking logic in the editor.

For the purposes of cleaning up the codebase and simplifying the implementation of future engine features, I propose that we...

  1. Add script class support to VisualScript.
  2. Add script class support to CSharpScript.
  3. Remove the Custom Type system and all references to it from the codebase.
  4. Optimize the slowness of the CreateDialog that occurs when you add too many script classes to your project (as demonstrated in this gif, shared by @HeartoLazor in this comment).
  5. Enable all scripting languages, not just GDScript, to access globally accessible variables for script classes (either global variables or things accessed statically through a namespace/singleton/etc.).
  6. Prevent disabled plugins' script classes from clogging up the global namespace.

Show a mock up screenshots/video or a flow diagram explaining how your proposal will work:

An interim PR that simply refactored Custom Types to be accessible by name, and which also optimized the CreateDialog can be found here. @HeartoLazor also shared this gif of the PR correctly optimizing the CreateDialog as needed, although with some other bugs related to autoloads. We would need to fix those bugs and split the work between 4 separate PRs that each introduces one of the features in isolation for testing purposes.

Describe implementation detail for your proposal (in code), if possible:

  1. VisualScript script classes:
    1. Add GUI elements to the VisualScript editor for defining an optional "class_name" and "class_icon" for a VisualScript.
    2. Implement VisualScriptLanguage::get_global_class_name to extract the values of these properties and return them to the EditorFileSystem for generating script class names.
  2. CSharpScript script classes:
    • Note: I haven't the foggiest idea how any of this stuff works!
    1. Implement CSharpScript::get_base_script(). This is frequently how the editor language-agnostically tracks inheritance hierarchies for scripted types, so I believe this would need to correctly return a Script object that refers to the parent class of the CSharpScript.
    2. Implement a class-level attribute for ClassName and ClassIcon.
    3. Implement CSharpScriptLanguage::get_global_class_name to extract the values for this attribute and return them to the EditorFileSystem for generating script class names.
  3. Custom Types removal:
    1. Find and refactor away all code that references "CustomType" anywhere.
  4. CreateDialog optimization:
    • The slowness is triggered from attempting to quickly rebuild the entire Tree GUI that renders the inheritance hierarchy of all types that exist AND...
    • When attempting to search for a type by subsequence match, it not only rebuilds the Tree again on every character change in the LineEdit, but it also performs additional inheritance checks on the matching classes against every inherited class of the currently preferred type (on this line) which only reduces the speed of building the Tree even further.
    • To solve this, one must do the following:
    1. Start rebuilding and caching the inheritance hierarchies used to build the Tree as early as possible, after every script class refresh in the EditorFileSystem.
    2. The first step involves determining which classes should even show up in the Tree, so all of the logic of filtering out classes that happens here can also be moved elsewhere to be processed ASAP rather than on every Tree rendering.
    3. The code for handling Custom Types can be stripped out.
    4. The re-occurring ternary if cpp_type do ClassDB else do EditorData::script_class stuff logic should be refactored to dedicated EditorData methods, such as in this existing "export Resources" PR.
    • I believe the problem is figuring out WHEN to actually execute these CreateDialog caching operations without triggering problems in other parts of the editor since I suspect that is the reason for the autoload issues that @HeartoLazor mentions in his comment (linking again for convenience).
  5. Add global script class references for all languages:
    • I'm not sure how each language sets up global variables yet, but here we go.
    1. For VisualScript, assuming it has a similar system to that of GDScript, you can probably just add to the global constants embedded in the VisualScriptLanguage like GDScriptLanguage and then make sure that an existing VisualScript node exposes access to global variables (one probably already exists by my guess).
    2. For CSharpScript, I suspect we would want to add the script classes as constants in the GD namespace, similar to other "global" methods in the @GDscript documentation. So, a script class with the name "MyClass" would become a constant of type Script that exists at Godot.GD.MyClass.
    3. For NativeScript, the GDNative bindings API would likely need some sort of string-based map for accessing the data statically, i.e. ScriptClasses::get("MyClass"). I'm not sure how all that would be done, but it's worth looking into.
  6. Do not bloat global namespace with disabled plugins' script classes:
    • As of right now, if someone installs a plugin, but does not have it active, the identifier for the script still clogs up the global namespace at runtime, even though the type is hidden from the CreateDialog. Those types should never end up registered as script classes in the first place.
    1. When the EditorFileSystem is refreshing the engine's list of script classes, strategically omit from those updates any script whose path lies within a res://addons/** subdirectory and whose relevant plugin has an inactive status in the ProjectSettings.
    • When any part of the engine updates the registration status of an EditorPlugin, the code must also instruct the EditorFileSystem to refresh its list of script classes.

If this enhancement will not be used often, can it be worked around with a few lines of script?:

Uhh....no.

Is there a reason why this should be core and not an add-on in the asset library?:

It involves extensive changes to multiple scripting languages and editor classes that already exist in the main repository. An addon cannot provide the necessary alterations.

@willnationsdev willnationsdev changed the title Complete, consolidate, and optimize user-defined types in Godot Engine. Complete, consolidate, and optimize user-defined types (Script classes/Custom types). Sep 5, 2019
@willnationsdev
Copy link
Contributor Author

Related to godotengine/godot#26875.

@nyenye
Copy link

nyenye commented Sep 5, 2019

This should get attention. Coherence with all scripting languages should be a must.

Xrayez added a commit to Xrayez/godot-vector-resource that referenced this issue Apr 12, 2020
No need to support two different systems (godotengine/godot-proposals#22).
Fixes an issue with the resource appearing in every resource popup.
@astrale-sharp
Copy link

I support this!

@willnationsdev
Copy link
Contributor Author

FYI, for those wondering, I would work on getting PRs submitted for this by updating my WIP branches and whatnot, but I can't even get the master branch to run the editor without it crashing, so I have to wait until things are more stable.

@tavurth
Copy link

tavurth commented Mar 7, 2021

Would love to see work done in this area, now addons feel quite far behind other package managers (npm, cargo etc) in terms of features.

There are also bugs and issues which prevent me for really using the system as intended (#2420)

@willnationsdev
Copy link
Contributor Author

@tavurth Chances are, PR #44879 will be merged for an upcoming 3.2 release (perhaps 3.2.5), and that will lay the groundwork for adding script class support and data serialization for all scripting languages. The next step would be to provide helper methods and scene binding for scripts that facilitate merging the inheritance hierarchies of the two divergent APIs. See #1935 for details on that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants