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

Ability to link scene with class #21187

Closed
kubecz3k opened this issue Aug 19, 2018 · 21 comments
Closed

Ability to link scene with class #21187

kubecz3k opened this issue Aug 19, 2018 · 21 comments

Comments

@kubecz3k
Copy link
Contributor

Issue description:
The new ability to define classes by using 'class' keyword is great. But it would be better if we could somehow link class script with proper scene for the purpose of instancing new nodes in 'create new node' dialog.
The spirit in which I work with Godot is that I'm using many small nodes when writing scripts, linking signals in inspector, defining positions with gizmos, or just dividing bigger logic into smaller pieces. This way my scenes are more verbose and are easier for me to work with.
So currently, most of my class scripts will be useless directly after instancing, which can be frustrating for other team members.

@Zylann
Copy link
Contributor

Zylann commented Aug 19, 2018

Why would a class necessarily be linked to a scene, when a scene could also not have a script as its root and still be registerable?
If this still goes to script-land, that would require yet another keyword, or this #20318

@willnationsdev
Copy link
Contributor

I agree with @Zylann that having this be something where we directly tie a script to a scene is a bad design because they don't actually have a 1-to-1 relationship. If they did, Godot wouldn't support multiple instances of a script in a scene. And arbitrarily preventing a "scene-enabled" script from being re-used in a scene, for safety reasons, would be a sensible response to this feature, but overall useless/awkward to do.

I agree that we should be able to define type names for scenes in general though and then be able to create them directly from the editor. If that scene just so happens to have a script at its root, then that's just a bonus.

I've detailed my own proposal in the separate issue referenced above.

@willnationsdev
Copy link
Contributor

willnationsdev commented Aug 20, 2018

Just to be clear @kubecz3k, if we were instead to let you manually tie a name and/or icon to a scene file (either in editing the PackedScene resource in the Inspector, by making changes in a new tab of the ProjectSettings window, or some other method that hasn't been discussed yet) and you could then instance a scene in the CreateDialog using that name/icon, is that something that would fulfill the needs of this Issue?

If the scene you've created has your script class at its root, then we could use the root node's inheritance information to determine where to place it in the CreateDialog. This would effectively link the scene to the script "matter-of-factly" and not because of some sort of explicit pairing, which is much more loose and flexible.

@willnationsdev
Copy link
Contributor

^ In fact, doing it this way would be even better since you wouldn't need to bother announcing a class name for the script itself; you therefore never end up in a situation where someone could instance the script by name and have it not work because the scene's content is absent from the script's environment. Instead, the most user-friendly way of creating the content is by creating the scene by name, rather than referencing the script at all.

@henriiquecampos
Copy link
Contributor

henriiquecampos commented Aug 31, 2018

I'm very interested in this as well.

Couldn't there be a way to simply "Register Scene as Node"?

Like we add an option in the dropdown menu:
image

Then somewhere in the project.godot a list of paths to PackedScenes is kept, and when we Add Child Node these said PackedScenes would be available.

Then we just load the PackedScene and add it, like we do with plugins

I think I can make a plugin to do that, but I dunno if we can access these dropdown menus from a plugin.

@willnationsdev
Copy link
Contributor

willnationsdev commented Sep 9, 2018

TL;DR; I have an idea on how to implement this in a relatively simple way. Check bottom for steps involved.

I've been doing some thinking about this. I keep thinking that I'd expect the notion of a "scene class" to exist, but the more I've examined the notion of what a class is, the less I see usefulness out of making scenes full classes. Instead, I can see how using named scenes in an editor-only context would be the simplest way to go. This actually solves the exact problem of this Issue without adding any undue complexity to the engine.

Here's a list of "class" features:

  1. Associate a globally recognized name with the class.
    1a. in the editor
    - The Editor has access to ClassDB which defines the names of existing engine classes.
    - The Editor has access to ScriptServer which defines the names of existing script classes.
    1b. at runtime (in scripts)
    - Both ClassDB and ScriptServer exist in core and are fully initialized (engine classes register themselves, script classes are registered on startup by reading from the Project Settings data).
  2. Associate an editor icon with the class (accessible only from the editor).
    • The editor's theme has access to the EditorIcons for all engine classes.
    • The EditorData object creates a map of the script classes' names and user-defined icon paths.
  3. Access reflection information for the class (editor AND runtime).
    • Engine classes have the ClassDB which manages reflection data.
    • Loading a Script resource gives you access to its reflection data via Script methods.
  4. Can generate engine documentation for the class (editor-only).
    • Engine classes' ClassDB can generate XML files using the DocData class and from there can generate in-engine documentation by parsing the XML.
    • Script classes do not support this in 3.1, but could support it in 3.2/4.0. if we add script annotations (Annotations in GDScript #20318). The script annotations could then be used to pull metadata about the script's contents
  5. Display the class in the Editor's CreateDialog to Nodes and Resources.
    • The ClassDB and ScriptServer are both accessible at editor-time, so the CreateDialog in the Editor can access their content to construct an inheritance hierarchy listing of all the classes.

Now, the feature that this Issue revolves around is about adding feature 5, the CreateDialog creation, to scenes and presumably to also allow these scenes to have an editor-recognized global name (feature 1a) and ideally also a custom icon (feature 2). These features alone could be implemented strictly within the Editor if we stored the information in the EditorData and serialized it in the Project Settings, just like we do with script class editor icons.

However, if we wanted to support feature 1b as well so that scripts can get the PackedScene resource by referencing its name alone, then the data would have to be stored in a runtime-accessible location, similar to the ScriptServer in core. This doesn't sound like something reduz would allow since it adds content to core purely for a possible use-case that no one has specifically requested.

Features 2 and 3 don't really make sense in the context of scenes anyway since scenes don't inherently have any logic API built into them outside of scripts.

As such, I think we should go with an implementation that adds custom names to scenes only within the editor and which subsequently allows those scenes to show up in the CreateDialog. Here's my implementation proposal:


  1. Define a SceneTemplates HashMap in EditorData. This would map names to scene filepaths.
  2. Add a ProjectSettings tab for SceneTemplates that works just like the Autoload tab, but only allows scenes to be added. This would enable users to manually define SceneTemplates they wish to use.
  3. The SceneTemplates HashMap would be serialized in ProjectSettings and initialized on editor startup (obviously).
  4. The CreateDialog would then pull from the HashMap when updating its list of typenames.
  5. The CreateDialog would incorporate the scene's root node or its script (if it exists) when determining its position in the inheritance hierarchy.
  6. If the scene template's name matched that of a script class, the CreateDialog would create only a single entry for that name. It would then display a toggle somewhere to enable you to specify whether you wished to instance the script class or the scene template. This toggle would default to using the scene template. This default could be configured in the ProjectSettings.
  7. If you right click a node in the SceneDock, and that node has a non-empty filename, meaning it is related to the root of a scene file, the context menu would include a new option to Create SceneTemplate from Scene.
    1. If the node has a non-empty filename property, i.e. is the scene root, or is the root of a different scene instanced into the current one, then the button would jump straight to the ProjectSettings > SceneTemplates tab with the path value prefilled with the filename value and with the name value prefilled with titlecase name of the scene's path.get_file().get_basename() value.
    2. If the node in 7i does not have a filename property, the user would have to first Save Branch as Scene... and THEN click the new scene's root or the previous scene's now-instanced node to access the Create SceneTemplate from Scene context-menu button.
    3. If the node in 7i has a script attached and that script is a script class, then the SceneTemplates tab would instead prefill the name of the SceneTemplate with the name of the script class so that users can more easily add scene templates which mirror their script classes.
  8. Right-clicking a scene file in the FileSystemDock should create the same Create SceneTemplate from Scene button that does the exact same thing as the vanilla behavior.
  9. Add EditorPlugin.add/remove_scene_template(p_name, p_scene) to add plugin support for dynamically adding and removing scene templates.

What do you all think?

Edit: Looking at the EditorAutoloadSettings class, it seems like it already does all of the data management and ProjectSettings serialization, so there wouldn't be a need to have any Hashmap in EditorData (steps 1 and 3 would be handled by step 2).

@willnationsdev
Copy link
Contributor

I've basically spent the whole day thinking about this Issue, and I'm starting to believe that it is actually the key to incredibly simplifying pretty much everything about Godot's user-defined type system.

  1. People want to be able to create an instance of a type without needing to worry about whether that type is a script or scene (.new() vs. .instance()).
  2. People want to be able to export a single value to represent engine classes, scripts, or scenes.
    • This would imply that one needs to either 1) define a string name for a scene or 2) allow engine classes to be assigned as values, e.g. the GDScriptNativeClass object.
      • naming scenes involves keeping Custom Types alive and runtime-accessible (or devising a new centralized resource-naming scheme entirely) with the sole purpose being able to a create a scene by that name.
      • Allowing GDScriptNativeClass objects to be value-types doesn't make sense since they don't represent direct access to an Object class the same way that Scripts do.
  3. People want to be able to get a list of types that inherit from an engine class, script, or scene.
    • This gets needlessly complex when scenes start to also become named types since you have to start testing whether a script extends a scene or a scene extends a script. If one instead mandates that a named scene MUST share a name with a named script and use that script on its root node, then you can defer to the standard, "does my script extend another script?" question which is much simpler to handle.

So each of the primary use cases of registering scenes under their own name each involve much more complex logic based on introducing more variance to the necessary API when handling "types" and each of these use cases become MUCH more simple to handle if one mandates a 1-to-1 relationship between scenes and named scripts. Rather than dictating how a scene is used, all this would do is dictate how a script is instantiated. And this makes sense anyway since scripts that are reliant on their scene structure to work don't make sense when instantiated independently in the first place.

In order to prevent "hiding information from the user," we can introduce a global GDScript method (which could also be made a global method in the GD namespace in C#, etc. for other languages) that instantiates a bound scene instead of just a script when one passes a scene-bound script class's name to it.

# my_type.gd
extends Node
class_name MyType
constructor "res://my_type.tscn" # must be a valid PackedScene filepath

# main.gd
func _ready():
    var node = instantiate("Node")
    var my_type = instantiate("MyType")

If we then combine this with other methods that fetch the inherited class name (until get_parent_class() is updated to return script class inherited names as well), or the list of types that inherit the class (a ClassDB.get_inheritors_list("type") for both engine types and script classes), then we flesh a lot more of the missing user-defined type methods that exist for classes in ClassDB, but not for scripts/scenes.

You can even then create a custom export alternative for strings that lets you export a class name, mandate a base class, and then have the Inspector generate a dropdown of available types to select for the class name (which could then be plugged into the global instantiate method to create the instance). And it would even work for scenes out-of-the-box because of the linking in this Issue!

Mandating the scene->script relationship also simplifies gathering reflection information down the line once we add support for generating documentation of scripted types. Scenes can't define methods and whatnot anyway, so it isn't like scenes need names that could potentially be passed to any such get_docs_for_type() sort of method in the future.

And, of course, this would also allow us to easily have the CreateDialog display (my_type.tscn) rather than my_type.gd and actually instantiate a scene rather than a script when selected.

This would eliminate several Issues (many of which I've created) with a simple and elegant solution that barely adds any complexity to the engine. So yeah...I'm probably gonna start working on this.

Thoughts?

@willnationsdev
Copy link
Contributor

Well, I talked with reduz, and making the changes I suggested (or any type-related changes to the CreateDialog for that matter) are being rejected.

@akien-mga You may want to close this Issue as a result. The only way to solve OP's problem would be to expand custom types to support scenes (which will soon be deprecated in favor of script classes), register scenes with names through some other means (probably considered as unnecessary extra settings / maintanence), or make changes to the engine's core so that scene data can be bound to a script class (which reduz won't approve).

[20:12] <reduz> willnationsdev: we only add what is needed. In this case it's just a simple function
...
[20:20] <reduz> willnationsdev: no, create dialog is perfect as is, needs no changes
...
[20:22] <reduz> willnationsdev: because this is a gdscript thing, not a Godot thing
[20:22] <reduz> willnationsdev: I told you a dozen times already, no changes to core
[20:23] <reduz> core is all explicit

Anything that abstracts scenes away behind a script will be rejected.

@willnationsdev
Copy link
Contributor

I noticed that you can pretty much already do this by making the following type of script class:

tool
extends Node
class_name TestScn

func _enter_tree():
    call_deferred("_replace")

func _replace():
    replace_by(load("res://test.tscn").instance())

I've tested it, and it does the job just fine. And since any derived type can pretty easily do this, one can define types which know how to replace themselves with scenes at any point of the inheritance hierarchy they want (effectively meeting the demand of this feature).

@kubecz3k
Copy link
Contributor Author

not quite since signals will not work in this scenario (nor connecting from or to the node)

@willnationsdev
Copy link
Contributor

@kubecz3k Connect in what way? Couldn't you instantiate the scene, set up any necessary connections, and then replace itself with it? Give an example?

@kubecz3k
Copy link
Contributor Author

@willnationsdev oh sorry! It's working in tool mode :)

@nobuyukinyuu
Copy link
Contributor

nobuyukinyuu commented Sep 15, 2019

@willnationsdev is it possible to use some variation on this code that can work after a node enters the tree, or otherwise have a script detect when it's a replacement of itself? I want to use this without needing to register a separate "scaffolding" class to set up the scene for each of my objects, but when I try using replace_by() after _ready() to add child checks, the base node doesn't get added to the scene and I get !owner_valid() and !p_node->is_inside_tree() errors.

If used as-is in a script to replace its attached node with a scene containing the same script, it causes an infinite loop and crashes the editor.

@willnationsdev
Copy link
Contributor

@nobuyukinyuu The script example above is not capable of having its own functionality within the generated scene. It is strictly a "template generation" script that masquerades the scene as a script class, so having it be present within the generated scene would naturally lead to an infinite loop.

If all you want to do is add some script-based configuration of the scene before it gets replaced, all you have to do is change _replace() to assign the instantiated scene to a local variable before calling replace_by(node). In between, you can make whatever changes to the scene you want (like connecting to signals, customizing property values, procedurally generating other nodes, etc.).

Is that what you meant?

@nobuyukinyuu
Copy link
Contributor

Mainly, I was just hoping of being able to set up all of the nodes with one call in a scene which I have an associated class_name for. I believe if I use a template to set up a different scene, I lose some of the advantages of using class_name, for example when trying to compare a set-up scene using is.

Since _replace() is still being called (presumably) in between _enter_tree() and _ready(), I can't check for children or if any other setup has taken place, although perhaps that's not true if the behavior mentioned in #28746 works as I might expect?

@willnationsdev
Copy link
Contributor

Mainly, I was just hoping of being able to set up all of the nodes with one call in a scene which I have an associated class_name for. I believe if I use a template to set up a different scene, I lose some of the advantages of using class_name, for example when trying to compare a set-up scene using is.

I'm not sure what you are losing out on. The way the node above works is that it will replace itself with the other scene, which presumably has its own script class as a root. You can then also create a script class for global namespacing that stores a constant for your scene, if you want to reference the scene directly by name in script code. So, for example, if you want the trifecta of CreateDialog instantiation, runtime instantiation, and script class type checking:

  • Scenes.MyClassScn, a MyClassScn constant in a Scenes script class, deriving Reference.
  • MyClass, root of my_class.tscn
    • Use this one when doing is checks
    • add a static instantiate method which loads Scenes.MyClassScn and returns its instance. You can then use MyClass.instantiate() rather than .new() to create the scene.
  • MyClassGen, the template generator script class, based on the script I made above.
    • Use this one when instantiating the script from the editor's CreateDialog

Now...I can see how this is a lot of boilerplate/complexity just to make this work for one script class in all the various use cases. So I'll have to think more on another approach that might simplify things more.

@nobuyukinyuu
Copy link
Contributor

I'll keep trying to think of a clever way to at least get these things without having to extend to special instantiators and copies of both registered classes and their templates showing up in the add node dialog. I really only want to make my existing stuff a little easier to add to my scenes instead of always having to drag them in from the FileSystem tab (they live in various places based on their use case).

You're right when you say all of these things are possible, but they're not clean and I think the complexity defeats the purpose, if only a little. Specifically, having 2 similarly-named things in the Add Node dialog for each registered type is making me flinch.

@willnationsdev
Copy link
Contributor

willnationsdev commented Sep 15, 2019

@nobuyukinyuu Well, what I just realized, thinking about it for a bit, is that we have two referential domains that cannot be coalesced without one losing access to a valid reference.

There are two things you could do:

  1. Use replace_by(node) to make the original node get swapped with the new node.
  2. Use a nonexistent become(node) to copy over all property values, signal connections, and child nodes from the new node to the original one.

In the first case, if executed at runtime, you run the risk of third-party nodes with cached references to you becoming invalidated. In the second case, you run the risk of the child nodes in the new scene caching a reference to their parent upon creation and THEN getting reparented to your new node's hierarchy, effectively invalidating their reference.

In either case, a reference could become invalid. The only guaranteed way would be if you could track all object references, but I don't think that is possible (it would require language-level reference tracking, like garbage collection, that would require periodic cleanups). Now, if you already know your domain situation and are confident that you could make use of either one of those algorithms effectively, then you could probably write a quick script to do things, but coming up with a generic engine/editor-level solution that comes with runtime consistency with the editor-time content doesn't seem realistic to me thus far. No matter what, you'd be developing some kind of hack.

@nobuyukinyuu
Copy link
Contributor

nobuyukinyuu commented Sep 15, 2019

I've figured out a way to do it by using get_node_or_null() when the deferred _replace() function is called. I'm betting it can also be done using get_child_count(). The only quirk with doing this is that the scene children are invisible to the editor (like when linking a scene), but the node is also missing the "Open in Editor" shortcut icon you expect to see with scene instances. It otherwise seems to work!
Example:

tool
extends Node  #Can be whatever
class_name MyClass

#....

#Toolscript magic to add the scene components when importing from "Add child node"
func _enter_tree():
	#Replace "ChildNode" below with something in your scene which exists!  
	#Maybe get_child_count()==0 will also work?
	if !get_node_or_null("ChildNode"):   
		print(get_class(), ":  Replacing scene with proto")
		call_deferred("_replace")
func _replace():
	replace_by(load("res://MyClass.tscn").instance())

Edit: I didn't think about references to an instance of this created at runtime by a script; this would likely invalidate a reference somewhere then, I'm assuming?

@willnationsdev
Copy link
Contributor

@nobuyukinyuu Correct.

var node = MyClass.new()
add_child(node) # has no children, _replace() triggers
# node is now an invalid reference
node = $"@11" # whatever the procedurally generated name would be

@Calinou
Copy link
Member

Calinou commented Oct 11, 2020

Feature and improvement proposals for the Godot Engine are now being discussed and reviewed in a dedicated Godot Improvement Proposals (GIP) (godotengine/godot-proposals) issue tracker. The GIP tracker has a detailed issue template designed so that proposals include all the relevant information to start a productive discussion and help the community assess the validity of the proposal for the engine.

The main (godotengine/godot) tracker is now solely dedicated to bug reports and Pull Requests, enabling contributors to have a better focus on bug fixing work. Therefore, we are now closing all older feature proposals on the main issue tracker.

If you are interested in this feature proposal, please open a new proposal on the GIP tracker following the given issue template (after checking that it doesn't exist already). Be sure to reference this closed issue if it includes any relevant discussion (which you are also encouraged to summarize in the new proposal). Thanks in advance!

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

7 participants