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

Make class_name act the same as add_custom_type but with typed name #1876

Closed
Shadowblitz16 opened this issue Nov 23, 2020 · 12 comments
Closed

Comments

@Shadowblitz16
Copy link

Shadowblitz16 commented Nov 23, 2020

Describe the project you are working on:
NetworkingTest

Describe the problem or limitation you are having in your project:
I want to make custom types which I can then use to return and inherit from using their type name.
the issue is the following..
add_custom_type doesn't register the type as a typed name and can't be used to staticly type things
class_name shows the name of the script attached unlike add_custom_type
class_name scripts can be removed unlike add_custom_type but can still be extended
class_name can't be disabled or enabled with plugins like add_custom_type

Describe the feature / enhancement and how it helps to overcome the problem or limitation:
Make class_name create true custom types like add_custom_type.
They would have the following features...

  • Scripts cannot be removed when created from node browser
  • Script names are not shown in node browser only type name and icon (icon is parent icon if nothing is specified)
  • plugins can register class_name types to only be enabled when the plugin is enabled

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:

#plugin types can be inherited and used for static typing
class_name Socket, "res://addons/network/socket.png" extends Node

class_name SocketClient, "res://addons/network/socket_client.png" extends Socket

class_name SocketServer, "res://addons/network/socket_server.png" extends Socket

func _enter_tree(): 
	add_existing_type(Socket )
	add_existing_type(SocketClient)
	add_existing_type(SocketServer)

Note

  • The one above is what class_name types would looks like and what add_custom_type already does
  • The one below is what class_name looks like now
    image

nodes created with the node browser have read only scripts like what add_custom_type has now
image

If this enhancement will not be used often, can it be worked around with a few lines of script?:
This would make types so much better.
You can the use custom types by type name now!

Is there a reason why this should be core and not an add-on in the asset library?:
Because it already is its just merging the pros of add_custom_type and class_name

@Shadowblitz16 Shadowblitz16 changed the title Make class_name act the same as add_custom_type Make class_name act the same as add_custom_type but with typed name Nov 23, 2020
@Shadowblitz16
Copy link
Author

Shadowblitz16 commented Nov 23, 2020

here is a related issue but not the same
Edit: again with the mistyped link

@willnationsdev
Copy link
Contributor

add_custom_type doesn't register the type as a typed name and can't be used to [statically] type things

This is effectively what Script Classes were made to do. They were introduced as a replacement for Custom Types that enable you to access the types at runtime and use them for static type checks.

class_name shows the name of the script attached unlike add_custom_type

Some people want this and some people don't. It isn't that big of a deal, so I'd just make it something configurable in the EditorSettings as it's just a small change in how the Editor displays data rather than a change in how the data is architected or interpreted on the backend.

class_name scripts can be removed unlike add_custom_type but can still be extended

If class_name scripts can be removed, I'd probably consider that almost a bug-like behavior (though technically it's an enhancement). So yeah, that should probably be fixed for Script Classes. The transparent script icon should also be introduced.

class_name can't be disabled or enabled with plugins like add_custom_type

This one's a little bit complicated. The problem is described here. It is possible to define script classes in an addon which register things globaly via the script class system. These could be custom visual graph nodes or maybe custom ResourceFormatLoaders, etc. And doing so does not require being cognizant of any enabled or disabled status related to a plugin because addons don't inherently require a plugin script to exist (e.g. an addon that just contains image/model/sound assets wouldn't need a plugin script to be turned on for those to be recognized by the engine). Because of this, there isn't a way to turn on or off those globally registered types.

I suppose you could manually check to see if the addon directory the type came from has any sort of plugin configuration file, and if not, just register it as "on" by default. Maybe that could work?

Well, the plan with #22 is to remove the CustomType system and add Script Class support for every language. Part of that would also involve attempting to recreate all of the existing features of CustomTypes with Script Classes (like having the transparent script icon, and not being able to remove the script, etc.). At least, that's how I see it.

@jonbonazza
Copy link

Irt script class files showing up in the editor lists, this is something id like to see be able to be configured per script class. The problem for me is when creating an addon, i dometimee have nodes that use class_name that are intended to stand alone, while other times, i have nodes that are intended to only be instanced as scenes, but still use class_name for static typing benefits. These latter scripts should not show up in the editor lists since they cannot be used on their own.

Im not really sure how to solve this tbh.

@Shadowblitz16
Copy link
Author

add_custom_type doesn't register the type as a typed name and can't be used to [statically] type things

This is effectively what Script Classes were made to do. They were introduced as a replacement for Custom Types that enable you to access the types at runtime and use them for static type checks.

I like how Custom Types work.
But I think the logic should be moved over to script classes

class_name shows the name of the script attached unlike add_custom_type

Some people want this and some people don't. It isn't that big of a deal, so I'd just make it something configurable in the EditorSettings as it's just a small change in how the Editor displays data rather than a change in how the data is architected or interpreted on the backend.

I don't see why the script is important. Once used they should be deriving from the type name not the script
This especially becomes true the more static typing is supported.
I would even say deriving the class from path would be better to become obsolete.

class_name scripts can be removed unlike add_custom_type but can still be extended

If class_name scripts can be removed, I'd probably consider that almost a bug-like behavior (though technically it's an enhancement). So yeah, that should probably be fixed for Script Classes. The transparent script icon should also be introduced.

I mean you can remove the script from the node in the scene view if it created with class_name.
I think it should be more like custom types were you can't remove them from the node

class_name can't be disabled or enabled with plugins like add_custom_type

This one's a little bit complicated. The problem is described here. It is possible to define script classes in an addon which register things globaly via the script class system. These could be custom visual graph nodes or maybe custom ResourceFormatLoaders, etc. And doing so does not require being cognizant of any enabled or disabled status related to a plugin because addons don't inherently require a plugin script to exist (e.g. an addon that just contains image/model/sound assets wouldn't need a plugin script to be turned on for those to be recognized by the engine). Because of this, there isn't a way to turn on or off those globally registered types.

I suppose you could manually check to see if the addon directory the type came from has any sort of plugin configuration file, and if not, just register it as "on" by default. Maybe that could work?

Well, the plan with #22 is to remove the CustomType system and add Script Class support for every language. Part of that would also involve attempting to recreate all of the existing features of CustomTypes with Script Classes (like having the transparent script icon, and not being able to remove the script, etc.). At least, that's how I see it.

I like the issue and I supported it, but I think if your going to remove custom type and support script classes, plugins need to beable to disabled and enable these features.

@jonbonazza I had a the same issue which is why I posted this.
I wanted to have the logic of a custom plugin type but I was unable to use it in static typing.

However I don't know how to go about making types unable to be initialized publicly, or protectedly and still beable to use them for static typing.
Languages like C# require a constructor to to inherit so IDK how it would be done for all languages

@willnationsdev
Copy link
Contributor

class_name shows the name of the script attached unlike add_custom_type

Some people want this and some people don't. It isn't that big of a deal, so I'd just make it something configurable in the EditorSettings as it's just a small change in how the Editor displays data rather than a change in how the data is architected or interpreted on the backend.

I don't see why the script is important. Once used they should be deriving from the type name not the script
This especially becomes true the more static typing is supported.
I would even say deriving the class from path would be better to become obsolete.

Note that your original assertion, that the class_name displays the name of the script in the CreateDialog, is what I'm commenting on. I simply say that some people might want the name of the script displayed while others might not. For example, I might see a user-defined type in the tree, and I might want to know that it is a user-defined type so that I can know that I have the ability to find the script in my project locally and view its source code, which I can't do for engine types (have to look on GitHub or in a cloned codebase for that).

If you don't care about looking at the source code for the types you are using, then you could simply turn it off, but I think, by default, it is important to identify which types are user-defined (and where they are defined) vs ones that are not. But making a toggle to switch it off doesn't have any negative repercussions in my mind.

However, your response is referring to something else entirely; namely, the ability to reference an inherited typename by its Script Class name or its filepath. And while you suggest that people shouldn't be able to reference the filepath at all, I would say that is not a good idea. There is a clear use case for having local filepaths to dependent scripts: Script Class names are global. In a large project, it would be extremely tedious to have to give unique names to every little script you develop, making it more and more like C code with things like Project_Namespace_Namespace2_Scene_SupportClass1 becoming the name of various scripts.

In contrast, if you "namespace" your scripts by which scene they are part of, and you give each scene its own directory, then it becomes quite easily to have naturally namespaced scripts purely by virtue of its filepath. Then, you only need to reference script class names for more centralized, system-wide types.

The point being, it should be up to the user to decide what their preference should be. Godot shouldn't have an opinionated method that it forces on users.

I mean you can remove the script from the node in the scene view if it created with class_name.
I think it should be more like custom types were you can't remove them from the node

Yeah, I was agreeing with you on that point. Script Classes should be modified to behave that way probably.

I like the issue and I supported it, but I think if your going to remove custom type and support script classes, plugins need to beable to disabled and enable these features.

Yeah. Whatever we do, we need to be able to successfully transfer all the existing features of CustomTypes to Script Classes, where at all possible.

@jonbonazza

Irt script class files showing up in the editor lists, this is something id like to see be able to be configured per script class. The problem for me is when creating an addon, i dometimee have nodes that use class_name that are intended to stand alone, while other times, i have nodes that are intended to only be instanced as scenes, but still use class_name for static typing benefits. These latter scripts should not show up in the editor lists since they cannot be used on their own.

I've had a couple of things crop up in my mind like this. You'd need another script class flag, like we already have script_class_name and script_class_icon_path. For example:

  • script_class_hidden: (your idea) Cannot be instantiated manually, but can be created as part of a scene.
    • Hides the class in CreateDialog (easy to do)
    • Prevents instantiation in non-scene contexts (complex*).
  • script_class_virtual: Cannot be directly instantiated, but inherited types can be, similar to CanvasItem/Container (complex*).
  • script_class_static: Cannot be instantiated in any form. Can only be used for static methods. (complex*).

* - For anything that requires controlled instantiation of scripted types, it would require a major refactoring of the entire engine, so implementing it would be extremely difficult, heavily invasive, and difficult to get approved. The engine only centrally manages the instantiation of a script from the Script class itself. But Script doesn't have any awareness of how script class information is configured. That is all metadata tracked by the ScriptServer and defined by concrete implementations of the Script and ScriptLanguage classes. Whenever the engine wants to instantiate a script, it must 1) load/get the script, 2) instantiate/get an Object of the base type of the script, and then 3) attach the script to the Object. The exact process of creating the script will change based on the circumstances, and none of the implementations for the logic are centralized anywhere. Therefore, you would need to implement a centralized instantiation/assignment of script data in the ScriptServer and then replace all instances of script instantiation with ScriptServer calls. This would be a massive undertaking, and could easily lead to dozens of bugs throughout the codebase if one isn't careful. If you were to attempt to enforce controlled runtime instantiation of scripted types, you'd better be prepared to argue your points and prove that the effort would be worth it to the core devs before any of them would approve the work.

For each of these "flags", you have to add new fields to the GlobalClass data in ScriptServer, add new fields to the FileSystem singleton's cache of file data (since it constantly scans changes to files, extracts this data, and caches it for subsequent reads by the engine), update the file caching code there to actually set those fields, and update every script-class-supported scripting language (see #22) so that the Script classes have flags for those fields as well as some sort of interface/interpretation (whether text or GUI) to define that data which the ScriptLanguage implementation extracts and assigns to the Script during system-wide scans.

You might also need to make changes to the runtime interpretation of script code to account for these flags in every language (e.g. create a parse error in GDScript if trying to instantiate a virtual class, rather than making it runtime error).

  • GDScript uses keywords in 3.1-3.2 and annotations in 4.0
  • VisualScript has GUI fields in the Editor (in 22)
  • C# has annotations (in 22)
  • NativeScript has properties on the actual NativeScript resource file, the *.gdns.

@Shadowblitz16
Copy link
Author

Shadowblitz16 commented Nov 25, 2020

@willnationsdev
The script name still isn't necessary because you can click on the script icon in the scene and it takes you to that script.
Then you can see what script it is from there.

The make load obsolete for scripts part wasn't a part of the suggestion it was just a an example of what could be done in the future.

I agree and do think that projects and namespace scope would need to be done if that was ever going to happen though.
but that's not part of this suggestion either

@willnationsdev
Copy link
Contributor

The script name still isn't necessary because you can click on the script icon in the scene and it takes you to that script.

Regardless, some users want to know, at a glance, whether a type is built-in or not straight from the CreateDialog. The only indicator is the script filename next to the class name. Otherwise, people would have to first make an instance of it to know if it was user-defined (assuming it had a unique icon already). I'm saying, I agree that some people, like yourself, might want it hidden. So just make it a setting that users can control. Then everyone can get what they want.

The make load obsolete for scripts part wasn't a part of the suggestion it was just a an example of what could be done in the future.

Ah, gotcha.

I agree and do think that projects and namespace scope would need to be done if that was ever going to happen though.
but that's not part of this suggestion either

Yeah, I brought namespacing up with Reduz on IRC once and he stated, in no uncertain terms, that the script class system won't be having namespaces brought into it. So, if namespacing were ever a thing, it would only ever be language-specific, similar to what C# already natively supports for its own C# types. There wouldn't ever be a cross-language namespacing system. You have to settle for C-style naming schemes by making the namespace part of the typename, e.g. GodotNextThirdPersonController or some such.

@Shadowblitz16
Copy link
Author

The script name still isn't necessary because you can click on the script icon in the scene and it takes you to that script.

Regardless, some users want to know, at a glance, whether a type is built-in or not straight from the CreateDialog. The only indicator is the script filename next to the class name. Otherwise, people would have to first make an instance of it to know if it was user-defined (assuming it had a unique icon already). I'm saying, I agree that some people, like yourself, might want it hidden. So just make it a setting that users can control. Then everyone can get what they want.

ah ok well then as long as its optional.

I agree and do think that projects and namespace scope would need to be done if that was ever going to happen though.
but that's not part of this suggestion either

Yeah, I brought namespacing up with Reduz on IRC once and he stated, in no uncertain terms, that the script class system won't be having namespaces brought into it. So, if namespacing were ever a thing, it would only ever be language-specific, similar to what C# already natively supports for its own C# types. There wouldn't ever be a cross-language namespacing system. You have to settle for C-style naming schemes by making the namespace part of the typename, e.g. GodotNextThirdPersonController or some such.

also that's too bad

@Shadowblitz16
Copy link
Author

@willnationsdev I have been thinking about this more and I really think that showing the class names is a bad thing.
Not only does it break the feel of oop but I honestly think it fights godot's own design philosophy of being oop

I honestly think if you want to search for it that bad you can either use the search function for the class name or click on the icon on the node.

This way its not really hiding anything except for the script name but you have the class name for that and you can easily look it up in the file explorer or click on the script icon.

Its not any less confusing then how other game engines do it.

@Shadowblitz16
Copy link
Author

closing in favor of #2612

@YuriSizov
Copy link
Contributor

You didn't actually close it. Do you still want to close it?

@Shadowblitz16
Copy link
Author

Shadowblitz16 commented Apr 16, 2021

oh sorry I can close it
ty for letter me know

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

5 participants