godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.12k stars 69 forks source link

Make it possible to pass arguments to `PackedScene.instance()` #1513

Open menip opened 4 years ago

menip commented 4 years ago

Describe the project you are working on: Any Godot project.

Describe the problem or limitation you are having in your project: I have a script with some parameters in it's _init(...). While trivial to pass in args if creating objects via .new(...), I have not found a way to pass something in when instancing a scene using said script.

Describe the feature / enhancement and how it helps to overcome the problem or limitation: It would be nice to be able to have scene instancing accept args to configure the object.

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams: Like: .instance(args), where those would then be passed into the scripts __init(...).

If this enhancement will not be used often, can it be worked around with a few lines of script?: A workaround is to instead have an additional init(...) function and call it right after instancing the scene.

Is there a reason why this should be core and not an add-on in the asset library?: It is not something that can be implemented as a (simple) add-on. Additionally, this would make scenes/scripts work more consistently.

Previous Discussion https://github.com/godotengine/godot/issues/27834 https://github.com/godotengine/godot/issues/28712

aaronfranke commented 4 years ago

What's wrong with just doing this?

var instance = some_scene.instance()
instance.some_property = some_value
Zireael07 commented 4 years ago

@aaronfranke: That's fine when you have one or two properties you need to set. When you have ten...

cammytown commented 4 years ago

Seems like an obvious and easy thing to do. If you need to keep the edit_state argument (which I admittedly do not understand), we can just pass the arguments in an array.

The biggest reason this is so useful in my mind is that it allows a cleaner architecture. We can have a singular _init() function that does what we want an initialization function to do, have it run when a node is initializing, and have an interface to it when we're initializing a Node. Makes sense to me.

AnEnigmaticBug commented 3 years ago

@aaronfranke Setting variables after creating an instance is error-prone. Consider this:

One more thing to consider is that a lot of newcomers expect to be able to use constructors since the chances are their old language supports them.

jcostello commented 3 years ago

@AnEnigmaticBug Right now every time you instance an object you are repeating the same pattern to create the instance, assign a transform (for example) and add it to the scene.

If you want to change your initialization logic, you'll need to go to every place where you create an instance

That happen in any lenguage. What we can have instead is a instance method that acceps basic and optional parameters like the parent scene, the transform, etc

I would like to have a init method to instance. If the firm defines what the object needs to exist. If it changes it should change everywere. To have it DRY we can have a factory object like in every lenguage

slapin commented 3 years ago

It is better to bind closely-related functions without forcing external functions to know intrinics, so I'm pro this. As for lines of code - you just shrink from 2 lines to 1, so generally it is not about shorter code, it is more about OOP way.

YuriSizov commented 3 years ago

That's fine when you have one or two properties you need to set. When you have ten...

I'm sorry to bump an older thread here, but what is implied here? That a method that takes 10 arguments is a better solution? Surely not? 🙃

cammytown commented 3 years ago

That's fine when you have one or two properties you need to set. When you have ten...

I'm sorry to bump an older thread here, but what is implied here? That a method that takes 10 arguments is a better solution? Surely not? 🙃

In the specific case of having a large collection of initializing parameters, the use case may seem less apparent. But not allowing this functionality does not solve the fact that you're passing 10 parameters.

In the case of having one or two parameters that need to be passed and observed during initialization, it makes complete sense to me. Why should we need both an _init() and an init() function?

In your proposed situation, I would likely have a configuration object that I passed in as a singular parameter.

This proposal is about improving the intuitiveness of the design of the language.

dalexeev commented 3 years ago

I abstained from voting, but I think the main idea is that if _init is called with the wrong arguments, it will cause a runtime error, and if you forget to set the parameter after creating the object, then the error may not be detected.

YuriSizov commented 3 years ago

In your proposed situation, I would likely have a configuration object that I passed in as a singular parameter.

Yeah, but then the original counter-argument by Zireael07 doesn't work: it's still one or two parameters.

This proposal is about improving the intuitiveness of the design of the language.

Oh no, I understand the motivation behind this proposal, though I am impartial to what the end result would be. The problem here is that people look at .instance() as if it is a constructor call when in fact it's a call to deserialize a serialized PackedScene resource into objects in memory. Not exactly the same thing.

Also reliance on required initialization parameters to have your nodes in a valid state can be error-prone as nodes can still be created without those parameters. And in fact they will be, because only a script attached to a node after it was created can validate those parameters, the node itself cannot do that. So the underlying problem is not a trivial "Why don't we just allow it" question.

azur-wolf commented 2 years ago

A solution for that is to have a Factory node (presumably an Autoload to avoid cyclic dependency problems), and call a method from it to create instances: Factory.create_mytype(init_args)

Inside such method:

func create_mytype(init_args):

  var obj = MyType.new()
  obj.custom_init(init_args)

So all gets nicely wrapped on a single method, respecting the DRY principle.

The problem here is that people look at .instance() as if it is a constructor call when in fact it's a call to deserialize a serialized PackedScene resource into objects in memory. Not exactly the same thing.

Then instance() is an obscure name.

"deserialize()" | _"get_deserializedinstance()" could be more descriptive.

Zireael07 commented 2 years ago

@KiwwiPity Nice workaround but not everyone uses factory pattern,

azur-wolf commented 2 years ago

@KiwwiPity Nice workaround but not everyone uses factory pattern,

ofc At least regarding the DRY the problem can be solved by wrapping all on a single method.

Out of curiosity, do you know any other workaround to share?

Zireael07 commented 2 years ago

No, in fact I'm using the same workaround ;)

azur-wolf commented 2 years ago

@Zireael07 I was about to say that can be also done with a static method on the own custom class but... oh → cyclic dependency error

pseidemann commented 1 year ago

fwiw the documentation of PackedScene's instance() method (wait, it seems to be renamed to instantiate() in latest) could be improved, to make clear that instance() is doing more than just instantiating, which was told here in the discussion.

see latest doc: https://docs.godotengine.org/en/latest/classes/class_packedscene.html#class-packedscene-method-instantiate

"Instantiates the scene's node hierarchy."

what does "Instantiates" mean here? I would think this just means _init() is being called, which is the classic OOP definition of this word.

"Triggers child scene instantiation(s)."

again weird description. for me, "triggering" something has something to do with signals or similar, but not with object/memory initialization or deserialization.


regarding the proposal, what about adding a new method to PackedScene like call_init() which will call the scene root node's _init() function? this way there needs to be only one _init() function and it can be called via doing Foo.new() or instance() + call_init(). no additional "setup/init" method needed and the behavior of instance() doesn't change.

YuriSizov commented 1 year ago

regarding the proposal, what about adding a new method to PackedScene like call_init() which will call the scene root node's _init() function? this way there needs to be only one _init() function and it can be called via doing Foo.new() or instance() + call_init(). no additional "setup/init" method needed and the behavior of instance() doesn't change.

The _init() methods should already be called by the initialization process. The issue is that there is no way to pass any parameters to it. If calling it again after the initialization, but this time with extra parameters, is a viable solution, then you don't need a new method, you can just call _init() itself.

what does "Instantiates" mean here?

"Instantiates" means "creates an instance of". In terms of nodes with scripts attached in Godot, this also means that the initialization code, defined in the script, will be immediately called. So the method creates an instance of the node hierarchy, stored in the packed scene, then calls initialization methods on every node, if there are any.

again weird description. for me, "triggering" something has something to do with signals or similar, but not with object/memory initialization or deserialization.

"To trigger" means "to cause something to happen", it's just a normal English verb without any specific jargon attached. Though I'm not saying that the descriptions cannot be improved.

dalexeev commented 1 year ago

I was about to say that can be also done with a static method on the own custom class but... oh → cyclic dependency error

load does not cause a cyclic dependency error.

# res://my_scene.gd
extends Node
class_name MyScene

var a: int
var b: String

# Use MyScene.instantiate() instead of MyScene.new().
static func instantiate(p_a: int, p_b: String) -> MyScene:
    var instance: MyScene = load("res://my_scene.tscn").instantiate()
    instance.a = p_a
    instance.b = p_b
    return instance

See also:

pseidemann commented 1 year ago

hi @YuriSizov

The _init() methods should already be called by the initialization process. The issue is that there is no way to pass any parameters to it. If calling it again after the initialization, but this time with extra parameters, is a viable solution, then you don't need a new method, you can just call _init() itself.

but then we need a mechanism that instance() will not call _init() if it has arguments, so it will not fail and it's possible to call _init() after instance().

my idea with the separate call_init() would work as follows:

YuriSizov commented 1 year ago

my idea with the separate call_init() would work as follows:

  • _init() has no arguments -> instance() calls it. call_init() will fail because call_init() needs at least one argument
  • _init() has arguments -> instance() will NOT call it. call_init(...) must be called after instance() with the desired arguments. the packed scene should remember if the node was initialized, to give an error, if the node is added to a tree without call_init() being called beforehand.

Then you, as a developer, can just as well rename your _init(...) to call_init(...) for those nodes with parameters and the end result would be the same (instantiate() has nothing to call, so it doesn't call anything, you manually call call_init(...) with your parameters afterwards).

In practice it's exactly the same as the workaround to have a dedicated setup method, except your suggestion to hide this logic inside of the engine would introduce implicitness which is not good for end users.

pseidemann commented 1 year ago

@YuriSizov,

Then you, as a developer, can just as well rename your _init(...) to call_init(...) for those nodes with parameters and the end result would be the same (instantiate() has nothing to call, so it doesn't call anything, you manually call call_init(...) with your parameters afterwards).

this would break the convention (or actually implementation) that _init() is the constructor. also Foo.new() wouldn't work as expected. furthermore, this again would introduce the issue that every project chooses a different convention how to name that pseudo constructor and two constructors (real + pseudo) might be needed or desired instead of the real one.

In practice it's exactly the same as the workaround to have a dedicated setup method, except your suggestion to hide this logic inside of the engine would introduce implicitness which is not good for end users.

to the contrary, it doesn't hide it, but makes it explicit and distinct how to initialize the scene. also because of call_init(), the engine can perform bookkeeping (hooks, errors, the error handling I talked about in my proposal etc.) and has full control how to actually initialize the scene (similar to Object.new()).

YuriSizov commented 1 year ago

@pseidemann You already propose that the engine, based on an arbitrary detail, stops treating _init as a constructor, and requires the user to call another method to trigger _init indirectly after the construction has already been done. The way you describe it, it already breaks all kinds of expectations.

And the implicitness comes from the fact that the choice of the normal execution path or this alternative one that you propose would entirely depend on the number of arguments the (not-)constructor takes.

pseidemann commented 1 year ago

@YuriSizov,

based on an arbitrary detail

it's not arbitrary, but the very culprit of why _init() cannot have arguments when instance() is used.

requires the user to call another method to trigger _init indirectly

_init() is today called "indirectly" via new() (and also instance()) so nothing bad about that.

it already breaks all kinds of expectations

could you please be more specific?

And the implicitness comes from the fact that the choice of the normal execution path or this alternative one that you propose would entirely depend on the number of arguments the (not-)constructor takes.

this would be fixed by allowing passing arguments via instance(args...) directly.

red1939 commented 1 year ago

One suggestion: for the love of god, please don't add something like _after_init() -> void semi-constructor.

Manual property setting before adding to the tree and wrapping _after_init() idiom inside a factory are not solutions, and I think we all agree. The first 'solution' lacks a clear indication of what is required for initialization (@required on a var anyone?) and when you should check it (_ready()?). Second one requires extra layer of boiler plate which is tedious and not supported/exposed by the ecosystem, and (once again) prohibits us from (potentially) raising an error during code editing, not failing at some random point during runtime.

markdibarry commented 1 year ago

This is an interesting topic, because both the omission or adding of this feature makes things problematic. The "best practices" documentation suggests that scenes should have no dependencies if possible, but to also modularize complex scenes into smaller ones when possible as well. The concept of architecting your objects to have no dependencies is ambitious, but not very realistic. They are diametrically opposed concepts. However, the concept "all scenes should not have any dependencies" must be enforced for one of Godot's biggest editor features to function (running individual scenes).

Allowing dependencies to be provided to instantiate(): -Prevents invalid objects from being created, preventing runtime errors. -Allows for testing scenes easier. -Removes the requirement of all nodes and dependencies to be flagged as Nullable in C#, preventing warnings and unnecessary null-checks across the codebase, and discouraging "just trust me bro" style of coding.

Of course, allowing scenes to have dependencies would not allow them to run individually or in the editor as tool scripts. Though, maybe there could be a warning, considering this moves the errors you would encounter away from runtime.

I'd be interested if anyone has any suggestions to satisfy both allowing Godot's editor features and a less error-prone coding architecture.

red1939 commented 1 year ago

I feel that we don't really miss out on complex scenes, but rather prevent runtime-created scenes from being instantiated via a clear interface. Now you need to do everything via the properties with special care being put at the moment when you add it to the tree/activate, as failure to do so will lead to a cryptic error.

On Wed, 25 Jan 2023 at 16:18, markdibarry @.***> wrote:

This is an interesting topic, because both the omission or adding of this feature makes things problematic. The "best practices" documentation https://docs.godotengine.org/en/latest/tutorials/best_practices/scene_organization.html suggests that scenes should have no dependencies if possible, but to also modularize complex scenes into smaller ones when possible as well. The concept of architecting your objects to have no dependencies is ambitious, but not very realistic. They are diametrically opposed concepts, however, this concept must be enforced for one of Godot's biggest editor features to function (running individual scenes).

Allowing dependencies to be provided to instantiate(): -Prevents invalid objects from being created, preventing runtime errors. -Allows for testing scenes easier. -Removes the requirement of all nodes and dependencies to be flagged as Nullable in C#, preventing warnings and unnecessary null-checks across the codebase, and discouraging "just trust me bro" style of coding.

However, allowing scenes to have dependencies would not allow them to run individually or in the editor as tool scripts. Though, maybe there could be a warning, considering this moves the errors you would encounter away from runtime.

— Reply to this email directly, view it on GitHub https://github.com/godotengine/godot-proposals/issues/1513#issuecomment-1403781594, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADNME2GE7G3ODW6M7AAAHETWUE73JANCNFSM4RMOPN6Q . You are receiving this because you commented.Message ID: @.***>

jkf16m commented 1 year ago

This is an interesting topic, because both the omission or adding of this feature makes things problematic. The "best practices" documentation suggests that scenes should have no dependencies if possible, but to also modularize complex scenes into smaller ones when possible as well. The concept of architecting your objects to have no dependencies is ambitious, but not very realistic. They are diametrically opposed concepts. However, the concept "all scenes should not have any dependencies" must be enforced for one of Godot's biggest editor features to function (running individual scenes).

Allowing dependencies to be provided to instantiate(): -Prevents invalid objects from being created, preventing runtime errors. -Allows for testing scenes easier. -Removes the requirement of all nodes and dependencies to be flagged as Nullable in C#, preventing warnings and unnecessary null-checks across the codebase, and discouraging "just trust me bro" style of coding.

Of course, allowing scenes to have dependencies would not allow them to run individually or in the editor as tool scripts. Though, maybe there could be a warning, considering this moves the errors you would encounter away from runtime.

I'd be interested if anyone has any suggestions to satisfy both allowing Godot's editor features and a less error-prone coding architecture.

This is exactly what happened to me yesterday. I tried making my own kind of architecture, composed of "components" and "entities". Components being small pieces of logic, derived from Node, and they have some constraints to a specific Node type. And entities, exposing the interface of events the components would use.

All of this was implemented on c#, but the issues are the same both GDScript and C#. And oh boy, I had to write a lot of boilerplate code for every single component made...

Exporting the properties, then overriding a protected _Init function, etc...

The solution I was thinking was basically making a new attribute that includes the property into the constructor.

But that's the solution I could think of in c#, where I can use generics.

I don't even know if someone can even make a custom decorator/attribute in GDscript.

wagfeliz commented 1 year ago

Sorry to revive this again, but I could not find a way to instantiate a packedscene with parameters other then using singleton, something that I just refuse to use, I rather use another engine.

The suggestion someone says of passing parameter after the instantiate dont work with godot4, any parameter set after the instantiate will be lost for some reason :

var instance = some_scene.instance() instance.some_property = some_value <--- this parameter will be lost after the scene loads

So, if there is a way to do it, ( other then using singleton ), please indicate here, because I ready the intire text and I could not find a way, if there is no way, then it should be a priority to change in the engine, maybe adding a init, or post_init func.

aaronfranke commented 1 year ago

The suggestion someone says of passing parameter after the instantiate dont work with godot4, any parameter set after the instantiate will be lost for some reason :

Which properties? Can you provide a minimal reproduction project? I don't experience this problem in my projects.

wagfeliz commented 1 year ago

The suggestion someone says of passing parameter after the instantiate dont work with godot4, any parameter set after the instantiate will be lost for some reason :

Which properties? Can you provide a minimal reproduction project? I don't experience this problem in my projects.

Any properties... There is no way to send anything to a new scene other then using singletons, that is in my opinion is not a solution. I dont know witch type of games you made, but in any advanced game you will need to send information to a new scene for transitions or for custom behaviors etc.

I do have a solution I made myself for it thought, and I could provide it as a propose, it basically changes the way new scenes are instantiated so you can send any information to this before its created.

aaronfranke commented 1 year ago

@wagfeliz Again, I can't reproduce the issue you are describing.

PassParameterAfterInstancingScene.zip

func _ready():
    var test = SOME_SCENE.instantiate()
    test.some_property = "some value"
    add_child(test)

I created this test project in a few minutes and it works fine. If something is broken, you need to provide a test project.

dalexeev commented 1 year ago

var instance = some_scene.instance() instance.some_property = some_value <--- this parameter will be lost after the scene loads

If some_property is an @onready variable, then it is, but it's not a bug.

wagfeliz commented 1 year ago

@wagfeliz Again, I can't reproduce the issue you are describing.

PassParameterAfterInstancingScene.zip

func _ready():
  var test = SOME_SCENE.instantiate()
  test.some_property = "some value"
  add_child(test)

I created this test project in a few minutes and it works fine. If something is broken, you need to provide a test project.

If you add a child to your node it works, if you change the scene it dont. The only workarround is completly change the way the engine works creating a empty main rootNode that is allways active, and add and remove nodes to it. In other worlds, if you create an new level for your game, and want to send information to your new scene, like player colors or something, if you use the normal approach change_scene it will not work, like change_scene_to_file.

wagfeliz commented 1 year ago

Also, sorry, I just figure out I forgot to mention the use of change_scene, I cant beleave I forgot to mention this.

Following some code : https://godot.community/topic/63/set-start-parameter-to-packedscene

scene.add_player( isMe , playerName , nodeId )
var pack: PackedScene = PackedScene.new()
pack.pack(scene)
get_tree().change_scene_to_packed( pack )
aaronfranke commented 1 year ago

@wagfeliz So, the issue you are describing is completely unrelated to PackedScene.instantiate().

get_tree().change_scene_to_*() methods accept either a PackedScene or a file name, not an instance (a node). If you already have an instanced scene (a node), you can change the scene manually by adding it as a child:

func _ready():
    var test = SOME_SCENE.instantiate()
    test.some_property = "some value"
    await get_tree().process_frame # Can't add child to the parent on ready.
    get_parent().add_child(test)
    queue_free()
wagfeliz commented 1 year ago

@wagfeliz So, the issue you are describing is completely unrelated to PackedScene.instantiate().

get_tree().change_scene_to_*() methods accept either a PackedScene or a file name, not an instance (a node). If you already have an instanced scene (a node), you can change the scene manually by adding it as a child:

func _ready():
  var test = SOME_SCENE.instantiate()
  test.some_property = "some value"
  await get_tree().process_frame # Can't add child to the parent on ready.
  get_parent().add_child(test)
  queue_free()

At first I thought it was a PackedScene issue, thats why I posted here, now I know its a change_scene issue. I guess this problem is not exactly related to this issue.

NilsMoller commented 11 months ago

After reading this whole thread, my understanding now is essentially "This is a complicated design problem. For now, there is no solution aside from manually setting properties." To me, wrapping this in a factory function seems like the DRYest solution. However @red1939 mentioned:

Manual property setting before adding to the tree and wrapping _after_init() idiom inside a factory are not solutions

Should I read this as "do not use a factory method" or "do not use a factory method specifically to wrap a semi-constructor"?

morphles commented 2 months ago

I dunno how complicated or not it is. But seems like super annoying thing as is.

Supposedly science composition and instancing is super promoted way of doing things. Ok I guess if you do "Static stuff" and by that I mean create your scenes in editor. But I do a lot of procedural generation/script based scene compositing. And one scene(A) has _ready() where it setups/loads some children, then other scene (B), will instantiate bunch of A, now would be nice to have some way to pass thingie from B to A when instantiating so that depending on it A _ready() could maybe setup some stuff differently (ie load one mesh or another). Now maybe there is simple way to do this and I'm just unable to find, and thus stumbled on this issue. But sounds like there is not... I guess in my case I can add separate method to call do to "choose which mesh to load" since it is quite simple. Still... gdscript seems to be extremely prone to "verbositis" already and in some cases you might want some smarter logic in _ready() and doing some weird irrelevant plumbing then just some passed parameters would do very cleanly is just pain...