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

Added Node name to print() of all Nodes by making to_string() in Object virtual, so it can be overriden in C++. #38819

Merged
merged 1 commit into from
Jul 15, 2021

Conversation

EricEzaM
Copy link
Contributor

Closes godotengine/godot-proposals#693

func _ready():
	var special_node = $MySpecialNode
	print(special_node)
	
	var node = Node2D.new()
	print(node)
	
	var node2: Node2D = null
	print(node2)

Outputs

MySpecialNode [Node2D:23370662652]
[Node2D:23454548333]
Null

I did not add position to Node2D/3D/Control as I wasn't sure if it was really need. Some people won't need it, and there is the question of whether to return local/global position (or other info).

@Calinou Calinou added cherrypick:3.x Considered for cherry-picking into a future 3.x release enhancement topic:core labels May 18, 2020
@Calinou Calinou added this to the 4.0 milestone May 18, 2020
scene/main/node.cpp Outdated Show resolved Hide resolved
@EricEzaM EricEzaM force-pushed the improve-to_string-for-nodes branch from 01a3df1 to f746455 Compare May 18, 2020 08:30
@ThakeeNathees
Copy link
Contributor

IMO all string log about the object must be inside the brackets
something like : [Node2D(MySpecialNode):23370662652]

@EricEzaM
Copy link
Contributor Author

I like that idea. I'll make the change when I get up in about 9 hours 😁

@mhilbrunner
Copy link
Member

mhilbrunner commented May 18, 2020

All languages that I know of that have this feature let the programmer completely override the output. I.e. if I only want it to print MySpecialNode, that works. If I still want it to print Node type/name/ID I can do that too by simply adding them to the string manually (as long as I have access to that data of course).

If we do this, we should IMO allow that freedom, and not do this halfway and constrain the output.

Java works that way, C# works that way etc.

Forcing the overriden output to be surrounded by the current output makes this unnecessarily less flexible and useful, IMO.

@akien-mga
Copy link
Member

@mhilbrunner That's how it works already AFAIK. If you have a script instance, it will return the result from its to_string() method, which should be implemented in all script instances to allow users to fully specify the return value.

See GDScriptInstance::to_string in modules/gdscript/gdscript.cpp for GDScript.

@mhilbrunner
Copy link
Member

Yes, I was talking about the other comments :) but maybe I misunderstood them.

@akien-mga
Copy link
Member

Well the discussion is about what the default representation of a Node should be:
https://github.com/godotengine/godot/pull/38819/files#diff-685aaf5e6e44dbc6a5700ebaae623e60R1960

NodeName [Node:ID] vs [Node(NodeName):ID].

@mhilbrunner
Copy link
Member

Then I indeed misunderstood. :) I don't personally care much, both are fine as default for me.

@EricEzaM
Copy link
Contributor Author

Hmm on second thought this morning, I think I will leave it at NodeName [Node:ID] unless changes are requested from core maintainers. I think NodeName [Node:ID], it is quicker to find the information you are looking for, whereas in [Node(NodeName):ID], all the information is condensed and I feel that your eyes have to do more work to find what you are after.

Additionally, doing it this way would not allow me to just use the Object::to_string() base implementation as akien mentioned in his review. Since the info would need to within square the brackets, I would need to construct the whole thing again. Not a big deal I guess, but still.

@Xrayez
Copy link
Contributor

Xrayez commented Sep 13, 2020

I got surprised that to_string is not virtual in the first place. I'd certainly find this useful for C++ modules development, it's strange that it's possible to do this in GDScript and not in C++, unless I'm missing something. 🤔

I'm currently implementing some data structures in a C++ module which I'd like to expose to scripting, having this feature built-in would solve a lot of hacks.

See goostengine/goost#14 and goostengine/goost#12 to justify this change.

@KoBeWi
Copy link
Member

KoBeWi commented Sep 15, 2020

This probably needs rebase after #42093 was merged.

I'm not sure about the string format. When you print a node as a part of longer string separated by spaces, it's less obvious that Name and [Object:ID] are related. Maybe they should be linked with : or -? Or [Name [Object:ID]] 🙃

scene/main/node.h Outdated Show resolved Hide resolved
@EricEzaM EricEzaM force-pushed the improve-to_string-for-nodes branch from f746455 to 4d3075a Compare July 11, 2021 14:40
@EricEzaM EricEzaM requested a review from a team as a code owner July 11, 2021 14:40
@EricEzaM
Copy link
Contributor Author

print(self)
# CoolNode:[SpinBox:28454159399]

scene/main/node.cpp Outdated Show resolved Hide resolved
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a comment to amend but otherwise it seems good to me.

@EricEzaM EricEzaM force-pushed the improve-to_string-for-nodes branch from 4d3075a to 3ca25ff Compare July 15, 2021 11:49
@KoBeWi
Copy link
Member

KoBeWi commented Jul 15, 2021

Is the get_name() ? condition even necessary? I don't think we can have an unnamed node, no?

@akien-mga
Copy link
Member

Is the get_name() ? condition even necessary? I don't think we can have an unnamed node, no?

Node must have a name when in the Scene Tree, but outside it they can be unnamed:

var node = Node.new()
print(node.name)
add_child(node)
print(node.name)

Prints:


@@2

@akien-mga akien-mga merged commit f79e79d into godotengine:master Jul 15, 2021
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 3.4.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jul 20, 2021
@EricEzaM EricEzaM deleted the improve-to_string-for-nodes branch October 5, 2021 12:20
@securas
Copy link

securas commented Oct 25, 2021

For the sake of compatibility, this should be made optional. Also, to reduce amount of printed text when necessary.

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.

Printing node should contain its name
8 participants