Add `Node.get_unique_node()` #64023

Mickeon · 2022-08-07T00:20:09Z

This PR adds a new method to Node called get_unique_node(). The code is almost copied and pasted from part of get_node(). However, unlike the latter that passes through a NodePath, this method requires a StringName name as argument, and accesses the owned_unique_nodes directly.

This method is complete with autocompletion:

Also updates description of find_child() to mention get_unique_node().

As a comparison, this method is much slower when the appropriate parameters are passed (NotePath VS. StringName), but it is faster when normal String parameters are passed to both, which is what most common users would write.

The reason for this massive discrepancy can be attributed to the computing time it takes to append "%":

const StringName key = StringName(UNIQUE_NODE_PREFIX + p_name);

This is a requirement because, FOR SOME REASON, the "%" is included in the key when the Node is set as an unique name. I find it very unnecessary, as there's no way anything without "%" can be inserted into the owned_unique_nodes HashMap anyhow, but okay. I am emphasising this, because such a simple thing was the burden of me for 8+ hours.

This can probably be backported or cherrypicked to 3.x with no issues.

Spartan322 · 2022-08-07T01:00:12Z

scene/main/node.cpp

+		if (!unique) {
+			return nullptr;
+		}
+		return unique


This is lacking a semicolon and a dereference.

I apologise for my ignorance, I'll update this PR soon, but what is a dereference in this case?

return *unique; You need to dereference unique because its currently becoming a pointer to a pointer of a Node and is not a pointer to a Node itself, you need to get the pointer to the Node, thus you dereference the pointer by doing *unique, (converting it from Node** to Node*) which is what get_node_or_null does.

Thank you, very gentle and insightful!

Spartan322 · 2022-08-07T01:01:48Z

scene/main/node.cpp

+		if (!unique) {
+			return nullptr;
+		}
+		return unique


This is lacking a semicolon and a dereference.

AaronRecord · 2022-08-07T15:47:31Z

Currently a draft because I don't know if this even works. I am, at least currently, unable to compile a version of Godot to test this out. If someone were to test for themselves, I'd appreciate that very much.

It's less convenient but FYI you can get compiler errors and download builds from the CI checks

Mickeon · 2022-08-07T18:01:41Z

Thank you! I "fixed" it and actually attempted to compile Godot, but this results into scene\main\node.cpp(1374): error C2440: 'initializing': cannot convert from 'const TValue *' to 'Node **. (Where // Has unique nodes in ownership is present).
Removing that part of the code makes it compile just fine, but now I'm just confused.

Spartan322 · 2022-08-07T19:00:44Z

scene/main/node.cpp

+
+	if (data.owned_unique_nodes.size()) {
+		// Has unique nodes in ownership
+		Node **unique = data.owned_unique_nodes.getptr(p_name);


data.owned_unique_nodes.getptr(p_name) here actually references the const function making it return a Node *const * instead of Node **, this needs to be fixed with Node *const *unique = data.owned_unique_nodes.getptr(p_name); instead. (I forgot to check this specifically, sorry bout that)

It's ok, how gentle! I figured just at the same time as this reply. It all compiles now.

Mickeon · 2022-08-08T10:48:21Z

Will implement autocompletion as well, soon.
I was trying to make it a static function for cleaner code, but it looks like the Node data and therefore owned_unique_nodes can't be accessed that way.

Mickeon · 2022-08-08T11:31:10Z

Explaining the concept in the docs is a bit untidy (scene /= owner). I may give a better shot at it another time. This is good for now.

Mickeon · 2022-08-08T12:28:39Z

Honestly afraid to push after rebase because it may unintentionally commit past, already committed PRs. Probably nothing to worry about...

Spartan322 · 2022-08-08T12:35:24Z

I feel like the unique node symbol being stored in the HashMap should be noted in an issue, personally I would think of it as a bug (even if its otherwise fairly insignificant except in this specific case) since there is no reason to store that, should leave it to functions to handle that manually.

Mickeon · 2022-08-08T12:40:46Z

I feel like the unique node symbol being stored in the HashMap should be noted in an issue

The code looks reasonably self-contained. I may just open another PR at a later point in time. It can't quite be reported as an issue and tested, because to the external eye, and the editor, it doesn't matter (owned_unique_nodes is not exposed)

It's possible this was done to speed up access using get_node("%Node"), as there would be no need to strip the prefix before using the name as a key.

Spartan322 · 2022-08-08T12:43:27Z

It can't quite be reported as an issue and tested, because to the external eye, and the editor, it doesn't matter (owned_unique_nodes is not exposed)

I guess, if it was immediately solved in a PR then bothering to put down the issue would be worthless, so long as an addressing PR isn't delayed, cause otherwise this problem would most certainly be forgotten.

It's possible this was done to speed up access using get_node("%Node"), as there would be no need to strip the prefix before using the name as a key.

Yeah, but since this PR overrides that specific case by being specific, maybe it should be addressed in this PR, I don't know.

kleonc · 2022-08-08T12:46:12Z

Honestly afraid to push after rebase because it may unintentionally commit past, already committed PRs. Probably nothing to worry about...

@Mickeon Your branch is currently 55 commits behind the godot/master, it probably has the state of the godot/master at the time when you've created your branch (+it has your commits). Now you've pulled the changes from the current state of the godot/master (and transformed your commits into one). Pushing will make it be up to date with the current godot/master and will have your one additional commit. Looks good to force push to me.

Mickeon · 2022-08-08T13:07:46Z

Thank you all for the support! This was my first (actual) code contribution. It seems all good now.

Mickeon · 2022-08-09T09:40:02Z

Updated to use <param> and [param] instead of <argument> and [code] in Documentation

Also updates description of find_child() and find_parent() to mention get_unique_node()

reduz · 2022-08-14T08:32:33Z

scene/main/node.cpp

+		ERR_FAIL_V_MSG(nullptr, "'name' parameter cannot be empty");
+	}
+
+	const StringName key = StringName(UNIQUE_NODE_PREFIX + p_name);


The problem with this approach is that this bit is actually quite inefficient compared to just requesting "%Node", so if we added this function and some users prefer it, it would be slower.

I am not sure how it can be done better, as it is not intended to be obtained like this.

This is true, yeah. In fact, please see the benchmark in #64274

adamscott · 2023-08-28T13:27:22Z

The GDScript team talked about this proposal during a meeting (that is somewhat related to the following godotengine/godot-proposals#5064).

We think that it would be a good idea to implement this kind of function for Node, especially if it gave some performance improvements.

Spartan322 reviewed Aug 7, 2022

View reviewed changes

kleonc added enhancement feature proposal topic:core labels Aug 7, 2022

Calinou added this to the 4.0 milestone Aug 7, 2022

Spartan322 reviewed Aug 7, 2022

View reviewed changes

Mickeon marked this pull request as ready for review August 7, 2022 19:33

Mickeon requested review from a team as code owners August 7, 2022 19:33

This comment was marked as outdated.

Sign in to view

Mickeon marked this pull request as draft August 7, 2022 21:03

Mickeon marked this pull request as ready for review August 8, 2022 08:49

Mickeon force-pushed the get-unique-node branch from e0cbd26 to 7168d1a Compare August 8, 2022 12:13

Mickeon force-pushed the get-unique-node branch from 7168d1a to 8d29dab Compare August 8, 2022 13:06

Mickeon force-pushed the get-unique-node branch 2 times, most recently from a00b2f4 to 742dfd1 Compare August 9, 2022 09:39

Mickeon force-pushed the get-unique-node branch 2 times, most recently from 63ce1ec to 742dfd1 Compare August 9, 2022 12:32

Mickeon force-pushed the get-unique-node branch from 742dfd1 to a562618 Compare August 9, 2022 13:26

Add Node.get_unique_node()

8c466cb

Also updates description of find_child() and find_parent() to mention get_unique_node()

Mickeon force-pushed the get-unique-node branch from a562618 to 8c466cb Compare August 9, 2022 14:18

Mickeon mentioned this pull request Aug 11, 2022

Do not include prefix in owned_unique_nodes HashMap #64274

Open

reduz reviewed Aug 14, 2022

View reviewed changes

Mickeon mentioned this pull request Aug 31, 2022

Include both name and prefixed name in owned_unique_nodes HashMap #65143

Open

akien-mga modified the milestones: 4.0, 4.x Sep 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Node.get_unique_node()` #64023

Add `Node.get_unique_node()` #64023

Mickeon commented Aug 7, 2022 •

edited

Spartan322 Aug 7, 2022 •

edited

Mickeon Aug 7, 2022

Spartan322 Aug 7, 2022 •

edited

Mickeon Aug 7, 2022

Spartan322 Aug 7, 2022 •

edited

AaronRecord commented Aug 7, 2022

Mickeon commented Aug 7, 2022

Spartan322 Aug 7, 2022 •

edited

Mickeon Aug 7, 2022

This comment was marked as outdated.

Mickeon commented Aug 8, 2022

Mickeon commented Aug 8, 2022

Mickeon commented Aug 8, 2022

Spartan322 commented Aug 8, 2022 •

edited

Mickeon commented Aug 8, 2022 •

edited

Spartan322 commented Aug 8, 2022 •

edited

kleonc commented Aug 8, 2022

Mickeon commented Aug 8, 2022 •

edited

Mickeon commented Aug 9, 2022

reduz Aug 14, 2022

Mickeon Aug 14, 2022 •

edited

adamscott commented Aug 28, 2023

Add Node.get_unique_node() #64023

Are you sure you want to change the base?

Add Node.get_unique_node() #64023

Conversation

Mickeon commented Aug 7, 2022 • edited

Spartan322 Aug 7, 2022 • edited

Choose a reason for hiding this comment

Mickeon Aug 7, 2022

Choose a reason for hiding this comment

Spartan322 Aug 7, 2022 • edited

Choose a reason for hiding this comment

Mickeon Aug 7, 2022

Choose a reason for hiding this comment

Spartan322 Aug 7, 2022 • edited

Choose a reason for hiding this comment

AaronRecord commented Aug 7, 2022

Mickeon commented Aug 7, 2022

Spartan322 Aug 7, 2022 • edited

Choose a reason for hiding this comment

Mickeon Aug 7, 2022

Choose a reason for hiding this comment

This comment was marked as outdated.

Mickeon commented Aug 8, 2022

Mickeon commented Aug 8, 2022

Mickeon commented Aug 8, 2022

Spartan322 commented Aug 8, 2022 • edited

Mickeon commented Aug 8, 2022 • edited

Spartan322 commented Aug 8, 2022 • edited

kleonc commented Aug 8, 2022

Mickeon commented Aug 8, 2022 • edited

Mickeon commented Aug 9, 2022

reduz Aug 14, 2022

Choose a reason for hiding this comment

Mickeon Aug 14, 2022 • edited

Choose a reason for hiding this comment

adamscott commented Aug 28, 2023

Add `Node.get_unique_node()` #64023

Add `Node.get_unique_node()` #64023

Mickeon commented Aug 7, 2022 •

edited

Spartan322 Aug 7, 2022 •

edited

Spartan322 Aug 7, 2022 •

edited

Spartan322 Aug 7, 2022 •

edited

Spartan322 Aug 7, 2022 •

edited

Spartan322 commented Aug 8, 2022 •

edited

Mickeon commented Aug 8, 2022 •

edited

Spartan322 commented Aug 8, 2022 •

edited

Mickeon commented Aug 8, 2022 •

edited

Mickeon Aug 14, 2022 •

edited