Merge #827

827: API consistency + documentation for geometric types r=Bromeon a=Bromeon

Primarily improves APIs of `Basis`, `Transform`, `Transform2D`.

Changes:

* General
  * Documentation improvements, especially w.r.t row/column vectors
  * Add links to Godot API doc
  * Replace some named types with `Self`
  * Group some methods according to functionality; sys() and from_sys() at end of impl
  * Move some types to `geom` module (internal only)
  * Remove dead code

* **`Transform`**
  * Remove `from_axis_origin()`, confusing semantics
  * Add `from_basis_origin()` instead
  * Add `FLIP_X`, `FLIP_Y`, `FLIP_Z` constants

* **`Basis`**
  * Rename `from_elements()` -> `from_rows()`
  * Add `from_basis_vectors()` constructor
  * `IDENTITY`, `FLIP_X`, `FLIP_Y`, `FLIP_Z` now constants instead of functions
  * Rename basis vectors `x, y, z` -> `a, b, c`
  * Rename `to_scale()` -> `scale()`
  * Change parameters `self` -> `&self` (except operators)
  * Change vector parameters to pass-by-value
  * Make modifying methods like `invert()`, `orthonormalize()`, `rotate()` private
  * Make `tdotx()` etc. private for now
  * Reuse code (use of dot products)

* **`Transform2D`**
  * Rename `from_axis_origin()` -> `from_basis_origin()`
  * Rename basis vectors `x, y` -> `a, b`


Co-authored-by: Jan Haller <[email protected]>

Author: bors[bot]

gdnative-core/src/core_types/geom/basis.rs

@@ -3,9 +3,15 @@
 mod aabb;
 mod basis;
 mod plane;
+mod quat;
+mod rect2;
 mod transform;
+mod transform2d;
 
-pub use self::aabb::Aabb;
-pub use self::basis::Basis;
-pub use self::plane::Plane;
-pub use self::transform::Transform;
+pub use aabb::*;
+pub use basis::*;
+pub use plane::*;
+pub use quat::*;
+pub use rect2::*;
+pub use transform::*;
+pub use transform2d::*;

gdnative-core/src/core_types/geom/mod.rs

@@ -10,30 +10,18 @@ pub struct Plane {
 }
 
 impl Plane {
-    #[doc(hidden)]
-    #[inline]
-    pub fn sys(&self) -> *const sys::godot_plane {
-        unsafe { std::mem::transmute::<*const Plane, *const sys::godot_plane>(self as *const _) }
-    }
-
-    #[doc(hidden)]
-    #[inline]
-    pub fn from_sys(c: sys::godot_plane) -> Self {
-        unsafe { std::mem::transmute::<sys::godot_plane, Self>(c) }
-    }
-
     /// Creates a new `Plane` from the ['Vector3'](./type.Vector3.html) normal and the distance from the origin.
     #[inline]
-    pub fn new(normal: Vector3, d: f32) -> Plane {
-        Plane { normal, d }
+    pub fn new(normal: Vector3, d: f32) -> Self {
+        Self { normal, d }
     }
 
     /// Creates a new `Plane` from four floats.
     /// a, b, c are used for the normal ['Vector3'](./type.Vector3.html).
     /// d is the distance from the origin.
     #[inline]
-    pub fn from_coordinates(a: f32, b: f32, c: f32, d: f32) -> Plane {
-        Plane {
+    pub fn from_coordinates(a: f32, b: f32, c: f32, d: f32) -> Self {
+        Self {
             normal: Vector3::new(a, b, c),
             d,
         }
@@ -42,13 +30,13 @@ impl Plane {
     /// Creates a new `Plane` from three [`Vector3`](./type.Vector3.html), given in clockwise order.
     /// If all three points are collinear, returns `None`.
     #[inline]
-    pub fn from_points(a: Vector3, b: Vector3, c: Vector3) -> Option<Plane> {
+    pub fn from_points(a: Vector3, b: Vector3, c: Vector3) -> Option<Self> {
         let normal = (a - c).cross(a - b).normalized();
 
         if normal.x.is_nan() || normal.y.is_nan() || normal.z.is_nan() {
             None
         } else {
-            Some(Plane {
+            Some(Self {
                 normal,
                 d: normal.dot(a),
             })
@@ -151,7 +139,7 @@ impl Plane {
 
     /// Returns the `Plane` normalized.
     #[inline]
-    pub fn normalize(mut self) -> Plane {
+    pub fn normalize(mut self) -> Self {
         let l = self.normal.length();
 
         if l == 0.0 {
@@ -170,6 +158,18 @@ impl Plane {
     pub fn project(&self, point: Vector3) -> Vector3 {
         point - self.normal * self.distance_to(point)
     }
+
+    #[doc(hidden)]
+    #[inline]
+    pub fn sys(&self) -> *const sys::godot_plane {
+        unsafe { std::mem::transmute::<*const Plane, *const sys::godot_plane>(self as *const _) }
+    }
+
+    #[doc(hidden)]
+    #[inline]
+    pub fn from_sys(c: sys::godot_plane) -> Self {
+        unsafe { std::mem::transmute::<sys::godot_plane, Self>(c) }
+    }
 }
 
 #[cfg(test)]

gdnative-core/src/core_types/geom/plane.rs

@@ -1,7 +1,12 @@
-use super::{Basis, IsEqualApprox, Vector3, CMP_EPSILON};
+use crate::core_types::{Basis, IsEqualApprox, Vector3, CMP_EPSILON};
 use glam::EulerRot;
 use std::ops::{Mul, Neg};
 
+/// Quaternion, used to represent 3D rotations.
+///
+/// Quaternions need to be [normalized][Self::normalized()] before all operations.
+///
+/// See also [Quat](https://docs.godotengine.org/en/stable/classes/class_quat.html) in the Godot API doc.
 #[derive(Copy, Clone, Debug, PartialEq)]
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
 #[repr(C)]
@@ -248,11 +253,11 @@ mod test {
     #[test]
     fn to_basis() {
         let quat = Quat::new(0.485489, 0.142796, -0.862501, 0.001113);
-        let expect = Basis::from_elements([
+        let expect = Basis::from_rows(
             Vector3::new(-0.528598, 0.140572, -0.837152),
             Vector3::new(0.136732, -0.959216, -0.247404),
             Vector3::new(-0.837788, -0.245243, 0.487819),
-        ]);
+        );
         let basis = Basis::from_quat(quat);
         assert!(basis.is_equal_approx(&expect));
     }

gdnative-core/src/core_types/quat.rs renamed to gdnative-core/src/core_types/geom/quat.rs

@@ -1,4 +1,4 @@
-use super::Vector2;
+use crate::core_types::Vector2;
 
 #[derive(Copy, Clone, Debug, PartialEq)]
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]

gdnative-core/src/core_types/rect2.rs renamed to gdnative-core/src/core_types/geom/rect2.rs

@@ -2,16 +2,29 @@ use std::ops::Mul;
 
 use crate::core_types::{Basis, Vector3};
 
-/// 3D Transformation (3x4 matrix) Using basis + origin representation.
+/// Affine 3D transform (3x4 matrix).
+///
+/// Used for 3D linear transformations. Uses a basis + origin representation.
+/// The
+///
+/// Expressed as a 3x4 matrix, this transform consists of 3 basis (column) vectors `a`, `b`, `c`
+/// as well as an origin `o`; more information in [`Self::from_basis_origin()`]:
+/// ```text
+/// [ a.x  b.x  c.x  o.x ]
+/// [ a.y  b.y  c.y  o.y ]
+/// [ a.z  b.z  c.z  o.z ]
+/// ```
+///
+/// See also [Transform](https://docs.godotengine.org/en/stable/classes/class_transform.html) in the Godot API doc.
 #[repr(C)]
 #[derive(Copy, Clone, Debug, PartialEq)]
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
 pub struct Transform {
-    /// The basis is a matrix containing 3 Vector3 as its columns: X axis, Y axis, and Z axis.
-    /// These vectors can be interpreted as the basis vectors of local coordinate system
-    /// traveling with the object.
+    /// The basis is a matrix containing 3 vectors as its columns. They can be interpreted
+    /// as the basis vectors of the transformed coordinate system.
     pub basis: Basis,
-    /// The translation offset of the transform.
+
+    /// The new origin of the transformed coordinate system.
     pub origin: Vector3,
 }
 
@@ -23,45 +36,63 @@ impl Default for Transform {
 }
 
 impl Transform {
-    #[doc(hidden)]
-    #[inline]
-    pub fn sys(&self) -> *const sys::godot_transform {
-        unsafe {
-            std::mem::transmute::<*const Transform, *const sys::godot_transform>(self as *const _)
-        }
-    }
+    /// Identity transform; leaves objects unchanged when applied.
+    pub const IDENTITY: Self = Self {
+        basis: Basis::IDENTITY,
+        origin: Vector3::ZERO,
+    };
 
-    #[doc(hidden)]
-    #[inline]
-    pub fn from_sys(c: sys::godot_transform) -> Self {
-        unsafe { std::mem::transmute::<sys::godot_transform, Self>(c) }
-    }
+    /// Transform that mirrors along the **X axis** (perpendicular to the YZ plane).
+    pub const FLIP_X: Self = Self {
+        basis: Basis::FLIP_X,
+        origin: Vector3::ZERO,
+    };
+
+    /// Transform that mirrors along the **Y axis** (perpendicular to the XZ plane).
+    pub const FLIP_Y: Self = Self {
+        basis: Basis::FLIP_Y,
+        origin: Vector3::ZERO,
+    };
 
-    pub const IDENTITY: Transform = Transform {
-        basis: Basis::identity(),
+    /// Transform that mirrors along the **Z axis** (perpendicular to the XY plane).
+    pub const FLIP_Z: Self = Self {
+        basis: Basis::FLIP_Z,
         origin: Vector3::ZERO,
     };
 
-    /// Creates a new transform from its three basis vectors and origin.
+    /// Creates a new transform from three basis vectors and the coordinate system's origin.
+    ///
+    /// Each vector represents a basis vector in the *transformed* coordinate system.
+    /// For example, `a` is the result of transforming the X unit vector `(1, 0, 0)`.
+    /// The 3 vectors need to be linearly independent.
+    ///
+    /// Basis vectors are stored as column vectors in the matrix, see also [`Basis::from_basis_vectors()`].
+    ///
+    /// The construction `Transform::from_basis_origin(a, b, c, o)` will create the following 3x4 matrix:
+    /// ```text
+    /// [ a.x  b.x  c.x  o.x ]
+    /// [ a.y  b.y  c.y  o.y ]
+    /// [ a.z  b.z  c.z  o.z ]
+    /// ```
     #[inline]
-    pub fn from_axis_origin(
-        x_axis: Vector3,
-        y_axis: Vector3,
-        z_axis: Vector3,
+    pub const fn from_basis_origin(
+        basis_vector_a: Vector3,
+        basis_vector_b: Vector3,
+        basis_vector_c: Vector3,
         origin: Vector3,
     ) -> Self {
         Self {
+            basis: Basis::from_basis_vectors(basis_vector_a, basis_vector_b, basis_vector_c),
             origin,
-            basis: Basis::from_elements([x_axis, y_axis, z_axis]),
         }
     }
 
     /// Returns this transform, with its origin moved by a certain `translation`
     #[inline]
-    pub fn translated(&self, translation: Vector3) -> Transform {
+    pub fn translated(&self, translation: Vector3) -> Self {
         Self {
-            origin: self.origin + translation,
             basis: self.basis,
+            origin: self.origin + translation,
         }
     }
 
@@ -85,24 +116,26 @@ impl Transform {
     /// transformation is composed of rotation and translation (no scaling, use
     /// affine_inverse for transforms with scaling).
     #[inline]
-    pub fn inverse(&self) -> Transform {
+    pub fn inverse(&self) -> Self {
         let basis_inv = self.basis.transposed();
         let origin_inv = basis_inv.xform(-self.origin);
-        Transform {
-            origin: origin_inv,
+
+        Self {
             basis: basis_inv,
+            origin: origin_inv,
         }
     }
 
     /// Returns the inverse of the transform, under the assumption that the
     /// transformation is composed of rotation, scaling and translation.
     #[inline]
-    pub fn affine_inverse(&self) -> Transform {
+    pub fn affine_inverse(&self) -> Self {
         let basis_inv = self.basis.inverted();
         let origin_inv = basis_inv.xform(-self.origin);
-        Transform {
-            origin: origin_inv,
+
+        Self {
             basis: basis_inv,
+            origin: origin_inv,
         }
     }
 
@@ -113,26 +146,40 @@ impl Transform {
     /// fully aligned to the target by a further rotation around an axis
     /// perpendicular to both the target and up vectors.
     #[inline]
-    pub fn looking_at(&self, target: Vector3, up: Vector3) -> Transform {
+    pub fn looking_at(&self, target: Vector3, up: Vector3) -> Self {
         let up = up.normalized();
         let v_z = (self.origin - target).normalized();
         let v_x = up.cross(v_z);
         let v_y = v_z.cross(v_x);
 
-        Transform {
-            basis: Basis::from_elements([v_x, v_y, v_z]).transposed(),
+        Self {
+            basis: Basis::from_rows(v_x, v_y, v_z).transposed(),
             origin: self.origin,
         }
     }
+
+    #[doc(hidden)]
+    #[inline]
+    pub fn sys(&self) -> *const sys::godot_transform {
+        unsafe {
+            std::mem::transmute::<*const Transform, *const sys::godot_transform>(self as *const _)
+        }
+    }
+
+    #[doc(hidden)]
+    #[inline]
+    pub fn from_sys(c: sys::godot_transform) -> Self {
+        unsafe { std::mem::transmute::<sys::godot_transform, Self>(c) }
+    }
 }
 
 impl Mul<Transform> for Transform {
     type Output = Transform;
 
     #[inline]
-    fn mul(self, rhs: Transform) -> Self::Output {
+    fn mul(self, rhs: Self) -> Self::Output {
         let origin = self.xform(rhs.origin);
         let basis = self.basis * rhs.basis;
-        Transform { origin, basis }
+        Self { origin, basis }
     }
 }