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_child() returns pointer to new child node #33926

Closed
wants to merge 1 commit into from

Conversation

dkotfis
Copy link

@dkotfis dkotfis commented Nov 26, 2019

Fixed #6521

Adding a child now returns a pointer to the resulting child node.
This enables one-line chaining commands.

On failure to add the node, returns null.

@nathanfranke
Copy link
Contributor

A discussion may still be needed for this. Personally, I am against this feature for the same reason as reduzio ... things should be explicit even if it means writing more code.

@akien-mga akien-mga changed the title addChild() returns pointer to new child node add_child() returns pointer to new child node Nov 27, 2019
@akien-mga akien-mga added this to the 4.0 milestone Nov 27, 2019
@groud
Copy link
Member

groud commented Nov 27, 2019

I agree with @nathanwfranke, that it kind of encourage hard to read code.

@rainlizard
Copy link
Contributor

Game Maker does in 1 line what Godot does in 4 lines. No one in the GM community ever complained about instance_create.

instance_create(x, y, object)
var scene = preload("res://file.tscn")
var id = scene.instance()
add_child(id)
id.position = Vector2(x, y)

add_child() always seemed weird and stiff to me, and now I'm finding out it was a deliberate design decision to make it that way? It should return a value for better code flexibility. If you don't like it don't use it.

@groud
Copy link
Member

groud commented Nov 27, 2019

instance_create(x, y, object)

This does not make any sense in Godot's philosophy, where everything is a node, not only objects in the scene (that have a position), but also animations players, sound players, generic nodes, etc...
So creating a function to add child only for positional node would make things inconsistent.

It should return a value for better code flexibility. If you don't like it don't use it.

Flexibility has always a cost. It will lead people to use different techniques in tutorials, which might cause confusion for beginners.

add_child() always seemed weird and stiff to me, and now I'm finding out it was a deliberate design decision to make it that way?

Honestly, I don't understand why. add_child() is a method to add a child to a node, it's simple and does only one thing.

The problem with this proposal is that it makes the API inconsistent for little gain. Why would the add_child() function return the added child, and all the setter functions not ? Like, why allowing this add_child(child).set_position(x,y) and not this add_child(child).set_position(x,y).set_rotation(10).set_color(...) ? If the point is to allow function chaining, it does not work here, as it only avoids writing one line of code.

@rainlizard
Copy link
Contributor

This does not make any sense in Godot's philosophy

It wasn't a suggestion, but rather pointing out how simple code can look while doing a lot. Meanwhile GDScript looks more complicated and takes more lines.

Flexibility has always a cost. It will lead people to use different techniques in tutorials, which might cause confusion for beginners.

Wasn't there a super negatively received issue called "No one knows how Godot works"? Read the first response #23052 (lol it's you)

Why would the add_child() function return the added child

Makes logical sense to me, you're adding a child so it returns the child you added. set_position doesn't add a child so why would it return the child?

it only avoids writing one line of code.

Right back at you, it's only one line of code so it's no big deal right? I guess the line would look like this:

var id = add_child(scene.instance())
id.set_position(x,y)

@groud
Copy link
Member

groud commented Nov 27, 2019

It wasn't a suggestion, but rather pointing out how simple code can look while doing a lot. Meanwhile GDScript looks more complicated and takes more lines.

That's where we disagree. More lines makes the code more explicit and easier to understand. The problem arises when your functions start to do multiple things at the same time. add_child() is meant to add a child, that's it. If you make it return the child for no reason, it does multiple things at once, thus making things more confusing.

Wasn't there a super negatively received issue called "No one knows how Godot works"? Read the first response #23052 (lol it's you)

Yeah, indeed. But this had nothing to do with code, but more with how to design your whole game systems architecture. This has little to do with the sugar syntax this feature proposes. At a macro level, flexibility is good, but at micro one, it's not.

Makes logical sense to me, you're adding a child so it returns the child you added.

Yeah, that's what makes no sense to me. If you add a child, it means you already have this child. So there is basically no point in returning something you passed as an argument.

it's only one line of code so it's no big deal right

Yeah no big deal, more line are ok if they make the code more explicit. So the current:

var node = scene.instance()
add_child(node)
node.set_position(x,y)

is perfectly fine to me.

@Sslaxx
Copy link

Sslaxx commented Nov 27, 2019

@rainlizard - @Xrayez proposed something similar (simpler?) at #6521 (comment)

@akien-mga
Copy link
Member

I suggest moving the discussion to #6521, this PR will not be merged for now as there is no consensus (and we debated this at length several times, always with a predominant opposition to adding such return value to add_child(), for reasons mentioned here and on #6521). The implementation of any feature that would end up being consensual should be straightforward, so we don't need a PR as support for the discussion.

This should ideally go through https://github.com/godotengine/godot-proposals, but since the original issue predates this workflow and has a lot of discussion, it's OK to continue it there (or we could move the issue to the proposals repo).

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.

Have add_child return a value.
7 participants