godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.06k stars 65 forks source link

Replace `class_name` with `MyClass extends Node` with implicit icon path #209

Closed aabmets closed 3 years ago

aabmets commented 4 years ago

Describe the project you are working on: Financial modeling software which requires a lot of custom classes.

Describe the problem or limitation you are having in your project: Using class_name is inelegant and looks ugly in the source code.

Describe how this feature / enhancement will help you overcome this problem or limitation: This improvement would stop making me cringe when I'm reading my code.

Show a mock up screenshots/video or a flow diagram explaining how your proposal will work: Replace -> "class_name" -> with -> "as"

Describe implementation detail for your proposal (in code), if possible: ~~Replace keyword "class_name" with the keyword "as", with the following implementation: extends Node as MyClass, "res://myicon.ico"~~

Replace keyword "class_name" with "MyClass extends Node", where the class icon is implicitly inferred from the class script file location, having the same name as the class script file.

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

Is there a reason why this should be core and not an add-on in the asset library?: Because it's elegant and pythonic.

Calinou commented 4 years ago

The keyword as is already used for type casting when using static typing, so I'm not sure about this. Surely it can be made to work, but it might not be trivial.

GameDevLlama commented 4 years ago

why not the other way around?

having a .gd script file like:

MyClass extends Node

func do_stuff():
    pass
aabmets commented 4 years ago

why not the other way around?

having a .gd script file like:

MyClass extends Node

func do_stuff():
    pass

Absolutely brilliant. But how would one define the class icon?

Jummit commented 4 years ago

why not the other way around?

having a .gd script file like:

MyClass extends Node

func do_stuff():
    pass

That's so clean, I love it. I hope it gets implemented in 4.0

Absolutely brilliant. But how would one define the class icon?

class_icon res://icon.png maybe?

Calinou commented 4 years ago

@Jummit To me, the as syntax proposed by OP feels more natural for declaring an icon. You also wouldn't need to write a second line when you want to declare an icon.

If using the as keyword is too cumbersome, we could just use class_name (which basically means we'd put the current class_name declaration on the same line as extends).

Jummit commented 4 years ago

I just had a realization. Simply turning class_name into class would also make things cleaner. Just a thought.

aabmets commented 4 years ago

MyClass extends Node with "res://myclass.ico" ???

GameDevLlama commented 4 years ago

Glad you like the suggestion :-) Got another idea for defining the icon, how about using braces for further class configuration?

MyClass("icon-res.png") extends Node

func do_stuff():
    pass

Probably breaks the clean look, but since the icon more refers to MyClass instead of Node it might be better to put it next to MyClass

aabmets commented 4 years ago

Hmm, well, using MyClass("res://myclass.ico") looks like a constructor, which makes sense. However, when the path of an icon is very long, it will make the code look very dirty.

What if the path of the icon is not up to the programmer to define, but the system implicitly looks for an .ico file with the same file name as the class file name in the same directory and if one is found, automatically assigns that icon to that class?

Example: I have "myclass.gd" in "res://classes/", and in it I have declared "MyClass extends Node". If I want this class to have an icon in the editor, I could put a file "myclass.ico" into "res://classes/" next to the "myclass.gd" file and the engine would automatically assign that icon file to the class node.

We have to think about the reason for class icons. They are useful for developers to make their classes better identifiable during the game development process. There is no need to change the class icons during the game execution, because these icons are irrelevant for the player and probably they are not included into the export exe anyways, which is why there's no need to explicitly declare their path in the source files, as most developers put the class icon files next to the class script files anyways I assume.

It would be like how Windows Media Player discovers and uses subtitle files - they need to have the exact same name as the movie file and be in the same folder next to the movie file.

Therefore, with this knowledge, the script file is spared of explicitly declaring class icons, which keeps the source cleaner and pretty.

Calinou commented 4 years ago

What if the path of the icon is not up to the programmer to define, but the system implicitly looks for an .ico file with the same file name as the class file name in the same directory and if one is found, automatically assigns that icon to that class?

This sounds good to me, but Godot expects PNG or SVG icons for classes, not ICO.

aabmets commented 4 years ago

What if the path of the icon is not up to the programmer to define, but the system implicitly looks for an .ico file with the same file name as the class file name in the same directory and if one is found, automatically assigns that icon to that class?

This sounds good to me, but Godot expects PNG or SVG icons for classes, not ICO.

I didn't know that, but this would work with PNG and SVG also.

Pseudocode running in editor tool:

if new_file in folder:
     if file.extension == "SVG" or file.extension == "PNG":
          var script_file = new_file.name + ".gd"
           if folder has script_file and script_file.class_exists:
                script_file.class_icon = new_file.path
if script_file.save:
     var script_file_png = script_file.name + ".png"
     var script_file_svg = script_file.name + ".svg"
     if folder has script_file_png:
          script_file.class_icon = script_file_png.path
     elif folder has script_file_svg:
          script_file.class_icon = script_file_svg.path
GameDevLlama commented 4 years ago

Therefore, with this knowledge, the script file is spared of explicitly declaring class icons, which keeps the source cleaner and pretty.

Your solution sounds perfect to me :-) There's also no reason why you wouldn't name the icon like you name the class

aaronfranke commented 4 years ago

For some reference, here's the current way things work.

GDScript file without global class:

extends Node

GDScript file with global class:

extends Node
class_name MyNode

GDScript file with global class and icon:

extends Node
class_name MyNode, "whatever.png"

C# file without global class:

public class MyNode : Node

Proposed in https://github.com/godotengine/godot/issues/26875, C# file with global class and icon:

[GlobalClass("res://icon_path")]
public class MyNode : Node

Another important existing proposal: #22 cc @willnationsdev

aaronfranke commented 4 years ago

EDIT: I don't like what I propose here anymore. I'm collapsing it into a <details>.

Now, that being said... to be honest, I don't see why we even need to provide a class name. It's best practice to have a class named `MyNode` stored in `MyNode.gd` (or `my_node.gd`). In C# providing the name of the class is required by the language, but also C# in Godot currently doesn't support mismatching class and file name for attached scripts. So, for GDScript, why not eliminate it altogether? Something like: class extends Node, "res://MyNode.png" Or, even further, we could even eliminate the icon path, and just have it search for `MyNode.svg` followed by `MyNode.png` in the same folder as the script. So, in a nutshell, my idea is to have the class and icon name inferred from the file name, like this: Folder structure: MyCustomNodeFolder/ MyNode.gd MyNode.svg OtherNode.gd NormalScript.gd MyNode.gd: class extends Node # Class name is inferred to be "MyNode", gets icon from "MyNode.svg" OtherNode.gd: class extends Node # Class name is inferred to be "OtherNode". # Searches for "OtherNode.svg" and "OtherNode.png", icon not found so it has no icon. NormalScript.gd: extends Node # No "class" keyword, so not registered as a global class
Jummit commented 4 years ago

How about a class keyword that works like tool? It would register it as a global class with the filename as a name.

GameDevLlama commented 4 years ago

While the idea of a class keyword isn't too bad I am not sure if it'd be a 100% good solution since I guess there are a couple of programmers out there who want to name their files in snake_case while having their classes in CamelCase.

This would force some people adapting their style to either filesystem or coding.

willnationsdev commented 4 years ago

@aaronfranke

It's best practice to have a class named MyNode stored in MyNode.gd.

Is it though? I've generally followed the C++ source code's conventions where possible. So I use snake_case filenames with PascalCase class names, as @TheWhiteLlama mentioned above.

In C# providing the name of the class is required by the language, but also C# in Godot currently doesn't support mismatching class and file name for attached scripts.

I would actually be okay with all scripting languages having to follow this convention, since it makes the codebase more consistent and would simplify writing the code. However, I also think that a lot of people would be frustrated by it since they often prefer the freedom Godot gives them to do things the way they want.

However, even if you did implement the auto-collection of the name/icon in the means you've specified, it wouldn't affect people who didn't use it, so I think it would be a good QOL improvement. The only concern I have would be the effect it has on online code readability. But in most cases, it would probably be alright since people often add a # my_file.gd tidbit to code segments anyway.

aaronfranke commented 4 years ago

It's also possible to have our cake and eat it too. We can have the class name "generator" start with a case conversion, where the first letter, and each letter following _, is capitalized, and all _ are removed. So my_node.gd and MyNode.gd would both set the class name to MyNode. This would also work for myNode.gd and My_Node.gd if someone wanted to do that.

To allow flexibility, perhaps the format class MyNode extends Node, "res://my_node.png" is best, just with class, MyNode, and the icon being optional.

willnationsdev commented 4 years ago

@aaronfranke That sounds really good to me! And that would include being able to have the icon seek out a same-class-named icon in the same directory, optionally. Those would all be really convenient.

erodozer commented 4 years ago

reusing the class keyword sounds like it could get confusing. GDScript files already allow you to define internal classes, something I use quite a lot in my own projects. It also wouldn't make sense to use an implied class name without a few adjustments to the limitations of class_name. Right now you can have scripts embedded in tscn files that have class_name on them but using implied name would not be compatible with that.

Instead of adding to the classdb using code, perhaps a cleaner approach would be allow you to specify class name and icon as part of the resource options in the editor, and that could provide a unified approach for both GDScript and C#

wyattbiker commented 4 years ago

I was looking at the reserved keywords and class_name stuck out like a sore thumb, and found this thread with good suggestions. My suggestion is:

extends <someclass> [as <class name>[, <icon path>]]

willnationsdev commented 4 years ago

@wyattbiker I like that syntax, personally. Avoids the class term confusion, but still communicates the intent decently without resorting to a custom keyword.

@nhydock I don't like the resource-property-based approach. It is convenient in the sense that all languages would be able to be configured with a single API, but there would be no place in the script source code that actually stores the information. You either can't pass the class name along if you share the script online (not conducive to open source) or Godot would have to edit the source code to match the property value (which puts you back where you started AND is just a bad idea overall).

Xrayez commented 4 years ago

I just had a realization. Simply turning class_name into class would also make things cleaner. Just a thought.

I just had a flashback now: https://github.com/godotengine/godot/issues/31056#issuecomment-518162389.

wyattbiker commented 4 years ago

I just had a realization. Simply turning class_name into class would also make things cleaner. Just a thought.

I just had a flashback now: godotengine/godot#31056 (comment).

class is a keyword used for inner classes. So would be confusing and possibly break existing .gd files.

https://docs.godotengine.org/en/3.1/getting_started/scripting/gdscript/gdscript_basics.html#example-of-gdscript

# Inner Class

class Something:
    var a = 10
Xrayez commented 4 years ago

Except that the definition of inner classes end with a colon:

godot_class_name

So instead of the error the parser could treat this as class_name definition instead (the first occurrence of such usage).

Xrayez commented 4 years ago

If we fear the possible collision between existing class keywords, how about renaming class_name to global_class, since this is what it actually does, making your class available globally, isn't it?

erodozer commented 4 years ago

I think at this point it'd be better to just leave things as is. It won't be changing for 3.2, and there's a lot of documentation already using class_name, as well as a bunch of publicly available scripts. We'd just be changing a key syntactical feature so it "feels good" without really adding much value. People can just look at the plethora of examples and tutorials that the community has come up with now to understand how it works and when to use it.

vnen commented 4 years ago

If we fear the possible collision between existing class keywords, how about renaming class_name to global_class, since this is what it actually does, making your class available globally, isn't it?

I thought about calling it global instead. Just a keyword rename, function is the same. Though I'm not sure if it's worth changing just for the sake of it.

vnen commented 4 years ago

I think at this point it'd be better to just leave things as is. It won't be changing for 3.2, and there's a lot of documentation already using class_name, as well as a bunch of publicly available scripts. We'd just be changing a key syntactical feature so it "feels good" without really adding much value. People can just look at the plethora of examples and tutorials that the community has come up with now to understand how it works and when to use it.

3.2 won't be the last Godot version, we can always change it later. About existing stuff, yes, it can be a bit problematic to breaking compatibility. But if we can make it objectively better then it might be worth it. "Feels good" is also an argument: if it's pleasant to write/read code we can be more productive.

The problem is not "understanding". The problem is that it doesn't feel natural. Which is true since it was pretty much botched in the engine without a lot of thought.

I still believe we can get to something better (I'm not fond of the class_name keyword either).

GammaGames commented 4 years ago

I really like that MyClass extends Node syntax

Edit: or the as option. They feel like a true extension and like they fit in woth the language, while class_name feels like it was hacked in (for lack of a better explanation)

vnen commented 4 years ago

I'm currently rewriting the GDScript parser and this proposal was brought to my attention since it's a good time to settle it.

I see no real consensus in this thread. People seems to want to change it but there's no clear winner in the alternatives.

To gauge opinions from the community, I started a Twitter poll: https://twitter.com/vnen/status/1257705167077679104

I do have my favorites, but I'll avoid pushing what I want just because I'm writing the code. Though some of the options makes parsing harder for no much gain, so I'll refrain from implementing them (unless there's no better alternative, which doesn't seem to be the case), and didn't put those in the poll. Twitter doesn't make it easy by allowing only four options, so I hope I got the better alternatives.

Duroxxigar commented 4 years ago

@vnen I can't vote on that on twitter because I don't have a twitter.

I'm in favor of just doing class Name extends parent and if it doesn't extend from some derivative of Node or Resource - it just doesn't get added to the global menu.

EDIT: I completely disregarded the possibility of inner classes extending from nodes/resources. My mistake.

vnen commented 4 years ago

I'm in favor of just doing class Name extends parent and if it doesn't extend from some derivative of Node or Resource - it just doesn't get added to the global menu.

Problem with class Name extends parent is that it looks exactly like an inner class, apart from the colon at the end. It makes it not only harder to parse, but also confusing for people reading the code.

Also, while only Node and Resource types are added to the menu, they are always added to the global namespace if use the class_name (or whatever the new alternative is). The menu presence is a bonus.

Duroxxigar commented 4 years ago

@vnen Yeah - you must've been typing the reply before I actually edited my statement. I mentioned that I completely didn't even think about that part with inner classes.

In that case, mark my vote for global as a keyword. (Again, because I don't have a twitter account)

eon-s commented 4 years ago

Why do inner class definition should be different? It may be even better change inner classes too to make these more clear, like:

class Something extends Node('res://icon.svg'):

  inner class SomethingElse extends Reference:
Calinou commented 4 years ago

@eon-s That adds a level of indentation to the whole script, which isn't great for small displays or split editors :slightly_frowning_face:

Xrayez commented 4 years ago

Yeah, I like the idea behind the inner class if this improves parsing stuff, but I wouldn't really want to indent script like that.

eon-s commented 4 years ago

@Calinou ok, make it without indentation, since it is has the inner prefix it is explicit.

vnen commented 4 years ago

I want to point out that extends is optional. So while I put Name extends Node int the poll I'll not use it because without extends it's the class name hanging there alone, which is weird.

kyleguarco commented 4 years ago

I really like the idea of using inner and global. I think ditching the class keyword altogether would be a good idea, since it's causing so much confusion.

My take on the syntax

global MyClass [icon_path]
extends Object

var _something

func _init() -> void:
    _something = InnerClass.new()

inner InnerClass:
    global
    extends Object

    # Maybe global can be used here as an "access modifier", for deciding whether
    # the class can be accessed outside MyClass (e.g MyClass.InnerClass.new())

    var _something
GammaGames commented 4 years ago

Maybe instead of inner, something like local? It would more closely match the way global is used in other areas (transforms come to mind)

Jummit commented 4 years ago

This is clean:

extends Object

class InnerClass extends Object:
   …

This is not:

extends Object
class_name MyClass, "path/to/icon"

class InnerClass extends Object:
   …

Solution:

MyClass extends Object, "path/to/icon"

class InnerClass extends Object:
   …

This does not change how it functions and doesn't add new keywords.

dalexeev commented 4 years ago

From the proposed options, in my opinion, it is extends Node as Name.

This option has two disadvantages:

  1. The reverse order of subclass and base class. Inner class: class MyClass extends Node Script: extends Node as MyClass

However, on the other hand, this is the opposite, an advantage. A script is not required to have a global name, but is required to have a base class. It is not necessary* for inner class to specify the base class, but the inner class must have a name.

* Proof ```gdscript tool extends EditorScript class T: var a func _run(): var t = T.new() print(t.get_class()) # Reference ```

It is logical that in both cases the obligatory part is written first, and the optional part at the end.

  1. Similarity to object as SomeClass syntax. (But I don't think this is critical.)

It also seems to me that annotations are good here:

@class_name MyClass
@class_icon ./my_class.png
extends Node

# ...

or:

@class MyClass
@icon ./my_class.png
extends Node

# ...

or even:

@class MyClass
@base_class Node
@class_icon ./my_class.png

# ...
Xrayez commented 4 years ago

Regarding annotations, I think it may be good to have tool keyword to be replaced by @tool annotation, especially when it comes to fine-tuning editor vs in-game behavior (@tool - runs everywhere, @plugin - runs only in editor) as described in godotengine/godot#17592.

Xrayez commented 4 years ago

@dalexeev

The reverse order of subclass and base class

Yeah that's what makes me cringe too, I always tend to write class_name Name extends Node, but I'm biased as a C++ dev. extends Node as Name seems to be familiar to those with Python programming experience (import os.path as path ?), and I'd personally like that we remove the association with Python for the good sake as people tend to request GDScript to look like Python nowadays, at least that's my impression.

dalexeev commented 4 years ago

@Xrayez The problem of notations like Name extends Node, global Name extends Node etc is that it seems that name is a required attribute of each script. What is wrong with GDScript, most often a script is an unnamed class.

There are two ways to avoid this:

  1. split class declaration into two lines (including current implementation);
  2. or place optional part at the end of line.

In other cases, I say "GDScript is not Python", but in this case it seems to me that this is the best way available, although not ideal. :smiley:

Jummit commented 4 years ago

it seems that name is a required attribute of each script.

That's not true.

Without name: extends Node With name: ClassName extends Node

dalexeev commented 4 years ago

ClassName extends Node

Not good, because all other entities use the preceding keywords: var, const, enum, signal, func.

My personal opinion, but usually something optional is added at the end, and not at the beginning of the line.

kane-thornwyrd commented 4 years ago

My 2cts:

In another language, the concept of class is not tied to a file, a file represent somewhat of a namespace/module which may contain classes, functions and constants, and the only things that are exposed are those which are explicitly exported via a dictionary…

And we can also create "meta-modules", which import some part of others, and export them under its own "namespace".

So, a big plus for those who like to:

Plus it's just getting ride of the current class_name keyword, keeping the class and extends ones and tweak them a bit, and wrapping each file/module load or preload in a function that return only the module dictionary, which enable any script that load them to get only the dictionary, which would act as a namespace , or only the required exposed parts we want via destructuring, but we don't have destructuring in GDScript (yet ?)… :cry:

Side effect, this is very nice for when the lambdas will be available :wink:


Example: myModule.gd

enum UnitStatusEnum {UNIT_NEUTRAL, UNIT_ENEMY, UNIT_ALLY}
class MyClass extends Node:
    func _init() -> void:
        var b = SomeInnerClass.new()
        print(b.a)

class SomeInnerClass extends Node:
    var a: String = "Hello" setget a_set, a_get

    func a_set(new_value: String) -> void:
        a = new_value

    func a_get() -> String:
        return a + " World !"

module = {
    "UnitStatusEnum" : UnitStatusEnum,
     "MyClass" : MyClass,
}

myOtherScript.gd:

const myModule = preload("myModule.gd")

onready var unknownUnit: Node = $unknownUnit

func _ready() -> void:
    var something := myModule.MyClass.new()
    if unknownUnit.status == myModule.UnitStatusEnum.UNIT_NEUTRAL:
        print(something.a)

P.S. The language highlighter for gdscript on Github is not yet aware about static typing, so I trolled it by specifying python as language, hence the "strange" coloration…

P.P.S. I just don't care about icons, but as annotations that would be cool I guess…

erodozer commented 4 years ago

I personally like the treating files as a module approach, that's how python and javascript both handle things. Honestly, I also like the explicitness of using preload instead of ever relying on the classdb because doing so means you have side effect free code and you don't have to worry about namespace collisions in your classdb when designing reusable libraries. The only thing stopping me from only using preloads is class_name is the only way to expose custom types to the editor, and preloads can't be used for type hinting. If the type hinting thing could be fixed, the I could honestly live without ever using class_name.

But, to throw out an alternative approach, have we considered possibly avoiding having this defined in code at all. Instead you could have an explicit file written by hand, akin to Java Spring Beans, where you register types that your project uses. I know this is currently already built for you automatically in your project.godot, but since it's only for gdscript it feels limited.

I guess it depends on how "easy" it is for users to understand that's what they'd have to do to expose types to the editor. Would also make it simpler I feel to allow defining types in other languages and native libs so we have a consistent way to do it. It would also be helpful since it acts as a form of documentation for addons of what they intend to expose to your project.

project.types
---
MyResource:
  source: res://resources/my_resource.gd # module is the class
  icon: res://resources/my_resource.png # custom icon in editor
OtherResource:
  # support exposing inner classes
  source: res://resources/extra.gd/OtherResource
  icon: res://resources/other_resource.png
MyCSType:
  # support paths to other languages and their exposed classes
  source: res://resources/example.cs/ExampleClass
# extend this types by including types defined in an addon
:include res://addons/cool-library/project.types
:include res://addons/music-mixer/project.types