-
-
Notifications
You must be signed in to change notification settings - Fork 22k
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 Node2D documentation #89256
base: master
Are you sure you want to change the base?
Overhaul Node2D documentation #89256
Conversation
doc/classes/Node2D.xml
Outdated
@@ -1,7 +1,7 @@ | |||
<?xml version="1.0" encoding="UTF-8" ?> | |||
<class name="Node2D" inherits="CanvasItem" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="../class.xsd"> | |||
<brief_description> | |||
A 2D game object, inherited by all 2D-related nodes. Has a position, rotation, scale, and Z index. | |||
A 2D game object, inherited by most 2D-related nodes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exceptions are there? It also contradicts below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CanvasItem is actually what the original description claims to be.
It also contradicts below
Yeah I didn't change anything there yet...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, 2D nodes are a distinct things, Controls are different, 2D nodes doesn't just mean "nodes drawn in 2D" I'd say, it's related to them being transformable etc. I'd say
Also if you include all canvas items then fewer than half of all those nodes are 2D nodes, there's more Controls than 2D nodes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CanvasItem's description states.
Abstract base class for everything in 2D space.
Would you say that is the misleading description, then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's different I'd say, that's true too
Also the manual talks about 2D as a distinct thing, so if we change this terminology that needs changing too
e0f4ee7
to
2671a63
Compare
e1befa0
to
84633b3
Compare
08e97c3
to
cab7c73
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR has actually been fine for a while. Mainly a few pain points I can't get through.
doc/classes/Node2D.xml
Outdated
<description> | ||
A 2D game object, with a transform (position, rotation, and scale). All 2D nodes, including physics objects and sprites, inherit from Node2D. Use Node2D as a parent node to move, scale and rotate children in a 2D project. Also gives control of the node's render order. | ||
The [Node2D] node is the base representation of a node in 2D space. All other 2D nodes inherit from this class. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for consistency with #87440 .
However, in hindsight, I can't help but notice loss of information. I personally think it's fine, given the very generic and oddly written nature of them ("also... gives... control of...").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, imo the previous version is better here, this change is in danger of removing all the "documentation" from the documentation...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I elaborate more on types that inherit Node2D. The rest is... true, but it actually applies to CanvasItem as a whole, and it feels wrong to mention it here, especially because of the [b]Note:[/b] Since both [Node2D] and [Control] inherit from [CanvasItem],
below.
<description> | ||
Transforms the provided local position into a position in global coordinate space. The input is expected to be local relative to the [Node2D] it is called on. e.g. Applying this method to the positions of child nodes will correctly transform their positions into the global coordinate space, but applying it to a node's own position will give an incorrect result, as it will incorporate the node's own transformation into its global position. | ||
Returns the [param local_point] converted from this node's local space to global space. This is the opposite of [method to_local]. | ||
</description> | ||
</method> | ||
<method name="to_local" qualifiers="const"> | ||
<return type="Vector2" /> | ||
<param index="0" name="global_point" type="Vector2" /> | ||
<description> | ||
Transforms the provided global position into a position in local coordinate space. The output will be local relative to the [Node2D] it is called on. e.g. It is appropriate for determining the positions of child nodes, but it is not appropriate for determining its own position relative to its parent. | ||
Returns the [param global_point] converted from global space to this node's local space. This is the opposite of [method to_global]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for consistency with #87440
However, it's a pretty nasty loss of information here. The corresponding 3D methods are not as verbose as this. At the moment I don't know what to make of it.
cab7c73
to
c40d135
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally some much needed changes, LGTM.
c40d135
to
1bb843a
Compare
1bb843a
to
8c51259
Compare
Meant to address godotengine/godot-docs#4681
This PR is a placeholder and a counterpart to #87440, until that PR is merged, so that I can use it as a point of reference for consistency.
Related to several other past efforts, but especially the #87175 and #87334 ones.
This PR aims to completely overhaul Node2D's class reference in an attempt to make it more easy to digest. Reasons are the same as prior PRs so I'm not sure I should explain myself too much here.