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 get_descendants, get_descendant_count, get_ancestors and get_ancestor_count to Node #76674

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

Wolfyxon
Copy link

@Wolfyxon Wolfyxon commented May 2, 2023

Node.get_descendants returns the whole descendant tree as a single array using recursion.
Node.get_descendant_count returns the amount of descendants.

Example:

scene (script assigned here)
|_node
    |_sprite
        |_shadow
    |_lights
        |_light
        |_light
|_other node
$node.get_descendants()

Result: [sprite,shadow,lights,light,light]

@Wolfyxon Wolfyxon requested review from a team as code owners May 2, 2023 14:08
@RandomShaper
Copy link
Member

Maybe this is the sort of thing that warrants a proposal so we can better assess use cases, possible workarounds, etc.

@Wolfyxon
Copy link
Author

Wolfyxon commented May 2, 2023

Let's say you have a bunch of timers in a scene, nested in many nodes.
One timer is not running but you don't know which so you want to quickly debug them all at once.

for i in get_descendants():
   if i is Timer:
      if i.is_stopped():
          prints(i.get_path(),"is not running!")

@Calinou Calinou added this to the 4.x milestone May 2, 2023
@AThousandShips
Copy link
Member

AThousandShips commented May 2, 2023

While that's an explanation it's not really what was asked for, a proposal is important at is creates a space for discussing the things that aren't directly connected to the code or implementation, having this space of the PR be full of people discussing the "if" or general "how" about this rather than the specific "how" is unhelful

@AThousandShips
Copy link
Member

AThousandShips commented May 2, 2023

The tabulation is still wrong in several of the files, you should use tabs not spaces, recommend using clang-format to get it correct directly and setting up settings in your editor to fit the required style

@Wolfyxon
Copy link
Author

Wolfyxon commented May 2, 2023

Is it correct now?

@AThousandShips
Copy link
Member

AThousandShips commented May 2, 2023

Still in the node.h file, if you look under "Files Changed" here in the PR it shows clearly

@Wolfyxon
Copy link
Author

Wolfyxon commented May 2, 2023

hold on, forgot one line

@Wolfyxon
Copy link
Author

Wolfyxon commented May 2, 2023

Ok, it should be correct now

@Wolfyxon Wolfyxon changed the title Add get_descendants and get_descendant_count in Node Add get_descendants and get_descendant_count to Node May 2, 2023
@AThousandShips
Copy link
Member

AThousandShips commented May 2, 2023

In the future I'd suggest using clang-format and setting up your editor to fit the style requirements, to prevent pushing multiple times and taxing the system. I've made it a habit to always run clang-format on C++ files I've changed even if I'm sure I'm not making any mistakes just to be sure.

@Wolfyxon
Copy link
Author

Wolfyxon commented May 2, 2023

I accidentally removed the return statement....
I'll carefully review the code for more errors.

}
return count;
}
TypedArray<Node> Node::get_descendants(bool p_include_internal) const {
Copy link
Member

Choose a reason for hiding this comment

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

For style there should be a line between these two functions, to make it easier to navigate and read

@AThousandShips
Copy link
Member

You're still not using correct indentation

@Wolfyxon
Copy link
Author

Wolfyxon commented May 2, 2023

Where exactly? I used clang-format and there were no changes.
I checked every line I added and all the indentations are tabs.

@AThousandShips
Copy link
Member

That is very strange, the return line isn't indented with tabs

@Wolfyxon
Copy link
Author

Wolfyxon commented May 2, 2023

For some reason the ClangFormat setting got disabled in my IDE.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

While I do not have any write access I'll add my thoughts since you requested and taking the opportunity to do a review for the first time

@@ -1289,6 +1289,31 @@ Node *Node::_get_child_by_name(const StringName &p_name) const {
}
}

int Node::get_descendant_count(bool p_include_internal) const {
int count = 0;
TypedArray<Node> children = get_children(p_include_internal);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure using the "get_children" function is the best way to achieve this, none of the other functions use this and instead access the children directly, see the function itself for that, not sure what the performance difference would be but that would be my suggestion

@kleonc
Copy link
Member

kleonc commented May 2, 2023

Let's say you have a bunch of timers in a scene, nested in many nodes. One timer is not running but you don't know which so you want to quickly debug them all at once.

for i in get_descendants():
   if i is Timer:
      if i.is_stopped():
          prints(i.get_path(),"is not running!")

@Wolfyxon Already possible using Node.find_children method:

for timer in find_children("", "Timer"):
	if timer.is_stopped():
		prints(timer.get_path(), "is not running!")

Note Node.find_children is a little buggy currently (see #75459) but it should work for this use case just fine.


So, as already mentioned, please open a proposal for this (in https://github.com/godotengine/godot-proposals (check its README)). It's not obvious such enhacement is needed in the first place, what for, etc. Proposals are for discussing such things.

@MewPurPur
Copy link
Contributor

MewPurPur commented May 3, 2023

I'd also point out that there exists propagate_call() which covers many of this proposal's use cases.

@Wolfyxon
Copy link
Author

Wolfyxon commented May 3, 2023

So, as already mentioned, please open a proposal for this (in https://github.com/godotengine/godot-proposals (check its README)). It's not obvious such enhacement is needed in the first place, what for, etc. Proposals are for discussing such things.

Well... the proposal would most likely get rejected since this can easily be made in GDScript with "few lines of code"

static func get_descendants(node:Node,includeInternal:=false) -> Array:
	var res = []
	for child in node.get_children(includeInternal):
		if node.get_child_count(includeInternal) > 0:
			res.append_array( getDescendants(child,includeInternal) )
		res.append(child)
		
	return res

@AThousandShips
Copy link
Member

AThousandShips commented May 3, 2023

If you're assuming it will be rejected then this PR would be as well, and ther PR should just be closed, so why not just make a proposal and see?

The usefulness and necessity of this isn't really established, so it's probably not going to be merged without a proposal clarifying this, we won't just add new features or functions without knowing that they actually are needed, it's an important part of the workflow and it's important that you comply with that as a potential contributor

@AThousandShips
Copy link
Member

Any update on how you want to proceed here?

@Wolfyxon
Copy link
Author

I will open a proposal but first I want to also make get_ancestors() and get_ancestor_count()

@Wolfyxon Wolfyxon changed the title Add get_descendants and get_descendant_count to Node Add get_descendants, get_descendant_count, get_ancestors and get_ancestor_count to Node May 11, 2023
@AThousandShips
Copy link
Member

The documentation needs to be in alphabetical order, use --doctool on the compiled version of your code to do this automatically

@AThousandShips
Copy link
Member

AThousandShips commented May 11, 2023

I think get_ancestors warrants a separate PR and proposal, and I see even less likelyhood of support for it, it's trivial to accomplish, or consider the option of having to remove it from this if it's not accepted

@AThousandShips
Copy link
Member

Here to poke about that proposal again, have you lost interest in this PR?

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.

6 participants