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

TypeDB and GDScript 'Scripts' global added. #19778

Closed

Conversation

willnationsdev
Copy link
Contributor

@willnationsdev willnationsdev commented Jun 26, 2018

@reduz @karroffel

This PR makes several changes to a variety of systems in Godot, but it's meant to mostly be confined to the editor and gdscript areas.

The functional summary is that it permits the namespaced registration of scripts (eventually scenes), enables scripting languages to inspect this data, improves editor responsiveness to the registration of types without hiding any information about the presence of a script, and gives GDScript an interface to access and inherit from these registered scripts by name.

Changes to Core

  • The Script class now has a get_script_metadata() method. This method's purpose is purely to have an implementation-specific means of acquiring script data that is important to the Editor. I initially attempted to NOT have a method like this whereby the Editor would directly inspect the file extension of a script, open up the file, and parse data directly from the source code in an effort to not modify core. However, this was just as dirty/hacky as it sounds, and it isn't even applicable to things like NativeScript which have no perceptible source code. Instead, any script may now define this method (probably a static method) to return a Dictionary containing the relevant editor metadata. It's simpler, cross-language compatible, and has been given a default implementation to return the results of the defined method, if it exists.

  • To simplify the implementation of a method in the DocData class which can generate an XML documentation file from a Script class, I appended a defaulted-to-false p_no_inheritance boolean parameter to the get_script_property_list method so that I could more easily extract only the properties related to the current script when generating documentation. I also have in mind to re-apply this method in the unrelated PR Script hierarchies for Inspector #13116.

Major Changes

  • The custom type system in EditorData has been replaced with a TypeDB class that allows users to register types of scripts. It is designed with the intention of eventually including scenes as well, but the UI for this has not been implemented.

    • The TypeDB has a set of methods which mimic the behavior of the ClassDB. The only one of these methods which actually wraps ClassDB functionality is the is_parent_class method.
    • TypeDB types are distinguished from ClassDB types in that they must have a namespace prefix, designated with a period '.' delimeter. Only C++ types will exist in the global namespace.
    • For backwards compatibility, the EditorPlugin::add_custom_type() method inserts the "Custom." namespace as a prefix to any registered types which do not already have a namespace.
    • Whenever the filesystem is changed, the TypeDB is serialized into a StringName typename => String filepath Dictionary stored in ProjectSettings at typedb/scripts and typedb/scenes.
  • The GDScript language accesses the TypeDB's serialized data from ProjectSettings to generate a pair of Scripts and Scenes globals.

    • The parser identifies usages of these globals and parses their subsequent identifiers for matching namespaced typenames. If one is found, the parser interprets it as a RESOURCE_LOAD operation with the filepath as a parameter.
    • The parser also puts together a Scripts.Namespace.Typename pattern for the extends_class array, enabling inheritance.
    • The compiler parses the inherited name and likewise tries to find a match in the Scripts global.
  • GDScript files may now optionally export their class name, whether they are custom, and if custom, whether they are abstract and/or have a custom icon. This is executed using the export keyword and is expected to be the first line of a GDScript file, save for any comments.

      # my_node.gd, variations include...
      export("Game.MyNode") # merely registers the name in the Scripts global. No editor functionality.
      export("Game.MyNode", CUSTOM) # registered as custom type using base type icon
      export("Game.MyNode", CUSTOM, "res://icon_my_node.svg") # registered as custom type with custom icon.
      export("Game.MyNode", ABSTRACT, CUSTOM) # registered as abstract custom type (similar to CanvasItem) with the base type icon.
      export("Game.MyNode", ABSTRACT, CUSTOM, "res://icon_my_node.svg") # everything
      extends Node
    
  • The Editor has improved responsiveness to custom types.

    • Custom types have a faded script icon in the Scene dock, indicating that the script is overrideable, but still a script. The "add a script" button is displayed if a node with a custom script is selected. Clicking on the faded script icon will take you to the custom script.
    • Clicking on the "add a script" button will open the ScriptCreateDialog with the inherits field pre-populated. The content here is determined by a method within the TypeDB in cases where the node already has a script that is a custom type.
    • Clicking the "remove script" button on a node with a script that derives a custom script in its inheritance hierarchy will remove the script, but reinstate the most-derived custom script.
  • The EditorPlugin (Edit, moved to the EditorInterface for EditorScript compatibility) now has exposed API methods for XML documentation, namely, generating a file from a Script and loading up all files within a directory. These methods are mostly implemented within the DocData class.

  • Edit: The EditorPluginSettings now has create and edit buttons which open a new PluginCreateDialog to simplify the process of building and editing plugins.

  • Edit: A docs reload button now exists in Projects -> Tools -> Rebuild Documentation. It simply calls the godot executable with "--doctool ." parameters.

  • Edit: The Editor now automatically adds and removes any registered custom types when the plugin is activated and deactivated, respectively. As such, if using exclusively GDScript at this point, all one needs is an EditorPlugin tool script defined (with no other content) in order to define custom types as a group.

A demo of the changes can be found on my YouTube video. Note that the following issues from the video are already fixed:

  1. Only pre-populating the inherits field of the ScriptCreateDialog when appropriate.
  2. Making the icon an optional parameter in the GDScript class export.

During my rebase with master, the most conflicts I had were in the editor_properties.cpp file where many references to the old custom type system had been made. I believe I made the updates properly, but I had trouble determining how to test the changed code.

Anyway, please let me know if there are additional changes I need to make before this can be integrated for 3.1.

Known Bugs At This Time (Will Update)

  • The Inspector is showing certain custom types in submenus when it shouldn't be.
  • Resolve conflicts that will inevitably happen with Typed GDScript #19264 based on changes to the compiler.
  • Refactor script accesses in GDScript (will be in separate branch, eventually merged in. Can be integrated before that's done).

@ghost
Copy link

ghost commented Jun 26, 2018

just curious what's the use case for this, and what does it solve? (just out of curiosity). a lot of that stuff went over my head, but wow you put in a lot of effort, so respect.

e: for example, how can just a random game dev take advantage / utilize TypeDB?

@willnationsdev
Copy link
Contributor Author

willnationsdev commented Jun 26, 2018

@girng It solves a variety of problems with the usability of custom types in Godot Engine.

Issues solved, at least in part, include #17387, #7402, #17348, #6767 (once scene support is added), and #6067. Probably more too.

This makes it so that the custom types you define in a plugin will feel more like their in-engine counterparts. In addition, it enables individual scripts and scripting languages to declare their own typenames / "custom"-ness to the editor so that an EditorPlugin isn't strictly necessary to define types.

^ Actually, now that I think about it, we'd still wanna be able to toggle entire sets of custom nodes on/off based on whether a plugin is active, so maybe it'd be a good idea to associate a namespace with a plugin and then have all of those nodes get disabled / enabled in the TypeDB when toggling the plugin.

(Edit: This method has now been added:
EditorPlugin::toggle_custom_namespace(p_namespace, p_active))

Anyway, you can then also create in-engine documentation for the custom types you've defined, the custom types will simulate being a "part" of the node (doesn't remove custom types when removing the script), and you'll automatically extend the custom types when you add a script. In addition, the custom types displayed in the CreateDialog previously didn't allow you to display multiple tiers of inheritance (only directly inheriting in-engine types was allowed). This PR also fixes that. These are usability concerns that rendered the old custom type series completely inadequate and pointless to use. Now, it can revitalize the creation of plugins for the engine overall.

@willnationsdev
Copy link
Contributor Author

@girng so, if you look at the YouTube video linked at the bottom of the PR, you'll see a practical demonstration of the TypeDB in use in the GDScript language. Users can simply use the "export" keyword to declare a typename for a script, and suddenly, they can now inherit from and access that script by name.

@bojidar-bg
Copy link
Contributor

Some really basic comments:

export("Game.Name")
# Better as, since everything is valid idents anyway:
export(Game.Name)
# Or maybe better as, since that's how you require it:
export(Scripts.Game.Name)

@swarnimarun
Copy link
Contributor

@willnationsdev Shouldn't visualscripting also be able extend from the custom node scripts.
From your video it seems like it's not possible yet. Building you branch now to check out all the stuff.

@willnationsdev
Copy link
Contributor Author

@swarnimarun VisualScript actually doesn't support inheritance at all, so no.

@willnationsdev
Copy link
Contributor Author

@bojidar-bg That's a good point. I'll try to see if I can refactor it in the parser to happen during identifier analysis.

@samdze
Copy link
Contributor

samdze commented Jun 26, 2018

I like the fact that an EditorPlugin is not needed to register a custom type, also agree with them having to strictly be defined within a namespace.

However, I don't like having to type Scripts. to reference any custom type. I know this could be helpful for autocompletion, but it's one more thing that differentiates custom types from in-engine types and I would like this procedure to be as transparent as possible, namespaces are enough.

Another point is: you talked about also registering custom scenes as custom types, that's cool, but having them into the Scripts.Something namespace seems not the right placement to me, as they are not scripts.
Having them in a separate Scenes.Something namespace is even worse in my opinion.

Again about scenes, I think you mentioned in the video that you would probably allow their registration with an import file, configurable in the import dock. At this point, I think it's much better to keep the process consistent and do this way for scripts too.

@willnationsdev
Copy link
Contributor Author

willnationsdev commented Jun 26, 2018

@samdze

Having them in a separate Scenes.Something namespace is even worse in my opinion.

This is actually what the plan is moving forward.

I don't like having to type Scripts. to reference any custom type. I know this could be helpful for autocompletion, but it's one more thing that differentiates custom types from in-engine types and I would like this procedure to be as transparent as possible, namespaces are enough.

Actually, using namespaces alone is fraught with needless complications. If someone were to define a namespace at any point that shares a name with a GDScript keyword or an in-engine type, then we would have to specifically yield functionality to those things rather than starting to look for entries in the deserialized TypeDB Dictionary. This means we'd have to manually check for all of these identifiers any time you are typing out an identifier.

In addition, under normal circumstances, if you try to append a dot to an identifier, the parser immediately attempts to evaluate that identifier to determine if its an entity like an Object or Dictionary that you can access. These searches would come up empty most of the time, and be incorrect 100% of the time, if you simply did Game. and failed to locate a Game member variable / global, etc.

Storing each namespace as its own global is also a bad idea because one can only ADD to the globals that exist, not remove, which does not match up with a mechanic that involves dynamically updating (adding and removing) the available references as time progresses.

To solve all of these problems in the simplest way, I simply defined a "magic" identifier. When it finds that identifier, it specifically starts looking for additional sub-identifiers WITHOUT evaluating anything and then checking the sub-identifiers in the TypeDB Dictionary.

I was originally going to keep it all in one global, but then differentiating between the Script and Scene types that might have the same name became a terrible usability issue with all sorts of unnecessary custom logic. In the end, it was much more simple and effective to stick with separate magic identifiers for each.

Again about scenes, I think you mentioned in the video that you would probably allow their registration with an import file, configurable in the import dock. At this point, I think it's much better to keep the process consistent and do this way for scripts too.

If anything, I'd want to cut-down on the possibility of generating clutter files that are unnecessary to a project. If the script alone can be used to provide the necessary information to the editor, then why waste time making users write out the data in the editor itself? I think people would largely prefer to keep the functionality embedded within the scripts themselves, that way they don't have to carry around .import baggage with them any time they want to move or share their scripts. (Edit: Imagine writing a C# script and then having to RE-define its type information in the Import dock. Then having to update that information if you use refactoring tools to change the name of the type later!)

As for scenes, I mentioned that it was possible we'd use the Import system, but I'd really prefer to invent a whole new means of storing the data, if possible, rather than doing that. There are several different ways it might be handled.

One other thought I'd had was simply storing metadata (like, the Object metadata) of the PackedScene itself somehow within the scene file which could be deserialized upon loading. However, that is a separate Issue altogether, and merits its own discussion space entirely.

@willnationsdev
Copy link
Contributor Author

In fact, if #18591 option number 2 is implemented, that would really increase my desire to go the PackedScene Metadata route, allowing users to edit the data in the Editor, but serialize all of the data directly within the scene file.

@willnationsdev
Copy link
Contributor Author

willnationsdev commented Jun 26, 2018

@samdze Also, it's still probably a good idea to create EditorPlugins for the various custom type APIs one might redistribute. The reason being that I plan to add a method to toggle a namespace in the EditorPlugin, and it will be important for people to be able to turn on/off the editor integration of a plugin's nodes all at once rather than having to quite literally remove the scripts from their project folder just to hide their presence in the editor.

Edit: and now this method has been added:
EditorPlugin::toggle_custom_namespace(p_namespace, p_active)

@willnationsdev willnationsdev force-pushed the custom-types-editor branch 2 times, most recently from 7d5a39b to 3f54d81 Compare June 26, 2018 14:44
@willnationsdev
Copy link
Contributor Author

willnationsdev commented Jun 26, 2018

@bojidar-bg Looks like doing it with the identifiers directly is impractical. Because the namespaces can be of an arbitrary depth, and because there's no way to have the parser go backwards once it has advanced, you'd get stuck only being able to have a single-level namespace. The string constant method allows users to type in whatever depth namespace they want.

Could probably add some validation though to make sure the string is a valid namespaced typename construction.

@willnationsdev
Copy link
Contributor Author

Well, with a bit more testing, I just discovered a bug in the editor_properties.cpp changes, as I suspected. A Label I defined as a custom type is now showing up in the dropdown options when I attempt to select a Texture in a TextureRect. I was right to think I'd be messing around with stuff I know nothing about. XD

custom_type_bug_1

@samdze
Copy link
Contributor

samdze commented Jun 27, 2018

Actually, using namespaces alone is fraught with needless complications. If someone were to define a namespace at any point that shares a name with a GDScript keyword or an in-engine type, then we would have to specifically yield functionality to those things rather than starting to look for entries in the deserialized TypeDB Dictionary. This means we'd have to manually check for all of these identifiers any time you are typing out an identifier.

I think it should be taken for granted that names also shared with in-engine types or keywords should not be allowed.

Storing each namespace as its own global is also a bad idea because one can only ADD to the globals that exist, not remove, which does not match up with a mechanic that involves dynamically updating (adding and removing) the available references as time progresses.

Could you elaborate on this point?

I was originally going to keep it all in one global, but then differentiating between the Script and Scene types that might have the same name became a terrible usability issue with all sorts of unnecessary custom logic. In the end, it was much more simple and effective to stick with separate magic identifiers for each.

Having two custom types with the same identifier should throw an error even if it happens with scripts only. If scripts and scenes are treated transparently then it makes no difference.

If anything, I'd want to cut-down on the possibility of generating clutter files that are unnecessary to a project. If the script alone can be used to provide the necessary information to the editor, then why waste time making users write out the data in the editor itself? I think people would largely prefer to keep the functionality embedded within the scripts themselves, that way they don't have to carry around .import baggage with them any time they want to move or share their scripts.

I don't see any problem in creating .import files for scripts and scenes, they are resources, after all.
The issue of having to carry around .import files sounds negligible to me, sharing a custom type often means sharing an add-on that is an entire folder with all the files it cointains.
In addition the import dock can guide the user to make it more immediate to specify the identifier, type and icon, and if the latter is defined one has to carry around multiple files anyway.

Also, it's still probably a good idea to create EditorPlugins for the various custom type APIs one might redistribute. The reason being that I plan to add a method to toggle a namespace in the EditorPlugin, and it will be important for people to be able to turn on/off the editor integration of a plugin's nodes all at once rather than having to quite literally remove the scripts from their project folder just to hide their presence in the editor.

Surely custom types should have a way to be part of a plugin, making them activable/deactivable, but I would integrate this funcionality into the plugin toggle system without considering namespaces. Every custom type defined inside a particular add-on folder gets activated or deactivated together with the add-on itself.
This however should be taken in consideration when decided how add-ons will change in Godot.

@razcore-rad
Copy link
Contributor

razcore-rad commented Jun 27, 2018

@willnationsdev I have a feature request for this awesome new feature:
Have you considered having a way to remove access to properties (from code/editor) when extending base nodes? Or if it's even possible.

The rationale is, say I want to make a custom node with the features of Sprite, but for whatever reason I don't want the user to have direct access to the scale property. I provide a different way of handling that stuff and I do it my way.

I was just thinking about streams these days and one node that can be used as a base class is Tween, it pretty much has the necessary basic functionality, except I wouldn't want the user to be able to change the repeat property manually from the editor or code or any of the playback properties etc.

This can be extended to built-in signals as well, perhaps I want to deliberately block these signals, I don't think there's an easy way, cause they'll still be emitted on the backend on the base node, they then need to be blocked from propagating.

@willnationsdev
Copy link
Contributor Author

willnationsdev commented Jun 27, 2018

@samdze

Storing each namespace as its own global is also a bad idea because one can only ADD to the globals that exist, not remove, which does not match up with a mechanic that involves dynamically updating (adding and removing) the available references as time progresses.

Could you elaborate on this point?

The GlobalConstants system that is in Godot Engine only maintains the concept of adding constants (precisely because they are not supposed to be modified at runtime). But if I have the possibility of dynamically adding and removing an entire namespace of types, I won't actually be able to remove the namespace of types unless I restart the engine without every having added them in the first place.

If I download and install a plugin that introduces 50 new types under the namespace "MyPlugin" and correspondingly define a "MyPlugin" global to identify another "magic" identifier (to begin the process of finding sub-identifiers without evaluating dereferences via the dot '.'), then trying de-activating or even completely deleting the "MyPlugin" plugin files WON'T actually remove the "MyPlugin" global and all of its (now invalidated) script/scene references. Because constants can't be removed at runtime.

The only way to get around this is to create a single global (or one global for each type like Scripts/Scenes) that is ALWAYS present, but which has its value updated over time. That way, the identifier remains consistent, but I get take the Variant stored there and update it with a new Dictionary automatically as the user modifies their project with plugins and new namespaces.

So, whether it be a single global ("GD" or "API", etc.) or different globals ("Scripts", "Scenes"), the fact remains that implementation-wise, there HAS to be some unchanging value that exists before the name of the actual namespace.

Surely custom types should have a way to be part of a plugin, making them activable/deactivable, but I would integrate this funcionality into the plugin toggle system without considering namespaces. Every custom type defined inside a particular add-on folder gets activated or deactivated together with the add-on itself.
This however should be taken in consideration when decided how add-ons will change in Godot.

I've gone ahead and created two different methods for toggling either one of them manually, and then I've modified the EditorNode so that when you add or remove an addon, it'll automatically do a directory-based toggle. That should give users maximum control, but also let the editor handle things for them (just in case). Thanks for the suggestion.

I don't see any problem in creating .import files for scripts and scenes, they are resources, after all.
The issue of having to carry around .import files sounds negligible to me, sharing a custom type often means sharing an add-on that is an entire folder with all the files it cointains.
In addition the import dock can guide the user to make it more immediate to specify the identifier, type and icon, and if the latter is defined one has to carry around multiple files anyway.

I'm still opposed to the idea of needing to resort to an import file to define type information for a script when the scripts often times have this data built into them already. The way I've written it, scripts can already provide any data they need simply by providing the proper method.

The purpose of the Import system is to integrate custom internal data for otherwise third-party assets. Making it used for .tscn or .gd files would be shoehorning in data that the editor should be able to handle on its own since it created the assets in the first place. I would much rather modify serialization processes and/or expose any previously hidden data in order to make this already-existing data viewable to users rather than forcing unrelated systems to work together just to hack a feature.

For my own work at least, I'm not convinced to use the Import system. But if you'd like to go ahead and make a PR for it to see if people like the feel of it that way, then I'd say go for it. It's difficult to fine-tune what the right "feel" for usability is without actually trying it. I just have my suspicions, and with limited development time available, I gotta pick and choose my battles.

@willnationsdev
Copy link
Contributor Author

@razcore-art That would be an entirely separate Issue altogether. You can already kinda do this for methods just by overriding the method in the derived class and calling an assert(false). Signals aren't really possible to do atm.

What you would have to do is present a means of having a script generate a blacklist of the properties, methods, and signals you want it to block, and then have the various methods in the Object class actually respect that blacklist by filtering it out. So: get_property_list, get_method_list, has_method, get_signal_list, has_user_signal, stuff like that...I think. Off the top of my head anyway.

But yeah, that isn't really within the purview of this PR.

@willnationsdev
Copy link
Contributor Author

@samdze

I think the easiest and most flexible way of approaching this scripts/scenes situation is to only allow scenes as custom types

Well, I expect the primary use case of this stuff to actually be scripts (scenes literally don't even WORK yet, lol). As such, I don't really think that's a good idea.

@willnationsdev
Copy link
Contributor Author

@chanon Yeah, I mean, removing the parentheses is lower on my priority list atm since it's just a syntax difference. If anything, I'd make that an optional thing later on or something. If it'll prevent it from getting merged for the upcoming beta, then I don't wanna waste time trying to make that work within the timeline.

@chanon
Copy link
Contributor

chanon commented Jun 28, 2018

Well, I expect the primary use case of this stuff to actually be scripts (scenes literally don't even WORK yet, lol). As such, I don't really think that's a good idea.

Yes, just the ability to export a script and then being able to refer it in other script files directly without having to do

const MyNodeType = preload("res://path/to/MyNodeType.gd")

all the time will already be very useful.

@samdze
Copy link
Contributor

samdze commented Jun 28, 2018

Well, I expect the primary use case of this stuff to actually be scripts (scenes literally don't even WORK yet, lol). As such, I don't really think that's a good idea.

Yes, just the ability to export a script and then being able to refer it in other script files directly without having to do

const MyNodeType = preload("res://path/to/MyNodeType.gd")

all the time will already be very useful.

Would be surely useful and the primary use case, yes, and in fact, scenes-only registration would support that.

It's all about tradeoffs, scenes-only registration requires a little more work to setup by the user to register a new "simple node script" type, but it's more powerful as it also allows the registration of all kinds of complex scenes, all with the same procedure.
And as already said, problems like being unsure of accessing a Scipt or a PackedScene would disappear.

The fact that scene registration doesn't work now has nothing to do with what I'm saying, I'm only proposing solutions.
Unfortunately it may not be sure that the best solution saves what you have already implemented.

However, this doesn't take into account custom Resources, therefore this whole single solution could be easily discarded as simple script custom types remain necessary, so I agree this is not a good idea.

Probably as long as valid identifiers have a hint of them being a Script or a PackedScene, like a different text color and the right new() or instance() autocomplete, prefixes and naming conventions are not needed.

More opinions are certainly needed.

@mhilbrunner
Copy link
Member

Have to agree that this and typed GDScript are awesome improvements.

cc @vnen @bojidar-bg :)

@willnationsdev
Copy link
Contributor Author

willnationsdev commented Jun 28, 2018

@samdze

It's all about tradeoffs, scenes-only registration requires a little more work to setup by the user to register a new "simple node script" type, but it's more powerful as it also allows the registration of all kinds of complex scenes, all with the same procedure.

You're exactly right. And I'll be getting to the scenes as well later on (probably with the 3.1.1 release or something), but in the meantime, scripts are already ready to go, more or less, so once I refactor things to account for @vnen's #19264 changes, the script side will be finished. We're still gonna get registration of complex scenes, and in fact they DO use the same procedure under-the-hood, but there's a lot of additional logic for the editor support and tooling that will need to go along with the scenes (which isn't ready yet).

And as already said, problems like being unsure of accessing a Scipt or a PackedScene would disappear.

@chanon's suggestion to "let Script exports not need Scripts. prefix, but Scenes will be in Scenes.. (Since Scripts exports will be the most commonly used/typed.)" already deals with this problem, and much more effectively since it preserves the simplicity of creating registered and custom type scripts.

The fact that scene registration doesn't work now has nothing to do with what I'm saying, I'm only proposing solutions.
Unfortunately it may not be sure that the best solution saves what you have already implemented.

It does matter since it determines whether or not this PR even gets integrated, or whether we delay till a different release when the functionality is better prepared.

However, this doesn't take into account custom Resources, therefore this whole single solution could be easily discarded as simple script custom types remain necessary, so I agree this is not a good idea.

Actually, it DOES take into account custom Resources. They are classes after all. Scripts. If you are talking about registering actual resource files (*.tres), that has never been a feature in the first place(?). I even solved a bug earlier on where my custom types were showing up in custom resource locations when they weren't deriving Resource (which I've since fixed).

Probably as long as valid identifiers have a hint of them being a Script or a PackedScene, like a different text color and the right new() or instance() autocomplete, prefixes and naming conventions are not needed.

That's an interesting proposition, but I think we'd still run into circumstances where a registered script or scene may share a name (which isn't a case we can just ignore), and then we'd have to force a "hiding" system where one takes priority over the other, forcing people to change the names of things and refactor if they want to use the hidden typename. It's simply much easier on the user's side if there is a syntactic way of clearly differentiating the two groups.

@samdze
Copy link
Contributor

samdze commented Jun 28, 2018

However, this doesn't take into account custom Resources, therefore this whole single solution could be easily discarded as simple script custom types remain necessary, so I agree this is not a good idea.

Actually, it DOES take into account custom Resources. They are classes after all. Scripts. If you are talking about registering actual resource files (*.tres), that has never been a feature in the first place(?). I even solved a bug earlier on where my custom types were showing up in custom resource locations when they weren't deriving Resource (which I've since fixed).

I was talking about my scenes-only solution, it doesn't support custom types that don't inherit from Node, so it's definitely not a good idea to only have this approach.

That's an interesting proposition, but I think we'd still run into circumstances where a registered script or scene may share a name (which isn't a case we can just ignore), and then we'd have to force a "hiding" system where one takes priority over the other, forcing people to change the names of things and refactor if they want to use the hidden typename. It's simply much easier on the user's side if there is a syntactic way of clearly differentiating the two groups.

This should be a problem that has to be solved anyway:
having two custom types with the same identifier should throw an error even if it happens with scripts only. If scripts and scenes are treated transparently identifier-wise then it makes no difference, the same identifier can't refer to more than one thing, no hiding and no priorities, unique identifiers only.
Were you referring to this circumstance?

@willnationsdev
Copy link
Contributor Author

@samdze Currently, I have made it so that you CAN have a script and a scene that share a name with one another since they are stored in completely separate HashMaps. And I intentionally made this change because I wanted to support that (that way, if someone wants to, they can name the script that goes on the root node of their scene the same thing - they'll still be cleanly accessible from different sources).

Because I do not want to block this possibility by design, I must in turn ensure that there is a syntactic means of referencing which HashMap the user would like to reference when doing a name lookup.

@willnationsdev
Copy link
Contributor Author

willnationsdev commented Jun 28, 2018

Ok, I have now uploaded the Rebuild Documentation logic which will update documentation based on res://addons/<dir>/doc_classes directories and the res://doc_classes directory in addition to all the in-engine locations. Now, only two things remain:

  1. The typed GDScript compatibility changes.
  2. The referencing of scripts without the Scripts. prefix.

@willnationsdev willnationsdev force-pushed the custom-types-editor branch 2 times, most recently from a998ba3 to d1069e9 Compare June 28, 2018 19:26
@jkb0o
Copy link
Contributor

jkb0o commented Jun 28, 2018

Good job! Amazing PR. Love it.
The only my 5 cents.

The first thing is about storing TypeDB in ProjectSettings. This will lead to conflicts if there is more then 1 developer in team. Serialising TypeDB into res://.import/type.db will solve this. Also, as far as scripts are resources too, Godot's import system can be used to make it work with less magic.

The second thing is about Scripts. again. It seems too aggressive. Using reserved character (@ for example) for splitting scripts from native classes in global namespace looks good. This fits into Godot's paradigm about splitting native classes and scripts. var sm = @MyStateMachine and if value is @AmazingThing is best option for me. As far as I understand how parser works this not much differ from parsing Scripts..

I also propose to leave Scenes for much later.

@reduz
Copy link
Member

reduz commented Jun 28, 2018

I admire and respect that you spent a lot of time on this but, as I mentioned before, I plan on resolving this in a different way eventually, and with a much simpler approach. We've been discussing it on Irc for a while and hope to be able to implement it in the coming weeks.

@reduz
Copy link
Member

reduz commented Jun 28, 2018

The way I plan on implementing this is much simpler, and does not require creating any extra singleton classes or modifying any existing ones. I hope you guys can be patient :)

@reduz
Copy link
Member

reduz commented Jun 28, 2018

And yes, it aims to solve all the issues mentioned above. I'm sure it will make more sense once you see the way I will implement it.

@willnationsdev
Copy link
Contributor Author

@reduz since I never heard any details or updates on it, and I didn't hear any feedback from you on the progress I had already made, I must've gotten some miscommunication with you. Didn't realize you had a plan.

There are actually many different parts to this PR. what about the plugin setting modifications, XML documentation tools, etc.? Are those also going to get dropped?

@willnationsdev
Copy link
Contributor Author

Last I recall, you had explained to me that you were fine with me making all of these changes so long as the changes were restricted to the editor and individual scripting language modules. Could you be more specific about what it is you plan to do, that way I can identify what aspects of this PR I can still salvage?

@reduz
Copy link
Member

reduz commented Jun 28, 2018

We have been discussing about adding an API for scripts to extract documentation from them, so it's more or less dependant on each type of Script (ScriptLanguage), and not something global. This really probably needs more discussion.

@reduz
Copy link
Member

reduz commented Jun 28, 2018

Problem is that right now all focus is put on finishing the remaining features to make a 3.1 alpha as soon as possible. I will try my best to add global class names and name based script assignment in the way we discussed it, but further enhancements will probably be better discussed for 3.2 at this point.

@willnationsdev
Copy link
Contributor Author

Well, seeing as how this PR is pretty much getting tossed, I'm gonna close it. I've salvaged what parts of it I can and will issue a new PR that bundles my other changes more cleanly.

@toger5
Copy link
Contributor

toger5 commented Jul 10, 2018

I would like to emphisize that I think this kind of feature would improove Godot A Lot!

I'm super curious for the implementation @reduz is thinking of.
(Hope it gets prioritized as much as possible)

@willnationsdev
Copy link
Contributor Author

@toger5 Yeah, in my opinion the features added by my PR are the biggest thing holding back Godot's plugin development overall, with the Asset Library revisions / add-on system changes being a close second.

Would love to hear reduz's plans in regards to this stuff, but I'm sure he's busy being awesome. ;-)

@willnationsdev willnationsdev deleted the custom-types-editor branch September 23, 2018 20:30
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.