Add basic impls of `Rect2`, `Rect2i`, `Aabb`, `Plane` by lilizoey · Pull Request #180 · godot-rust/gdext (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation32 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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.


use super::Vector3;

/// Axis-aligned bounding box.

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Comment on lines 43 to 70

/// 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))
}

Choose a reason for hiding this comment

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

I'm thinking about the names here, maybe:

from_normal_at_origin from_normal_point

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

Comment on lines 118 to 135

impl Neg for Plane {
type Output = Plane;

fn neg(self) -> Self::Output {
Self::new(-self.normal, -self.d)
}
}

Choose a reason for hiding this comment

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

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

Comment on lines 323 to 340

impl Mul 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)
}
}

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

A few more thoughts 🙂

Comment on lines 36 to 59

/// 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
}

Choose a reason for hiding this comment

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

Godot equivalents should be:

Aabb.end property

Comment on lines 14 to 30

/// 3D plane in Hessian form: `ab + by + 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,
}

Choose a reason for hiding this comment

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

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. /// /// 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.

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

Choose a reason for hiding this comment

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

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

Comment on lines 65 to 84

/// 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)
}

Choose a reason for hiding this comment

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

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".

Comment on lines 88 to 99

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

Choose a reason for hiding this comment

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

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...

Choose a reason for hiding this comment

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

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

Comment on lines +54 to +70

/// 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))
}

Choose a reason for hiding this comment

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

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.


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)

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

probably yeah tbh

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

Choose a reason for hiding this comment

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

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

.unwrap()
.to::(),
\|a, b
"operator: Transform2D * Rect2 1"

Choose a reason for hiding this comment

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

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

bors bot added a commit that referenced this pull request

Mar 20, 2023

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

// 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

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

Choose a reason for hiding this comment

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

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.

Comment on lines +52 to +87

assert_eq_approx!(
TEST_TRANSFORM * vec,
TEST_TRANSFORM
.to_variant()
.evaluate(&vec.to_variant(), VariantOperator::Multiply)
.unwrap()
.to::(),
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::(),
\|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::(),
\|a, b
"operator: Transform3D * Plane"
);

Choose a reason for hiding this comment

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

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)?

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

bors r+

Add basic impls of Rect2, Rect2i, Aabb, Plane by lilizoey · Pull Request #180 · godot-rust/gdext (original) (raw)

Conversation

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

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

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

Add basic impls of `Rect2`, `Rect2i`, `Aabb`, `Plane` by lilizoey · Pull Request #180 · godot-rust/gdext (original) (raw)