-
-
Notifications
You must be signed in to change notification settings - Fork 97
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
Make it possible to pass arguments to PackedScene.instance()
#1513
Comments
What's wrong with just doing this? var instance = some_scene.instance()
instance.some_property = some_value |
@aaronfranke: That's fine when you have one or two properties you need to set. When you have ten... |
PackedScene.instance()
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. |
@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. |
@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.
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 |
It is better to bind closely-related functions without forcing external functions to know intrinics, so I'm pro this. |
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. |
I abstained from voting, but I think the main idea is that if |
Yeah, but then the original counter-argument by Zireael07 doesn't work: it's still one or two parameters.
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 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. |
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: Inside such method:
So all gets nicely wrapped on a single method, respecting the DRY principle.
Then "deserialize()" | "get_deserialized_instance()" could be more descriptive. |
@KiwwiPity Nice workaround but not everyone uses factory pattern, |
ofc Out of curiosity, do you know any other workaround to share? |
No, in fact I'm using the same workaround ;) |
@Zireael07 |
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: "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 |
The
"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.
"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. |
# 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: |
hi @YuriSizov
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:
|
Then you, as a developer, can just as well rename your 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. |
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.
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()). |
@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. |
it's not arbitrary, but the very culprit of why _init() cannot have arguments when instance() is used.
_init() is today called "indirectly" via new() (and also instance()) so nothing bad about that.
could you please be more specific?
this would be fixed by allowing passing arguments via instance(args...) directly. |
One suggestion: for the love of god, please don't add something like Manual property setting before adding to the tree and wrapping |
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 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. |
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
<#1513 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADNME2GE7G3ODW6M7AAAHETWUE73JANCNFSM4RMOPN6Q>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
This is exactly what happened to me yesterday. All of this was implemented on c#, but the issues are the same both GDScript and C#. 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. |
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() 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. |
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. |
@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 |
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. |
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
|
@wagfeliz So, the issue you are describing is completely unrelated to
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. |
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:
Should I read this as "do not use a factory method" or "do not use a factory method specifically to wrap a semi-constructor"? |
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 |
This comment was marked as off-topic.
This comment was marked as off-topic.
@thedinosoar Please don't bump issues without contributing significant new information. Use the 👍 reaction button on the first post instead. |
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
godotengine/godot#27834
godotengine/godot#28712
The text was updated successfully, but these errors were encountered: