godotengine / godot-proposals

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

Add a generic parameter to PackedScene (like Array) to allow for easier type safety #6694

Open jaymehta-g opened 1 year ago

jaymehta-g commented 1 year ago

Describe the project you are working on

A game in which the player can shoot bullets. The class Bullet has an init method that sets it up and returns self. bullet_scene contains a scene in which the root has the Bullet script.

Describe the problem or limitation you are having in your project

I want to instance and set up the bullet like so:

var bullet = bullet_scene.instantiate() \
    .init(Vector2.UP * 200)

which works fine but it isn't type safe. I can't have godot infer the type of bullet with := and its because instantiate just returns a node of unknown type. I can't autocomplete Bullet's methods and mistakes aren't caught until they come up in runtime

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

A typed PackedScene that, when instantiate is called, returns a value statically typed as whatever type it only supposed to be able to contain.

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

Godot 4 brings typed arrays where getting an item from them automatically types that value as the type they are marked to contain.

var array : Array[Bullet]
array.append(Bullet.new())
array[0].init(Vector2.UP)
# green line numbers, autocomplete!

Seeing that this is possible, I imagine it wouldn't be too much more difficult to type a PackedScene in a similar way, say

@export var bullet_scene : PackedScene[Bullet]

In the export section in the inspector, the editor should either somehow refuse scenes that do not have the given type as the root, or if this is not possible the game should create a runtime error as soon as you try to put a scene that doesn't fit into a packed scene variable.

instantiate would return a value that is statically types just as typed arrays return a typed value when you call the indexing method.

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

You can ignore type safety and hope for the best (bad) or cast the value from instantiate before using it (ugly and unreadable)

var bullet = (bullet_scene.instantiate() as Bullet) \
    .init(Vector2.UP)
# gross

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

Changing the way gdscript works isn't possible through an addon

I am unfortunately not able to implement this myself looking through godots source code made my head spin

dalexeev commented 1 year ago

This seems to be difficult to implement. Even for typed arrays, the methods return a Variant, not the element type.

To work around the problem, you can use an intermediate typed variable or the as keyword:

var bullet: Bullet = bullet_scene.instantiate()
bullet.init(Vector2.UP * 200)
var bullet = (bullet_scene.instantiate() as Bullet).init(Vector2.UP * 200)

Also you can use the following approach (see #1935):

extends Node2D
class_name Bullet

var velocity: Vector2

static func create(p_velocity: Vector2) -> Bullet:
    var bullet: Bullet = load("res://bullet.tscn").instantiate()
    bullet.velocity = p_velocity
    return bullet
KoBeWi commented 1 year ago

If you do var node = preload("res://some/scene.tscn").instantiate(), your node variable will provide autocompletion for the instantiated scene. image Although this feature doesn't seem very reliable. In my example I used a built-in script.

jaymehta-g commented 1 year ago

@KoBeWi It would be perfectly fine if typed PackedScenes returned a variant like Arrays do as long as Godot can enforce using the variable properly the same way. But I hadn't thought of the workaround of instantiating the scene in Bullet, that might be even better than the initial approach!

VapidLinus commented 1 year ago

When instantiating nodes at runtime, I have found that I need to declare a PackedScene field and then use the Instantiate<T>() method on that, but this provides no safety in the editor. You can assign any type to it and there's not even a "hint" in the editor that lets the designer know what type of node can be assigned to this export.

Coming from Unity, this way of instantiating prefabs in a typesafe way was very comforting and convenient. I'd love to have a PackedScene<T> or similar in C# as well.

// No way in editor to only assign the correct type,
// nor any hints about what the correct type is.
[Export] public PackedScene PlayerPrefab { get; private set; }

public void SpawnPlayer()
{
    // Could crash at runtime if the wrong type of PackedScene was assigned
    var player = PlayerPrefab.Instantiate<FpvPlayer>();
    ...
}
Zireael07 commented 1 year ago

What you want is more like PackedScene subtypes, though? Because you shouldn't be able to assign non scene things to an export of type PackedScene?

Workaround I use in GDScript: check that the scene has expected variables.

Shadowblitz16 commented 1 year ago

I had a suggestion like this where we could export something like...

@export var scene : PackedScene[MyPlayerNode]

it would actually be really useful to make sure when you instance a type your getting what you need. else you either have to instance it, check and deleted it if it's not that type, or use some sort of packed scene function to look up the root scene.

Either way though it requires alot of code that could just be as easy as a simple export.

I do agree that we should be able to pass Scenes directly to Node types If I were in change I would do something like this..

@export var path_to_node2d : NodePath[Node2D] 
@export var scene_to_node2d : PackedScene[Node2D] 
@export var path_or_scene_to_node2d : Node2D

and of course I would support custom types from the get go

jaymehta-g commented 1 year ago

I had a suggestion like this where we could export something like...

@export var scene : PackedScene[MyPlayerNode]

im a little confused, is this different from the original suggestion?

Shadowblitz16 commented 1 year ago

I had a suggestion like this where we could export something like...

@export var scene : PackedScene[MyPlayerNode]

im a little confused, is this different from the original suggestion?

No but it was just to show we could do this with NodePath's as well and then we can treat exporting Node's and either a NodePath or a PackedScene

WagnerGFX commented 1 year ago

What about adding a new annotation for scenes? Something like:

@export_scene(MyType) var my_scene : PackedScene
@export_scene(MyType) var my_scenes : Array[PackedScene]

It might not help with type safety on the code side, but the editor side will be tasked with filtering PackedScenes with that root node type (or subtype). This can make casting much less error-prone.

Although, the editor would have to keep an active lookout for cases where the root node of a scene has its type changed.

Shadowblitz16 commented 1 year ago

What about adding a new annotation for scenes? Something like:

@export_scene(MyType) var my_scene : PackedScene
@export_scene(MyType) var my_scenes : Array[PackedScene]

It might not help with type safety on the code side, but the editor side will be tasked with filtering PackedScenes with that root node type (or subtype). This can make casting much less error-prone.

Although, the editor would have to keep an active lookout for cases where the root node of a scene has its type changed.

Isn't that only a editor thing? I would like this for scripting too. And it looks nicer if we just do : PackedScene[MyType]

skylord-a52 commented 9 months ago

This would be extremely helpful for resources with a tree-like structure. In my case, attacks are defined as a root node with a tree of subproperties and subattacks. If I define them as a Resource, I lose the Node/Scene editor (as well as transform behavior), and quickly editing trees of attack definitions becomes much more cumbersome. But if I define my attacks as Scene of nodes instead of a Resource, there's no way to ensure at compile-time that I'm giving my objects the attack definitions they expect.

Giving PackedScenes a "type" would solve this issue.

Ikaroon commented 5 months ago

I think this issue should be linked: https://github.com/godotengine/godot-proposals/issues/5255 It actually nicely describes why this feature is needed. Especially when working in a team