Add basic impls of `Rect2`, `Rect2i`, `Aabb`, `Plane` #180

lilizoey · 2023-03-14T18:19:45Z

adds basic impls of Rect2, Rect2i, Aabb, Plane, so that every function/operator is usable (though many through InnerX)
adds Mul<X> impls for Transform2/3D for the new types
adds min/max functions for Vector2/3/4

i did not add Mul<Transform2/3D> for these types because they are actually kinda odd in how they work in godot. We could consider adding them later but it seems there are some outstanding issues in godot related to them (such as godotengine/godot#71035) so it'd probably be good to wait and see if anything is changing there.

min/max is there mainly to make the implementation of Transform2D * Rect2 and Transform3D * Aabb cleaner. But are convenient functions to have in general.

Bromeon · 2023-03-14T18:26:58Z

godot-core/src/builtin/aabb.rs

+
+use super::Vector3;
+
+/// Axis-aligned bounding box.


Maybe "Axis-aligned bounding box in 3D space"?

This is copied straight from the gdnative docs on Aabb so could be worth changing there too in that case

Bromeon · 2023-03-14T18:31:29Z

godot-core/src/builtin/plane.rs

+    /// Create a new `Plane` through the origin from a normal.
+    ///
+    /// # Panics
+    /// See [`Self::new()`].
+    ///
+    /// _Godot equivalent: `Plane(Vector3 normal)`_
+    #[inline]
+    pub fn new_origin(normal: Vector3) -> Self {
+        Self::new(normal, 0.0)
+    }
+
+    /// Create a new `Plane` from normal and a point in the plane.
+    ///
+    /// # Panics
+    /// See [`Self::new()`].
+    ///
+    /// _Godot equivalent: `Plane(Vector3 normal, Vector3 point)`_
+    #[inline]
+    pub fn new_normal_point(normal: Vector3, point: Vector3) -> Self {
+        Self::new(normal, normal.dot(point))
+    }


I'm thinking about the names here, maybe:

from_normal_at_origin from_normal_point

?

To me it's also quite unintuitive that Godot lists normal first, then position.
We can gladly flip the parameter order here -> from_point_normal.

Bromeon · 2023-03-14T18:32:27Z

godot-core/src/builtin/plane.rs

+    /// _Godot equivalent: `Plane(Vector3 point1, Vector3 point2, Vector3 point3)`_
+    #[inline]
+    pub fn from_points(a: Vector3, b: Vector3, c: Vector3) -> Option<Self> {
+        let normal = (a - c).cross(a - b).normalized();


Is normalized() necessary for the correctness of is_finite() below?
Otherwise, I would omit it.

it is yeah, the cross product will simply be 0. either way we want to create a plane that is usable without crashing at the end here so we'd need to normalize the normal eventually. all the other constructors disallow creating a non-normalized one.

as for why this one uses option and the others panic, i just followed what the gdnative implementation did.

either way we want to create a plane that is usable without crashing at the end here so we'd need to normalize the normal eventually

Yes, but then this could be done only in the Some case.

as for why this one uses option and the others panic, i just followed what the gdnative implementation did.

Good point -- it probably makes sense to align the behavior.

I would probably tend to panic and add a # Panic section, because:

the user most likely just uses unwrap() otherwise, which has a worse error message

this class has just "weak invariants" due to public fields, so we should focus on detecting bugs and not get in the user's way

it's always possible to add try_from_points later, if truly needed

Bromeon · 2023-03-14T18:34:14Z

godot-core/src/builtin/plane.rs

+impl Neg for Plane {
+    type Output = Plane;
+
+    fn neg(self) -> Self::Output {
+        Self::new(-self.normal, -self.d)
+    }
+}


Could you please quickly document the geometric meaning of this operation? 🙂

godot-core/src/builtin/plane.rs

Bromeon · 2023-03-14T18:38:22Z

godot-core/src/builtin/transform2d.rs

+impl Mul<Rect2> for Transform2D {
+    type Output = Rect2;
+
+    fn mul(self, rhs: Rect2) -> Self::Output {
+        // https://web.archive.org/web/20220317024830/https://dev.theomader.com/transform-bounding-boxes/
+        let xa = self.a * rhs.position.x;
+        let xb = self.a * rhs.end().x;
+
+        let ya = self.b * rhs.position.y;
+        let yb = self.b * rhs.end().y;
+
+        let position = Vector2::min(xa, xb) + Vector2::min(ya, yb) + self.origin;
+        let end = Vector2::max(xa, xb) + Vector2::max(ya, yb) + self.origin;
+        Rect2::new(position, end - position)
+    }
+}


Maybe quickly describe how this works for non-obvious cases, e.g. rotation. It's probably enough to mention that the resulting bounding-box includes all points transformed individually.

Bromeon

A few more thoughts 🙂

Bromeon · 2023-03-20T17:59:36Z

godot-core/src/builtin/aabb.rs

+    /// The end of the `Aabb` calculated as `position + size`.
+    ///
+    /// _Godot equivalent: `Aabb.size`_
+    #[inline]
+    pub fn end(&self) -> Vector3 {
+        self.position + self.size
+    }
+
+    /// Set size based on desired end-point.
+    ///
+    /// _Godot equivalent: `Aabb.size`_
+    #[inline]
+    pub fn set_end(&mut self, end: Vector3) {
+        self.size = end - self.position
+    }


Godot equivalents should be:

Aabb.end property

Bromeon · 2023-03-20T18:08:54Z

godot-core/src/builtin/plane.rs

+/// 3D plane in Hessian form: `a*b + b*y + c*z + d = 0`
+///
+/// Currently most methods are only available through [`InnerPlane`](super::inner::InnerPlane).
+///
+/// Note: almost all methods on `Plane` require that the `normal` vector have
+/// unit length and will panic if this invariant is violated. This is not separately
+/// annotated for each method.
+#[derive(Copy, Clone, PartialEq, Debug)]
+#[repr(C)]
+pub struct Plane {
+    pub normal: Vector3,
+    pub d: real,
+}


The first line is confusing, because the Hessian form is precisely not the equation a*b + b*y + c*z + d = 0.
(yes, it's wrong in gdnative too).

From Wolfram:

The Hessian form is the representation via normal and distance (which is what we do here).
The plane equation involving a, b, c, d is another representation.

So I would write instead:

/// 3D plane in [Hessian normal form](https://mathworld.wolfram.com/HessianNormalForm.html). /// /// The Hessian form defines all points `point` which satisfy the equation /// `dot(normal, point) + d == 0`, where `normal` is the normal vector and `d` /// the distance from the origin.

Bromeon · 2023-03-20T18:16:30Z

godot-core/src/builtin/plane.rs

+    ///
+    /// _Godot equivalent: `Plane(Vector3 normal, float d)`_
+    #[inline]
+    pub fn new(normal: Vector3, d: real) -> Self {


Maybe name the parameter unit_normal, so IDE suggestions help understand it must be unit length.

Bromeon · 2023-03-20T18:42:14Z

godot-core/src/builtin/plane.rs

+    /// Creates a new `Plane` from normal and origin distance.
+    ///
+    /// `a`, `b`, `c` are used for the `normal` vector.
+    /// `d` is the distance from the origin.
+    ///
+    /// # Panics
+    /// See [`Self::new()`].
+    ///
+    /// _Godot equivalent: `Plane(float a, float b, float c, float d)`_
+    #[inline]
+    pub fn from_coordinates(a: real, b: real, c: real, d: real) -> Self {
+        Self::new(Vector3::new(a, b, c), d)
+    }


Given the above discussion, this can easily be mistaken as a constructor for a*b + b*y + c*z + d = 0.

I would suggest:

pub fn from_coordinates(nx: real, ny: real, nz: real, d: real) -> Self

I was also thinking about the name, but I'm not sure here... from_normal_coords_d isn't exactly great...

Maybe better from_components than from_coordinates? This would be analogous to rectangles, and avoids a typical association of "coordinates" with "points/vectors in space".

Bromeon · 2023-03-20T18:44:28Z

godot-core/src/builtin/plane.rs

+        assert!(
+            normal != Vector3::ZERO,
+            "The points {a}, {b}, {c} are all colinear"
+        );


Is clippy not complaining about using assert_ne! here?

It's very weird that we get such flaky behavior, there were lots of cases where this was rejected...

oh huh yeah that's weird that it didn't complain

Bromeon · 2023-03-20T18:54:36Z

godot-core/src/builtin/plane.rs

+    /// Create a new `Plane` from a normal and a point in the plane.
+    ///
+    /// # Panics
+    /// See [`Self::new()`].
+    ///
+    /// _Godot equivalent: `Plane(Vector3 normal, Vector3 point)`_
+    #[inline]
+    pub fn from_point_normal(point: Vector3, normal: Vector3) -> Self {
+        Self::new(normal, normal.dot(point))
+    }


Btw, I think I now understood why Godot begins with normal -- because all the constructors do, and normal (as opposed to point) is a direct member/property of Plane.

So I'm not sure which is better... It's probably fine as it is now.

Bromeon · 2023-03-20T19:12:17Z

godot-core/src/builtin/transform2d.rs

+
+        let position = Vector2::min(xa, xb) + Vector2::min(ya, yb) + self.origin;
+        let end = Vector2::max(xa, xb) + Vector2::max(ya, yb) + self.origin;
+        Rect2::new(position, end - position)


What do you think, would begin/end constructors make sense?

Rect2::from_corners(position, end) Aabb::from_corners(position, end)

probably yeah tbh

what should the argument names be?
position, end
start, end
top_left, bottom_right
something else?

I would probably stay with position/end, because that's also the name of the corresponding fields or getters/setters.

Bromeon · 2023-03-20T19:26:48Z

itest/rust/src/transform2d_test.rs

+            .unwrap()
+            .to::<Rect2>(),
+        |a, b| Rect2::is_equal_approx(&a, &b),
+        "operator: Transform2D * Rect2 1"


maybe the final "1" could be in parentheses "(1)" to separate it from the types

Bromeon · 2023-03-20T19:28:29Z

bors try

bors · 2023-03-20T19:45:18Z

try

Build succeeded:

full-ci

ttencate · 2023-03-23T08:37:19Z

Whoopee! Could you maybe uncomment these while you're at it?

gdext/godot-core/src/builtin/mod.rs

Lines 355 to 373 in eb30a8b

    
           // impl_export_by_clone!(Aabb); // TODO uncomment once Aabb implements Clone 
        
           impl_export_by_clone!(Basis); 
        
           impl_export_by_clone!(Color); 
        
           impl_export_by_clone!(GodotString); 
        
           impl_export_by_clone!(NodePath); 
        
           impl_export_by_clone!(PackedByteArray); 
        
           impl_export_by_clone!(PackedColorArray); 
        
           impl_export_by_clone!(PackedFloat32Array); 
        
           impl_export_by_clone!(PackedFloat64Array); 
        
           impl_export_by_clone!(PackedInt32Array); 
        
           impl_export_by_clone!(PackedInt64Array); 
        
           impl_export_by_clone!(PackedStringArray); 
        
           impl_export_by_clone!(PackedVector2Array); 
        
           impl_export_by_clone!(PackedVector3Array); 
        
           // impl_export_by_clone!(Plane); // TODO uncomment once Plane implements Clone 
        
           impl_export_by_clone!(Projection); 
        
           impl_export_by_clone!(Quaternion); 
        
           // impl_export_by_clone!(Rect2); // TODO uncomment once Rect2 implements Clone 
        
           // impl_export_by_clone!(Rect2i); // TODO uncomment once Rect2i implements Clone

Bromeon · 2023-03-27T07:09:47Z

godot-core/src/builtin/plane.rs

+        assert_ne!(
+            normal,
+            Vector3::ZERO,
+            "The points {a}, {b}, {c} are all colinear"


Small nitpick regarding panic messages: they should not start capitalized (and not end with period), and feel free to omit any "boilerplate" that doesn't carry information, to have concise, keyword-style messages. That's e.g. what serde recommends.

"points {a}, {b}, {c} are all colinear"

Also in a few other places.

Bromeon · 2023-03-27T07:12:32Z

itest/rust/src/transform3d_test.rs

+    assert_eq_approx!(
+        TEST_TRANSFORM * vec,
+        TEST_TRANSFORM
+            .to_variant()
+            .evaluate(&vec.to_variant(), VariantOperator::Multiply)
+            .unwrap()
+            .to::<Vector3>(),
+        Vector3::is_equal_approx,
+        "operator: Transform3D * Vector3"
+    );
+
+    let aabb = Aabb::new(Vector3::new(1.0, 2.0, 3.0), Vector3::new(4.0, 5.0, 6.0));
+
+    assert_eq_approx!(
+        TEST_TRANSFORM * aabb,
+        TEST_TRANSFORM
+            .to_variant()
+            .evaluate(&aabb.to_variant(), VariantOperator::Multiply)
+            .unwrap()
+            .to::<Aabb>(),
+        |a, b| Aabb::is_equal_approx(&a, &b),
+        "operator: Transform3D * Aabb"
+    );
+
+    let plane = Plane::new(Vector3::new(1.0, 2.0, 3.0).normalized(), 5.0);
+
+    assert_eq_approx!(
+        TEST_TRANSFORM * plane,
+        TEST_TRANSFORM
+            .to_variant()
+            .evaluate(&plane.to_variant(), VariantOperator::Multiply)
+            .unwrap()
+            .to::<Plane>(),
+        |a, b| Plane::is_equal_approx(&a, &b),
+        "operator: Transform3D * Plane"
+    );


Is there a technical reason why the first function is passed as Vector3::is_equal_approx, but the others are clusres like |a, b| Aabb::is_equal_approx(&a, &b)?

cause the assert_eq_approx always passes by value, Aabb::is_equal_approx takes by reference and Vector3::is_equal_approx takes by value

Add `Mul<X>` impls for `Transform2/3D` for the new types Add `min/max` functions for `Vector2/3`

Bromeon

Thanks a lot, yet another great addition to builtin types! 🎉

bors r+

bors · 2023-03-28T11:27:48Z

Build succeeded:

full-ci

lilizoey force-pushed the feature/rect branch from 5968b52 to 3c8afe7 Compare March 14, 2023 18:21

Bromeon added feature Adds functionality to the library c: core Core components labels Mar 14, 2023

Bromeon reviewed Mar 14, 2023

View reviewed changes

lilizoey force-pushed the feature/rect branch 2 times, most recently from 3789037 to c82e566 Compare March 15, 2023 14:31

Bromeon reviewed Mar 20, 2023

View reviewed changes

bors bot added a commit that referenced this pull request Mar 20, 2023

Try #180:

fa8eb0c

lilizoey force-pushed the feature/rect branch 3 times, most recently from e1358bd to 5e999e3 Compare March 26, 2023 14:29

lilizoey requested a review from Bromeon March 26, 2023 14:29

lilizoey force-pushed the feature/rect branch from 5e999e3 to 4758f67 Compare March 26, 2023 14:51

Bromeon reviewed Mar 27, 2023

View reviewed changes

Add basic impls of Rect2, Rect2i, Aabb, Plane

00de6ac

Add `Mul<X>` impls for `Transform2/3D` for the new types Add `min/max` functions for `Vector2/3`

lilizoey force-pushed the feature/rect branch from 4758f67 to 00de6ac Compare March 27, 2023 22:42

Bromeon approved these changes Mar 28, 2023

View reviewed changes

bors bot merged commit 08c6594 into godot-rust:master Mar 28, 2023

lilizoey mentioned this pull request Apr 13, 2023

Project status overview #24

Open

lilizoey deleted the feature/rect branch April 16, 2023 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic impls of `Rect2`, `Rect2i`, `Aabb`, `Plane` #180

Add basic impls of `Rect2`, `Rect2i`, `Aabb`, `Plane` #180

lilizoey commented Mar 14, 2023

Bromeon Mar 14, 2023

lilizoey Mar 14, 2023

Bromeon Mar 14, 2023

Bromeon Mar 14, 2023 •

edited

Loading

Bromeon Mar 14, 2023

lilizoey Mar 14, 2023

Bromeon Mar 14, 2023

Bromeon Mar 14, 2023

Bromeon Mar 14, 2023

Bromeon left a comment

Bromeon Mar 20, 2023

Bromeon Mar 20, 2023

Bromeon Mar 20, 2023

Bromeon Mar 20, 2023

Bromeon Mar 20, 2023

lilizoey Mar 20, 2023

Bromeon Mar 20, 2023

Bromeon Mar 20, 2023

lilizoey Mar 24, 2023 •

edited

Loading

Bromeon Mar 25, 2023

Bromeon Mar 20, 2023

Bromeon commented Mar 20, 2023

bors bot commented Mar 20, 2023

ttencate commented Mar 23, 2023

Bromeon Mar 27, 2023 •

edited

Loading

Bromeon Mar 27, 2023

lilizoey Mar 27, 2023

Bromeon left a comment

bors bot commented Mar 28, 2023

Add basic impls of Rect2, Rect2i, Aabb, Plane #180

Add basic impls of Rect2, Rect2i, Aabb, Plane #180

Conversation

lilizoey commented Mar 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilizoey Mar 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon commented Mar 20, 2023

bors bot commented Mar 20, 2023

try

ttencate commented Mar 23, 2023

Bromeon Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon left a comment

Choose a reason for hiding this comment

bors bot commented Mar 28, 2023

Add basic impls of `Rect2`, `Rect2i`, `Aabb`, `Plane` #180

Add basic impls of `Rect2`, `Rect2i`, `Aabb`, `Plane` #180

Bromeon Mar 14, 2023 •

edited

Loading

lilizoey Mar 24, 2023 •

edited

Loading

Bromeon Mar 27, 2023 •

edited

Loading