Add to_global_basis() and to_local_basis() to Spatial/Node2D

[Cykyrios]

Describe the project you are working on:
This proposal is not related to any particular project.

Describe the problem or limitation you are having in your project:
Transforming direction vectors from local space to global space (or the other way around) can be cumbersome and unintuitive, with calls such as spatial.get_global_transform.basis.xform_inv(vector3).

Describe the feature / enhancement and how it helps to overcome the problem or limitation:
I propose adding a to_global_basis(vector3) method (and the equivalent to_local_basis) to Spatial nodes. This is mainly for convenience, similar to the already available to_global method. For Node2D, I am not sure whether to_global_basis or to_global_rotation would make more sense.

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:
I have already added these methods in the following way:

Vector3 Spatial::to_global_basis(Vector3 p_local) const {
  return get_global_transform().get_basis().xform(p_local);
}
Vector3 Spatial::to_local_basis(Vector3 p_local) const {
  return get_global_transform().get_basis().xform_inv(p_global);
}

Point2 Node2D::to_local_rotation(Point2 p_global) const {
  return get_global_transform().basis_xform_inv(p_global);
}
Point2 Node2D::to_global_rotation(Point2 p_local) const {
  return get_global_transform().basis_xform(p_local);
}

If this enhancement will not be used often, can it be worked around with a few lines of script?:
This can of course be worked around easily, the point of this proposal is to add convenience to manipulation of vectors, which could be used often, especially by beginners.

Is there a reason why this should be core and not an add-on in the asset library?:
Again, this is mainly for convenience, in the same way to_global and to_local have already been made available, but are only suitable for position vectors.

[Cykyrios]

Note that I opened this proposal instead of directly making a PR because I added these methods on the 3.2 branch. master is somewhat broken for me, and there is also the issue of renamed nodes: I could not find Spatial in the master branch, and I didn't see any Node3D either, so I would appreciate if someone could tell me what the new name for the Spatial node is.

Also, unless this would only be added to Godot 4, the change has to be made specifically for both 3.2 and 4.0.

Edit: I found this PR which states Spatial is now Node3D, but for some reason it doesn't show up in QtCreator for me - though the files are in the source and the Godot.files and Godot.includes do list them as well.

Edit again: I fixed my issue with QtCreator, the node_3d files were not actually in Godot.files, so I have also added this to Node3D on master. My question then is how should I make my PR? Doing it on master is the obvious option, but is not compatible with 3.2 for Spatial nodes. The changes are however simple enough that they can be backported to 3.2 with minimal effort.

[Calinou]

My question then is how should I make my PR? Doing it on master is the obvious option, but is not compatible with 3.2 for Spatial nodes. The changes are however simple enough that they can be backported to 3.2 with minimal effort.

In this case, you should remake the PR manually against the 3.2 branch once the PR in the master branch is merged.

[aaronfranke]

I don't think this is a useful feature, the use cases are pretty uncommon. In the demo projects:

$ grep -RIn "\.xform" | grep -v xml | grep global

There are only 7 places where these functions could be used, all of them in the 3D IK demo, 6 of them in ik_fabrik.gd, which isn't even necessary anymore due to Godot having a built-in IK system (it's just that nobody has bothered to remove it). So, really just one use case for these in the demo projects.

It also hides the fact that get_global_transform is being called every time, and this operation is expensive enough that I think calling it should be explicit, in case users want to cache global_transform for performance. In the end I think this should be closed because it's not a common or useful feature and could lead to accidentally writing slow code in the few cases where it is useful.

(copied from my review in the PR)

[groud]

I don't think this should be added.

The to_global() and to_local() make sense, they are simple enough and a lot of people are using it to transform mouse positions. They are definitely useful even if we don't use them a lot in the demo projects. (I basically advised yesterday to use it, as it's a lot simpler than using transforms, to map a coordinate from a tilemap to the global coordinates)

But regarding the proposal, I think the "basis" ones are kind of confusing. While it's easy to understand the current versions, those are significantly harder to understand, and bring confusion to the user when having to choose which function to use.
So for such cases, which are more complex are rare use cases, I'd let the user use the transform directly.

[Cykyrios]

Looks like I proposed this while thinking more about my own project (3D and physics based, I have lots of basis.xform calls) than what most users do. It's true that it looks somewhat niche, though I believe vector transforms (with or without basis, and not only from/to local and global space but also between arbitrary nodes) are important enough to warrant a dedicated demo project, since it appears their use is seldom covered in the demos.
A basic idea for a demo would be to have 2 or 3 nodes moving around and displaying their positions, velocities and forward vectors in global space and in each node's local space.

[aaronfranke]

We already have documentation articles such as Matrices and transforms, which comes with a minimal project that allows users to manually play around with transforms. We don't currently have a policy on what kinds of demos we should have (see 349 and 390), I'm not sure we need such a demo.

[akien-mga]

The PR implementing this proposal was rejected, so closing this too.
godotengine/godot#38902