godot-core: builtin: reimplement Plane functions/methods #268

T4rmyn · 2023-05-09T20:14:58Z

Partly addresses issue #209 by re-implementing Plane's functions/methods.

I know there's a need to implement test cases for it by looking at the Rect2i commits, but I would like to know if this part is correct yet since I am an still an absolute beginner at this stuff. Please let me know of anything that is an issue.

Also, would implementing test cases for Plane be similar in implementation as the Rect2i ones?

GodotRust · 2023-05-09T20:21:41Z

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-268

Bromeon

Thanks a lot for this pull request. This is a great feature addition to the library! 😀

I know there's a need to implement test cases for it by looking at the Rect2i commits, but I would like to know if this part is correct yet since I am an still an absolute beginner at this stuff. Please let me know of anything that is an issue.

That's a great approach, being iterative 👍 you can definitely add some tests once we've sorted out other issues 🙂

Also, would implementing test cases for Plane be similar in implementation as the Rect2i ones?

Yes, can be similar; a good way is also to look at Godot tests or even adopt those.

Many documentations start with a longer sentence, sometimes 3 and more lines. For API docs, it would be great to have an additional brief description, which is to the point and very concise. For example:

    /// Returns the intersection point of a segment from position `from` to position `to` with the 
    /// `self` `Plane` enclosed in `Some`. If no intersection is found, `None` will be returned.
    ///
    /// _Godot equivalent: `Plane.intersects_segment(Vector3 from, Vector3 to)`_
    #[inline]
    pub fn intersects_segment(&self, from: Vector3, to: Vector3) -> Option<Vector3> {

could become:

    /// Finds intersections of the plane with a line segment.
    ///
    /// Returns the intersection point of a segment from position `from` to position `to` with the 
    /// `self` `Plane` enclosed in `Some`. If no intersection is found, `None` will be returned.
    ///
    /// _Godot equivalent: `Plane.intersects_segment(Vector3 from, Vector3 to)`_
    #[inline]
    pub fn intersects_segment(&self, from: Vector3, to: Vector3) -> Option<Vector3> {

Bromeon · 2023-05-09T20:18:09Z

godot-core/src/builtin/plane.rs

+    /// The plane's normal component.
    pub normal: Vector3,
+    /// The plane's d component.
    pub d: real,


The current documentation has not more information than the field names -- could you instead write what they mean?

Doesn't need to be long, something like "normal vector pointing away from the plane" is enough.

godot-core/src/builtin/plane.rs

Bromeon · 2023-05-09T20:30:30Z

godot-core/src/builtin/plane.rs

+    /// Returns a normalized copy of the `self` `Plane`.
+    ///
+    /// _Godot equivalent: `Plane.normalized()`_
+    #[inline]
+    pub fn normalized(&self) -> Self {
+	    self.normal.normalized();
+        self.clone()
+    }


This implementation is just returning self.clone().

The past-tense also indicates that a copy is generated (rather than mutating the object directly), so self.normal.normalized() has no effect.

You can use the #[must_use] attribute here, to make sure the caller gets a warning if they don't use the result.

godot-core/src/builtin/plane.rs

Bromeon · 2023-05-09T21:05:17Z

Regarding CI failure, the godot-itest one is unrelated to this PR -- looks like Godot broke something and our nightly builds no longer work.

To see the API docs of your PR, check the comment from our bot. The docs will be updated every time you push (with a few minutes latency).

godot-core/src/builtin/plane.rs

T4rmyn · 2023-05-10T06:52:16Z

Alright, thank you very much for the inputs! I will try to resolve them one by one.

T4rmyn · 2023-05-11T15:27:34Z

Alright, this update (hopefully) addresses many of the issues pointed out with the previous version of the commit. Again, let me know of any more issues in this one!

lilizoey

This seems pretty good in general so far to me! Just got some nitpicks here and there.

Also it seems like you're using mixed tabs and spaces in some places? I feel like rustfmt should be able to fix that? So run that if you haven't. And if rustfmt doesn't catch it then please manually fix it (turn the tabs into spaces).

lilizoey · 2023-05-12T16:29:15Z

godot-core/src/builtin/plane.rs

-use super::{is_equal_approx, real, Vector3};
+use super::{is_zero_approx, is_equal_approx, real, Vector3};
+
+pub const CMP_EPSILON: real = 0.00001;


This already exists in builtin/math.rs

lilizoey · 2023-05-12T16:46:06Z

godot-core/src/builtin/plane.rs

+
+    /// Projects a point orthogonally to the plane.
+    ///
+    /// Returns a orthogonal of the variable `point` into a point in the `self` `Plane`.


This is not very clearly worded. Returns (an) orthogonal what? a point? points cant really be orthogonal to other points.

lilizoey · 2023-05-12T16:50:35Z

godot-core/src/builtin/plane.rs

+        assert_eq!(plane_a.is_equal_approx(&plane_e), false);
+        // Both attributes are approximately equal.
+        assert!(plane_a.is_equal_approx(&plane_f));


you should be able to use assert_eq_approx/assert_ne_approx here something like

Suggested change

assert_eq!(plane_a.is_equal_approx(&plane_e), false);

// Both attributes are approximately equal.

assert!(plane_a.is_equal_approx(&plane_f));

assert_ne_approx!(&plane_a, &plane_e, Plane::is_equal_approx);

// Both attributes are approximately equal.

assert_eq_approx!(&plane_a, &plane_f, Plane::is_equal_approx);

lilizoey · 2023-05-12T16:50:59Z

godot-core/src/builtin/plane.rs

+    /// Finds whether the plane is finite or not.
+    ///
+    /// Returns `true` if the `self` `Plane` is finite by calling `is_finite` on `normal` and `d`. 


The first sentence here seems a bit redundant to me. is_finite already communicates that this function finds whether the plane is finite or not. I think you can just cut out the first sentence. Same for is_point_over.

lilizoey · 2023-05-12T16:51:17Z

godot-core/src/builtin/plane.rs

+    /// Creates a normalized copy of the plane.
+    ///
+    /// Returns a normalized copy of the `self` `Plane`.
+    #[inline]
+    pub fn normalized(&self) -> Self {


In the docs here you're just repeating yourself. Also this function can take self instead of &self.

T4rmyn · 2023-05-12T17:17:54Z

Gotcha, thanks for reviewing! I'll get these issues ironed out.

T4rmyn · 2023-05-13T10:03:34Z

So, this latest commit should addresses all the previous issues and action checks (besides from the integration tests I assume) here.

If there's no more issues here, I'll move on to the integration tests.

Bromeon

Thanks a lot for the update! 🙂

I have added a few more comments, some related to concise documentation and some to testing.

Bromeon · 2023-05-14T11:16:50Z

godot-core/src/builtin/plane.rs

+    /// Returns `true` if the specified `Vector3` is inside the `self` `Plane`, will return otherwise
+    /// if not. Tolerance of the function unless specified will be using the default value of 1e-05.


Documentation, if provided, should contain more information than the method name itself. Otherwise it should be omitted.

In this case, you already mentioned twice that an "inside check" is happening: through the name contains_point, and the through the brief description "Finds whether a point is inside the plane or not."

So, instead of repeating the same thing a 3rd time, you should directly focus on the important part, i.e. the tolerance.

Also, don't mention the value of the tolerance here; the value may change over time and the docs become obsolete. Instead, since CMP_EPSILON is a public symbol, you can refer to it via intra-doc link.

Suggested change

/// Returns `true` if the specified `Vector3` is inside the `self` `Plane`, will return otherwise

/// if not. Tolerance of the function unless specified will be using the default value of 1e-05.

/// A point is considered part of the plane if its distance to it is less or equal than

/// [`CMP_EPSILON`][crate::builtin::CMP_EPSILON].

Bromeon · 2023-05-14T11:18:44Z

godot-core/src/builtin/plane.rs

+    /// Finds the intersection point of three planes.
+    ///
+    /// Returns the intersection point of the `Plane`s of the current `Plane`, `b`, and `c` enclosed
+    /// in `Some`. If no intersection point is found, `None` will be returned.
+    #[inline]
+    pub fn intersect_3(&self, b: &Self, c: &Self) -> Option<Vector3> {


Also here, no need to repeat or mention what is obvious from the signature.

Suggested change

/// Finds the intersection point of three planes.

///

/// Returns the intersection point of the `Plane`s of the current `Plane`, `b`, and `c` enclosed

/// in `Some`. If no intersection point is found, `None` will be returned.

#[inline]

pub fn intersect_3(&self, b: &Self, c: &Self) -> Option<Vector3> {

/// Finds the intersection point of three planes.

///

/// If no intersection point is found, `None` will be returned.

#[inline]

pub fn intersect_3(&self, b: &Self, c: &Self) -> Option<Vector3> {

Bromeon · 2023-05-14T11:24:12Z

godot-core/src/builtin/plane.rs

+    /// Finds the intersection point of the plane with a ray.
+    ///
+    /// Returns the intersection point of a ray consisting of the position `from` and the direction
+    /// normal `dir` with the `self` `Plane` enclosed in `Some`. If no intersection is found, `None`
+    /// will be returned.
+    #[inline]
+    pub fn intersects_ray(&self, from: Vector3, dir: Vector3) -> Option<Vector3> {


"direction normal" -> "direction normal"

Name should also be intersect_ray without "s" 🙂

Also a bit shorter sentences for easier understanding. The "enclosed in Some" is implied. Also added a bit of geometrical context.

Suggested change

/// Finds the intersection point of the plane with a ray.

///

/// Returns the intersection point of a ray consisting of the position `from` and the direction

/// normal `dir` with the `self` `Plane` enclosed in `Some`. If no intersection is found, `None`

/// will be returned.

#[inline]

pub fn intersects_ray(&self, from: Vector3, dir: Vector3) -> Option<Vector3> {

/// Finds the intersection point of the plane with a ray.

///

/// The ray starts at position `from` and has direction vector `dir`, i.e. it is unbounded in one direction.

///

/// If no intersection is found (the ray is parallel to the plane or points away from it), `None` will be returned.

#[inline]

pub fn intersect_ray(&self, from: Vector3, dir: Vector3) -> Option<Vector3> {

Bromeon · 2023-05-14T11:24:53Z

godot-core/src/builtin/plane.rs

+    /// Finds the intersection point of the plane with a line segment.
+    ///
+    /// Returns the intersection point of a segment from position `from` to position `to` with the
+    /// `self` `Plane` enclosed in `Some`. If no intersection is found, `None` will be returned.
+    #[inline]
+    pub fn intersects_segment(&self, from: Vector3, to: Vector3) -> Option<Vector3> {


Maybe similar structure as suggested above. Could be worth mentioning that a line segment is bounded in both directions.

Also: intersect_segment.

Bromeon · 2023-05-14T11:26:55Z

godot-core/src/builtin/plane.rs

    #[inline]
    pub fn is_equal_approx(&self, other: &Self) -> bool {
        (self.normal.is_equal_approx(other.normal) && is_equal_approx(self.d, other.d))
            || (self.normal.is_equal_approx(-other.normal) && is_equal_approx(self.d, -other.d))
    }

+    /// Returns `true` if the `self` `Plane` is finite by calling `is_finite` on `normal` and `d`.


I think it helps readability if you use "plane" instead of "Plane". Instead, the monospace style should be used for other symbols, like involved parameters.

This applies to many of the documentations, a search&replace might do a lot.

Bromeon · 2023-05-14T11:59:27Z

godot-core/src/builtin/plane.rs

+        // From a point straight up the origin point, the distance from the origin to the intersection point
+        // formed by a slanted ray should follow the Pythagorean Theorem.
+        assert_eq!(
+            plane.intersects_ray(vec_c, vec_e - vec_c).unwrap().length(),
+            (vec_c.distance_squared_to(vec_e) - vec_c.length_squared()).sqrt()
+        );
+        assert_eq!(
+            plane.intersects_ray(vec_b, vec_e - vec_b).unwrap().length(),
+            (vec_b.distance_squared_to(vec_e) - vec_b.length_squared()).sqrt()
+        );


What is the rationale for testing derived properties?

We're testing our implementation, not geometry... So I wonder if it weren't more straightforward to directly check the outcome of intersects_ray, like you did above.

Well, I thought it would be useful to check for mathematically accurate results for the implementations too since it's a mathematical object 😅, but I can see why not comparing straight from the results of intersect_ray (already removed the s) would be problematic for just testing for implementation.

Bromeon · 2023-05-14T12:01:07Z

godot-core/src/builtin/plane.rs

+        // From a point straight up the origin point, the distance from the origin to the intersection point
+        // formed by a slanted segment should follow the Pythagorean Theorem only if the segment ended on or
+        // beyond the `plane`.
+        assert_eq!(
+            plane.intersects_segment(vec_c, vec_f).unwrap().length(),
+            (vec_c.distance_squared_to(vec_f) - vec_c.length_squared()).sqrt()
+        );
+        assert_eq!(
+            plane.intersects_segment(vec_b, vec_f).unwrap().length(),
+            (vec_b.distance_squared_to(vec_f) - vec_b.length_squared()).sqrt()
+        );


See above; these might not be necessary

Bromeon · 2023-05-14T12:02:30Z

godot-core/src/builtin/plane.rs

+        // Same planes should be equals.
+        assert_eq_approx!(&plane_a, &plane_a, Plane::is_equal_approx);
+        assert_eq_approx!(&plane_b, &plane_b, Plane::is_equal_approx);
+        // Although similar, planes below shouldn't be approximately equals.
+        assert_ne_approx!(&plane_a, &plane_b, Plane::is_equal_approx);
+        assert_ne_approx!(&plane_a, &plane_c, Plane::is_equal_approx);
+        // Planes below should be approximately equal because it's lower than the set tolerance constant.
+        assert_eq_approx!(&plane_a, &plane_d, Plane::is_equal_approx);
+        // Although approximately equal in the `normal` part, it is not approximately equal in the `d`
+        // part.
+        assert_ne_approx!(&plane_a, &plane_e, Plane::is_equal_approx);
+        // Both attributes are approximately equal.
+        assert_eq_approx!(&plane_a, &plane_f, Plane::is_equal_approx);
+        // Although considered approximately equal with `plane_a`, `plane_b` is not considered approximately
+        // equal with `plane_e` because the baseline comparison is tighter.
+        assert_ne_approx!(&plane_b, &plane_e, Plane::is_equal_approx);


Really appreciate the comments explaining why or why not approximate equality applies! 👍

Could you group them to have first all the equality checks, then the non-equality ones? Also please add an empty line before a new // comment block, to make it immediately clear that a comment refers to the code below it.

Bromeon · 2023-05-14T12:05:03Z

godot-core/src/builtin/plane.rs

+        let mut plane_a: Plane = Plane::new(Vector3::new(0.7, 2.0, 6.0).normalized(), 0.0);
+        let mut plane_b: Plane = Plane::new(Vector3::new(1.5, 7.2, 9.1).normalized(), 2.0);
+        let mut plane_c: Plane = Plane::new(Vector3::new(1.4, 9.1, 1.2).normalized(), 5.3);
+        let mut plane_d: Plane = Plane::new(Vector3::new(4.2, 2.9, 1.5).normalized(), 2.4);
+        let plane_e: Plane = Plane::new(Vector3::new(5.1, 3.0, 2.1).normalized(), 0.2);
+
+        // Denormalize planes.
+        plane_a.normal = Vector3::new(0.7, 2.0, 6.0);
+        plane_b.normal = Vector3::new(1.5, 7.2, 9.1);
+        plane_c.normal = Vector3::new(1.4, 9.1, 1.2);
+        plane_d.normal = Vector3::new(4.2, 2.9, 1.5);


The normals above are overwritten without ever being used.

Why not just create the planes with the struct syntax Plane { normal: ..., d: ... }?
Needs half the code and avoids mut as well.

Bromeon · 2023-05-14T12:08:15Z

godot-core/src/builtin/plane.rs

+        // Initial ordinary planes.
+        let mut plane_a: Plane = Plane::new(Vector3::new(0.7, 2.0, -6.0).normalized(), 10.2);
+        let mut plane_b: Plane = Plane::new(Vector3::new(1.5, 7.2, 9.1).normalized(), 20.0);
+        let mut plane_c: Plane = Plane::new(Vector3::new(1.4, 9.1, 1.2).normalized(), 50.3);
+        let mut plane_d: Plane = Plane::new(Vector3::new(-4.2, 2.9, 1.5).normalized(), 20.4);
+        let plane_e: Plane = Plane::new(Vector3::new(7.2, -2.9, 2.2).normalized(), 3.3);
+
+        // Test with `normal`s.
+        plane_a.normal = Vector3::new(0.7, real::INFINITY, -6.0);
+        plane_b.normal = Vector3::new(0.7, 2.0, real::NEG_INFINITY);
+        plane_d.normal = Vector3::new(real::NAN, real::INFINITY, real::NEG_INFINITY);
+
+        // Test with `d`s.
+        plane_c.d = real::INFINITY;
+        plane_d.d = real::NAN;


As above, directly initialize the planes with their final values.

It would even make sense to have initialization and test statements together, for clarity.
Just reuse the same variable name:

let plane = Plane { normal: Vector3::new(0.7, real::INFINITY, -6.0), d: 10.2 }; assert!(!plane.is_finite()); let plane = Plane { normal: Vector3::new(0.7, 2.0, real::NEG_INFINITY), d: 10.2 }; assert!(!plane.is_finite()); ...

Bromeon · 2023-05-14T12:09:43Z

godot-core/src/builtin/plane.rs

@@ -9,24 +9,26 @@ use std::ops::Neg;
 use godot_ffi as sys;
 use sys::{ffi_methods, GodotFfi};

-use super::{is_equal_approx, real, Vector3};
+use crate::builtin::math::*;


Please avoid glob imports.

T4rmyn · 2023-05-14T14:04:14Z

Thanks again! I'll try resolve these issues.

T4rmyn · 2023-05-22T17:03:47Z

Alright, another round of review requested. With the names, some of them are a bit hard to write expressively, since some of them are quite hard to do while being short.

Also, what's up with the workflows thing?

Bromeon · 2023-05-22T18:18:34Z

Also, what's up with the workflows thing?

GDExtension upstream breakage. I'm on it, and while doing so, improving 4.0 <-> 4.1.dev compatibility 🙂

Bromeon · 2023-05-24T18:50:33Z

bors try

Bromeon · 2023-05-24T19:32:37Z

bors p=6

bors · 2023-05-24T20:12:04Z

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

full-ci

Bromeon · 2023-05-24T20:13:24Z

bors r+

bors · 2023-05-24T20:24:48Z

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

full-ci

Bromeon · 2023-05-24T20:32:14Z

Thanks a lot! 🚀

T4rmyn · 2023-05-25T00:03:47Z

Thanks for merging! If necessary, I'll put the integration tests in a separate pr.

Bromeon · 2023-05-25T00:17:36Z

If you have the time, that would be nice, no rush though 🙂

295: Implementation of Plane integration tests r=Bromeon a=T4rmin Adds integration tests to the re-implemented Plane functions (#268, and fixing a function in it) and should also address the Plane section of issue #209. This PR is also a recreation of #286 since it has the great error of force-pushing after rebasing. Co-authored-by: Aaron Lawrence <[email protected]>

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

Bromeon reviewed May 9, 2023

View reviewed changes

godot-core/src/builtin/plane.rs Outdated Show resolved Hide resolved

Bromeon reviewed May 9, 2023

View reviewed changes

godot-core/src/builtin/plane.rs Outdated Show resolved Hide resolved

T4rmyn force-pushed the reimplement_plane branch from 949ac64 to 148fd29 Compare May 11, 2023 15:22

T4rmyn requested a review from Bromeon May 11, 2023 15:25

lilizoey reviewed May 12, 2023

View reviewed changes

T4rmyn force-pushed the reimplement_plane branch 4 times, most recently from 1a1c86f to 3ca4fe6 Compare May 13, 2023 09:58

T4rmyn requested a review from lilizoey May 13, 2023 10:10

Bromeon reviewed May 14, 2023

View reviewed changes

lilizoey mentioned this pull request May 15, 2023

Reimplement Rect2, Rect2i, Aabb, and Plane in rust #209

Closed

4 tasks

godot-core: builtin: reimplement Plane functions/methods

332b750

T4rmyn force-pushed the reimplement_plane branch from 3ca4fe6 to 332b750 Compare May 22, 2023 16:47

T4rmyn requested a review from Bromeon May 22, 2023 17:13

bors bot added a commit that referenced this pull request May 24, 2023

Try #268:

b561db7

bors bot merged commit bc24100 into godot-rust:master May 24, 2023

T4rmyn deleted the reimplement_plane branch May 24, 2023 23:56

T4rmyn mentioned this pull request May 27, 2023

Implementation of Plane integration tests #286

Closed

T4rmyn mentioned this pull request Jun 4, 2023

Implementation of Plane integration tests #295

Merged

lilizoey mentioned this pull request Jun 14, 2023

Tracking issue for builtin feature-completeness #310

Open

33 tasks

		/// Returns `true` if the specified `Vector3` is inside the `self` `Plane`, will return otherwise
		/// if not. Tolerance of the function unless specified will be using the default value of 1e-05.

godot-core: builtin: reimplement Plane functions/methods #268

godot-core: builtin: reimplement Plane functions/methods #268

Conversation

T4rmyn commented May 9, 2023

GodotRust commented May 9, 2023

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

Bromeon commented May 9, 2023

T4rmyn commented May 10, 2023

T4rmyn commented May 11, 2023

lilizoey 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

T4rmyn commented May 12, 2023

T4rmyn commented May 13, 2023

Bromeon left a comment

Choose a reason for hiding this comment

Bromeon May 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon May 14, 2023 • edited Loading

Choose a reason for hiding this comment

Bromeon May 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

T4rmyn May 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon May 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

T4rmyn commented May 14, 2023

T4rmyn commented May 22, 2023 • edited Loading

Bromeon commented May 22, 2023

Bromeon commented May 24, 2023

Bromeon commented May 24, 2023

bors bot commented May 24, 2023

try

Bromeon commented May 24, 2023

bors bot commented May 24, 2023

Bromeon commented May 24, 2023

T4rmyn commented May 25, 2023

Bromeon commented May 25, 2023

Bromeon May 14, 2023 •

edited

Loading

Bromeon May 14, 2023 •

edited

Loading

Bromeon May 14, 2023 •

edited

Loading

T4rmyn May 15, 2023 •

edited

Loading

Bromeon May 14, 2023 •

edited

Loading

T4rmyn commented May 22, 2023 •

edited

Loading