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

[Doc] Fix some incorrect uses of "children" #88920

Merged
merged 1 commit into from
Feb 29, 2024

Conversation

AThousandShips
Copy link
Member

The plural form is not normally used when used attributively, as in "dog catchers", not "dogs catchers"

I didn't search for any other words in the docs but I suspect this mainly happens with "children"

@AThousandShips AThousandShips added bug documentation cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Feb 27, 2024
@AThousandShips AThousandShips added this to the 4.3 milestone Feb 27, 2024
@AThousandShips AThousandShips requested review from a team as code owners February 27, 2024 16:25
@@ -1220,13 +1220,13 @@
Never process. Completely disables processing, ignoring [member SceneTree.paused]. This is the inverse of [constant PROCESS_MODE_ALWAYS].
</constant>
<constant name="PROCESS_THREAD_GROUP_INHERIT" value="0" enum="ProcessThreadGroup">
If the [member process_thread_group] property is sent to this, the node will belong to any parent (or grandparent) node that has a thread group mode that is not inherit. See [member process_thread_group] for more information.
Process this node based on the thread group mode of the first parent (or grandparent) node that has a thread group mode that is not inherit. See [member process_thread_group] for more information.
Copy link
Member Author

Choose a reason for hiding this comment

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

Forgot that I set out to fix this and came across the "children" part, can split it off if desired

Copy link
Contributor

Choose a reason for hiding this comment

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

I was going to completely change these thread-related descriptions completely at some point but this is better than before.

@@ -1220,13 +1220,13 @@
Never process. Completely disables processing, ignoring [member SceneTree.paused]. This is the inverse of [constant PROCESS_MODE_ALWAYS].
</constant>
<constant name="PROCESS_THREAD_GROUP_INHERIT" value="0" enum="ProcessThreadGroup">
If the [member process_thread_group] property is sent to this, the node will belong to any parent (or grandparent) node that has a thread group mode that is not inherit. See [member process_thread_group] for more information.
Process this node based on the thread group mode of the first parent (or grandparent) node that has a thread group mode that is not inherit. See [member process_thread_group] for more information.
Copy link
Contributor

Choose a reason for hiding this comment

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

I was going to completely change these thread-related descriptions completely at some point but this is better than before.

doc/classes/Node.xml Outdated Show resolved Hide resolved
doc/classes/PackedScene.xml Outdated Show resolved Hide resolved
@@ -977,7 +977,7 @@
Set the process thread group for this node (basically, whether it receives [constant NOTIFICATION_PROCESS], [constant NOTIFICATION_PHYSICS_PROCESS], [method _process] or [method _physics_process] (and the internal versions) on the main thread or in a sub-thread.
By default, the thread group is [constant PROCESS_THREAD_GROUP_INHERIT], which means that this node belongs to the same thread group as the parent node. The thread groups means that nodes in a specific thread group will process together, separate to other thread groups (depending on [member process_thread_group_order]). If the value is set is [constant PROCESS_THREAD_GROUP_SUB_THREAD], this thread group will occur on a sub thread (not the main thread), otherwise if set to [constant PROCESS_THREAD_GROUP_MAIN_THREAD] it will process on the main thread. If there is not a parent or grandparent node set to something other than inherit, the node will belong to the [i]default thread group[/i]. This default group will process on the main thread and its group order is 0.
During processing in a sub-thread, accessing most functions in nodes outside the thread group is forbidden (and it will result in an error in debug mode). Use [method Object.call_deferred], [method call_thread_safe], [method call_deferred_thread_group] and the likes in order to communicate from the thread groups to the main thread (or to other thread groups).
To better understand process thread groups, the idea is that any node set to any other value than [constant PROCESS_THREAD_GROUP_INHERIT] will include any children (and grandchildren) nodes set to inherit into its process thread group. this means that the processing of all the nodes in the group will happen together, at the same time as the node including them.
To better understand process thread groups, the idea is that any node set to any other value than [constant PROCESS_THREAD_GROUP_INHERIT] will include any child (and grandchild) nodes set to inherit into its process thread group. this means that the processing of all the nodes in the group will happen together, at the same time as the node including them.
Copy link
Contributor

Choose a reason for hiding this comment

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

This entire wall of text makes me viscerally angry.

Although I'm fine with saying "grandchild", one thing I would've liked to do is be consistent and call them "ancestors", or imply that recursion happens (which I may prefer in this case?).

I've actually noticed some confusing inconsistency behind "ancestor" and "descendant" which may be worth fixing another time...

Copy link
Member Author

@AThousandShips AThousandShips Feb 27, 2024

Choose a reason for hiding this comment

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

Do you mean calling them descendants? Descendant is the opposite of ancestor. Or do you mean that we aren't using "descendant" or "ancestor" instead of "child/grandchild" or "parent/grandparent"?

Also realized a punctuation error here will fix

Copy link
Contributor

@Mickeon Mickeon Feb 27, 2024

Choose a reason for hiding this comment

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

do you mean that we aren't using "descendant" or "ancestor" instead of "child/grandchild" or "parent/grandparent

I did mean this.

Ok, so.
There's the is_ancestor_of method, which implies that the word of a non-direct parent node is "ancestor", which means that "descendants" is the opposite.
And I would love to have this word consistent across the class reference, as it makes the whole concept easier to understand.
"child/grandchild" or "parent/grandparent" sounds good, great even, but it's about being consistent, and sadly the method isn't named is_grandparent_of.

Copy link
Member Author

@AThousandShips AThousandShips Feb 28, 2024

Choose a reason for hiding this comment

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

Sounds like a great future improvement, I'd prefer this PR to not be bogged down by discussion which I feel it would if I changed that here, as AFAIR there was pushback at that because it was considered less clear, as "get_parent" and "get_child" is well established, and "grandparent" or "grandchild" can easily be extrapolated from that, but "ancestor" and "descendant" might not be as obvious, despite having the relevant methods (which also have a description that are elaborating I think, explaining what that part means)

It'd also involve, I'd say, cleaning up the use of "child(ren)" to mean not just direct children, as we somewhat commonly (or even regularly) use "child" to mean "descendant", which isn't unusual AFAIK in data structures

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we need to discuss this another time. It's all over the place.

It'd also involve, I'd say, cleaning up the use of "child(ren)" to mean not just direct children, as we somewhat commonly (or even regularly) use "child" to mean "descendant", which isn't unusual AFAIK in data structures

Once again same issue for consistency because get_children only returns immediate children, but making a distinction is way more important in Godot than your average data structure since some behavior only affects direct children (collision shapes, Controls as a whole, etc..)

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, but find_children is not immediate only, so we generally don't treat this as immediate only

@AThousandShips AThousandShips force-pushed the group_doc_fix branch 2 times, most recently from 938671e to ff631f7 Compare February 27, 2024 19:43
doc/classes/Node.xml Outdated Show resolved Hide resolved
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Fine (Even the accidental PROCESS_THREAD_GROUP_INHERIT change)

@fire
Copy link
Member

fire commented Feb 29, 2024

There's at least 4 ways we can use these related words:

  1. Parent and Ancestors
  2. Children, and descendants. Each node may have immediate children or find children which is all the descendants
  3. A grandchild is a descendant of a node, but not including the immediate children
  4. A child is an immediate descendant of a node, and get_children() gets a list of immediate children.

doc/classes/Viewport.xml Outdated Show resolved Hide resolved
@akien-mga akien-mga merged commit c9b531c into godotengine:master Feb 29, 2024
16 checks passed
@AThousandShips AThousandShips deleted the group_doc_fix branch February 29, 2024 12:58
@akien-mga
Copy link
Member

Thanks!

@AThousandShips
Copy link
Member Author

Thank you!

@akien-mga
Copy link
Member

Cherry-picked for 4.2.2.
Cherry-picked for 4.1.4.

@akien-mga akien-mga removed cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Mar 11, 2024
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.

4 participants