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

Add scene binding for optional tight-coupling between Node-extending Scripts and PackedScene resources #1935

Open
willnationsdev opened this issue Dec 3, 2020 · 43 comments

Comments

@willnationsdev
Copy link
Contributor

willnationsdev commented Dec 3, 2020

Building on proposals #1224, #1906, #1909, based on a suggestion from @dalexeev (last time I'll @ you about this, promise).

Describe the project you are working on

No active project, but have developed plugins where this was relevant (relates to any project that creates a scene with a root script...so most everything).

Describe the problem or limitation you are having in your project

Mostly same as #1909. There's a difference between PackedScene and Node-extending Script even though there are specific subsets of Scripts that are completely dependent upon a PackedScene to function correctly. But if you start to use a PackedScene, the resource is no longer a "class" as far as typing goes, since only Scripts constitute a "class" in Godot's runtime type system (so static typing doesn't work properly). Furthermore, Scripts and Scenes have potentially divergent inheritance hierarchies. This complexity can be frustrating to deal with in code (custom logic to handle each case or write custom tools for handling scene inheritance) and from an architecture perspective (have to document that a given script should never be instantiated without its corresponding scene, etc.).

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

Similar to how the Script Class system enables a script to have a global name and icon associated with it, I recommend we introduce a separate Scene Binding feature which allows a Script to optionally declare that it should be created with a specific scene.

(Code sample courtesy of dalexeev)

# my_scene.gd
extends Node
class_name MyScene
@scene("res://my_scene.tscn")
# At parse time, the Godot Editor verifies that the script is attached to the root of `res://my_scene.tscn`.

# another.gd
var my_scene := MyScene.new()
# At runtime, the GDScript implementation loads and instantiates `res://my_scene.tscn`, not the script alone.

Because it's an annotation (or some form of script metadata) rather than a custom resource like #1909, this approach enables any scripting language, not just ones supporting "built-in" types, to add support for Scene Binding.

Specifically, this "hardcodes" a 1-to-1 relationship between a given Script and a PackedScene, so for scripts with this flag, you would not be able to create another PackedScene that has the same script on its root node.

If you wanted to create a new PackedScene that derives a base PackedScene, it would necessarily be required to also have a derived Script extending the base/root script of the base scene. We'd likely need the create-inherited-scene / derive-script utilities in the SceneDock to automatically adapt by checking for a scene-bound script to better automate this process for usability reasons.

Once you have a scene-bound script, similar to #1909, you would no longer be able to create future scripts in the same inheritance hierarchy that do not bind themselves to a scene in the same scene inheritance hierarchy.

In addition, as #1909 mentions, because you'd have a scene bound to a script, you don't have to rely on the "currently open scene" to get accurate hinting for script editing (like NodePaths, etc.). Since you know exactly which scene is related to the script, you can just look it up in the ScriptEditor backend.

Ordinarily, a script that is "built-in" doesn't have support for script classes. This is because they are bound to a PackedScene and don't have a dedicated file. However, I'd suggest we amend that support as a supplemental feature, that way a built-in script that has a script class name and a scene binding enables users to use built-in scripts for their scenes if they so choose, but still have static typing support. Could even, in that case, create a parser/compiler warning if the script class name doesn't match the PascalCase naming of the scene file (or something to that effect?).

This would not impact the typical usage of any script that doesn't bind itself to a scene. It simply creates more assurances/guarantees for the scripts which do have the information present.

I also don't necessarily think (yet) that every scene-bound script must have a script class name. The ability to guarantee synchronization of inheritance hierarchies, presence of dependencies, and relevant scene-hints irrespective of the currently open scene are not ultimately related to having access to a global variable and static typing of the type. Although, someone can feel free to disagree on this point in the discussion below.

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

  1. Create another map in the ScriptServer to maintain which scripts have a scene assigned to them.
  2. Create a hidden Dictionary in project.godot to serialize the information.
  3. Modify EditorFileSystem to...
    1. add it to the file cache
    2. pull the data from Scripts
    3. pull the data from built-in scripts at the root node of a PackedScene resources
  4. Create new methods in the ScriptLanguage interface to gather this information from scripts (similar to script class info).
  5. Update each scripting language implementation to...
    1. have the Script implementation store and interpret/gather a scene binding value.
    2. have the Script implementation check/guarantee that the script satisfies scene-binding requirements if value is present.
    3. have the ScriptLanguage implementation implement the required interface method
  6. Modify the SceneDock to facilitate creating new scene/script combinations, either on their own or by way of deriving an existing script bound to a scene.
  7. Modify the CreateDialog to give the name of the scene file a type is linked to, rather than the script file, if the script is scene-bound (thoughts?).

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

There are many users who just prefer this structure and would use it for all of their scripts, especially because it more closely matches other popular tools (e.g. Unreal Blueprints). It simplifies the project structure and improves usability without sacrificing the ability to use third-party text editors or have dedicated files for script content.

No. While there are ways to create scene instances from a script automatically, none of them are satisfactory.

  1. Requires boilerplate code for every class with scene-binding. No inheritance or trait system would assist here as it would require parameterization of the information.
  2. Every workaround either replaces the script with a scene or creates a child that is an instance of the scene. The former means that you have a different script (and, in essence, you have a crap script that purely exists to connect a script with a scene which is an egregious code smell). The latter makes it so you have a different node hierarchy than intended (an intermediate node is inserted to handle the connecting of typename with scene).
  3. None of the workarounds easily or safely ensure that static typing with the scene type is possible.
  4. None of the workarounds address the issue of synchronizing divergent script/scene inheritance hierarchies.
  5. None of the workarounds address the issue of script instantiation without scene dependencies (i.e. it's still possible to create the script without the scene). This should be 100% prevented at the language implementation level.

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

The changes themselves require changes to backend Editor systems (file caching, SceneDock tools, CreateDialog rendering) as well as each language implementation. Therefore, it cannot be accomplished with scripting alone.

@dalexeev
Copy link
Member

dalexeev commented Dec 3, 2020

Just for completeness:

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

The closest working analogue is

# my_scene.gd
extends Node
class_name MyScene

# another1.gd
var my_scene := preload("my_scene.tscn") as MyScene

# another2.gd
var my_scene := preload("my_scene.tscn") as MyScene

But this approach has problems:

1) you have two separate files which are strictly related or dependent on one another, but yet no system helps to facilitate the maintenance of that relationship, and 2) by extension of point 1, if you suddenly decided to change the scene file later on to use a different script, then suddenly your GDScript example would stop working. You'd have to manually go to the code and change it to reference a different type hint.

This suggestion is to move and automate scene instantiation into one script:

# my_scene.gd
extends Node
class_name MyScene
@scene("res://my_scene.tscn")

# another1.gd
var my_scene := MyScene.new()

# another2.gd
var my_scene := MyScene.new()

And further. Maybe I'm wrong, but the description of how the proposal works looks too complicated. I'm not familiar with the implementation, but something tells me that relatively little code will need to be rewritten to add a single annotation and change the behavior of .new(). Probably, it will take much more effort to support this feature in the editor.

@willnationsdev
Copy link
Contributor Author

willnationsdev commented Dec 3, 2020

As far as I know, there is no centralized place that handles instantiation of a script. You simply have an underlying Object, and then you set the script resource on that Object, at which point, the Script resource creates a ScriptInstance and manages it. The fact that there is a "constructor" for the script is a kind of lie where the Script implementation, say, GDScript, may provide its own utility function to enhance usability.

For example, if you look at the scripting API for the base Script class, you won't see a new method (link). It just-so-happens that GDScript, CSharpScript, and NativeScript all separately implement their own independent new function. For some weird reason, VisualScript doesn't do this (wut).

Now, if you amended things so that Script wasn't purely an interface and actually had its own base implementation of new which each scripting language had to respect in their overridden versions (or something like that?), then it might be easier cause you could add this logic there. But it also wouldn't change the fact that the things that happen during a new() are still happening on an existing instance when you simply assign a script to an Object. So, whatever work you do to setup scene-based dependencies on a Node during new(), you'd also have to redo that same logic to some extent when you just assign a script to a Node. That is, you'd have to not only assign the script to the node, but also ensure that any child node trees are created and attached to the Node, signal connections are made, etc. Effectively turning the node into an instance of the "scene" associated with the class (if those dependencies don't exist yet) before actually creating the ScriptInstance and having the script's constructor / _ready logic trigger.

Edit: Either that, or you simply agree to only enforce the scene dependencies when a new instance of an object is created, not when the runtime type of the object is changed by re-assigning the script. But, if you did that, idk if editor logic for changing a script would work properly or not. Or perhaps, going the other way, doing all of this stuff during script assignment would make things in the Editor too complicated, and so it would have to be done only during new object instantiation (which is the main use case this proposal is targeting anyway).

@andy-noisyduck
Copy link

andy-noisyduck commented Jan 15, 2021

+1 from me.

We went through the same process of wanting to tightly couple scenes and scripts. We ended up implementing a solution in entirely in script, that whilst not perfect, does achieve some of what you want. I wanted to outline it here because the fact we felt we needed to build this is a decent indicator that the feature is missing, but also to indicate how far you can get in a "script only" solution, and whether that solves the problem "enough" for most (some?) cases.

Apologies this is C# (I also think that the lack of metadata means this probably isn't possible in GDS either)...

We ended up with an attribute that tied an implementation class to a scene path:

[PackedScene("res://UI/SettingsMenu.tscn")]
public class SettingsMenu : Control
{
   /* ... */
}

We then implemented a SceneResolver which takes a strong type T and spits out an instanced scene for that type. E.g.

var menu = resolver.Instance<SettingsMenu>();

It's then just a case of enforcing convention of only instancing scenes that way.

You get some of your wishlist this way:

  • No boilerplate code to instancing.
  • You get a strongly typed instance out of the resolver.
  • The SceneResolver has checks to ensure the associated class matches the script type on the node. This prevents you changing the script type on the scene to something incompatible, and also catches the if you rename the scene path to another valid scene but of the wrong type.

Whilst beyond the scope of this proposal, having scene instantiation via common method also allows us handle dependency injection, auto-wire up any [Ready] properties, and run localisation on the affected scene.

However, this doesn't solve the problem that scene scripts can still be instantiated directly without a scene attached. Worse, an implementer requires knowledge of the class being instantiated to know if its "a plain old class", or "with a scene" so that they can instance it with the right method. It also only works for C# scripts (though in our case, that is fine).

Having something at engine level would be able to catch these cases. It's also a non-trivial amount of work to implement something at script level for what is a pretty desirable design goal. Tight coupling of scenes is potentially useful to many.

@Shadowblitz16
Copy link

Shadowblitz16 commented Jan 24, 2021

why is this needed @scene("res://my_scene.tscn")?

I think it would be better to do something like...
extends SceneName it would then extend the type the scene root is

@willnationsdev
Copy link
Contributor Author

@Shadowblitz16

It would be too system-breaking to make scenes themselves type-able entities. This proposal adds scene paths as metadata just like the global class names are script metadata. But I do have an alternate proposal (which I don't like as much) where a scene is tightly bound to a built-in script (#1909). In that version, you do simply extend the ScriptScene (since it is a type of Script), but it also can be loaded as a PackedScene resource. The downside is that it doesn't support non-built-in scripts, so you can only use the ScriptEditor to modify the files (which mostly leaves GDScript, VisualScript, or no-autocompletion-or-syntax-highlighted C#).

@andy-noisyduck
Copy link

I think it would be better to do something like...
extends SceneName it would then extend the type the scene root is

It's not clear how that would work with user defined types? If I understand you correctly, either that restricts scenes to extending built in types, or requires the editor to know about all user defined types so that it can be set on the root. Maybe the latter is OK, but it expands the scope of the proposal somewhat.

It would also make compatibility with non GDScript languages a bit tricky as they will not be able to resolve the appropriate superclass from the scene.

@willnationsdev
Copy link
Contributor Author

@andy-noisyduck I think what he is suggesting is that...

  1. Scenes should be valid "types" as far as the type hints are concerned, i.e. you should be able to derive from a PackedScene what the engine class or script if any the root node of that PackedScene is/has.
  2. Scenes should have global names assigned to them just like scripts do.

This is relatively difficult to do though since much of the engine specifically treats only Scripts like they are classes. PackedScenes are very clearly treated as simply premade constructions of nodes, not as classes. You'd have to make very far-reaching changes all throughout the engine and add a lot more complication to operations all over the place to account for PackedScenes as additional valid interpretations of a "type". So I find it extremely unlikely that it would ever be done.

It's much more practical to just form a 1-to-1 relationship between the two C++ types so that you can derive one from the other and add account for scenes in a few specific places by converting them to their Script counterpart.

Also, if you had globally named scenes, then you'd end up 1) bloating the global namespace even further, and 2) having always-loaded PackedScene resources which are naturally going to be much beefier in memory consumption compared to scripts. If you started assigning global names to scenes, and loading them on engine startup, it would generally slow down startup times and make Godot overall start wasting RAM by keeping scenes around when they aren't actually needed anymore.

So that idea is very unlikely to ever be implemented.

@Shadowblitz16
Copy link

I don't think scenes should be tied to scripts or types but instead we should be creating new types in the scene

@Shadowblitz16
Copy link

Shadowblitz16 commented Apr 20, 2021

@willnationsdev why can't the scene load the script instead of the script needing to bind the scene?
not only is it cleaner in the script but it achieves the same thing.
scenes aren't edited outside the godot editor anyways.

@aaronfranke
Copy link
Member

@willnationsdev Having read the OP, I don't buy the reasoning behind this system (aside from the NodePath hints), I have not encountered issues like this. If I don't want a script attached to a node it doesn't belong on, I just... don't attach it.

That said, this proposal would have no downsides as far as I can tell, so I gave it a 👍. People who don't want this can just not use it, and people who do want this can be happy.

@Shadowblitz16 why can't the scene load the script instead of the script needing to bind the scene?

It already does. The scene already loads the script. That's what happens when you attach the script to a node in the scene.

@Shadowblitz16
Copy link

Shadowblitz16 commented Apr 20, 2021

@aaronfranke thats not the same thing
yes the scene loads the script but I don't think thats the issue.
I meant having a option to lock in a script and force inheritanced scenes to either a: leave the script or b: extend it

also note that scenes have a built in broken requirement fixer dialog, scripts don't so if a scene path changes the script will break and we will have to manually fix it, if a script path changes in the scene we can fix it next time we open the scene.

@dalexeev
Copy link
Member

@willnationsdev Having read the OP, I don't buy the reasoning behind this system (aside from the NodePath hints)

Let's say I want to make a dialog box for an RPG game. I have a scene like this:

DialogBox: Control
  Icon: TextureRect
  Label: RichTextLabel

How to use this scene?

1. Classic version

const DialogBox = preload("res://dialog_box.tscn") # PackedScene type

func _ready():
    # ...
    var dialog_box = DialogBox.instance() # Node type
    add_child(dialog_box)
    dialog_box.set_icon("my_character")
    yield(dialog_box.say("Hello!"), "completed")

Too verbose and doesn't support static typing. In every script where I want to use DialogBox, I have to preload the class into a constant. I can use class_name in the DialogBox script, but DialogBox.new() will not load the entire scene for me, only its root node.

2. The current compromise
I have to change the structure of the scene a little:

DialogBox: Control <-- dialog_box.gd
  Body: Control <-- dialog_box.tscn
    Icon: TextureRect
    Label: RichTextLabel
# dialog_box.gd
extends Control
class_name DialogBox

func _init() -> void:
    name = "DialogBox"
    add_child(preload("dialog_box.tscn").instance())
    App.get_tree().current_scene.add_child(self)

# ...

And now, after all the agony, using DialogBox becomes more convenient, plus static typing starts working:

func _ready() -> void:
    # ...
    var dialog_box := DialogBox.new()
    dialog_box.set_icon("my_character")
    yield(dialog_box.say("Hello!"), "completed")

This proposal allows you to avoid adding an extra intermediate Body node, allows you to leave the original structure in the scene file, not to delete the script from the scene file.

@aaronfranke
Copy link
Member

aaronfranke commented Apr 20, 2021

@dalexeev Thanks, this is a really good explanation. The "current compromise" you posted isn't how I would do this though. What I would do in the current version of Godot (assuming you want to have a named class for the scene root):

DialogBox: Control <-- dialog_box.tscn, with dialog_box.gd attached
    Icon: TextureRect
    Label: RichTextLabel
# dialog_box.gd
extends Control
class_name DialogBox

# Whatever else you want in dialog_box.gd
# If you want it to always work for detecting the scene, don't attach this script anywhere else.
const DialogBoxScene = preload("res://dialog_box.tscn") # PackedScene type

func _ready():
    # ...
    var dialog_box = DialogBoxScene.instance() # Scene instance
    add_child(dialog_box)
    if dialog_box is DialogBox: # Static typing works
        pass

It doesn't solve the problem of needing to load in the scene, and you can't use DialogBox.new(), but it does have static typing, and doesn't have an intermediate body node. It also allows you to replace or extend the script on the scene without worrying about the script being vital because it sets up a scene.

But yes, I see your point, it makes sense. It's not something I've desired myself, but it does make sense.

@dalexeev
Copy link
Member

dalexeev commented Apr 20, 2021

I understand there are different approaches. For example, you can create a global method App.create_dialog_box() or something else. Perhaps my example is not the best one. But there really is some problem in this, since I am not the only person who goes in this direction and faces this.

const DialogBoxScene = preload("res://dialog_box.tscn") # PackedScene type

func _ready():
    # ...
    var dialog_box = DialogBoxScene.instance() # Scene instance
    add_child(dialog_box)
    if dialog_box is DialogBox: # Static typing works
        pass

You can just use the as keyword:

const DialogBoxScene = preload("res://dialog_box.tscn") # PackedScene type
 
func _ready():
     # ...
     var dialog_box = DialogBoxScene.instance() as DialogBox # Static typing works

@Shadowblitz16
Copy link

Shadowblitz16 commented May 8, 2021

I would just like to put my opinion here.
can we please not use annotations to link scripts and scenes together?

I am sure there are other ways of doing it.
linking the script in the scene instead of the scene in the script would be one way.

I also don't like preloads because if the asset is moved it breaks the scripts.
type names don't do that because they are registered from within the script

EDIT: I have a suggestion on just forcing inherited scenes to extend the scripts already there.
This could be made optional if needed.
It also seems like it would be fairly easy to implement.

the only thing we would need to do is disable the remove and add script options in the editor and make the script property setter check to see if the scene is inherited and if it is return without setting the script

there could be a additional check so see if the script is locked in that scene and disallow changing the lock outside of the scene its in.

so people can choose to use the feature or not

EDIT2:
I guess the point is to create custom types?
my true oop suggestion is linked as a reference so that's why I am wondering.
@willnationsdev can you provide what it would look like in the editor?
would this be creating custom node types and linking them to a scene?
or would it be guaranteeing the scripts are on the nodes?

@aaronfranke
Copy link
Member

linking the script in the scene instead of the scene in the script would be one way.

This can already be done though. Just attach the script to a node in the scene.

@Shadowblitz16
Copy link

Shadowblitz16 commented May 8, 2021

that's not true though the script from the scenes can be removed so there is no guarantee its that type.
its a very lose relationship

@willnationsdev
Copy link
Contributor Author

willnationsdev commented May 10, 2021

@Shadowblitz16

linking the script in the scene instead of the scene in the script would be one way.

That's a viable option. Shift the script->scene relationship to a scene->script relationship, that way the Editor can handle auto-updates to the scene binding settings should the scene or script move. There's no need to edit the script when updates occur, and no stale info creeps into the data. Might actually be a better approach. Perhaps as some sort of script-locking toggle button in the viewport toolbar? The interface details would need to be ironed out.

I also don't like preloads because if the asset is moved it breaks the scripts.

If we changed it to store the info in the scene rather than the script, then the relationship would still be stored using a path, but the path could be programmatically updated at a whim by the Editor since it isn't contained within a script file. So the above change would actually fix any breakages caused by moving assets.

type names don't do that because they are registered from within the script

If this comment is suggesting we use typenames rather than filepaths to establish the relationship, I'd advise against it. That would block users from using built-in scripts with scene bindings, and there's technically nothing stopping someone from doing that otherwise so I don't see any reason to block it. While a built-in script wouldn't auto-instantiate a scene when the script attempts to instantiate, it would get the other benefits of guaranteeing that child scenes must have a script that inherits the same type. And with the script being a built-in script that is uninheritable, it effectively forces the scene to be a sealed scene that cannot be inherited. Not sure if there's a use case for that, but it certainly becomes a possible "feature" with this system.

I have a suggestion on just forcing inherited scenes to extend the scripts already there.

If scene-bindings existed, then they would include this requirement. The scene and script would have a 1-to-1 relationship (so no other scene could be bound to the same script). If a derived scene were to be made, it would be required to use either the same script or a derived script, lest it violate the scene-binding relationship and trigger a static compile-time error.

there could be a additional check so see if the script is locked in that scene and disallow changing the lock outside of the scene its in.

so people can choose to use the feature or not

Well, naturally we wouldn't require all scenes to be bound to a script, so yeah, it'd be entirely optional. Otherwise we'd be breaking compatibility and locking down use cases to the point that it irritates a ton of people.

I guess the point is to create custom types?
my true oop suggestion is linked as a reference so that's why I am wondering.

The point is to better capture the essence of your "true oop" concept in a way that is manageable and doesn't conflict with the Godot philosophy. Namely, we end up forcing an alignment between the script and scene inheritance hierarchies and modify the scripting languages so that they defer to scene instantiation rather than their default script instantiation when a scene-binding is defined for the script.

can you provide what it would look like in the editor?

Well, the original plan, with an annotation, wouldn't really have any rendering in the editor. Although, I suppose, you could have the CreateDialog display a scene file rather than a script file beside any global script class names.

With the new approach you inspired above however, you'd need somewhere in the editor to turn on the binding between the script and the scene, so a locking toggle button would work well imo. Like I said, would probably be some button added to the viewport toolbar, or maybe in the scene dock somewhere. Not sure.

would this be creating custom node types and linking them to a scene?
or would it be guaranteeing the scripts are on the nodes?

Under the hood, this feature would do a few things.

First, add a Dictionary to the project.godot file that maps a script file path to a scene file path. If any changes were made to the filesystem, that Dictionary gets regenerated (and so paths are automatically re-mapped), just like how the script class system's data is generated. So, it wouldn't link "custom node types" to a scene, but rather any script that has the binding defined for it.

Second, the ScriptServer would then load in this scene-binding settings data and make it available to the rest of the engine API at runtime.

Third, language modules and maintainers would then program logic into the instantiation of each scripting language's Script implementation that would explicitly check for the presence of the scene-binding in the ScriptServer. If present, instantiation would simply instance the scene (though, it would mean you wouldn't be able to use custom constructors for those classes). If not present, the language would continue down the traditional instantiation path.

Fourth, the Editor would provide support for maintaining 1-to-1 script-scene relationships by checking various operations for violation of the relationship and reporting errors. These checks would be things like...

  • renamed files are automatically remapped to prevent stale data errors
  • if somehow a mapping existed for a nonexistent script/scene file path, that would be logged as an error.
  • When setting a root node's script property: ensure the assigned script respects the script-scene binding requirements if present for an inherited scene. If not, operation would be blocked and an error logged.
  • Would probably want the built-in ScriptEditor to have some kind of visual indication that a given script is bound to a scene, either in the sidebar list of scripts or at the top when the given script is open (I prefer the former).

@Shadowblitz16
Copy link

it sounds good to me.

@willnationsdev
Copy link
Contributor Author

willnationsdev commented May 10, 2021

The biggest concern with this approach would be the fact that it shifts storing the binding information's source material from the script file to the PackedScene file, which means you have to make changes to the .tscn/.scn file format. Thankfully, since it's an optional feature, it would hopefully not break compatibility, but I'm not sure if that's the case since scene files have a version number for their format. If that were incremented, after the 4.0 launch, it could mean that this feature has to wait till 5.0 (definitely too late to add it to 4.0 at this point since I don't have time and no other heroes are stepping up. Alpha is coming soon to my understanding). Unless it ends up being totally okay to add an optional .tscn file format change in 4.1, but idk what the rules are for stuff like that.

@dalexeev
Copy link
Member

dalexeev commented May 10, 2021

linking the script in the scene instead of the scene in the script would be one way

One of the problems is that a script in another programming language can be attached to the node. That is why the script cannot inherit the scene and vice versa, as you suggested earlier, that is why it will be difficult to change the direction of the Script → Scene link. Type systems operate in the languages themselves, so the scene type must be defined/set in the script, not in the scene.

Let's start with how the scenes are loaded. Using the PackedScene.instance() -> Node method. The problem is that the type is lost. Instead of Node, the root node of the scene can be any built-in node, and in addition a script (preferably GDScript) can be attached to it. Then, in terms of "statically typed" GDScript, the result will be some custom type that extends Node.

If GDScript supported parametric types (and there would be no difficulty with the fact that scripts are just resources for the core of Godot, and not full-fledged types), then ideally it would be like this: PackedScene<T>.instance() -> T. But, although GDScript does not support parametric types, there is an analogue: PackedScene.instance() as T. This already works in 3.x. By using the as keyword, you can make sure that the scene is of type T, otherwise the expression will be null. Static typing works great with as, hints work, type mismatches show an error, and so on.

It's just that it is not very convenient to write preload("res://my_scene.tscn").instance() as MyType every time, plus you cannot pass arguments to the MyType constructor. Also it doesn't work with built-in scripts, but they have other problems as well.

I suggested (and willnationsdev supported) a script-side approach to the problem so that MyType.new() could be used for these purposes and the linked scene was implicitly loaded (when using special annotation). Yes, it is theoretically possible to store this data in a scene file, not a script. But it still needs to be read by the GDScript implementation, even though the data is stored in the scene file (if we still want MyType.new() option). This is not good, because scenes are universal and GDScript is not the only scripting language in Godot. I'm not familiar with the implementation, but I'm pretty sure it will add unnecessary complexity and degradation to Godot's code.

Another possible solution to this problem is named scenes (scene-side approach). Load entire scenes, but somehow determine their type (global registry?). And this should somehow work in other languages as well. But reduz is categorically against named scenes, and it also seems to me that this is a bad option, because it will still require separate integration with language type systems, so there is no point in creating a duplicate system.

I also don't like preloads because if the asset is moved it breaks the scripts.
type names don't do that because they are registered from within the script

Perhaps we need to improve preload so that renames automatically update paths?
Already, if you try to delete a resource referenced by preload, Godot will warn you:

The @scene annotation path can also change automatically when renaming/moving the scene.

I also want to emphasize knowing your love for guarantees. We shouldn't try to treat the entire scene as a single type, only its root node is meaningful to the type system. Yes, we need the entire scene to load, with all child nodes. The nodes are linked into a tree, but these links are not indestructible. But there is no strict guarantee that remove_child() will not be called. And this is another of the reasons why we need to go from the script, and not from the scene.

I guess it's time to get @vnen's opinion, as we can argue with each other endlessly without a useful outcome.

@willnationsdev
Copy link
Contributor Author

@dalexeev

One of the problems is that a script in another programming language can be attached to the node. That is why the script cannot inherit the scene and vice versa

The feature of script-scene binding need not be constrained only to GDScript. All that matters is that a given language's Script implementation incorporates the Editor-defined script-scene binding settings into its logic for deciding how to instantiate itself.

Type systems operate in the languages themselves, so the scene type must be defined/set in the script, not in the scene.

Let's start with how the scenes are loaded. Using the PackedScene.instance() -> Node method. The problem is that the type is lost.

It is the Script implementation which effectively decides whether any "static typing" rules are satisfied by an operation. Since the script-scene binding must be defined statically at editor-time, the editor logic is actually the thing that can perform these sorts of type checks and ensure that the root node's type matches up with the associated script. This has no bearing on the direction of the relationship.

plus you cannot pass arguments to the MyType constructor.

Regardless of which direction you choose to go, if you had logic that conditionally deferred instantiation to a PackedScene resource based on whether a script-scene binding setting was present, then you'd be forced to ignore any parameters and use a parameterless constructor just like PackedScene expects. After all, since you don't know what the script's constructor is doing with the arguments, you can't just make assumptions and do things in this alternate scene-based constructor. This feature would really be about assigning a PackedScene as the constructor for the type, more or less.

Yes, it is theoretically possible to store this data in a scene file, not a script. But it still needs to be read by the GDScript implementation, even though the data is stored in the scene file

Yes, this is true. And I think this should be perfectly doable if it isn't a problem to change the scene file format accordingly for an optional piece of data without breaking compatibility. If not, then this feature would probably have to wait till 5.0.

This is not good, because scenes are universal and GDScript is not the only scripting language in Godot.

Again, no reason the original feature posted here should only be constrained to GDScript.

I'm not familiar with the implementation, but I'm pretty sure it will add unnecessary complexity and degradation to Godot's code.

I don't think so. If the information is coming from scripts, then the existing logic in the EditorFileSystem singleton that singles out script files and queries them for script class information can be reused to additionally ask for scene binding information. No matter what, this information would get 1) saved to project.godot, 2) deserialized back into the ScriptServer on startup, and 3) made available to the constructor logic of Script instantiation procedures in each respective language that wants to support and implement the scene-binding feature. If you were to draw the data from scenes instead, the only difference would be a few small adjustments to the EditorFileSystem so that it also singles out scene files. And of course, the data would have a GUI interface in the editor rather than being defined in script.

Another possible solution to this problem is named scenes (scene-side approach).

But reduz is categorically against named scenes,

@Shadowblitz16's "true oop" concept can be more or less realized effectively with the scene-binding feature alone imo (at least, most of it), provided that the scripting languages react to scene-bindings properly. Having global variable names for scenes isn't required. You can have scenes define a binding to a script without the scene needing to have a global name.

The @scene annotation path can also change automatically when renaming/moving the scene.

It would be very strange if Godot made changes to non-machine-generated text-based file formats when hardcoded file paths were changed. It certainly could be done, but that isn't conventionally how Godot handles user-edited documents.

The nodes are linked into a tree, but these links are not indestructible. But there is no strict guarantee that remove_child() will not be called. And this is another of the reasons why we need to go from the script, and not from the scene.

I don't really see the justification either way. If a script has elected to defer its constructor to a PackedScene instantiation, then it knowingly is dealing with whatever consequences are associated with the end result of instantiating that scene, even if nodes are removed after-the-fact. The same holds true of a derived class removing child nodes that are created and maintained by a base script class. So it isn't really relevant imo.

The main thing that changes with Shadowblitz16's "define it from a scene" suggestion is having the scene-binding itself be determined from the PackedScene resource, without a hardcoded path in the script content. This doesn't have any bearing on where the instantiation process is triggered from, which can still be done from a Script. It's all about where the editor would be drawing the scene-binding data from. And the data source for that is fairly arbitrary and can be adapted to our needs. Putting it in a PackedScene, which has a machine-generated file format, makes it easier to defer to the Editor for maintenance when file paths change.

@dalexeev
Copy link
Member

dalexeev commented May 10, 2021

Yes, it's probably not as bad as I drew in my imagination, but the change of the relationship direction still adds a few restrictions compared to the original proposal, which has the biggest problem only with preload. But this problem can (and probably should) be solved. preload("path") can be painlessly updated automatically, for preload(CONSTANT_EXPRESSION) to display a warning when renaming resource, and dynamic expressions are not supported. This is important for all kinds of resources, not just scenes. Likewise for @icon annotation and potential @scene.

saved to project.godot

Having global variable names for scenes isn't required.

Even without global names, having a list of all scenes in a project is a duplication of information that should be avoided whenever possible. project.godot already has a list of named classes (but not all scripts!), and to link a scene to a script, the latter does not need to have a global name.

@willnationsdev
Copy link
Contributor Author

willnationsdev commented May 10, 2021

This is important for all kinds of resources, not just scenes. Likewise for @icon annotation and potential @scene.

The only way this sort of thing gets fixed is if Godot creates an API for automated systems to overwrite targeted segments of the text body for user-edited files. Like, the resources would need to expose a list of machine-editable values that, if overwritten, end up replacing a specific substring of the text file. And the data could only be edited statically at editor-time. Kind of like an any-resource-applicable macro system that the Editor can leverage to hack resource data.

We do have the export variables, but those work differently. Exported properties have their names declared and then the editor provides overrides for those property names which are then assigned after instantiation of the object. But because of that, exported properties are inherently mutable and runtime-dependent. They are not constant values, and therefore are not compatible with things like const or preload.

An applicable API would have to be something different that directly hacks in and modifies the resource data rather than registering an override in the scene file or something like that.

Even without global names, having a list of all scenes in a project is a duplication of information that should be avoided whenever possible.

If a scene-binding system were implemented, the ScriptServer would need to store the information about which script is bound to which scene. This would have to be deserialized from project.godot on startup. So regardless, you're going to be storing the scene file paths in that file if they have a script binding no matter what. At least, if you mimic the way the script classes were implemented.

You don't end up duplicating any more information than script class names are duplicated (actually, even less so since you store a lot less information).

has a list of named classes (but not all scripts!), and to link a scene to a script, the latter does not need to have a global name.

The scene bindings don't need to be applied only for script classes either. It's an entirely separate configuration that is independent of script class status. Could work for script classes, anonymous scripts, and even built-in scripts (which I mention as a way of creating sealed types in the Godot API).

@dalexeev
Copy link
Member

It is the Script implementation which effectively decides whether any "static typing" rules are satisfied by an operation. Since the script-scene binding must be defined statically at editor-time, the editor logic is actually the thing that can perform these sorts of type checks and ensure that the root node's type matches up with the associated script.

Curious point (probably has little to do with this question): a script can be created dynamically (at runtime, even in release builds).

var script = GDScript.new()
script.source_code = "func run(): print('Hello!')"
script.reload()
var instance = script.new()
instance.run() # Prints "Hello!".

It is unlikely that someone will need it, but the original proposal most likely worked even in this exotic case, but when moving data into scene, it probably did not.

@willnationsdev
Copy link
Contributor Author

@dalexeev

It is unlikely that someone will need it, but the original proposal most likely worked even in this exotic case

No, the original proposal would have insisted on scene-bindings only being supported on statically defined scripts. The whole concept is based around scene-bindings providing improved editor-time contextual awareness in addition to automating object instantiation with a PackedScene. If scene-bindings could be applied to runtime-generated scripts, then they wouldn't fulfill their role of performing type checks and the like.

@Calinou Calinou changed the title Add Scene Binding for optional tight-coupling between Node-extending Scripts and PackedScene resources. Add scene binding for optional tight-coupling between Node-extending Scripts and PackedScene resources Sep 28, 2021
@KoBeWi
Copy link
Member

KoBeWi commented Dec 16, 2021

There's actually a trivial workaround if you want to have instantiable class:

extends Node
class_name MyClass

static func instance() -> MyClass:
    return load("res://my_scene.tscn").instance() as MyClass

And then do:

var my_class = MyClass.instance()

This way you can even provide parameters to the instance() method.
Works with scripts without class_name too:

var my_class = load("res://my_class.gd").instance()

@dalexeev
Copy link
Member

dalexeev commented Dec 16, 2021

You forgot .instance():

extends Node
class_name MyClass

static func instance() -> MyClass:
    return preload("res://my_scene.tscn").instance() as MyClass
#                                        ^^^^^^^^^^^

Yes, I had this idea, but I don't remember if I wrote about it. I haven't tested if this works in 4.0-dev, but it doesn't work in 3.x due to cyclic dependencies. And besides, right now you cannot pass arguments to the class constructor.

ADD. Does not work. Parser Error: Could not preload resource file "res://test.tscn".

@KoBeWi
Copy link
Member

KoBeWi commented Dec 16, 2021

You forgot .instance():

Right, thanks.

it doesn't work in 3.x due to cyclic dependencies

It does work in 3.x too, I'm using it in my game. A class can reference itself without problems.
Although you need to use load, I updated my code.

And besides, right now you cannot pass arguments to the class constructor.

This is irrelevant. The static method acts as a constructor.

@andy-noisyduck
Copy link

andy-noisyduck commented Dec 16, 2021

There's actually a trivial workaround if you want to have instantiable class:

extends Node
class_name MyClass

static func instance() -> MyClass:
    return preload("res://my_scene.tscn").instance() as MyClass

This is what some of us have leaned towards already, but this doesn't fix the underlying issues.

With this method you end up with 2 different ways of instantiating classes (i.e. new vs instance) which is a bit funky. It means any implementer working with a node type needs to know whether this is a type they should instance with new or with instance (and there's no protection if you unknowingly call new instead).

When working with a type I want to just be able to use it. I don't really want to have to go through the codebase and see if this is a type coupled to a scene, or a type that exclusively uses code. I want to avoid caring about the implementation details of a class before I can use it.

@aaronfranke
Copy link
Member

Over time, I've been thinking that this proposal is actually a really good idea. There are plenty of scripts that are locked to specific scenes, and it would be helpful to explicitly have an attribute to indicate this.

Or, an even smaller idea that would work for my use cases: Instead of having an instanced class MyClass.new() automatically instance a scene like in this proposal, we could have the annotation simply prevent instancing the class in GDScript, and also preventing the editor from attaching the script anywhere else, so instead of MyClass.new() you would have to do const MY_SCENE = preload("res://my_scene.tscn") and MY_SCENE.instantiate() to instance it. This API would have few surprises in it, because instancing a class would never automatically cause a scene to be instanced (unless the class itself did so).

@dalexeev
Copy link
Member

dalexeev commented Jun 28, 2022

Yes, the following solution works quite well:

extends Node
class_name MyClass

static func instantiate() -> MyClass:
    return load("res://path/to/scene.tscn").instantiate()

However, it has several disadvantages:

  1. For static typing, you must use class_name because there is no keyword to access the current class. May be fixed with GDScript: Add keyword to access the current class #391.
  2. There are two constructors: new() and instantiate(). Using new() will only create the root of the scene. new() cannot be disabled (for example by adding assert to _init) as it is used by PackedScene.instantiate(). Not very critical, but a little frustrating.
  3. class_name causes the class to be added to the node creation dialog. When using this dialog, only the root will be created. In the case of the @scene annotation, the dialog can instantiate the scene instead.
  4. This proposal assumes a more rigid one-to-one connection between the scene and the script. But for me personally, this point does not seem as important as the previous three.

@dalexeev
Copy link
Member

dalexeev commented Nov 4, 2022

@nlupugla
Copy link

nlupugla commented Aug 30, 2023

Hi folks! I love the idea of having scripts tightly coupled to scenes. In fact, I even came up with my own proposal for something similar without having read through this one first: #7572. The key differences between my proposal and this are that 1) you can bind a script to any node in a scene, not just the root, and 2) static analysis automatically infers the class of nodes in the scene and prevents certain parts of the scene tree from being restructured (eg: moving/deleting/renaming nodes). I would love to get some more feedback on the proposal :)

@Akhura
Copy link

Akhura commented Oct 13, 2024

Ok, I've been into deep GDScript dive for just 2 months..without any substantial code expirience whatsoever..except for some custom small scripts for blender. But I can accept almost any harsh critics if they occur.

But I've also encontered this problem.

Let me, just view this problem EXCLUSIVELY from the Quality_of_Life perspective.

I create a custom class_name, with custom methods and functionality, that is bound to a SPECIFIC scene. Check.

Now, I want to add this OBJECT (and that's assumed to be a specific OBJECT with predefined functions and components).
What do I do? I press Ctrl+A, type in "class_name" of my object, find and add it. and I should be good to go, yeah?

But as it has already been mentioned. No, you're not good. Because what you need is your CUSTOM SCENE with your CUSTOM CLASS_NAME script.

And to be honest until now, I didnt even realise clearly why GDScript throws errors at me.

But then I've just drag-and-dropped this custom scene in "main_scene". And now - no errors.

And I guess, I've understood what the core elements of this problem.

  1. We ideally don't want to address to our file_system tab in order to add to scene our needed object. But just "Add Node" dialogue window. Because it's obviously takes much more time, than just Ctrl+A, 3-4 starting letters and "Enter"

  2. We also don't want Ctrl+SHIFT+A, even though its basically the same as Ctrl+A in terms of convenience. BUT as far as I can see..very few people even know about this shortcut. But effectively it solves the current problem. If our custom scene and custom class share the same name - omitting the case differences. (edited: ONLY for Editor-wise adding of the object. Adding via code still remains problematic, if such an instantiation is needed)

So, my suggestion is:

  1. add a core_function that relates to class_name functionality and checks if there are any .tscn with this class_name script attached to the scene root_node and return this scene instead of just extended root_node with this class_name_script.

  2. Or maybe add the same as "Autoload" tab in Project Settings. Or maybe combine with "Autoload" (not to bloat the interface). Where we could manually assign that this or that custom class should return specific .tscn, when called through Ctrl+A. That's even a better solution, because it would not lead us to recursion check every time we want to add a Node

Welp, that's just my insight of the problem at hand. I defintely can be wrong.

P.S.: also, if it somehow possible. That would be nice, if "class_name" would eventually return the actuall custom class name. Let it be even just a string. But as for now .get_class() or .get_typed_class_name() return the extended class. Though, that's the matter of another topic.

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

10 participants