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 Node3D documentation #87440

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Jan 21, 2024

Closes #87896
Closes #93651

Related to several other past efforts, but especially the Basis and Transform3D ones.

This PR aims to completely overhaul Node3D's class reference in an attempt to make it more easy to digest, as well as bring on par with the rest of the documentation.

Why? It generally suffers from the same issues highlighted in the past. The most notable of which being that a lot of descriptions are dull, succinct, or put the focus on the wrong subject:

  • "Set subgizmo selection for this node in the editor."
  • "Updates all the [Node3D] gizmos attached to this node."
  • "Transforms [param global_point] from world space to this node's local space."
  • "Rotates the local transformation around the X axis by angle in radians."

More specific notes:

  • Remove any leftover references to "Spatial" nodes from the class reference.
  • Modified to_local and to_global's descriptions to imply they do return something.
  • Elaborate on when visibility_changed is emitted.
    • The sentence is almost word-for-word from the way some NOTIFICATION constants are written in the Node class.
  • Revamp the descriptions of NOTIFICATION_TRANSFORM_CHANGED & similar.
    • "In order for [constant NOTIFICATION_TRANSFORM_CHANGED] to work, users first need to ask for it, with [method set_notify_local_transform]."
      a grand majority of Node3Ds will receive NOTIFICATION_TRANSFORM_CHANGED without needing for the "user" to opt-in. They need this otherwise they would not work.

This PR may contain commented XML. I use it as a point of reference or to symbolize alternatives of the same sentence. Even if this is a draft, feedback on things worth mentioning is still very welcome.

@Mickeon Mickeon force-pushed the doc-peeves-Node3D-glasses branch 3 times, most recently from 741226a to a8315c8 Compare January 21, 2024 13:55
Copy link
Contributor

@RedMser RedMser left a comment

Choose a reason for hiding this comment

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

One bit of the Node3D methods (and corresponding docs) always confused me as a user. I've added comments there, but am unsure about proper terminology.

</description>
</method>
<method name="rotate">
<return type="void" />
<param index="0" name="axis" type="Vector3" />
<param index="1" name="angle" type="float" />
<description>
Rotates the local transformation around axis, a unit [Vector3], by specified angle in radians.
Rotates this node's local [member transform] around the [param axis] by the given [param angle], in radians. The operation happens in local space.
Copy link
Contributor

Choose a reason for hiding this comment

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

From how I understand it, rotate and rotate_[xyz] specify the axis in global space (parent space? not sure about proper terminology), but both modify the local space transform. Whereas rotate_object_local also transforms the rotation axis into local space. Right now, both of these methods have the same description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have kept these in mind for further testing. These methods have also always confused me so it would be nice to finally write something easier to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to give closure, rotate is indeed in parent space (same as the transform property). If the position was anything but Vector3(0, 0, 0), the node would rotate and move around the parent.

Changes the node's position by the given offset [Vector3].
Note that the translation [param offset] is affected by the node's scale, so if scaled by e.g. [code](10, 1, 1)[/code], a translation by an offset of [code](2, 0, 0)[/code] would actually add 20 ([code]2 * 10[/code]) to the X coordinate.
<!-- Changes the node's position by the given offset [Vector3]. -->
Adds the given translation [param offset] to the node's [member position] in local space.
Copy link
Contributor

Choose a reason for hiding this comment

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

From how I understand it, translate specifies the offset in global space (parent space? not sure about proper terminology), but modifies the local space transform. Whereas translate_object_local also transforms the offset into local space. Right now, both of these methods have the same description.

@lostminds
Copy link

Node2D has a lot of the same properties as the 3D version, and often with very short descriptions in the docs. So if you add improved descriptions of the concepts in the Node3D versions perhaps some of them can be applied to Node2D as well? Like for example what the global_position and global_transform properties do and how they work.

@Mickeon
Copy link
Contributor Author

Mickeon commented Feb 4, 2024

That's the idea! One thing at the time. I decided to tackle Node3D first because it's fresh on the mind after both #87175 and #87334.

doc/classes/Node3D.xml Outdated Show resolved Hide resolved
doc/classes/Node3D.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-Node3D-glasses branch from d828b4c to 656493d Compare March 7, 2024 16:11
@Mickeon
Copy link
Contributor Author

Mickeon commented Mar 7, 2024

I have updated the PR with just... more. I am still not fully happy on what is here, as there's a constant fear of losing important information through all of this. So this is still gonna be WIP for a bit more time until I come back with a fresher mind and answers to some questions.

Comment on lines 54 to 61
<!-- Returns the parent [Node3D], or [code]null[/code] if no parent exists, the parent is not a [Node3D], or [member top_level] is [code]true[/code]. -->
Returns the parent [Node3D] that directly affects this node's [member global_transform]. Returns [code]null[/code] if no parent exists, the parent is not a [Node3D], or [member top_level] is [code]true[/code].
Copy link
Contributor Author

Choose a reason for hiding this comment

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

HEAVY indecision between these two. Above is entirely correct, but does not explain why this method exists.

Comment on lines 8 to 9
The [Node3D] node is the base representation of a node in 3D space. All other 3D nodes inherit from this class.
Affine operations (rotate, scale, translate) happen in parent's local coordinate system, unless the [Node3D]'s [member top_level] is [code]true[/code]. Affine operations in this coordinate system correspond to direct affine operations on the [Node3D]'s transform. The word local below refers to this coordinate system. The coordinate system that is attached to the [Node3D] object itself is referred to as object-local coordinate system.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Affine, direct Affine operations... 😖

</member>
<member name="visibility_parent" type="NodePath" setter="set_visibility_parent" getter="get_visibility_parent" default="NodePath(&quot;&quot;)">
Defines the visibility range parent for this node and its subtree. The visibility parent must be a GeometryInstance3D. Any visual instance will only be visible if the visibility parent (and all of its visibility ancestors) is hidden by being closer to the camera than its own [member GeometryInstance3D.visibility_range_begin]. Nodes hidden via the [member Node3D.visible] property are essentially removed from the visibility dependency tree, so dependent instances will not take the hidden node or its ancestors into account.
Path to the visibility range parent for this node and its descendants. The visibility parent must be a [GeometryInstance3D].
Any visual instance will only be visible if the visibility parent (and all of its visibility ancestors) is hidden by being closer to the camera than its own [member GeometryInstance3D.visibility_range_begin]. Nodes hidden via the [member Node3D.visible] property are essentially removed from the visibility dependency tree, so dependent instances will not take the hidden node or its descendants into account.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still find a bit difficult to understand what this is for, despite my best efforts.

@Mickeon
Copy link
Contributor Author

Mickeon commented Jun 30, 2024

Marking this as ready to be lynched by anyone more savvy than me, no beating around the bush.

Copy link
Member

@kleonc kleonc left a comment

Choose a reason for hiding this comment

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

Only skimmed through, added some brief notes. Please fix these "local space (relative to the parent)".

AFAIK "local" in Godot's properties/method names always refer to object-local, not to parent-local. Docs should be consistent with this.
If there are some exceptions that I'm not aware of, please point to them. 🙂

</description>
</method>
<method name="rotate">
<return type="void" />
<param index="0" name="axis" type="Vector3" />
<param index="1" name="angle" type="float" />
<description>
Rotates the local transformation around axis, a unit [Vector3], by specified angle in radians.
Rotates this node's local [member transform] around the [param axis] by the given [param angle], in radians. The operation happens in local space (relative to the parent).
Copy link
Member

Choose a reason for hiding this comment

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

This does rotate so-called local transform (which represents the transformation from the local to the parent coordinate system), around an axis specified in the parent coordinate system. So the operation does not happen in the local/object space, but in the parent space (as axis is defined in the parent space)!

Caution

"local space" != "parent space"/"relative to parent"

Please don't mix them.
If some existing docs refers to the parent space as "local", I would consider this as a bug. Are there such docs? 🤔

I hope you haven't got merged any PRs introducing similar mixing of local/parent spaces? 🙃


rotate_{x/y/z}(angle) also rotate around parent space axis (+X/+Y/+Z), they're equivalent to rotate(Vector3(1, 0, 0), angle)/rotate(Vector3(0, 1, 0), angle)/rotate(Vector3(0, 0, 1), angle).


BTW I often use local/global space etc. but I think it might be better if in the docs we'd refer to them explicitly as "local/global coordinate system", it should be clearer and more precise at the same time.
(What is space? Baby don't hurt me...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am taking this to heart.

I hope you haven't got merged any PRs introducing similar mixing of local/parent spaces? 🙃

I am not sure, as aaronfranke scrutinised #87175 which was accounted for when making #87334 , but a second look from you cannot hurt anything other than my self-esteem.

doc/classes/Node3D.xml Outdated Show resolved Hide resolved
doc/classes/Node3D.xml Outdated Show resolved Hide resolved
doc/classes/Node3D.xml Outdated Show resolved Hide resolved
doc/classes/Node3D.xml Outdated Show resolved Hide resolved
Returns the parent [Node3D], or [code]null[/code] if no parent exists, the parent is not of type [Node3D], or [member top_level] is [code]true[/code].
[b]Note:[/b] Calling this method is not equivalent to [code]get_parent() as Node3D[/code], which does not take [member top_level] into account.
Returns the parent [Node3D] that directly affects this node's [member global_transform]. Returns [code]null[/code] if no parent exists, the parent is not a [Node3D], or [member top_level] is [code]true[/code].
[b]Note:[/b] This method is not always equivalent to [method Node.get_parent], which does not take [member top_level] into account.
Copy link
Member

Choose a reason for hiding this comment

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

Was it intentional to get rid of the mention of as Node3D?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I believe directly linking to get_parent is more important than using [code] tags for an example that only works in GDScript.

[b]Note:[/b] Unless otherwise specified, all methods that have angle parameters must have angles specified as [i]radians[/i]. To convert degrees to radians, use [method @GlobalScope.deg_to_rad].
[b]Note:[/b] Be aware that "Spatial" nodes are now called "Node3D" starting with Godot 4. Any Godot 3.x references to "Spatial" nodes refer to "Node3D" in Godot 4.
The [Node3D] node is the base representation of a node in 3D space. All other 3D nodes inherit from this class.
Affine operations (rotate, scale, translate) happen in parent's local coordinate system, unless the [Node3D]'s [member top_level] is [code]true[/code]. Affine operations in this coordinate system correspond to direct affine operations on the [Node3D]'s [member transform]. The word local below refers to this coordinate system. The coordinate system that is attached to the [Node3D] object itself is referred to as object-local coordinate system.
Copy link
Member

Choose a reason for hiding this comment

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

We should avoid using the word "local" for multiple things. See @kleonc's comments.

In general, Godot's properties are parent-relative by default unless further qualified, where the word global qualifies it as fully global relative to the world space, and the word local qualifies it as relative to within the node's own transform. For the docs where we need to clarify this, let's use "parent-relative", not "local".

doc/classes/Node3D.xml Outdated Show resolved Hide resolved
@Mickeon
Copy link
Contributor Author

Mickeon commented Nov 22, 2024

Given your recent review I'll revisit this soonish. The "local" distinction is bit overwhelming to think about, but I must pursue for the sake of a nicer class reference.

@Mickeon Mickeon force-pushed the doc-peeves-Node3D-glasses branch from 650faa8 to 763701a Compare November 23, 2024 18:47
@Mickeon
Copy link
Contributor Author

Mickeon commented Nov 23, 2024

I have rebased. My general confusion stems from the original documentation's definition of "local" which has been irritatingly misleading.

BTW I often use local/global space etc. but I think it might be better if in the docs we'd refer to them explicitly as "local/global coordinate system", it should be clearer and more precise at the same time.

In general, Godot's properties are parent-relative by default unless further qualified, where the word global qualifies it as fully global relative to the world space, and the word local qualifies it as relative to within the node's own transform. For the docs where we need to clarify this, let's use "parent-relative", not "local".

Thank you @kleonc and @aaronfranke for writing detailed explanations on the distinction between these 3 "spaces"/"coordinate systems", but we have to discuss this. We need a consistent way to refer to these in order to reduce ambiguity as much as possible.
It doesn't help that in some other places, different terms are used to refer to the same concept 12. I myself understand now that both "object-local" and "parent-local" spaces are, in fact, "local", but a problem arises when both are summarised with that single word.

Right now, in this PR, I am using the term "parent space". This term is never mentioned anywhere in the class reference. Even though a lot of wording can still be improved, this has been done for sake of accuracy first.

Footnotes

  1. https://docs.blender.org/manual/en/latest/animation/constraints/interface/common.html#space-types

  2. https://help.autodesk.com/view/MAYAUL/2024/ENU/?guid=GUID-A63AC5C8-8822-42AC-827E-164B5266DA03

doc/classes/Node3D.xml Outdated Show resolved Hide resolved
doc/classes/Node3D.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-Node3D-glasses branch from 763701a to 54ba20e Compare November 24, 2024 15:33
[b]Note:[/b] Not all nodes are visually scaled by the [member scale] property. For example, [Light3D]s are not visually affected by [member scale].
Scale of this node in parent space (relative to the parent node). This value is obtained from [member basis]'s scale.
[b]Note:[/b] The behavior of some 3D node types is not affected by this property. These include [Light3D], [Camera3D], [AudioStreamPlayer3D], etc.
[b]Warning:[/b] The scale's components must either be all positive or all negative, and [b]not[/b] exactly [code]0.0[/code]. Otherwise, it won't be possible to obtain the scale from the [member basis]. This may cause the intended scale to be lost when reloaded from disk, and potentially other unstable behavior.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatives to the same note. This is the same as the original, but switched around:

Suggested change
[b]Warning:[/b] The scale's components must either be all positive or all negative, and [b]not[/b] exactly [code]0.0[/code]. Otherwise, it won't be possible to obtain the scale from the [member basis]. This may cause the intended scale to be lost when reloaded from disk, and potentially other unstable behavior.
[b]Warning:[/b] Due to the way scale is represented with transformation matrices in Godot, the scale's components must either be all positive, all negative, or all [code]0.0[/code]. Mixed negative scales in 3D cannot be decomposed from the [member transform].

This... I don't know

Suggested change
[b]Warning:[/b] The scale's components must either be all positive or all negative, and [b]not[/b] exactly [code]0.0[/code]. Otherwise, it won't be possible to obtain the scale from the [member basis]. This may cause the intended scale to be lost when reloaded from disk, and potentially other unstable behavior.
[b]Warning:[/b] The scale's components must either be all positive or all negative, and [b]not[/b] exactly [code]0.0[/code]. Otherwise, this may cause unstable behavior, as it won't be possible to obtain the intended scale from the [member basis].

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't the first alternative invert the meaning...? I thought that scale shouldn't be all 0.0, as per this note. You can check this trivially in the editor, you get Condition "det == 0" is true. errors.

Copy link
Contributor Author

@Mickeon Mickeon Nov 24, 2024

Choose a reason for hiding this comment

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

Indeed, it shouldn't. Nothing stops you but a scale of Vector3(0, 0, 0) is extremely unreliable and is what happens when even one component is 0.0, due to multiplication.

doc/classes/Node3D.xml Outdated Show resolved Hide resolved
</member>
<member name="transform" type="Transform3D" setter="set_transform" getter="get_transform" default="Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 0)">
Local space [Transform3D] of this node, with respect to the parent node.
The transformation of this node, in parent space (relative to the parent node). Contains and represents this node's [member position], [member rotation], and [member scale].
Copy link
Member

Choose a reason for hiding this comment

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

This drops the [Transform3D].

For the transform itself, I think describing it as local space is both accurate and helpful, because people new to Godot need to immediately know that this isn't global.

There are 2 sets of transforms (all parents, and this node's transform), which result in 3 spaces (global with neither, parent-relative with just all parents, and object-local with both). For other properties and methods, it makes sense to clarify parent-relative or object-local because we need to clarify "Is it local in the same way that the node's transform is local, or is it local within the node's transform, even more local than the local transform?" but for the transform itself, there is no confusion, local can only mean one thing.

Suggested change
The transformation of this node, in parent space (relative to the parent node). Contains and represents this node's [member position], [member rotation], and [member scale].
Local space [Transform3D] of this node, relative to the parent node. Contains and represents this node's [member position], [member rotation], and [member scale].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This drops the [Transform3D].

It's intentional. There's often no need to specify the type when it is already defined by the property header.

Scale part of the local transformation.
[b]Note:[/b] Mixed negative scales in 3D are not decomposable from the transformation matrix. Due to the way scale is represented with transformation matrices in Godot, the scale values will either be all positive or all negative.
[b]Note:[/b] Not all nodes are visually scaled by the [member scale] property. For example, [Light3D]s are not visually affected by [member scale].
Scale of this node in parent space (relative to the parent node). This value is obtained from [member basis]'s scale.
Copy link
Member

@aaronfranke aaronfranke Nov 24, 2024

Choose a reason for hiding this comment

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

I am begging you, when you make changes to the documentation, you need to take the time to actually verify the information is correct. The scale is applied after the rotation, which means the scale is not relative to the parent node. For example, here is a box scaled by 3 on the local Z axis, rotated by 90 degrees around the Y axis, and therefore it gets bigger on the global X axis.

Screenshot 2024-11-24 at 11 25 05 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it's in object-local space!

@Mickeon Mickeon force-pushed the doc-peeves-Node3D-glasses branch from 54ba20e to c856c68 Compare November 25, 2024 00:51
doc/classes/Node3D.xml Outdated Show resolved Hide resolved
</description>
</method>
<method name="translate_object_local">
<return type="void" />
<param index="0" name="offset" type="Vector3" />
<description>
Changes the node's position by the given offset [Vector3] in local space.
Adds the given translation [param offset] to the node's position, in local space (relative to this node). As such, the final translation is relative to the node's facing direction.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Adds the given translation [param offset] to the node's position, in local space (relative to this node). As such, the final translation is relative to the node's facing direction.
Adds the given translation [param offset] to the node's position, in object-local space (relative to this node). As such, the final translation is relative to the node's facing direction.

Copy link
Contributor Author

@Mickeon Mickeon Nov 27, 2024

Choose a reason for hiding this comment

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

I gather this means every mention of it should say "object-local" instead?

</description>
</method>
<method name="rotate_x">
<return type="void" />
<param index="0" name="angle" type="float" />
<description>
Rotates the local transformation around the X axis by angle in radians.
Rotates this node's local transformation around its X axis by the given [param angle], in radians. This changes the pitch, without affecting the [member position].
Copy link
Member

@aaronfranke aaronfranke Nov 27, 2024

Choose a reason for hiding this comment

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

I checked the code and tested, it actually doesn't use the node's local axis, it's parent-relative. Therefore, it can also affect non-pitch rotations.

Note: Not "the parent's X axis", but rather "the parent-relative X axis", because conceptually speaking the tail of the vector is the position of the object, but it points in the direction of the parent's X axis.

Suggested change
Rotates this node's local transformation around its X axis by the given [param angle], in radians. This changes the pitch, without affecting the [member position].
Rotates this node's local transformation around the parent-relative X axis by the given [param angle], in radians, without affecting the position.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tested it myself but I can trust you on this.
Isn't there some truth to the original statement? Why is it not specifically the position property?

doc/classes/Node3D.xml Outdated Show resolved Hide resolved
doc/classes/Node3D.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-Node3D-glasses branch from c856c68 to edd13a2 Compare November 27, 2024 13:28
@Mickeon Mickeon requested a review from aaronfranke November 27, 2024 13:28
Copy link
Member

@kleonc kleonc left a comment

Choose a reason for hiding this comment

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

I've commented some on the methods' descriptions changes (skipped properties).

@Mickeon please don't apply my suggestions before @aaronfranke would comment on some, as I think I'm suggesting some changes to what he already suggested before.


BTW I think Euler angles explanations will need some further overhaul, they're not fully clear as is (see this forum post, and my response with the info which should be somehow contained in the docs (not fully sure where exactly)). But this possibly could be done in a separate PR later.

</description>
</method>
<method name="global_rotate">
<return type="void" />
<param index="0" name="axis" type="Vector3" />
<param index="1" name="angle" type="float" />
<description>
Rotates the global (world) transformation around axis, a unit [Vector3], by specified angle in radians. The rotation axis is in global coordinate system.
Rotates this node's [member global_transform] around the global [param axis] by the given [param angle], in radians. This operation is calculated in global space (relative to the world).
Copy link
Member

Choose a reason for hiding this comment

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

node3d.global_rotate(axis, angle):

  • is equivalent to node3d.global_basis = node3d.global_basis.rotated(axis, angle),
  • is equivalent to node3d.global_transform = node3d.global_transform.translated(-node3d.global_position).rotated(axis, angle).translated(node3d.global_position) which rotates the global_transform around a global axis going through the node3d's global_position/global_transform.origin,
  • is not equivalent to node3d.global_transform = node3d.global_transform.rotated(axis, angle) which rotates the global_transform around a global axis going through the global origin ((0, 0, 0)).

godot/scene/3d/node_3d.cpp

Lines 1025 to 1030 in 0c45ace

void Node3D::global_rotate(const Vector3 &p_axis, real_t p_angle) {
ERR_THREAD_GUARD;
Transform3D t = get_global_transform();
t.basis.rotate(p_axis, p_angle);
set_global_transform(t);
}

So either state that it rotates only the Basis (in which case the axis on its own provides enough info, as we know that the rotation-axis goes through such Basis' origin):

Suggested change
Rotates this node's [member global_transform] around the global [param axis] by the given [param angle], in radians. This operation is calculated in global space (relative to the world).
Rotates this node's [member global_basis] around the global [param axis] by the given [param angle], in radians. This operation is calculated in global space (relative to the world).

An alternative would be to keep saying that it rotates the global transform but also specifying a point which such rotation-axis goes through:

Suggested change
Rotates this node's [member global_transform] around the global [param axis] by the given [param angle], in radians. This operation is calculated in global space (relative to the world).
Rotates this node's [member global_transform] around the global [param axis] going through this node's origin by the given [param angle], in radians. This operation is calculated in global space (relative to the world).

Simpler (if it rotates global_basis around a global axis then isn't it already clear it happens in the global space? 🤔):

Suggested change
Rotates this node's [member global_transform] around the global [param axis] by the given [param angle], in radians. This operation is calculated in global space (relative to the world).
Rotates this node's [member global_basis] around the global-space [param axis] by the given [param angle], in radians.

doc/classes/Node3D.xml Outdated Show resolved Hide resolved
</description>
</method>
<method name="is_scale_disabled" qualifiers="const">
<return type="bool" />
<description>
Returns whether this node uses a scale of [code](1, 1, 1)[/code] or its local transformation scale.
Returns [code]true[/code] if this node's [member scale] is ignored. See also [method set_disable_scale].
Copy link
Member

Choose a reason for hiding this comment

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

Looking into the code data.disable_scale affects only the global_transform, not the transform. It also doesn't really make the scale be ignored. The scale (or rather the whole basis) is taken into account for calculating the global transform as always, the same for all the parent transforms. The difference is that such calculated-as-always global transform is then orthonormalized. But it doesn't mean that the intermediate scalings are ignored, they still do affect the final orthonormalized global_transform.

godot/scene/3d/node_3d.cpp

Lines 851 to 854 in 0c45ace

void Node3D::set_disable_scale(bool p_enabled) {
ERR_THREAD_GUARD;
data.disable_scale = p_enabled;
}

godot/scene/3d/node_3d.cpp

Lines 465 to 495 in 0c45ace

Transform3D Node3D::get_global_transform() const {
ERR_FAIL_COND_V(!is_inside_tree(), Transform3D());
/* Due to how threads work at scene level, while this global transform won't be able to be changed from outside a thread,
* it is possible that multiple threads can access it while it's dirty from previous work. Due to this, we must ensure that
* the dirty/update process is thread safe by utilizing atomic copies.
*/
uint32_t dirty = _read_dirty_mask();
if (dirty & DIRTY_GLOBAL_TRANSFORM) {
if (dirty & DIRTY_LOCAL_TRANSFORM) {
_update_local_transform(); // Update local transform atomically.
}
Transform3D new_global;
if (data.parent && !data.top_level) {
new_global = data.parent->get_global_transform() * data.local_transform;
} else {
new_global = data.local_transform;
}
if (data.disable_scale) {
new_global.basis.orthonormalize();
}
data.global_transform = new_global;
_clear_dirty_bits(DIRTY_GLOBAL_TRANSFORM);
}
return data.global_transform;
}

Given the current behavior this should be correct (not sure how to describe it simpler):

Suggested change
Returns [code]true[/code] if this node's [member scale] is ignored. See also [method set_disable_scale].
Returns [code]true[/code] if this node's [member global_transform] is automatically orthonormalized as the last step of calculating it. See also [method set_disable_scale] and [method Transform3D.orthonormalized].

Might also make sense to add something like: "Note that the [member transform] is unaffected.".

Copy link
Contributor Author

@Mickeon Mickeon Nov 29, 2024

Choose a reason for hiding this comment

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

From an outside perspective, in most scenarios, it does just affect the node's scale. I don't think the suggestion makes that clear enough and makes it look more complicated than it is.

Your suggestion is very accurate and worth noting some way still...
Something roughly similar to...?

Suggested change
Returns [code]true[/code] if this node's [member scale] is ignored. See also [method set_disable_scale].
Returns [code]true[/code] if this node's [member global_transform] is automatically orthonormalized. This results in the node appearing undistorted, as if its [member global_scale] were set to [constant Vector3.ONE]. See also [method set_disable_scale] and [method Transform3D.orthonormalized].

I tested it and it seems true.

Copy link
Member

Choose a reason for hiding this comment

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

"as the last step of calculating it" indeed is not needed, just "automatically orthonormalized" sounds the same but simpler. 👍

"as if its [member global_scale] were set to [constant Vector3.ONE]"

In theory an orthonormal basis could have scale factor being any combination of -1/+1. After quick testing, in Godot basis.orthonormalized().get_scale() can return either (1, 1, 1) or (-1, -1, -1) (approx, you can get e.g. ( 1.0000000000, 0.9999998212, 0.9999999404)).

@tool
extends EditorScript

func _run() -> void:
	var counts := {}
	for i in 1_000_000:
		var b := Basis(
			Vector3(randf_range(-2, 2), randf_range(-2, 2), randf_range(-2, 2)),
			Vector3(randf_range(-2, 2), randf_range(-2, 2), randf_range(-2, 2)),
			Vector3(randf_range(-2, 2), randf_range(-2, 2), randf_range(-2, 2))
		)
		#var s := b.orthonormalized().get_scale()
		var s := b.orthonormalized().get_scale().round()
		counts[s] = counts.get(s, 0) + 1
	for s in counts:
		#print("%13.10v %d" % [s, counts[s]])
		print("%2.0v %d" % [s, counts[s]]))
( 1,  1,  1) 500005
(-1, -1, -1) 499995

But (-1, -1, -1) should be only if the basis involves some negative scaling, like e.g. for Basis(Vector3(-1, 0, 0), Vector3(0, 1, 0), Vector3(0, 0, 1)).

Copy link
Contributor Author

@Mickeon Mickeon Nov 30, 2024

Choose a reason for hiding this comment

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

I am unsure on how to word this, in that case. I have a feeling I'll come back to this in another PR, for now I'm trying to stick to accuracy.

Returns [code]true[/code] if this node's [member global_transform] is automatically orthonormalized. This results in this node not appearing distorted, as if its global scale were set to the default. See also [method set_disable_scale] and [method orthonormalize].
[b]Note:[/b] [member transform] is not affected by this setting.

</description>
</method>
<method name="rotate">
<return type="void" />
<param index="0" name="axis" type="Vector3" />
<param index="1" name="angle" type="float" />
<description>
Rotates the local transformation around axis, a unit [Vector3], by specified angle in radians.
Rotates this node's [member transform] around the [param axis] by the given [param angle], in radians. This operation is calculated in parent space (relative to the parent).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Rotates this node's [member transform] around the [param axis] by the given [param angle], in radians. This operation is calculated in parent space (relative to the parent).
Rotates this node's [member basis] around the [param axis] by the given [param angle], in radians. This operation is calculated in parent space (relative to the parent).

The same as for global_rotate, but here it's parent relatve instead of global. Either say that we're rotating the transform around the parent-space axis going through this node's position (which is parent-space). Or just state that it's rotating the basis around the parent-space axis.


Simpler:

Suggested change
Rotates this node's [member transform] around the [param axis] by the given [param angle], in radians. This operation is calculated in parent space (relative to the parent).
Rotates this node's [member basis] around the parent-space [param axis] by the given [param angle], in radians.

</description>
</method>
<method name="rotate_x">
<return type="void" />
<param index="0" name="angle" type="float" />
<description>
Rotates the local transformation around the X axis by angle in radians.
Rotates this node's local transformation around the parent-relative X axis by the given [param angle], in radians, without affecting the position.
Copy link
Member

Choose a reason for hiding this comment

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

This seems kinda correct as it states "without affecting the position".

But I think think this should be the same as / a simplified version of the rotate's description, as rotate_x(angle) is equivalent to rotate(Vector3(1, 0, 0), angle) (might be even a good idea to mention such equivalence here):

Suggested change
Rotates this node's local transformation around the parent-relative X axis by the given [param angle], in radians, without affecting the position.
Rotates this node's [member basis] around the X axis by the given [param angle], in radians. This operation is calculated in parent space (relative to the parent).

?
(same for rotate_{y|z})


Simpler:

Suggested change
Rotates this node's local transformation around the parent-relative X axis by the given [param angle], in radians, without affecting the position.
Rotates this node's [member basis] around the parent-space X axis by the given [param angle], in radians.

doc/classes/Node3D.xml Outdated Show resolved Hide resolved
doc/classes/Node3D.xml Outdated Show resolved Hide resolved
doc/classes/Node3D.xml Outdated Show resolved Hide resolved
doc/classes/Node3D.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-Node3D-glasses branch from edd13a2 to b353b18 Compare November 29, 2024 21:17
@Mickeon
Copy link
Contributor Author

Mickeon commented Nov 29, 2024

Pushed an update to address all of the above. But there's just a bit more to do.

  • I did not go with the shortened version of each suggestion, as I feel like the descriptions can afford to be explicit here.
  • I am not sure it's necessary to state that, for example, rotate_x is equivalent to using an axis of Vector3(1, 0, 0 in rotate. I am not dying on that hill, though.
  • I like saying "basis" instead of "local transformation". That's actually genius. The local transformation's origin is always void, so this simplifies things a lot to me.
    • I think mentioning that the position is unaffected could still be warranted, though. Just to make it clearer.
    • Perhaps in global_basis and basis's own description these are the object-local transformations?
  • I have handled similar descriptions as suggested, but the idea of saying "around the global [param axis] going through this node's origin" interests me. It sounds a whole lot easier to imagine the whole operation that way.
  • I have written a note encouraging to use translate_object_local instead of translate
  • is_scale_disabled and set_disable_scale are a bit up in the air. I think mentioning orthonormalization is a technical detail that wouldn't matter to most, so while I've changed it based off this comment, I think it should be simplified further.

@Mickeon Mickeon requested review from kleonc and tetrapod00 November 29, 2024 22:32
@Mickeon Mickeon force-pushed the doc-peeves-Node3D-glasses branch 2 times, most recently from 169b40b to 1914168 Compare November 30, 2024 20:22
@kleonc
Copy link
Member

kleonc commented Nov 30, 2024

  • I think mentioning that the position is unaffected could still be warranted, though. Just to make it clearer.

Nothing against making things clearer.

  • Perhaps in global_basis and basis's own description these are the object-local transformations?

If by "object-local transformation" you mean non-translating / keeping the position of the given object/node unchanged then sure you could say so. But I wouldn't add anything like that, this will be confusing. There's already Node3D::translate_object_local method. If "object-local" would mean "not changing position" then how could you ever "translate-object-local"? Makes no sense. Let's not add potential additional confusion.

The "object-local" being used means the same as what we already call just "local" in the context of the given node/object. Like:

  • parent == parent-local == (parent-object)-local
  • local == (self/this)-local == (self/this-object)-local == object-local

In fact I was wondering whether we should maybe use e.g. parent-local/self-local instead of just parent/local. Local is always relative to something, so it's potentially easy to misinterpret. Object-local is meant to be more clear, as it specifies that the "local" refers to the given object, not its parent etc. But I think that in context of e.g. Node3D it's already clear that "local" refers to this specific node/object? 🤔

Anyway, if we'd want to use e.g. "object-local" instead of just "local" then we should probably do so consequently across the whole docs. AFAICT currently we use just "local" to refer to the given object/node, so I'm saying let's keep using it.

  • I have handled similar descriptions as suggested, but the idea of saying "around the global [param axis] going through this node's origin" interests me. It sounds a whole lot easier to imagine the whole operation that way.

Again, nothing against clarifications. The purpose of the docs is not to be as concise as possible (while being correct). So if the descriptions can be made a little longer but easier to grasp for "a regular user" then sure it's welcomed (we'll try to keep them correct).

If we want to encourage using translate_object_local then maybe we should make translate's description only redirect to translate_object_local? Something like: "Same as [method translate_object_local], for compatibility reasons. Prefer using it instead." This way the user would have to check it out and hence more likely use it? 🤔 Just throwing an idea, I don't have a strong opinion on that, it's fine as is / up to you.

But might be a good to add additional note/warning to translate like "(For compatibility reasons, )this operation is not calculated in the parent space. To translate in parent space use node_3d.position += offset.".

@Mickeon Mickeon force-pushed the doc-peeves-Node3D-glasses branch from 1914168 to 286ef8d Compare November 30, 2024 21:51
@Mickeon
Copy link
Contributor Author

Mickeon commented Nov 30, 2024

I have updated to add "and preserves the position" where appropriate, as well as update translate and translate_object_local again.

Anyway, if we'd want to use e.g. "object-local" instead of just "local" then we should probably do so consequently across the whole docs. AFAICT currently we use just "local" to refer to the given object/node, so I'm saying let's keep using it.

Mhm. Hence my reluctance to say object-local space. If it needs to be clarified it needs to be done in one huge swoop across the docs.

  • I have handled similar descriptions as suggested, but the idea of saying "around the global [param axis] going through this node's origin" interests me. It sounds a whole lot easier to imagine the whole operation that way.

Again, nothing against clarifications. The purpose of the docs is not to be as concise as possible (while being correct). So if the descriptions can be made a little longer but easier to grasp for "a regular user" then sure it's welcomed (we'll try to keep them correct).

Problem here is that I'd have to give up on directly mentioning the basis otherwise the sentence doesn't make sense, and for consistency that may need to escalate for the surrounding descriptions as well.

Same as [method translate_object_local], for compatibility reasons. Prefer using it instead.

This sounds exactly like a deprecation message, but the method is not deprecated until we decide that separately. So it's a bit odd to do this right now.

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