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

Add a way to cancel adding a Node to tree #5102

Closed
KoBeWi opened this issue Aug 7, 2022 · 52 comments
Closed

Add a way to cancel adding a Node to tree #5102

KoBeWi opened this issue Aug 7, 2022 · 52 comments

Comments

@KoBeWi
Copy link
Member

KoBeWi commented Aug 7, 2022

Describe the project you are working on

A couple rather big game projects.

Describe the problem or limitation you are having in your project

Quite often I have some nodes as part of the scene that get removed instantly. I have a few different usage examples:

  • collectibles that appear only once - when you collect them and re-enter the level, they should stay gone
  • enemies that appear as part of an event. I want to remove them at start and add to scene as ambush event etc.
  • pickups in a chest. I have pickups that can just lie on ground or they can be placed under a chest instance. On start, the chest will remove them from tree and re-add when opened
  • running level scenes independently. Like most projects, I have a Game scene, which loads the level and adds it as a child; the level can't exist alone, so you can't run it directly. I made a sophisticated workaround for this use-case, but recently I created a level that couldn't use it and the best solution I could do results in error spam on start. In best-case scenario, you want the level to detect that there is no game, remove itself from tree, add a Game node and then add itself; this is almost never possible, because of the initialization logic that has some expectations about the scene tree

There are 2 problems with this:

  • in some cases you can't remove the object immediately. E.g. when the removing logic is part of the node's code ("I was already collected, kill me"). In such cases you are forced to use queue_free. This obviously makes the node exist for a single frame and causes a visible ghost node when entering level. I worked it around by just making the screen black for a single frame on start, so this is not that bad
  • the worse problem is that nodes often have initialization logic as part of (usually) _ready() method. The thing is that in all of the cases I listed above, I don't want this logic to run. Sometimes it's harmless - the node initializes itself and then gets removed. But sometimes the initialization code has unwanted side-effects. You can avoid some of them by deferring the initialization and returning when the node gets removed, but this is impossible for @onready variables

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

All of these cases can be solved if I were able to cancel adding a node to tree. Like, I load a scene, but there is a node I don't need, so I create it without adding to the tree. Its _enter_tree() and _ready() (and @onready) is not called.

You might ask what happens to the node when it's not added. Well, it's up to the node. In my case I'd either free it or call some method on the parent node to ensure that a reference is stored somewhere. This is tricky though - if the node is not freed, unaware users might run into leaks. But IMO it's an advanced feature, so it should just be used with care.

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

For implementation maybe a virtual method could be added, like _can_instance() (name up to discussion; this one is obviously misleading). When a node is instanced, right before adding it to its parent this method would be called, providing the would-be parent as an argument, and return either true or false. When false, the node is simply not added as a child and needs to be handled by the user (the method itself is a nice place to put this logic).

Example usage could be:

func _can_instance(parent: Node) -> bool:
  if parent is Chest:
    parent.items.append(self)
    return false
  
  return true

EDIT:
Opened a PR
godotengine/godot#64107

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

There are no perfect workaround unfortunately.

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

It requires changes to scene instantiation logic.

@Zireael07
Copy link

Another use case: I want debug and/or editor stuff that I don't want to have at game time. This would be a step towards making sure debug stuff doesn't sneak into the game.

@Diddykonga
Copy link

Diddykonga commented Aug 7, 2022

This seems like a hack, I feel as though proper separation of Scenes into sub-scenes and the like would be a possible solution to all the issues raised.
Either an entire API/Interface should be built for Nodes/Scenes to fill out to manipulate how they are interacted with Scenes otherwise this feels hacky.
Also, If there was an Interface for this though, I would expect it to be on Scenes and not Nodes, as this is not an issue, if spawning straight up Nodes, but only an issue when using Nodes with Scenes.

Maybe I'm wrong, but that is my take at the moment.

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 7, 2022

This seems like a hack

Even if, it makes life much easier.

I feel as though proper separation of Scenes into sub-scenes and the like would be a possible solution to all the issues raised.

It would maybe help with initialization logic breaking with unexpected use, but it forces a very specific design and removes some convenience/freedom. You don't need perfect separation to make a good code.

Either an entire API/Interface should be built for Nodes/Scenes to fill out to manipulate how they are interacted with Scenes otherwise this feels hacky.

That sounds complicated. My solution is simple and solves all problems. Not sure what more would you want.

Also, If there was an Interface for this though, I would expect it to be on Scenes and not Nodes

There is no difference in this case. The logic needs to happen during scene instancing, where sub-scenes are just more grouped nodes.

@Diddykonga
Copy link

There is no difference in this case. The logic needs to happen during scene instancing, where sub-scenes are just more grouped nodes.

My point was that if can_instance() is some one-off method on Node, you would need much more information to reliably fix the issues your discussing then what is the Parent node, what if this Node can be added in two different Scenes with two different parent nodes? What if this node is used in Scenes, but also Standalone?
This is why I suggested a more fleshed out API/Interface for this, because I dont see how with the suggested one-off method this is any more then just a hack that justifys your narrow use cases.

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 7, 2022

what if this Node can be added in two different Scenes with two different parent nodes?

The method provides a way to check under what parent the node is added, so it can adapt the logic based on that. Also you are instancing the node, so you have full control over where it's added.

What if this node is used in Scenes, but also Standalone?

Then the method will be called on the root and the provided parent will be scene tree root.

This is why I suggested a more fleshed out API/Interface for this,

You didn't really suggest anything, just threw "this needs better API" without any specifics.

narrow use cases.

Eh...

@Diddykonga
Copy link

Diddykonga commented Aug 7, 2022

I was too rude, sorry about that.

So 3 different ways to add/instance a Node/Scene:

  1. via Scenes:
@onready var my_editor_child_scene : MyChildScene = $MyChildScene #This was added in Editor, so the Editor adds it. (1)
var my_child_scene : MyScene
var my_scene : PackedScene = preload("blah:blah")
func _ready(): 
    my_child_scene = my_scene.instance() as MyScene
    add_child(my_child_scene) # (2)
  1. via Other Nodes:
var my_label : MyLabel
func _ready(): 
    my_label = MyLabel.new()
    Label3D.new().set_script(my_label)
    add_child(my_label) # (3)

Does this method get called in all 3 scenarios?

@Diddykonga
Copy link

Diddykonga commented Aug 7, 2022

So I have noticed through a lot of Proposals and PR Discussions, that Scenes could be useful as Custom Resources.
This could build off the work that william is doing godotengine/godot#48201

Which would allow storing Custom Data along with each Scene, and could allow virtual methods to be overridden for Custom Behaviour.

Such as the ability to override instance() allowing you to instance only the Nodes from your PackedScene collection that is deemed correct for the Custom Data that is stored along with the Scene Resource.

This a fuller interface, that could provide for many more use cases while still providing for your own, while at the same time, not introducing a new interface into Node but rather Scene, so that only Scene usage is changed.

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 7, 2022

Does this method get called in all 3 scenarios?

No, only 1 and 2, as they involve scene instancing (editor just uses instantiate() under the hood). I think the method should be called somewhere here:
https://github.com/godotengine/godot/blob/836fe9adcd94e68c30d6042d1ce5bfabde6b3473/scene/resources/packed_scene.cpp#L340-L344

Such as the ability to override instance() allowing you to instance only the Nodes from your PackedScene collection that is deemed correct for the Custom Data that is stored along with the Scene Resource.

Not sure if this will be possible. I'd need a custom resource deriving from PackedScene, but overriding built-in methods is not possible, so it wouldn't work anyway. And even if I could do that, I highly doubt it would give fine-grained control over every node, unless it was exposed as an explicit virtual method (which goes back to my proposal again...).

@Lielay9
Copy link

Lielay9 commented Aug 8, 2022

I don't really see how this could work at the node level. It'd be incredibly messy to know what signal connections will remain and what scripts will run. Surely this would be much more beneficial at the scene level, no?

If you're following best practices, the absence of a child scene shouldn't affect other nodes (You can't even edit the children of that scene by default). It'd alleviate the guesswork and make scenes work more predictably. Also, the function will likely be inside the root of a (child)scene anyway since entities often consist of multiple nodes (mesh instance, physics body etc.).

In short, even if the function (can_instance()) would be part of a node, I think it should only allow deleting itself if it's a root of a scene. This though' leads to my second point...

We kinda already have this. You can load a scene as a placeholder. I wouldn't blame anyone for not knowing since they're so impractical to use. But even right now, you can mark a child scene to be loaded as a placeholder, replace its script with a new one that extends Node (cannot extend InstancePlaceholder directly) and looks something like this:

extends Node

func _ready() -> void:
	if Constants.flag:
		call_deferred(&"create_instance", true)
	else:
		queue_free()

Obviously, this isn't an ideal workflow (cannot retain signal connections to the root node or change any of its exported properties for example), but since it exists we can consider whether it'd be more worthwhile to try to improve working with placeholders over coming up with an entirely new approach.

@Zireael07
Copy link

Placeholders really need to be better documented - they could likely solve many problems that cause repeated proposals around adding/removing nodes to tree etc.

@TheDuriel
Copy link

TheDuriel commented Aug 8, 2022

I would advocate towards using spawner and factory classes to create your objects, you'll likely want something like that anyways. Creating a custom placeholder class for your use case isn't that crazy.

export a property that holds the scene file you wish to import
export a property that dictates if it should spawn the scene on _ready, save this property as you wish
always spawn it in the editor, but set no owner, for easy preview

So in my opinion: can it be worked around with a few lines of script?

Yes.

The existing placeholder node in godot seems to be targeted for editor performance use cases. Where at runtime the scene is always meant to load, but you don't want it visible in the editor. A niche use case.

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 8, 2022

The problem is that you end up with factory objects instead of actual objects on your scene. I'd need to write some custom code that instantiates the scene in the editor to have a preview and even more code to expose the properties of the underlying scene. It basically requires a dedicated system to have the same convenience as simply placing the objects on the scene, which is definitely more than a few lines.

@TheDuriel
Copy link

It's really not.

export(PackedScene) var scene: PackedScene setget set_scene
export(bool) var spawn: bool = true

var instance: Node

func set_scene(new_value: PackedScene) -> void:
	clear_instance()
	scene = new_value


func _get_configuration_warning() -> String:
	if not scene:
		return "No Scene set."


func _ready() -> void:
	
	SaveSystem.get_data_for_myself_or_something() # Every save game system is unique.

	if not scene:
		return
	
	if Engine.editor_hint or spawn:
		instance_scene()


func instance_scene() -> void:
		instance = scene.instance()
		add_child(instance)
		# Do not set owner. So in the editor the instance won't be saved


func clear_instance() -> void:
	if instance:
		instance.queue_free()


func get_save_data() -> Dictionary:
	# Here you save a unique ref to this node, and the state of spawn
	pass


func apply_save_data(data: Dictionary) -> void:
	# Here you apply the spawn property, presumably called in response to the node requesting it during _ready

The only complicated thing here is: How you built your savegame system. Something the engine can't help you with anyways.

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 8, 2022

Well, your code doesn't handle exported variables. You probably need _get_property_list() that instances the scene, goes over all variables and exposes them. Then they need to be applied when instancing.

@TheDuriel
Copy link

TheDuriel commented Aug 8, 2022

collectibles that appear only once
enemies that appear as part of an event.
pickups in a chest.

Sounds like it would handle these just fine without exposing the property list of those objects? They're either generic and there is no need to access their properties. Or they're wholly unique and you are loading a unique scene file for them.

If your scene does need a per instance unique configuration, you can easily expose the properties that matter to you and set them in instance_scene().

I don't think "make a few instancing classes / factories for your coins and enemies" is too much to ask of someone developing a game with a need for such a thing.

@Lielay9
Copy link

Lielay9 commented Aug 8, 2022

I would advocate towards using spawner and factory classes to create your objects, you'll likely want something like that anyways. Creating a custom placeholder class for your use case isn't that crazy.

Creating factories is a lot more work than it needs to be, and they have the drawback of messing with your intended scene-hierarchy. I think it'd be beneficial to have the child scene itself be aware when it needs to be instanced.

Also the provided factory implementation could be simplified a lot by using instanceplaceholders:

@export var instance_placeholders := true

func _ready() -> void:
	if instance_placeholders:
		for child in get_children():
			if child.get_scene_instance_load_placeholder():
				(child as InstancePlaceholder).create_instance(true)

This is practically what they were made for anyway... but you still have to use an extra node, you can't at a glance see what nodes are placeholders etc.

The existing placeholder node in godot seems to be targeted for editor performance use cases. Where at runtime the scene is always meant to load, but you don't want it visible in the editor. A niche use case.

The above isn't entirely true unless you meant the opposite. The scene is loaded and is editable in the editor, but when the scene is loaded ingame it is replaced by a InstancePlaceholder. The only drawback of using them in terms of the problems raised in this proposal is that they need someone else to call them to instance themselves bar the cumbersome solution in my previous comment #5102 (comment).

If you'd be able to attach a script inside the editor to the instanceplaceholder that replaces the scene, it'd be a perfect solution.

@TheDuriel
Copy link

If you'd be able to attach a script inside the editor to the instanceplaceholder that replaces the scene, it'd be a perfect solution.

You... can just extend the class yes. Like any other Godot object.

@Lielay9
Copy link

Lielay9 commented Aug 8, 2022

If you'd be able to attach a script inside the editor to the instanceplaceholder that replaces the scene, it'd be a perfect solution.

You... can just extend the class yes. Like any other Godot object.

I'm not certain if we are on the same page here? Nodes can only have one script attached to them at a time. Extending doesn't really make any sense in this context.

You can mark a scene to be loaded as a Placeholder (rightclick a childscene root node and toggle "Load As Placeholder" on). Barely anything changes inside the editor, but at runtime the scene will be replaced by a InstancePlaceholder node. You cannot choose what node will replace the scene and therefore you cannot create your own implementation of InstancePlacholder. You can change the script that is attached to the child-scene root, in which case the script gets attached to the InstancePlaceholder but in that case you no longer can change the values of the child-scene-root-script.

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 8, 2022

Sounds like it would handle these just fine without exposing the property list of those objects?

No, my collectibles do have some configuration. For some of them I even have a tool script that changes their sprite depending on some variables.

I don't think "make a few instancing classes / factories for your coins and enemies" is too much to ask of someone developing a game with a need for such a thing.

If anything it should be one factory. Multiple factories just to place their associated objects is just very cluttered and messy. And still, while implementing it is relatively easy, it requires a significant workflow change. So in this case the best workaround is very involved, while the core change would require a few lines. Simple workarounds exist and I'm using them, but they come with drawbacks, as I described in the OP.

@Lielay9
Copy link

Lielay9 commented Aug 8, 2022

Alright, I checked the pull request and I'm starting to warm up to the idea. That said, I thought about it a bit more, and I believe that this could actually be done in combination with editor-only nodes. Relevant: godotengine/godot#56446 and #3433. Each node will get a boolean property editor_only; if set to true, the scene won't load that node. In addition, the scene could check that property once more before adding the node to a tree so that you can change the value inside _init(). That way the repercussions of doing so are clear and the property could be visually toggleable and visualized inside the editor.

The drawback compared to the function is that you can't access the parent node, but I'm not convinced it's that important, not to mention it kind of goes against our written instructions on best practices.

Edit: I forgot of course that editor-only nodes will always be present inside the editor so this approach wouldn't work at stripping tool-nodes from scenes. Not sure how important that is.

Edit2: Of course, the implementation of this proposal and editor-only nodes aren't necessarily mutually exclusive. Both can be implemented if deemed necessary.

@Lielay9
Copy link

Lielay9 commented Aug 8, 2022

Also.

...(name up to discussion; this one is obviously misleading)...

Would _can_enter_tree() be a more fitting name than _can_add_to_scene()? The former is somewhat self-explanatory: The node is constructed (calls _init()), but not added to the tree (doesn't call enter_tree()).

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 8, 2022

_can_enter_tree() is a bit misleading too, because you can add the node afterwards.

Also editor-only nodes are a bit related in terms of the implementation (similar code in the same file), but the use-cases are vastly different. Editor-only node needs to be freed as soon as possible. But in my case, the node needs to:

  • do all pre-initialization (connections, groups; they are harmless, so don't need to be blocked and sometimes they are useful)
  • keep the node intact (no freeing)
  • give some access to parent (for scene context purposes)

I need control whether the node should be freed before being added to scene or simply added later.

@Lielay9
Copy link

Lielay9 commented Aug 8, 2022

_can_enter_tree() is a bit misleading too, because you can add the node afterwards.

I see. I guess it makes the pulls name makes then a bit more sense even if I'm not a big fan of it either...

Also editor-only nodes are a bit related in terms of the implementation (similar code in the same file), but the use-cases are vastly different. Editor-only node needs to be freed as soon as possible. But in my case, the node needs to:

  • do all pre-initialization (connections, groups; they are harmless, so don't need to be blocked and sometimes they are useful)
  • keep the node intact (no freeing)
  • give some access to the parent (for scene context purposes)
    I need control whether the node should be freed before being added to scene or simply added later.

Also, I must've missed this one in the implementation and yeah, makes sense. Though I can't help but note how close all these needs are met by InstancePlaceholders or would be if they weren't such a pain to use. I can see the advantages of the simple function though and prefer using it. That said we should probably still assess if we need three things (this, placeholders and maybe editor-nodes) that do almost the same thing.

@me2beats
Copy link

me2beats commented Aug 9, 2022

See this one as well #1992

func _can_instance(parent: Node) -> bool:

does this mean I can't use it for multiple parents?
Or say like: "this node should be a singleton".

Btw now autoload singletons are not real singletons (I guess?) because you can instantiate it and add to the tree multiple times. The engine could prevent it imo

@me2beats
Copy link

me2beats commented Aug 9, 2022

Oh it seems I kinda get it now
That is each time the node is added to some parent, the function _can_instance is called and if false is returned it doesn't add it.

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 9, 2022

No in my case this is only called during scene instancing. The node just can be part of a scene and the user has no control whether it should be added or not. But for simply adding to parent you have that control, so calling this function is not necessary.

@me2beats
Copy link

me2beats commented Aug 9, 2022

Does instancing mean creating an object, not adding in
Or just creating an instance of a scene (some_packed_scene.instance())

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 9, 2022

Instancing i.e. unpacking the PackedScene object. Either when running a scene or when using instantiate().
You can check the PR, it might make more sense if you see the code.

@Mickeon
Copy link

Mickeon commented Aug 9, 2022

I like this proposal as I find the problem it's trying to solve to be noteworthy, but I feel like there's gotta be another solution to this dilemma that isn't a a virtual _can_add_to_scene() method, or what have you. It feels... It may just be me, but it feels botched.

@Mickeon
Copy link

Mickeon commented Aug 9, 2022

What if it was more broad? I apologise, as this may have been already suggested in another reply.

A virtual method that is called when the Node is instantiated, if it's been defined? Would that be too slow? Or awkward?

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 9, 2022

Not sure what do you mean exactly. Like, every time a Node is created? This is only useful when the node is being added to scene. Broader method might be maybe useful for controlling how others use your node (kind of what #1992 wanted), but that's out of scope for this proposal.

Also virtual methods are rather cheap. Nodes already have a few callbacks (enter tree, ready etc) and they don't really affect performance if you don't use them.

I feel like there's gotta be another solution to this dilemma that isn't a a virtual _can_add_to_scene() method

That's the best solution I could come up with for this particular problem, but if you have a better one feel free to suggest it.

@Mickeon
Copy link

Mickeon commented Aug 9, 2022

Actually, in truth, it does strike me as odd that the very Node that is being instanced by PackedScene is deciding its fate. That may cause trouble if its dependencies aren't handled properly. Shouldn't that be a responsibility of the owner of this action? In this case, either the root Node of the Scene, or the PackedScene itself, somehow?

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 9, 2022

Root node could work, but it's impractical. E.g. in the 'item in a chest' case, the only two nodes that need to know about it are the chest and the item, the scene root is uninvolved. If it was handled by the root, the method would need to pass both nodes and in the end the logic would be delegated to the chest node.

Handling by PackedScene is impossible. Or I just can't imagine how it would work; by attaching a script to PackedScene?

@me2beats
Copy link

me2beats commented Aug 9, 2022

You can check the PR, it might make more sense if you see the code.

I tried but I failed (not very familiar with those engine methods)

Now I read the proposal again, and it seems I get it better now

I load a scene, but there is a node I don't need, so I create it without adding to the tree. Its _enter_tree() and _ready() (and @onready) is not called.

So this would work like a PackedScene instruction to not add some of its children, leave them orphan when creating an instance of a scene.

That is if you write

    return false

this would mean the node will be created but will not be added, you can add it later manually with add_child(), storing the node in some variable/array/dictionary is up to you.

@me2beats
Copy link

me2beats commented Aug 9, 2022

If so, _can_add_to_scene() [the name I can see in the PR] sounds better to me than _can_instance().

but on the other hand _can_add_to_scene() is kinda still not perfect?
this may sound like the node can never be added, but you can

It is kinda like:
can_add_to_scene_when_the_scene_is_instantiated

@Mickeon
Copy link

Mickeon commented Aug 9, 2022

Handling by PackedScene is impossible. Or I just can't imagine how it would work; by attaching a script to PackedScene?

At it's core it'd still have to be a method to be executed...

Like, a Callable? something to be set to the PackedScene, before instanciate() is called. For each Node that is being created, it is passed to the Callable, that then decides what to do with it.

I can see this causing problems with nested PackedScenes, though...

@me2beats
Copy link

me2beats commented Aug 9, 2022

So the proposal could be named like
Add a way to disable auto-adding of some nodes to a scene when instancing it

And it makes more sense to me now, because you don't want things to always do something automatically, sometimes you want to do this manually.
Scenes create all nodes, then add them all "automatically". But you may not want this for all nodes in the scene

@Pigzinzspace
Copy link

Pigzinzspace commented Aug 9, 2022

I see the implementation like this:
we can specify for each child node that it is included in template_1, template_2, template_10 in the inspector, just like in the group editor.
and then

var my_scene = preload("blah:blah")
  func _ready() :
      my_scene_instance = my_scene.instance("template_10")

or

const my_scene = preload("blah:blah").instance("template_10")
  

in "my_scene" script

func _init(template) : 
  match template :
       "template_1" :
          code.....
        "template_10" :
          code.....

when we need to add or remove a specific node
we are writing,

   var my_scene = preload("blah:blah")
   func _ready() :
   # if we need to add nodes to the template

       my_scene.active_in_tree("template_10", [node_path]) 
       
  # if we need to remove nodes from the template
       my_scene.not_active_in_tree("template_10", [node_path]) 
       my_scene_instance = my_scene.instance("template_10")

@Lielay9
Copy link

Lielay9 commented Aug 9, 2022

Created a discussion about issues of InstancePlaceholders and possible enhancements: #5128
There is a section at the end that briefly goes into how they could be used to create scene variations (an idea stolen from this proposal). It wouldn't be an exact match with what is proposed here, but you might still want to take a quick glance at it since the topics are relatively close to one another.

@Diddykonga
Copy link

Diddykonga commented Aug 9, 2022

Not sure if this will be possible. I'd need a custom resource deriving from PackedScene, but overriding built-in methods is not possible, so it wouldn't work anyway. And even if I could do that, I highly doubt it would give fine-grained control over every node, unless it was exposed as an explicit virtual method (which goes back to my proposal again...).

It is 100% possible, an API will have to be bikeshedded. If no else does, I'll open a PR about it.

This could give you even more fine grain control then what you are asking for now.

extends PackedScene

class_name MainLevel

@export var is_ready = false

func _instantiate() -> Node: # Called by `instantiate()` if overridden, to provide the Nodes
   # Dont return any Nodes, because we aren't ready
    if not is_ready:
        return null

    # Either check SceneState data, to prevent unnecessary allocations, this would require additional helper methods
    # Or allocate all Nodes, then check which ones to remove/modify/etc.
    var root = get_state().instantiate()
    root.get_node("Collectables/Collectable").free()
    root.get_node("Players/Player").global_transform.origin = Vector3()
    return root 
    

    # Default behavior
    return get_state().instantiate()

@willnationsdev
Copy link
Contributor

willnationsdev commented Aug 11, 2022

I know that, currently, there is code in the engine that specifically rules out Script and PackedScene as base resource types that can have scripts attached to them to customize their behavior. If you were to implement a solution to this with @Diddykonga's approach, it would likely have to be implemented as an exported Callable property that can be assigned to the direct, engine-level C++ PackedScene class to override its instantiation behavior rather than a virtual method you override with a script. So something more like this?

extends Node
class_name Game

const MainScene := preload("res://main_scene.tscn") as PackedScene

static func instantiate_main_scene(is_ready: bool) -> Node:
    MainScene.instantiate_override = func():
        # do same logic rather than this. By default `instantiate_override`
        # would be `null` and would do the same as this logic.
        return MainScene.get_state().instantiate()
    return MainScene.instantiate()

Alternatively, if you wanted it to be something that is built into the existing Godot systems (so you don't have to hand-implement custom logic for each individual scene file via preloaded constants), you would have to add some sort of "handler" API to the core engine that the C++ PackedScene::instantiate() method directly iterates over and executes virtual methods on, similar to ResourceLoader and the ResourceFormatLoader class for which users can extend the CustomResourceLoader class to automatically hook into that API. If you did something similar, you might have, say, a CustomSceneConstructor class that scripts could extend to provide alternative handlers for building a scene.

However, this would likely have a lot of complex ramifications for the Editor and other parts of the engine C++ code as everything currently expects all of the details of a scene to exist after the scene is instantiated. There would need to be some way for each individual detail of a scene to provide declarative metadata to differentiate which things were purposefully ignored/handled rather than those that failed to be setup erroneously; otherwise, you'll end up with error messages popping up in the debug log with every instantiation of the customized scene.

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 11, 2022

I think this is going too far xd

I presented my problem and a very simple solution that solves it. If you want some super complicated PackedScene API, first show a problem that it solves (a real, actual one). Otherwise there is no reason to make it any more complex.

@willnationsdev
Copy link
Contributor

lol, yeah, you're probably right. Honestly, your solution is simple and solves the problem decently enough. It could always be added and then if people find that the solution needs more refinement/expansion, an alternative can be suggested/implemented. I was simply noting that Diddy's suggestion alone wouldn't work out-of-the-box due to it relying on extending the PackedScene type.

@Diddykonga
Copy link

I was simply noting that Diddy's suggestion alone wouldn't work out-of-the-box due to it relying on extending the PackedScene type.

While It wont work out of the box, or as the Engine is, is there any real reason why those specific rules you mentioned couldn't be adjusted to only disallow Scripts and not PackedScenes?

@KoBeWi
Copy link
Member Author

KoBeWi commented Sep 18, 2023

Sooo I re-evaluated this proposal 🤔

collectibles that appear only once

Turns out queue_free() in _ready() is sufficient here. The object does not appear for a frame like I mentioned before. It may happen when you wait one frame before removing it; I was doing it in my case, because it was too early to access an information that I needed to remove the object, but I found another way to determine it.

enemies that appear as part of an event.

I just tested this one; enemies removed in parent's _ready() also don't appear and they don't cause unwanted physics interactions.

pickups in a chest

Same as above tbh. In worst case I could take advantage of the fact that parent's _enter_tree() runs when the child is available, so I could set a flag that prevents _ready() initialization.

running level scenes independently

This is not solvable currently, but it's editor-only problem and in worst case it causes some error prints, so it's minor.

I was going to trying to get this feature into 4.2, but then realized I don't need it that much. My most gripes came from one project that was just badly coded, but now I'm finished with it.

I will keep this open for now. Maybe I will still find a case that isn't possible to reasonably resolve, but I think this proposal can be retired at this point. Unless there is someone else who needs this feature.

@Mickeon
Copy link

Mickeon commented Sep 18, 2023

I was going to trying to get this feature into 4.2, but then realized I don't need it that much. My most gripes came from one project that was just badly coded.

Insightful. Some users may feel the same way for features they have proposed in the past.

@KoBeWi
Copy link
Member Author

KoBeWi commented Jul 7, 2024

The only case I found where queuefreeing in ready was not sufficient was a bug (godotengine/godot#93871). I also found a way to delete nodes before they are added to scene tree (they are still created, but almost all logic usually runs once the node is added to scene), so I think this can be closed.

@KoBeWi KoBeWi closed this as not planned Won't fix, can't repro, duplicate, stale Jul 7, 2024
@KoBeWi KoBeWi added the archived label Jul 7, 2024
@shanayyy
Copy link

shanayyy commented Jul 8, 2024

I think adding an "installed" property to the node is enough. If installed is false, the node is "created" but not "added" during scene tree instantiation.
This will make the node an orphan node, but it is your responsibility to store it somewhere else beforehand.

@shanayyy
Copy link

shanayyy commented Jul 8, 2024

My use case is to use some nodes as pluggable behaviors, whose plugging and unplugging are managed by other nodes. Since these behavior nodes have some side effects in _enter_tree and _exit_tree, it is not desirable to manually remove them after the scene tree is loaded.

@KoBeWi
Copy link
Member Author

KoBeWi commented Jul 8, 2024

Well my newest solution is manually removing the nodes before the parent is added to scene. E.g.:

var level = load("res://Levels/Level1.tscn")
level.prepare()
add_child(level)

And then in Level:

func prepare():
	for node in get_children():
		if node.has_method(&"can_add") and not node.can_add():
			node.free()

The advantage is that you can safely call free() and the node is completely erased before doing anything.

@shanayyy
Copy link

shanayyy commented Jul 8, 2024

Well my newest solution is manually removing the nodes before the parent is added to scene. E.g.:

var level = load("res://Levels/Level1.tscn")
level.prepare()
add_child(level)

And then in Level:

func prepare():
	for node in get_children():
		if node.has_method(&"can_add") and not node.can_add():
			node.free()

The advantage is that you can safely call free() and the node is completely erased before doing anything.

Thanks for the suggestion, however I still need to mark each node for removal and add a parent to call this prepare function. This can be a temporary solution.

I found that I can directly remove the child node in the parent node's _enter_tree, and then the child node's _enter_tree and _ready will not be called

@shanayyy
Copy link

shanayyy commented Jul 9, 2024

My new solution:

class_name InstalledNode extends Node

@export var will_be_installed: bool = true

func _notification(what: int) -> void:
		if what == NOTIFICATION_PARENTED and not will_be_installed:
			get_parent().tree_entered.connect(remove_from_parent, CONNECT_ONE_SHOT)

func remove_from_parent():
	get_parent().remove_child(self)
	will_be_installed = true #Manual add_child later will not be removed

Then if I have any nodes that don't need to enter the scene tree immediately, I can just add them as children of InstalledNode.

Now I can use the full power of Godot editor when configuring dependencies for behavior nodes. 🌝🌝

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants