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 ready property (boolean) to nodes #5537

Closed
Nukiloco opened this issue Oct 4, 2022 · 9 comments
Closed

Add ready property (boolean) to nodes #5537

Nukiloco opened this issue Oct 4, 2022 · 9 comments
Milestone

Comments

@Nukiloco
Copy link

Nukiloco commented Oct 4, 2022

Describe the project you are working on

I'm working on an utility add-on for Godot called Quik. I'm on Godot version 3.5.1 so I'm not sure if what I want to do is possible with Godot's 4.0 await and lambdas.
One of the features I'm trying to add is allowing the user to wait for a node ready's state in a setter function, and if the node is already ready, then i allow it to continue through the code.
This is so that I can wait for nodes in setters by changing this code:

onready var label_node = get_node('Label')
onready var sprite_node = get_node("Sprite")
onready var player_node = get_node("Player")
onready var tree_node = get_node("Tree")

export var text = "" setget set_text
export(Texture) var icon = null setget set_icon
export var player_name = "" setget set_player_name
export var fruits_dropped = 5 setget set_fruits_dropped
# This set_text node will be called before ready so we have to check
# that the label_node is available, which in this case, it's not ready.

# Instead, we allow it to set the value, then in the _ready function
# we can then set the label_node text.
func set_text(val):
	text = val
	if label_node:
		label_node.text = val

func set_texture(val):
	texture= val
	if sprite_node:
		sprite_node.text = val

func set_player_name(val):
	player_name = val
	if player_node:
		player_node.player_name = val

func set_fruits_dropped(val):
	fruits_dropped = val
	if tree_node:
		tree_node.fruits_dropped = val

func _ready():
	set_text(text)
	set_texture(texture)
	set_player_name(player_name)
	set_fruits_dropped(fruits_dropped)

to

onready var label_node = get_node('Label')
onready var sprite_node = get_node("Sprite")
onready var player_node = get_node("Player")
onready var tree_node = get_node("Tree")

func set_text(val):
	text = val
	
	# This is a utility function in my add-on that waits for this node to be ready
    # When it is ready, we simply end the yield and continue on
	# With the proposal, we can check for the "ready" property" without having to store a lot of nodes in our script and costing a lot of performance in our code.
	# (Quik add-on global).(utility).(wait for ready)
	yield(q.u.wfr(self), "completed")
	label_node.text = val

func set_texture(val):
	texture = val

	yield(q.u.wfr(self), "completed")
	sprite_node.text = val

func set_player_name(val):
	player_name = val

	yield(q.u.wfr(self), "completed")
	player_node.player_name = val

func set_fruits_dropped(val):
	fruits_dropped = val

	yield(q.u.wfr(self), "completed")
	tree_node.fruits_dropped = val

Describe the problem or limitation you are having in your project

There seems to be no easy way currently to know what's the ready state on a node without having the user to manually write on_ready properties in their nodes, or I would have to store every single node in an array and detect when it's ready signal was emitted. This can be very costly on code performance.

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

The feature is to add a boolean property accessible in GDScript called ready. When the node is ready, the ready property is true, when it's not ready, then it's false. This overcomes having to make my utility script add every node on start-up, keep track of the ready states from their signals, and having to loop over a huge array, to make sure the node is ready. This is also so that the code can safely yield for a node's ready without having to make the mistake of having an infinite yield. This can reduce boilerplate and tediousness of checking for ready states on dynamic nodes too.

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

Utilities script

func wait_for_ready(node: Node):
	if node.ready:
		# Node is already ready, we should return as if we don't in this case, we can end up waiting forever
		return
	yield(node, "ready")

User's script

# in a setter
func set_text(val):
	yield(util.wait_for_ready(self), "completed")

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

It could be possibly worked around with around 150 or 300 lines of code. However this can be costly for performance especially when you use a lot of setter and getters, and call any setter or getter a lot of times.

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

Because it's impossible to grab any node and know it's ready in a script without having to keep track of all the nodes in your project.

@Mickeon
Copy link

Mickeon commented Oct 4, 2022

It shouldn't be an actual property, I don't believe users should mess with it as if it is one. For these use-cases, I believe a is_ready() would suffice (similar to request_ready()), which would come somewhat handy, actually.

Currently, in 3.x, one really common way to check for this is with a similar snippet of code:

onready var my_sprite = $MySprite
export var my_texture setget set_my_texture # The property is set in the Scene.

func set_my_texture(new: Texture):
	my_texture = new
	if (not my_sprite and not is_inside_tree()):
		yield(self, "ready")
	
	my_sprite.texture = my_texture
	

This, however, is not infallible.
The Node being "ready" doesn't mean it is inside the Tree, it's only part of the typical procedure. Once it is outside the tree, it still is ready. Also, my_sprite may be intended by the developer to simply not exist sometimes, meaning it may be possible that by executing set_my_texture, the function is left hanging indefinitely.

Frankly, because this is the only major example I can think of, maybe onready should work appropriately for non-Node properties, when the Scene assigns them. As in, the setter method is called after the Node is ready. However, that would introduce some very strange order of execution quirks, so it is probably best to have it be explicit.

@Nukiloco
Copy link
Author

Nukiloco commented Oct 4, 2022

@Mickeon
Ah I had bad wording there. I meant the ready property to be a getter. But the is_ready() function seems to be a more better solution to the problem as people would probably get confused with the ready property with the virtual method _ready.

I know that the code solution like above is what I have done in multiple projects with setters and getters. I'm used to checking for the node existing a lot of times in my setters when they are executed before ready. In my opinion, it just feels tedious to keep writing the same boilerplate ready code for each setter and I wish there was a better way of handling it.

I was wishing for there to be a is_ready or ready getter so I can quickly check when a node is ready to make a quick utility script so I didn't have to have to keep writing:

if not SPRITE_NODE:
	yield(self, "ready")

if not LABEL_NODE:
	yield(self, "ready")

if not PLAYER_NODE:
	yield(self, "ready")

# etc...

for the hundreds of setters that I use in every script.
Maybe what I'm truly looking for is a way for a setter to be ran when we are truly ready and most nodes are available to us.

@Mickeon
Copy link

Mickeon commented Oct 4, 2022

Indeed, with the getter that situation would be improved, already.

In Godot 4 it'd look something like this which, as much as the engine avoids boilerplate code, feels reasonable in comparison:

@export var my_texture: Texture2D:
	set(new):
		my_texture = new
		if not is_ready():
			await ready
		
		sprite.texture = my_texture

@Nukiloco
Copy link
Author

Nukiloco commented Oct 4, 2022

@Mickeon
Yea that code example in 4.0 seems way better to write than in 3.x.
Though maybe an end to most of the boilerplate in setter, would something like this work?

By adding a ready word to the set function here to mark that it should only be called when our node is ready.

@export var my_texture: Texture2D:
	ready set(new):
		my_texture = new
		if not is_ready():
			await ready
		
		sprite.texture = my_texture

or maybe @onready set to match with the already existing @onready feature.
Though I thought I already seen someone made a proposal like this before?

Well if 4.0 is coming around soon, I guess I would have to keep using boilerplate code in 3.x or move to 4.x though I'm not sure when 4.x is going to be stable enough?

@KoBeWi
Copy link
Member

KoBeWi commented Oct 4, 2022

Related: #325

btw we can't have ready property, because it conflicts with ready signal. Also from my experience, checking for is_inside_tree() and then awaiting ready signal is sufficient. I don't see how does is_ready() bring any benefit.

@Nukiloco
Copy link
Author

Nukiloco commented Oct 4, 2022

@KoBeWi
Does is_inside_tree being false always mean that we aren't in the ready state? I thought @Mickeon said above that a node can possibly leave the tree, and then re-enter it, but this wouldn't signal the ready state again.
If that's the case, then I think is_ready() is a good solution to probably shave off a little bit more boilerplate.
Because instead of doing this:

# sprite setter
if (not sprite and not is_inside_tree()):
		yield(self, "ready")

# player setter
if (not player and not is_inside_tree()):
		yield(self, "ready")

# label setter
if (not label and not is_inside_tree()):
		yield(self, "ready")

We can shave of some time by doing this:

# sprite setter
if not is_ready():
		yield(self, "ready")

# player setter
if not is_ready():
		yield(self, "ready")

# label setter
if not is_ready():
		yield(self, "ready")

Of course if your node is intended to leave and enter the tree at various points in your code, you can do this:

# sprite setter
if not is_inside_tree():
	return
if not is_ready():
		yield(self, "ready")

# player setter
if not is_inside_tree():
	return
if not is_ready():
		yield(self, "ready")

# label setter
if not is_inside_tree():
	return
if not is_ready():
		yield(self, "ready")

Again though #325 sounds like what I want..

@KoBeWi
Copy link
Member

KoBeWi commented Oct 4, 2022

Ah right, if you use is_inside_tree() with ready, the setter will hang forever if you call it when the node leaves the tree xd I never had this problem, so I just didn't think about it.

if (not sprite and not is_inside_tree()):

No need to do 2 checks. not sprite means that the node is not initialized yet, so awaiting ready is sufficient.

if not sprite:
	yield(self, "ready")

and

if not is_ready():
	yield(self, "ready")

are pretty much the same thing, because sprite being null means @onready is not initialized means node is not ready. (and I know you can assign null to sprite, but then you need a null check anyway)

Again though #325 sounds like what I want..

Yeah, it solves the same problem. It's simpler to use, but more complex to implement.

@L4Vo5
Copy link

L4Vo5 commented May 11, 2023

is_node_ready() was added by godotengine/godot#75751

@KoBeWi
Copy link
Member

KoBeWi commented May 11, 2023

Yes, closing as implemented.

@KoBeWi KoBeWi closed this as completed May 11, 2023
@KoBeWi KoBeWi added this to the 4.1 milestone May 11, 2023
@Calinou Calinou moved this to Implemented in Godot Proposal Metaverse May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Implemented
Development

No branches or pull requests

4 participants