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

Improve dependency-injected classes for addons, plugins, and modding tools #376

Open
willnationsdev opened this issue Jan 12, 2020 · 0 comments

Comments

@willnationsdev
Copy link
Contributor

Describe the project you are working on:
Plugins where users can override how certain nodes and scenes execute their processes.

Describe the problem or limitation you are having in your project:
Let's say I want to create instances of some class, but I want the user to decide which class it is. I could, theoretically, export some data type, have the user assign a value to that data type, and then use that data type to instantiate the class. I will also frequently want to constrain the selected data type to refer to a type of Node class, Resource class, or some user-defined class via Script or even a PackedScene. In fact, at one point I even tried having meta-tools (tools that help other programmers create tools for designers/writers/etc.) that allow the constraints themselves to be a user-defined class.

However, the problem is that there are multiple ways to represent a class, with no single API for handling them, instantiating them, or checking their inheritance to guarantee a type constraint.

Currently, if I allow engine classes to be used, then I have to export a String for the user to type in the name of the class. And if I want to give better UX, then I need to create some sort of utility to help the user get the name right, e.g. a popup for autocompletion of the class name or a button that, once clicked, opens the CreateDialog and lets the user choose a class to then fill in the name.

If we allow user-defined scripts, then names alone won't suffice. We will need to export a Script. And checking inheritance / instantiating a Script is a very different process from doing so with a class name, so I'll need unique logic for handling that case.

And yet again, if we allow scenes, then names and scripts alone won't suffice. We will need to export a PackedScene. Again, instantiating a scene is different, and checking inheritance for a scene is way more complicated since it involves diving into the packed data and crawling its data for the things you need (which aren't well documented anywhere). Furthermore, there are two different types of inheritance for scenes: scene inheritance and root node inheritance. This is because a derived scene could conceivably change the script on the root node and change the node type (actually a very common practice).

This means that, to get a decent UX, I would have to create either 3 separate exported properties and tons of customized logic for handling each of them, or add even more logic to dynamically generate a single exported property based on some kind of toggleable setting interactived-with via a button that is placed by an EditorInspectorPlugin. It's just a rabbit hole of complexity that should really not be there for something seemingly so simple.

To get much improved UX, I would prefer to export a class that could abstract all 3 representations behind a single data type.

Until this is resolved, proper and smooth dependency injection for plugins, addons, and especially modding tools (which are Godot projects and not in the Editor) is a whole bunch of unnecessary complexity.

Another point is that, once a class has been selected, then at runtime, I would like to show the class's information to the user. Aside from documentation generation itself being a separate issue, knowing which class's documentation to show is already problematic without these changes since you have to add logic to check for which data type is used to represent the class and how to fetch the corresponding data. This relies on information from ClassDB for engine classes and Script reflection methods (godotengine/godot#31021) for user-defined scripts. Scenes just run off of whichever category their root node falls into.

Describe how this feature / enhancement will help you overcome this problem or limitation:

It would abstract away the complexity of Godot's class system behind a single API so that basic lookup, instantiation, type checking, and reflection can all be performed from a single entity and in a single method call for each operation. This would dramatically improve the UX of any dependency injection related to class usage.

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

I would imagine one could create an EditorProperty that looks like this:

| A | B | C |------- D -------|
|-------------- E ------------|
  • A: ToolButton for browsing files on the filesystem, filtered to any compatible script or scene file.
  • B: ToolButton for opening the CreateDialog and selecting a class name.
  • C: ToolButton with a ? to display a popup which displays the name or file path of the type constraint assigned to the property. Defaults to "Object".
  • D: A string field for displaying the class name or file path of the resource representing the class.
  • E: A "result" field indicating what field C evaluates to.
    • When C is Nil or empty string, it displays a message about defaulting to the type constraint.
    • When C is an invalid class name or path, displays an error message.
    • When C is a valid engine class name, it displays the name of the class.
    • When C is a valid script class name or a valid path to a script or scene file, it displays an expandable resource bar that can open a sub-inspector, just like the EditorPropertyResource does.
      • If the given resource does not meet the type constraints, then this section displays an error message instead.

Exporting the property might then look something like this:

const EnemyUnitScn = preload("res://enemy_unit.tscn")

# Somehow get `ClassType` to be an acceptable symbol (core/module/editor cases)
export(ClassType, Control) list_item_type: ClassType = null
# If an addon, then can't reference the symbol and need to represent as a string.
# Can't be statically typed in that case, because differing export/static type.
export(String, TYPE, EnemyUnitScn) enemy_unit_class = ClassType.new(enemy_unit_class)

With a PropertyInfo resulting in these kinds of structures:

{
    "name": "list_item_type",
    "type": TYPE_OBJECT,
    "hint": PROPERTY_HINT_TYPE_STRING,
    "hint_string: "Control"
    "usage": ...
    "class_name": "ClassType"
},
{
    "name": "enemy_unit_class",
    "type": TYPE_OBJECT,
    "hint": PROPERTY_HINT_TYPE_STRING,
    "hint_string: "res://enemy_unit.tscn"
    "usage": ...
    "class_name": "ClassType"
}

Maybe we'd need a separate PROPERTY_HINT to be allocated for this purpose? Ideally we'd be able to get away with using what we already have. I was hoping the TYPE_OBJECT and PROPERTY_HINT_TYPE_STRING combination would be the flag that triggers the unique behavior here.

Any other suggestions are welcome.

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

Except for the export/editor portion, I already have a slightly tested and functional script that can handle all of the above logic. It is Godot Next's ClassType. There are some extra things in there that I would probably remove if ported to an official Godot class of some sort, but overall it fulfills a lot of the abstraction outlined above.

So, the implementation would involve porting that class to some sort of data type that abstracts the notion of a class, and then creating a new EditorProperty* type that knows how to react to that data type.

The difficult part is how and/or where to put such a class to fulfill its tactical needs:

  • If you put it in /editor, then it can't be used in modding tools unless the users bundle in the entire Godot Editor with their modding tools (which they may not want to do).
  • If you put it in /modules, then the editor can't reference the class from the editor. You would only be able to have the class export properly if you detected and reacted to specific settings (like a module-dependent unique PROPERTY_HINT) to customize the Inspector while iterating over properties on the object. And GDScript can't even directly build PropertyInfos (nor is that user-friendly at all), so it wouldn't be able to create such a property anyway. You'd have to use a different language.
  • If you put it in /core, then it is accessible everywhere (the editor can use it AND modding tools can use it), but it's a problem because the core doesn't actually use the class, so it is nothing but bloat as far as the core is concerned.
  • * If you make it an addon that other addons would use as a dependency, then the UX of the concept drops considerably. If it isn't a built-in feature, then designing plugins that expect users to have the other plugin installed becomes much more difficult.
    It's also slightly awkward since the functionality itself, "being able to use dependency injection to plug in a class for a script's logic," is incredibly basic functionality that, due to the variety of class constructs in Godot, is far more complicated than it should be. Users shouldn't have to go through extra effort like an addon just to use dependency injection consistently. It should be a built-in feature.
    • If you were to use an addon, then the implementation would shift from using an EditorProperty* to using an EditorInspectorPlugin to inject a custom Control node with the mentioned features/interface.

And no matter what you do, you'd still have to find a way of encoding this concept into the PropertyInfo so that an EditorProperty* or EditorInspectorPlugin can detect when it should have the custom GUI code in the EditorInspector. There isn't really a clean way to do this since GDScript can't export property hints directly (unless they are derived from property hints / hint strings already in core and the gdscript parser can be updated to convert a certain export pattern to the matching PropertyInfo data).

So, part of this proposal is figuring out which implementation can be made to work in the most clean and effective way. The option that sounds most likely would be a module in the engine core, but I think you would need to make changes to how exports work in order for the connection between GDScript, PropertyHint, Inspector, and ClassType can resolve cleanly.

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

Whether it is used often or not depends on whether someone...

  1. uses tools written by someone else (which are using dependency injection).
  2. how many people are on their team (if they write tools for other team members that use dependency injection).
  3. is busy writing the tools for other people to use (my use case).

So, some people may not use this feature at all, but most stand to benefit from it since dependency injection is a core programming principle that spans all scripting languages and software development paradigms.

And no, it is anything but a few lines of script. My class_type.gd script is nearly a thousand lines of code by itself (probably could be shortened to around 800 lines of code if the spaces, inlined docs, and bloat are removed). Then you'd have to add all the editor tools on top of that which is probably another few hundred lines of code.

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

Restating the asterisked section above:

If you make it an addon that other addons would use as a dependency, then the UX of the concept drops considerably. If it isn't a built-in feature, then designing plugins that expect users to have the other plugin installed becomes much more difficult.
It's also slightly awkward since the functionality itself, "being able to use dependency injection to plug in a class for a script's logic," is incredibly basic functionality that, due to the variety of class constructs in Godot, is far more complicated than it should be. Users shouldn't have to go through extra effort like an addon just to use dependency injection consistently. It should be a built-in feature.

@Calinou Calinou changed the title Improve dependency injected classes for addons, plugins, and modding tools. Improve dependency-injected classes for addons, plugins, and modding tools Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants