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

Overhaul Node documentation #68560

Merged
merged 1 commit into from
Jan 3, 2024

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Nov 12, 2022

Here we go again

Aims to close godotengine/godot-docs#2975.
Closes #72486.
Closes #76593.
Closes #82836.

Continuation of #67072, #67100, #67208, #67718, especially #67880...

Note that this PR barely touches the leading description and the virtual method's descriptions. This is intentional. These are so long, verbose and tricky, that they are worth addressing them in another PR, following this up.

Here a big list of changes. Someone reading this may take it as a future point of reference. Not just that, but some information deemed important by the maintainers could have been lost during the rewrite. Criticism is encouraged.

General

  • Made the wording match how other classes are documented a lot more closely.
  • Made use of [param] and several other strong references more often.
  • Corrected a few typos.
  • Stripped awkward, needlessly long sentences.
  • Changed words where it felt necessary, where they could be mistaken for another concept of Godot's API, where technical terms are more appropriate. For example:
    • "Contrarily to" -> "Unlike";
    • "instancing" -> "instantiating";
    • "stdout" -> "console";
    • "tree"/"branch" -> "node and its children, recursively" (sometimes);
  • Added and updated a few more examples.
  • Used [b]Warning:[/b] over [b]Note:[/b] where appropriate.
  • Changed include_internal parameter's note to "(see add_child's internal parameter)".
    • This flows better. In context, bringing internal to the end reduces repetition.

Methods

add_group

is_in_group

remove_from_group

get_groups

  • TODO: add short group explanation, instead of pointing to the leading description?
  • The persistent parameter description didn't match how other boolean parameters are written.
  • Updated get_groups's example.

can_process

Reworked description. It was incorrect and a bit misleading.

duplicate

Reworked description entirely.

find_child

find_children

find_parent

Marginally reduced number of paragraphs. The methods are complicated, but not that complicated, guys.

get_child

Added example.

get_node

  • "Fetching absolute paths only works when [...]"
    I thought we were fetching nodes here?

  • Changed the example to resemble print_tree_pretty's output.

get_node_or_null

  • "[...] but does not log an error [...]"
    Log? What's a log?

get_node_and_resource

get_path

get_path_to

  • "Both nodes must be in the same scene"
    Ehhh... No. Not the "scene", the SceneTree. Let's not muddy the water.

  • Specify empty NodePath return value.

get_physics_process_delta_time

get_process_delta_time

  • Mentioned that is the same "delta" as _physics_process and _process, respectively.
  • Directly linked to _physics_process and _process and notification constants.

get_scene_instance_load_placeholder

  • "[...] if this is an instance load placeholder [...]"
    Who's "this"?

get_tree

Specify generated error.

get_viewport

Expanded the description. It has been too vague for a while.

get_viewport

Expanded the description. It has been too vague for a while.

is_displayed_folded

set_display_folded

is_editable_instance

set_editable_instance

print_tree

print_tree_pretty

  • "Prints the tree to stdout"
    The heck is a "stdout"? Oh, and the tree?

  • Mentioned that the node does not need to be inside the tree for the method to work (misleading, right?).
  • Updated inaccurate print_tree example output.

remove_child

It's not "may set the `owner`". It's consistent behavior.

remove_from_group

replace_by

  • "Replaces a node"
    Which one?

  • "Subscriptions that pass through this node will be lost."
    What?? First, what does "subscription" mean? Second, if it refers to signals, this statement is misleading anyway.

request_ready

The entire previous description made it seem more complicated than it actually is.

rpc

Removed inaccurate return value, explain the Error code return values.

rpc_config

Explained required config entries individually.

rpc_id

Mentioned Error code return values.

queue_free

  • "Queues a node"
    Which one?

set_process_input

set_process_internal

  • Removed "Any calls to this before _ready() will be ignored.". This was not just wrong, but also too much information.
  • Reduced verbosity of internal processing setters.

set_scene_instance_load_placeholder

Specify! Specify! Specify!

update_configuration_warnings

Removed unnecessary new paragraph.

PROPERTIES

editor_description

The previous description made it look like a method.

name

Mentioned not-allowed characters.

owner

Marginally reduced description bloat.
This entire owner "mechanic" should be (as it probably is) explained more into detail in PackedScene.

process_mode

Reworked entirely to point to can_process.

process_priority

Reduced verbosity.

scene_file_path

Reworked.

unique_name_in_owner

  • "from any node within that scene"
    I'm aware that this property is mostly useful in the editor, but saying that is outstandingly misleading. The feature works so long as they share owner.

  • The previous description made it look like a method.

SIGNALS

child_entered_tree

child_exited_tree

  • "either because it entered on its own or because this node entered with it."
    Who's "it", this node? That node? This node... it? These descriptions were a muddy mouthful.

renamed

Did you know that this signal is not emitted when outside the tree? Now you do.

CONSTANTS

NOTIFICATION_PROCESS

NOTIFICATION_PHYSICS_PROCESS

NOTIFICATION_INTERNAL_PHYSICS_PROCESS

NOTIFICATION_INTERNAL_PROCESS

  • "Notification received every frame when the physics..."
    Which "frame" are we talking about here?

  • Specify "from the tree". This way, it is implicit that if the node is not inside the tree, these notifications are not received.

NOTIFICATION_POST_ENTER_TREE

  • "Notification received when the node is ready"
    No. It is received right before _ready, but the method may or may not be called.

DUPLICATE_SIGNALS

  • "Duplicate the node's signals."
    Not the signals. The incoming signal connections to this node. In fact, shockingly, signals created with add_user_signal() aren't even copied.

DUPLICATE_USE_INSTANCING

  • "An instance stays linked to the original so when the original changes, the instance changes too."
    The original what? Anyway, not correct at all. Completely wrong. It's just through PackedScene.instantiate().


I recommend taking a look at the documentation by building from this PR, to read the documentation from the Editor itself. Feedback is very, very welcome.

@Mickeon Mickeon force-pushed the doc-peeves-nodeworthy branch from d3816d5 to c49af3c Compare November 12, 2022 14:02
@Geometror Geometror added this to the 4.0 milestone Nov 12, 2022
@Mickeon Mickeon force-pushed the doc-peeves-nodeworthy branch 3 times, most recently from 3382c5b to a0f2513 Compare November 13, 2022 01:24
@Mickeon Mickeon marked this pull request as ready for review November 13, 2022 01:25
@Mickeon Mickeon requested a review from a team as a code owner November 13, 2022 01:25
@Mickeon
Copy link
Contributor Author

Mickeon commented Nov 13, 2022

Ready for review now! The remaining "TODO"'s aren't particularly pressing.

@Mickeon Mickeon force-pushed the doc-peeves-nodeworthy branch 3 times, most recently from a23646c to ddcea02 Compare November 13, 2022 17:59
@Mickeon Mickeon requested a review from timothyqiu November 13, 2022 17:59
@Mickeon
Copy link
Contributor Author

Mickeon commented Jul 31, 2023

Rebased (once more).

Copy link
Member

@mhilbrunner mhilbrunner left a comment

Choose a reason for hiding this comment

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

Hey, this one is on its 1-year anniversary!

I think it can be nitpicked to death endlessly just due to the large surface of the changes (and how much more could always be changed) and how core and important Node is.

I've read over it twice now, and overall it seems a great improvement over the current state. After fixing the single typo I found above and rebasing once more, I say we merge it as is, and do any further improvements iteratively.

Great work @Mickeon :) If I may, I suggest splitting changes up in the future where they touch core parts and become this large, so its easier to review and merge bite-size. Kinda hard to do for these kinda overhauls that touch everything for a doc pages, I know.

@Mickeon Mickeon force-pushed the doc-peeves-nodeworthy branch 2 times, most recently from 1c730a7 to 0e49f9e Compare November 12, 2023 11:05
@Mickeon Mickeon force-pushed the doc-peeves-nodeworthy branch from 0e49f9e to b5ca06c Compare November 12, 2023 11:10
@Mickeon
Copy link
Contributor Author

Mickeon commented Nov 12, 2023

Rebased for hopefully one last time, omg. To possibly make this easier to look at, the conflicting descriptions before the rebase are the following:

  • _ready;
  • _request_ready
    • There's now an addendum specifying what happens to the children, so it kinda gets overridden by this PR.
  • NOTIFICATION_POST_ENTER_TREE
  • get_tree;
    • There's now the same note about its error, which was in this PR already, so it kinda gets overridden.

@YuriSizov
Copy link
Contributor

Just FYI we will try to merge it as soon as we can in 4.3. It may require one more rebase if there are some conflicting changes done in the meantime, but hopefully not.

@akien-mga akien-mga merged commit 078ed36 into godotengine:master Jan 3, 2024
@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 3, 2024

Oh great heavens

@akien-mga
Copy link
Member

Thanks for your work, and your patience :)

@Mickeon Mickeon deleted the doc-peeves-nodeworthy branch January 3, 2024 17:45
@akien-mga akien-mga changed the title Overhaul Node Documentation Overhaul Node documentation Jan 11, 2024
@YuriSizov YuriSizov added cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release and removed cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Jan 25, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants