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

Split class_name responsibility into two keywords #4493

Open
kubecz3k opened this issue May 4, 2022 · 21 comments · May be fixed by godotengine/godot#63758
Open

Split class_name responsibility into two keywords #4493

kubecz3k opened this issue May 4, 2022 · 21 comments · May be fixed by godotengine/godot#63758

Comments

@kubecz3k
Copy link

kubecz3k commented May 4, 2022

Describe the project you are working on

TailQuest, other networked project involving bigger team of developers

Describe the problem or limitation you are having in your project

I would like to propose to separate the functionalities of class_name keyword.

Currently it's doing two distinct things that, from my experience very often should not happen at the same time:

  1. It registers new class so we can use it in static typing when writing our gdscript
  2. It registers script to be in 'add child node' window.

The fact that it does the second point feels off for me. If I would write static typed game the 'add new node' window would be polluted with hundreds of entries which are not usable at all.
It's so for two reasons:

  • Many of the types are not even meant to be public, they are used by some well encapsulated systems and they never should be instanced like that casually, their purpose is very strict and they are made only for that one use case (we are simply breaking encapsulation here).
  • I'm maybe old fashioned but, my godot workflow is very strongly scene-centric. In most project that I worked in, I always recommended to think rather in scenes then scripts, it has implication that most scripts cannot even function when they are not part of the proper scene, and what we are adding to the tree with class_name keyword is unusable.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

The ideal way to solve this would be to separate the functionality of class_name into couple distinct features:

  1. Ability to define programming type which is used when writing strongly typed code
  2. Ability to register script to be part of 'add new node' window
  3. Ability to register a scene to be part of 'add new node window' (oh yes, please!)

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

  1. We need new keyword if we want for the node to be present in 'add new node' window (alternatively we could ignore it if no icon is provided)
  2. When that keyword is used, the editor should automatically check if in the same directory as a script is there a scene that has the same filename, if yes the scene should be instantiated instead of 'raw' node.

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?

It's core

@kubecz3k
Copy link
Author

kubecz3k commented May 4, 2022

I wanted to separate the problem from my opinion, this post will be mostly about my point of view, and what I personally see as an ideal solution, and where I come from (we don't need to discuss that part, just wanted to give a chance to understand my point of view a little better):

So I would even go as far as merging 2 and 3 into one, and only allow to register scenes in add new node window, let me explain. (2 is Ability to register script to be part of 'add new node' window 3 is Ability to register a scene to be part of 'add new node window')

When writing any game various systems will have tendency to grow and became more complex. Let's say I wrote a script that represents a prop in the game world. Level designer used it in 12 levels dozens of time in each level. Now it turns out that we need to add whole new subsystem to the prop, the only way we have to do it is to do it from script, even if given subsystem is very complex and having it as a configurable at editor time makes a lot of sense. Not to mention it's quite normal during development cycle for any given system to continue growing and became more complex. Having ability to just separate systems into well encapsulated subscenes is simply helpful to be able to understand the relations between subsystems quicker. But it's too late, since that decision need to be taken early on: in this case the prop should have been a scene not a script.
Since there is no clear way to predict future, (some logics might grow exponentially, other might stay small) the best course of action most of the time is to just think (and use) in terms of scenes, not scripts.
To think of it in other way, the way how I see a scene -> it's a node that has ability to grow while internally preserving very well defined and encapsulated structure of subsystems.

@kubecz3k
Copy link
Author

kubecz3k commented May 4, 2022

References:
this proposal is similar to #1047 but I think it gives more reasoning on why current design is undesirable
The more complete proposal that deals with script <-> scene pair part of the proposal: #1909

@brunosxs
Copy link

brunosxs commented May 4, 2022

I agree. I feel discouraged to use class_name exactly because of the polution of classes that gets added to the "Add Node" menu.

@MarianoGnu
Copy link

In godot 4 we could use class_name for point1 and class_name specific annotations for points 2 and 3

@aaronfranke
Copy link
Member

Instead of two keywords, it would make sense for the editor behavior to be an annotation. I would do something like this:

# Visible in the editor and usable as a class in GDScript.
@editor
class_name MyClass
extends Node

# Not visible in the editor, still usable as a class in GDScript.
class_name MyClass
extends Node

# Bonus idea, optional: Visible in the editor, but not usable as a class in GDScript.
@editor
extends Node

@kubecz3k
Copy link
Author

kubecz3k commented May 5, 2022

#1909, seems to deal with 'script <-> scene pair' part of the proposal in a more complete way

@h0lley
Copy link

h0lley commented May 6, 2022

doesn't class_name already take additional arguments? I'm pretty sure you can do something like
class_name MyClass, "res://path/to/an/icon.png"

so rather than introducing new keywords or annotations (btw @editor is way too easy to confuse with @tool), why not make it possible pass some more arguments.

Ability to register a scene to be part of 'add new node window' (oh yes, please!)

I think this would only make sense as a possible feature of built-in scripts. Or how else would you bind a script to a particular scene? edit: Ah, just saw the other proposal you linked, guess that takes care of that.

@Mickeon
Copy link

Mickeon commented May 6, 2022

so rather than introducing new keywords or annotations why not make it possible pass some more arguments.

I think it's proven at this point that functions with lots of undescriptive arguments prove hard to maintain and read. I suppose that'd be the same case, here.

The @editor annotation is interesting. I can see the same annotation potentially having different purposes based on context, depending on what it is associated to (Script, Property, Function...), which could be confusing, but is once again interesting nonetheless.

@mrpedrobraga
Copy link

mrpedrobraga commented Jun 13, 2022

Perhaps,,,

extends Node
class_name MyClass
@editor_scene_tree_instantiable

@willnationsdev
Copy link
Contributor

I support @aaronfranke's approach to this, but I don't think that an @editor annotation without class_name should be a thing. All of the editor functionality (showing up in the CreateDialog and appearing in my exportable Resources feature) relies on the ScriptServer which is all about associating a global name with a specific filepath. If there's no global name for the file, then you'd have to add another tracking mechanism for scripts to show up in the engine (which I wouldn't recommend).

@h0lley
Copy link

h0lley commented Jul 30, 2022

the current behavior should be the default though.

not wanting certain node classes to be instantiable from the create new node modal is likely much less common, so that's when the extra annotation should be needed - not the other way around.

that would also be more beginner friendly, as users learn about the ability to have their custom node classes appear in the create new node modal by stumbling across it rather than having to read up on a special annotation.

also, rather than completely hiding nodes, it would probably make more sense to instead utilize this existing feature:

image

this way, the modal can still be used as a nice overview to browse the full inheritance tree, while at the same time not being "polluted" as these grayed out classes are not searchable.

with that, it would no longer make sense for the annotation to be @editor, which was a poor choice anyways as that essentially has the same meaning as @tool. nothing super intuitive comes to mind right now though. can someone check what the flag to make node classes appear as these grayed-out non-selectable items is called in the source?

Ability to register a scene to be part of 'add new node window' (oh yes, please!)

that would be really cool, but seems out of scope for this proposal.
the way I envisioned that once was that users would add class_name to a scene's root node script that is built-in (stored with the tscn file), which would couple the node class with the scene and thus enable scenes to appear in the create new node modal.

@willnationsdev
Copy link
Contributor

the current behavior should be the default though.

not wanting certain node classes to be instantiable from the create new node modal is likely much less common

I actually don't think it's "much less common" necessarily. The bigger a project is, the more likely it is you will have more people on your team and therefore the more likely it is you will want to be able to define types that you don't want to be easily accessible from the editor. And if we are going to have a compatibility-breaking change, this would be the time to do it.

also, rather than completely hiding nodes, it would probably make more sense to instead utilize this existing feature

while at the same time not being "polluted"

This would mean allowing scripts to define uninstantiable, abstract classes. Godot Engine does not currently support this for Scripts (don't ask me why cause I don't quite understand it myself). Either way, the types would still be showing up in the hierarchy (it's still polluted because it's visible) and thus would become things that users may not want to have exposed at all. You wouldn't want people to start asking about these greyed out types when it will never be relevant to that team member in the first place.

by stumbling across it rather than having to read up on a special annotation.

If the examples/tutorials they learn from already include the "special annotation", then it will work the same way. Especially if...

with that, it would no longer make sense for the annotation to be @editor, which was a poor choice anyways as that essentially has the same meaning as @tool

I propose that we simply re-use the @export annotation, but on the class_name keyword as it then falls in line with the "export to the editor / rest of the engine" concept that feels intuitive when combined with the other uses of the @export annotation. Then users don't need to learn a "special" annotation, and they simply re-apply the concept to the top-level class of the file rather than any given property inside of it.

@h0lley
Copy link

h0lley commented Jul 31, 2022

hm, I can't really think of any situation where it would be an actual problem for node classes to be listed in the modal. granted, I'm not working in a big team, but in my mind, when someone doesn't know what a thing is and does, then they're not going to touch it. as long it's not something that's in their way, it's generally not a problem.

I would however have actual use for the ability not to hide, but to "disable" classes from being selectable.

image

in this case, if the feature existed as I described it, I'd like to flag Actor and Combatant as non-selectable & grayed out.
while it is desirable that they still appear in the inheritance tree to provide context for the selectable Enemy, Player, and NPC.

This would mean allowing scripts to define uninstantiable, abstract classes.

ok, I haven't thought about that those classes are not only displayed in a special way in the modal, but also not instantiable in code - so that exact feature cannot be adapted for user classes. but all we care about is the display part. so I don't see any blocker in showing classes that are registered with a special annotation in this way.

(it's still polluted because it's visible)

hardly an issue when those classes are somewhere nested & collapsed, and again, they don't show up the moment you are filtering which is the only practical way to use the modal.

If the examples/tutorials they learn from already include the "special annotation", then it will work the same way. Especially if...

bold of you to assume that new users go through those :)

@willnationsdev
Copy link
Contributor

bold of you to assume that new users go through those :)

If anything, I would just suggest that the default script template automatically add the @export annotation to the class_name declaration if the user creates the script with a non-empty Class Name: field in the ScriptCreateDialog. That way new users randomly trying things out will still get the same behavior.

@nathanfranke
Copy link
Contributor

nathanfranke commented Jul 31, 2022

the current behavior should be the default though.

not wanting certain node classes to be instantiable from the create new node modal is likely much less common, so that's when the extra annotation should be needed - not the other way around.

Usually I don't want the script to be instantiable from the editor, but rather the scene.

Proposal

Will replace the create node item with a scene:

(Note: This looks similar to godotengine/godot#21187)

@editor_scene("res://game/player.tscn")
# relative paths would be awesome too - @editor_scene("player.tscn")
extends Node
class_name Player

As proposed earlier, a class name can be hidden from the editor:

@editor_hidden
extends Node
class_name SuperSecretNode

@willnationsdev
Copy link
Contributor

Perhaps you are more thinking of my scene-binding proposal, #1935? If so, this would be a totally separate thing from whether or not the node is visible in the editor though. It forms a 1-to-1 relation between a script and a scene and makes instantiation of the script with .new() and from the add-a-node dialog defer to instantiation of the bound scene rather than the script (as opposed to the normal behavior of instantiating the native base type and then simply assigning the script to the new instance, thereby instantiating a ScriptInstance which runs its constructor from the script contents).

If you believe that idea is a good approach, then I would prefer that you go and upvote that proposal as opposed to trying to mix that idea into this one (otherwise, this proposal is just trying to "propose" too many different things).

From my understanding, the primary purpose of this proposal is to provide a means for users to clearly distinguish between a request for assigning a global name to a script file and a request to expose said globally named script file to the Godot Editor. As a matter of scale, I believe that exposure to the Editor should be an opt-in operation syntactically (even if a default template could add it out-of-the-box for scripts with class names to make it user-friendly), so I'm going to try and implement it with the @export annotation.

I could then ideally / potentially follow it up with implementing @global and/or @scene(...) to implement #4740 and #1935.

@aaronfranke
Copy link
Member

aaronfranke commented Nov 4, 2022

If #5641 is implemented, it would help with many of the use cases here.

  • You could annotate a script as @abstract and then you will no longer be able to use .new() on it, but you can still attach it to a node in a scene. This somewhat solves the problem of tying a script to a scene, because you can no longer directly instance the script by itself. It might feel weird to call a script you instance as abstract, but you can think of it as the scene is extending the script.

  • In the Create New Node dialog, abatract classes will either be hidden or show as unselectable (like in the image in @h0lley's comment). Same with the resource picker dialog. This solves the problem of not wanting them to show up in the editor.

It hits several birds with a remarkably small stone (only about 50 lines of additions and nearly zero performance cost).

@kubecz3k
Copy link
Author

kubecz3k commented Nov 4, 2022

I'm not sure if being able to instance scene which has abstract script is such a good idea.
I would more see such scene as an abstract scene, that needs to be inherited (relation between script inheritance and scene inheritance usually is 1 to 1, so when I have abstract script it quite often is attached to some abstract scene which in reality should never be instanced directly in the project)

@kubecz3k
Copy link
Author

kubecz3k commented Nov 4, 2022

To give some examples, in most games I was working on there were couple the same abstractions like "Level" and "Enemy" which had quite significant node structures defined by scene but which could never be instanced directly. They always needed to have an inherited scene complementing the node structure and a script which was complementing the base logic.
I love the idea of having abstract classes, but it will not change a lot if you can instance abstract class and use it normally just by doing your usual workflow of composing game from scenes...
(That being said I'm not opposing this behavior in initial implementation if it makes the implementation simpler, but I would say we should not make it officially part of the workflow since I strongly feel it eventually should not behave like that)

@willnationsdev
Copy link
Contributor

I'm with @kubecz3k on this. If there's an @abstract annotation added to GDScript, then it shouldn't deviate from the expected behavior of what "abstract" means in every other typical programming model. That term should be reserved for if/when that functionality is actually added to GDScript itself.

If you're looking to define a script that should only ever be integrated into a scene and never used for direct instantiation, then that should really be it's own entirely separate proposal.

@Shadowblitz16
Copy link

I would like this so that I can type name my scenes.
It would be really useful if built in scripts could extend from other built in scripts and could be type named but not exported.

This would mean I can define everything in a scene and when I pass the scene/node instance as a parameter I can use static typing

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

Successfully merging a pull request may close this issue.