godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.16k stars 97 forks source link

Static Scenes #7572

Open nlupugla opened 1 year ago

nlupugla commented 1 year ago

Describe the project you are working on

Godot

Describe the problem or limitation you are having in your project

It would be great if there was a way to tell the engine that certain aspects of a scene's tree will never change. This could provide many advantages, such as node type safety, better editor autocompletion, and potentially performance improvements.

Consider a simple example of a Character scene laid out as

It's easy to imagine that you would want all Character instances to have a valid Sprite of type Sprite2D and valid HealthBar of type ProgressBar. Currently, the engine provides no built-in way to enforce such a constraint; there's nothing preventing you from instancing a Character scene, then deleting/renaming/moving/replacing the Sprite or ProgressBar node. This lack of constraint has a few important consequences:

  1. $Sprite is not cached for fast lookup (see https://github.com/godotengine/godot-proposals/issues/996)
  2. $Sprite is typed as a Node, not Sprite2D.
  3. If you inadvertently delete/rename/move/replace Sprite somewhere in code, the engine can only warn you about this at runtime when you try to access $Sprite and the node isn't found.

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

Static scenes provide a mechanism for the user to tell the engine what aspect of a scene's tree will remain constant across all instances. The engine can then use that knowledge to solve all the issues I mentioned above.

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

Here's how this might look in practice. In the editor, there is a way to mark a node as "static". This could be a checkbox near where the "Access as Scene Unique Node" option appears. Checking this option makes that node, and all of its children, "static", meaning they cannot be deleted/renamed/moved/replaced, although they can still acquire new children. A script can then take advantage of this property by declaring the node as static in some way.

For example

class_name Character 
attaches_to ^"Character" in "res://character.tscn"
extends Node2D

func _ready() -> void:
    $Sprite.visible = true
    $ProgressBar.visible = true
    # the lines above got to benefit from autocomplete because the editor knew their types

Such a script will only be valid if a "static" node exists in the file "res://character.tsn" at path ^"Character". In addition, the editor will highlight as an error any line that attempts to delete/rename/move/replace Sprite or HealthBar.

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

The closest equivalent to the static scene proposal that I can think of involves declaring all of the "static" nodes in a scene as @onready variables. For example

class_name Character
extends Node2D

@onready var sprite : Sprite2D = $Sprite
@onready var health_bar : ProgressBar = $HealthBar

Not only does this approach introduce a line of boilerplate per node in the scene, it also doesn't allow for any static guarantees that the "static" nodes exist. For example, if I accidentally free sprite or health_bar, I won't know until runtime, whereas static scenes would be able to catch this mistake in the editor.

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

Scenes are core to the Godot user experience.

dalexeev commented 1 year ago

This is similar to #1935 with two differences:

  1. You can specify that the script is attached not to the scene root, but to an arbitrary node (attaches_to ^"Character" in "res://character.tscn").
  2. $Node shorthands in such a script starts inferring hard types using the scene data (#1935 only mentions autocompletion, not hard types).
    • Also caching should be performed, as far as I understand?
    • I think some separate operator would be better. It would be more obvious, you wouldn't be able to use it in regular scripts, and you could still use $ for dynamic nodes in static scene scripts.

This is too "magical" behavior in my opinion, you can just use @onready variables (which solves both the static types issue and the caching issue). Even if it's a bit boilerplate, it's much more obvious and already implemented.

nlupugla commented 1 year ago

Thanks for the feedback dalexeev :)

Your summary looks correct to me and indeed, my intention is that nodes would be cached. Perhaps another operator would be make sense, but I prefer the simplicity of using the same operator. Can you elaborate on why you feel the behavior seems too "magical"? In my mind, "magic" behavior changes despite seemingly unrelated modifications. For example I would consider certain garbage collection schemes "magic" when the user has very little explicit control over when things get deleted. In the case of static scenes, the user has to explicitly tell the engine which nodes will be static (and thus cached).

An important advantage of static scenes over @onready is the extra safeguards it provides against accidentally deleting/moving/renaming/replacing nodes. I've spent plenty of time debugging issues that ultimately came down to nodes not being where I expected them to be. In fact, you can use static scenes to build up "scene interfaces" or "templates", if you will.

Here is an example from one of my projects. I have a scene that represents an abstract turn-based battle laid out (roughly) as

Specific encounters inherit this scene. For example, one of the inherited scenes might look like

This setup works pretty well for me already, as long as I'm careful not to modify the scene tree too much. However, if I could make my abstract Battle scene static, it would become a rigid template/interface backed up by static guarantees that I could rely on. I can't accomplish the same thing with just @onready variables.

theraot commented 1 year ago

There could be, other code, external to the scene, that takes references to the nodes marked and static and attempts to delete them. Presumably you would want a runtime error for that case. However, deleting the scene instance (which contains static nodes) should be OK. For such runtime error to work, the node needs to store that it is an static node.

Speaking of storing, you want all the static nodes to be cached. We would need to decide when and where.

Why would a node be static and not have a unique name? I do not think we need that. Instead, I would suggest to replace "Access as Unique Name" with a menu with "Unique Name" and "Static" options. And every static node, is also a node with a unique name. This way: 1. We save precious vertical space in the menu. 2. We do not need a new syntax to access static nodes.


Regardless, even if this proposal gets implemented, I believe it would be good if Godot had warning in the editor if it detects that the node being accessed via onready is not present on the scene or isn't of the correct type (this requires the checking if the edited scene has a node with the current edited script attached). If anybody wants to make a proposal for that, go ahead.

dhoverml commented 1 year ago

@nlupugla In the example you described, the original scene, as a whole, is no longer static because nodes were added to it. At least, the scene itself was modified.

Also, I think this proposal only makes sense from the editor's perspective, since the editor's tabs allow you to view each scene individually, using their own root node.

However, at runtime (so, outside of the editor), there is only 1 active scene tree (well, not counting autoloads), and everything else is nested under it. Instantiating scenes, and nesting them, is like adding a bunch of nodes to the active scene tree. Once you start nesting static and non-static scenes, it becomes ambiguous where static-scenes begin and end. This ambiguity makes it even harder to know which parts of the tree can be deleted, since I'm guessing that deleting a static scene as a whole is allowed (unless if the health bars are allocated once and never destroyed?). The example you provided demonstrates the ambiguity, since a static scene contained non-static nodes, and these nodes could be the root nodes to other static or non-static scenes.

For some context, I've been thinking a lot about let (https://github.com/godotengine/godot-proposals/issues/820), const, immutable (similar to let, but cannot call const functions. I don't know if this makes sense or is useful), an annotation called encapsulate (I also don't know if this makes sense or is useful), and how/if they are generic enough to be used to implement something similar to static scenes. I'm pretty sure a scene-level feature like this proposal isn't feasible due to ambiguities though, but I'm not a contributor, and I may have misunderstood something, so take my opinion lightly.

nlupugla commented 1 year ago

Just by writing a comment on the proposal you are a contributor @dhoverml <3

It's possible that the name "static scene" might be a bit misleading as my proposal doesn't require the entire scene to be static, just parts of it. In the editor, the user marks nodes in the scene as being static or not. A node marked as static is guaranteed to be inside its enclosing scene by the time the scene is finished instantiating. As you point out, the enclosing scene is free to add nodes to its heart's content and remove any node not marked as static. Perhaps "static nodes" might be a better name for the proposal, although to me, that would imply that the actual values in the nodes marked static would have to be constant, which is not my intention.

I can see why you bring up additional language features like let. However, this is a feature that should work at the level of the editor and be language independent. In other words, it should work whether I am using GDScript, C#, or C++. The information regarding which nodes are marked as static would need to be embedded in the .tscn file itself, as it would impact how the scene is instantiated.

girng commented 2 weeks ago

$Sprite is not cached for fast lookup (see https://github.com/godotengine/godot-proposals/issues/996)

can simply use onready var, example: onready var sumator = get_node("Sumator")

$Sprite is typed as a Node, not Sprite2D.

wdym it's typed as a Node not a Sprite2D? a scene that is instanced or inherited maintains its types for all children

If you inadvertently delete/rename/move/replace Sprite somewhere in code, the engine can only warn you about this at runtime when you try to access $Sprite and the node isn't found.

you can use weakrefs or has_node. or one of my favorites, get_node_or_null to check. the resource it referenced will become a [Deleted Object] and later be freed