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 Node._scene_instantiated() method and @oninstantiated annotation #87594

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dalexeev
Copy link
Member

This PR has two parts:

1. (core) This is a very small change. Add Node._scene_instantiated() virtual method called when NOTIFICATION_SCENE_INSTANTIATED is sent. An additional rationale is that _notification() is called for other notifications (e.g. if you override _process(), _draw(), _input(), etc.) and _notification() is a multi-level call even in GDScript (see #81139), so it's a bit redundant to use _notification() if you just want early initialization.

2. (GDScript) Add the @oninstantiated annotation, which works similar to @onready. I'm not sure about this since NOTIFICATION_SCENE_INSTANTIATED is more specific (only called on the scene root). Perhaps we should first accept the first part only, get user feedback and accept the second part if the community likes the feature. Or maybe we can first improve the NOTIFICATION_SCENE_INSTANTIATED behavior in core.

Example:

var title: String:
    set(value): _title.text = value
    get: return _title.text

@oninstantiated var _title: Label = %Title
@oninstantiated var _buttons: HBoxContainer = %Buttons

func _scene_instantiated() -> void:
    # Clear placeholders.
    title = ""
    clear_buttons()

func add_button(button: Button) -> void:
    _buttons.show()
    _buttons.add_child(button)

func clear_buttons() -> void:
    _buttons.hide()
    for child in _buttons.get_children():
        child.queue_free()

@Mickeon
Copy link
Contributor

Mickeon commented Jan 26, 2024

I am certain the virtual method will be used a whole lot, but not convinced on the @oninstantiated one bit. For things that would be done in _scene_instantiated, you are likely to need more control that isn't just automatically assigning them like @oninstantiated would do.
The annotation has to at least wait until there's substantial demand, otherwise GDScript risks being polluted with specific annotations such as these. And that's not mentioning how long the name itself is.

@tokengamedev
Copy link

I am surprised as I looked into the discussion, it seemed to explain the exact problem I have been facing.

As part of the solution both are required and moreover, I will give preference to annotation than the virtual method. The reason is well explained in the discussion, but I will reiterate.

# parent.gd/parent.tscn
class_name parent extends Node2D

const CHILD = preload("res://child.tscn")

func _ready()
    # Dynamically create the child
    var new_child = CHILD.instantiate()
    child.title = "Some Text" # You will not be able to do it without the annotation, Annotation will fill the control
    add_child(child)

the child scene

# child.gd/child.tscn
class_name child extends Node2D

var title: String:
    set(value): _title.text = value
    get: return _title.text

@onready var _title: Label = $Title # This will only be filled after child is added to the parent and you cannot define the variable without the @onready

I am not saying there are workarounds as mentioned in the discussion and even more, but it is always a headache and redundant code written again and again.

Ideally all controls that needed the scene_tree (for animation and others) should be @onready and for which I need to set some property values, it should be declared with @oninstantiated.

@Mickeon
Copy link
Contributor

Mickeon commented Jan 26, 2024

The thing about the annotation is that other solutions to the same issue may be found in the future, which may apply to a wider range of contexts, not just during scene instantiation. If or when it happens, it would render one entire annotation basically redundant. So let's tread carefully.

@Mickeon
Copy link
Contributor

Mickeon commented Jan 26, 2024

For example, and I'm suggesting on an absolute complete whim here: instead of @oninstantiated, @onready itself could be allowed to take optional parameters (similar to @rpc) to further customize when the property is set, and one of them could be "instantiate".

doc/classes/Node.xml Outdated Show resolved Hide resolved
doc/classes/Node.xml Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants