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 CanvasItem documentation (no draw methods) #93735

Merged
merged 1 commit into from
Mar 19, 2025

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Jun 29, 2024

Related to #89256, #87440 and many others.

I forgot to ever make this pull request, this has been good enough for a while. Would be nice to merge before the above PRs, actually. I feel like this is a better starting point than the Node2D documentation overhaul.

This PR aims to overhaul CanvasItem's class reference in an attempt to be more... comprehensible and easy to understand. Reasons are the same as prior PRs so I'm not sure I should explain myself too much here.

This PR specifically avoids all of the draw methods in order to make it easier to merge sooner. These are tackled separately. See #93751.

@Mickeon Mickeon added this to the 4.x milestone Jun 29, 2024
@Mickeon Mickeon requested a review from a team as a code owner June 29, 2024 12:01
@Mickeon Mickeon removed the request for review from a team June 29, 2024 12:01
@Mickeon Mickeon marked this pull request as draft June 29, 2024 12:02
@Mickeon Mickeon force-pushed the doc-peeves-CanvasItem branch from 3f8fb68 to 4706262 Compare June 29, 2024 12:53
</description>
</method>
<method name="get_viewport_transform" qualifiers="const">
<return type="Transform2D" />
<description>
Returns the transform from the coordinate system of the canvas, this item is in, to the [Viewport]s embedders 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.

Insane that this description has been left like this for so long. I don't even have any idea on what this is trying to say.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I will try to explain the idea for this description in the hopes, that someone will come up with a better way to express the content.

Godot uses different 2D coordinate systems. e.g. CanvasItem, Viewport, ....

When you want to convert a coordinate from one coordinate system to a different coordinate system, you use a Transform2D.
The fixed transform2D between CanvasItem and Viewport for example allows you to convert all coordinates relative to the CanvasItem to coordinates relative to the viewport.

In short: viewport coordinate = transform * canvas item coordinate.

Some of the coordinate systems are easy to name:

  • the coordinate system of the CanvasItem
  • the coordinate system of the Viewport
  • the coordinate system of the Screen

Others are not easy to name, when one wants to be precise, like both coordinate systems that get_viewport_transform adresses.
CanvasItem.get_viewport_transform is in my opinion one of the most difficult to explain transform function and I believe, that it has nearly no use-case and should at some point in the future be removed from the API.
It is not even used in the code-base. (See my analysis and proposal here: godotengine/godot-proposals#4250)

To better visualize, what that transform does, have a look at the the second green bar from the bottom in the graphic here (click that graphic to enlarge):
https://docs.godotengine.org/en/latest/contributing/development/core_and_modules/2d_coordinate_systems.html

The current description is my attempt of precisely naming the two coordinate systems in the shortest possible way.

Copy link
Member

Choose a reason for hiding this comment

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

@Mickeon Can you update the description based on the comment above, or do you think it's fine as is? (I assume it's not, given your first comment; users that don't know the explanation might be confused too)

Copy link
Contributor Author

@Mickeon Mickeon Mar 17, 2025

Choose a reason for hiding this comment

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

As of answering right now, I don't have the time to come up with an updated description.
No, it's not fine indeed... but I am keeping track of the above, immensely detailed explanation.

Copy link
Contributor Author

@Mickeon Mickeon Mar 18, 2025

Choose a reason for hiding this comment

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

What I can maybe say is as follows, as I right now don't have a concrete usage example:

Returns the transform of this node, converted from its registered canvas's coordinate system to its viewport embedder's coordinate system. See also [method Viewport.get_final_transform] and [method Node.get_viewport].

Viewport's get_final_transform does the same conversion to the embedder's coordinate system, but from the Viewport's transform.
I did not touch these "transform conversion" methods in this PR too much, but I guess I'll have to reword get_canvas_transform as well.
If the purpose of the get_viewport_transform method is so arcane, it may be a good idea to deprecate it sometime soon.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if the comma should be here, but it sounds better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@Rindbee Rindbee Mar 19, 2025

Choose a reason for hiding this comment

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

Some are relative transforms (Apply the transformation ending to or starting from this node), and some are intermediate transforms (for caching purposes, not directly related to the current node it is not a transformation of one node relative to another).

Currently Viewport::get_global_canvas_transform() is an intermediate transform. It may be better to rename it to Viewport::get_base_canvas_transform().

From the perspective of relative transformation, this method is more like get_global_canvas_transform(), can be added in CanvasLayer and Viewport.

@Mickeon Mickeon force-pushed the doc-peeves-CanvasItem branch from 4706262 to 9b3c49f Compare June 29, 2024 13:18
</description>
</method>
<method name="get_visibility_layer_bit" qualifiers="const">
<return type="bool" />
<param index="0" name="layer" type="int" />
<description>
Returns an individual bit on the rendering visibility layer.
Returns [code]true[/code] if the layer at the given index is set in [member visibility_layer].
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternative:

Suggested change
Returns [code]true[/code] if the layer at the given index is set in [member visibility_layer].
Returns [code]true[/code] if the given [param layer] bit is set in [member visibility_layer].

</description>
</method>
<method name="move_to_front">
<return type="void" />
<description>
Moves this node to display on top of its siblings.
Internally, the node is moved to the bottom of parent's child list. The method has no effect on nodes without a parent.
Moves this node below its siblings, usually causing the node to draw on top of its siblings. Does nothing if this node does not have a parent. See also [method Node.move_child].
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternative:

Suggested change
Moves this node below its siblings, usually causing the node to draw on top of its siblings. Does nothing if this node does not have a parent. See also [method Node.move_child].
Moves this node below its siblings. This usually causes the node to draw on top of its siblings. Does nothing if this node does not have a parent. See also [method Node.move_child].

@Mickeon Mickeon marked this pull request as ready for review June 29, 2024 13:28
@Mickeon Mickeon requested a review from a team June 29, 2024 13:28
@Mickeon
Copy link
Contributor Author

Mickeon commented Jun 29, 2024

This is actually more complete than I envisioned it to be. It's good to be looked at as is, too.

@Mickeon Mickeon modified the milestones: 4.x, 4.4 Jun 29, 2024
@Mickeon Mickeon added cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Jun 29, 2024
@Mickeon Mickeon force-pushed the doc-peeves-CanvasItem branch from 9b3c49f to a4a3063 Compare August 19, 2024 10:24
@Mickeon
Copy link
Contributor Author

Mickeon commented Aug 19, 2024

Rebased with no changes.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@Repiteo Repiteo modified the milestones: 4.4, 4.x Feb 24, 2025
@Mickeon Mickeon force-pushed the doc-peeves-CanvasItem branch from a4a3063 to 53db0db Compare March 17, 2025 00:02
@Mickeon
Copy link
Contributor Author

Mickeon commented Mar 17, 2025

This PR has been neglected for so long, that other PRs have come through to conflict with visible & y_sort_enabled's description. I have not changed these in the PR (anymore) to avoid invalidating a localization string. I would gladly rewrite them, but I do not want this PR to take even longer to review.

@Calinou 's suggestions have been accepted, assuming accuracy, with a few slight changes.

@Mickeon Mickeon requested a review from Calinou March 17, 2025 00:18
@Mickeon Mickeon added cherrypick:4.4 Considered for cherry-picking into a future 4.4.x release and removed cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Mar 17, 2025
@Mickeon Mickeon modified the milestones: 4.x, 4.5 Mar 17, 2025
@Mickeon Mickeon force-pushed the doc-peeves-CanvasItem branch from 554d974 to 93a7584 Compare March 18, 2025 14:36
@akien-mga akien-mga removed the cherrypick:4.4 Considered for cherry-picking into a future 4.4.x release label Mar 19, 2025
@akien-mga akien-mga merged commit 100862c into godotengine:master Mar 19, 2025
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@Mickeon Mickeon deleted the doc-peeves-CanvasItem branch March 19, 2025 15:29
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.

8 participants