Implement methods on Rect2 and Aabb #867

parasyte · 2022-02-26T07:09:35Z

The docs are based on the Godot documentation for these types, and the code is based on the C++ that implements them.

Only documented methods are implemented here. This is especially notable for the Aabb type, which has many methods in C++ which are not documented for GDScript.

I also took some liberties with a few method names and types. For instance, Rect2::grow_margin takes an enum argument, and Aabb::get_endpoint returns Option<Vector3>. These are different from the GDScript counterparts. Aabb::get_area was renamed to Aabb::get_volume, etc.

Finally, I haven't added any tests for Aabb because I did that file last and got bored/lazy before writing any tests (this is what happens when you are not adamant about TDD). The Rect2 tests did help find plenty of minor bugs, so it is worthwhile to test Aabb.

Closes #864

parasyte · 2022-02-28T23:58:02Z

🎉 I added tests for the new Aabb methods. Didn't find any bugs this time. I was a bit surprised to see the ridiculous translation of intersects_segment() actually worked.

I think this leaves get_support() as the only untested method. According to the code, it should return the vertex closest to the given direction vector (with some precision loss) and get_endpoint() which is also trivial.

Bromeon

Thanks a lot for this great contribution! 🙂
Also thanks a lot for the added tests, this really helps.

Added comments in code.

Some general thoughts:

I would probably make the methods operate on self, not &self. We do it also for Quat (however, Plane is a bit inconsistent). I don't know where exactly we should have the "boundary", maybe the Transform, Basis etc. should keep using by-ref.
Methods which have an ambiguous name could use the #[must_use]. This makes sure the user doesn't think they are changing self. Examples:
- clip
- expand
- merge
- grow*
You noted that some operations on negative sizes are unreliable. I think is invites bugs and we should rather write assert! to check preconditions, wherever they are needed. This might need a # Panics section in the method documentation.
The get_longest_axis[_{index|size}] methods are duplicating a lot of code and force the user to recompute the same thing multiple times, if he is interested in more than one. They are also redundant:
- get_longest_axis_index() == size.max_axis()
- get_longest_axis_size() == size.as_ref().iter().max()
- get_longest_axis() == size.max_axis().to_unit_vector()
  (not yet existing, but something like this could be added)
Furthermore, it's not clear how these methods behave if two edges have the same length. User lower-level primitives (arrays/slices) makes this more explicit.

So either we remove them entirely, or find a good way to represent their API. But at the very least, we should not have 6 times the almost identical code.

bors try

Bromeon · 2022-03-01T09:35:18Z

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

+        let position = self.position
+            + Vector3::new(
+                self.size.x.min(0.0),
+                self.size.y.min(0.0),
+                self.size.z.min(0.0),
+            );


Could also be:

let position = self.position + self.size.glam().min(glam::Vec3A::ZERO)

(they have pub(super) visibility, if necessary can be extended to pub(crate)).

Bromeon · 2022-03-01T09:37:39Z

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

    pub size: Vector3,
 }

 impl Aabb {
+    /// Creates an `Aabb` by position and size.


This is implied by the constructor signature. Maybe instead mention that it's possible to have negative sizes.

Bromeon · 2022-03-01T09:48:07Z

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

+    /// Creates an `Aabb` by x, y, z, width, height, and depth.
+    #[inline]
+    pub fn from_components(x: f32, y: f32, z: f32, width: f32, height: f32, depth: f32) -> Self {
+        let position = Vector3::new(x, y, z);
+        let size = Vector3::new(width, height, depth);
+
+        Self { position, size }
+    }


Do we need this constructor? I'd generally tend to encourage the user to think in vectors rather than separate coordinates. It's typically also easier to understand than 6 numbers in an argument list.

Similar how we do it in Transform, Transform2 and Basis (but unlike Plane).

Bromeon · 2022-03-01T09:52:31Z

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

+    /// Note: This method is not reliable for bounding boxes with a negative size. Use
+    /// [`abs`][Self::abs] to get a positive sized equivalent box to check for contained points.
+    #[inline]
+    pub fn has_point(&self, point: Vector3) -> bool {


If we rename some methods already, this should be contains_point().

Also, if it's not reliable for negative sizes, we should panic in that case -- no point in letting the user run into bugs.

Bromeon · 2022-03-01T10:01:55Z

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

+    /// Returns the support point in a given direction. This is useful for collision detection
+    /// algorithms.


Maybe quickly explain what the support point is?

🤷 This one confused me. The only information I have about it is the not too useful GDScript documentation and the implementation itself. I was able to infer its behavior from code (enough to write a test) but I don't really "get" its purpose for existing. I'm happy to remove it!

Found an explanation here, or here.

Maybe something like:

Returns a point on the boundary of the AABB, which is the furthest in the given direction dir.
Mathematically, that corresponds to the point which maximizes its dot product with dir.

Interesting! Good find. The unfortunate part of this implementation on Aabb is that it is a support point on a bounding box, and not the actual shape itself. Perhaps it is relevant here due to the simplicity of a cube, but I imagine it's more useful on a convex hull.

Bromeon · 2022-03-01T10:45:37Z

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

+        let points = [
+            self.position,
+            self.position + Vector3::new(0.0, 0.0, self.size.z),
+            self.position + Vector3::new(0.0, self.size.y, 0.0),
+            self.position + Vector3::new(0.0, self.size.y, self.size.z),
+            self.position + Vector3::new(self.size.x, 0.0, 0.0),
+            self.position + Vector3::new(self.size.x, 0.0, self.size.z),
+            self.position + Vector3::new(self.size.x, self.size.y, 0.0),
+            self.position + self.size,
+        ];


Why not reuse the code?

let corners = [Vector::ZERO; 8]; for i in 0..8 { corner[i] = self.get_endpoint(i); }

If you find a more compact functional approach, that's also fine!

Funny enough, I was initially using get_endpoint here, but reneged and switched to duplication like the C++ does.

Bromeon · 2022-03-01T10:54:00Z

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

+        let from = from.as_ref().iter();
+        let to = to.as_ref().iter();
+        let begin = self.position.as_ref().iter();
+        let end = self.get_end();
+        let end = end.as_ref().iter();
+
+        for (((from, to), begin), end) in from.zip(to).zip(begin).zip(end) {


I'm not sure if we do ourselves a favor by using functional programming and iterators over components, instead of a simple 0..3 indexing.

I assume you had a reason to deviate from the Godot AABB::intersects_segment() implementation?

Possibly, a function

fn Vector3::get_coord(self, axis: Axis) -> f32

(or similarly named) could help here.
We could then index a vector component using a simple Vector3::get_coord(i.into()).

This code specifically went through a few iterations. This is how it started: 48e1d60#diff-77f6ed9f07c676ba6ec235e93f078192c45ed106f1b4619410f94ddcab499fd1R321-R392

Bromeon · 2022-03-01T10:56:47Z

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

+        if !self.intersects(b) {
+            return Self::default();
+        }


This method should return Option<Self>. We can then implement intersects() in terms of this.

Bromeon · 2022-03-01T11:05:33Z

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

+    /// Note: This method is not reliable for `Rect2` with a negative size. Use [`abs`][Self::abs]
+    /// to get a positive sized equivalent rectangle to check for contained points.
+    #[inline]
+    pub fn has_point(&self, point: Vector2) -> bool {


Same here: contains_point() + assert!(/* size is non-negative */)

Bromeon · 2022-03-01T11:08:14Z

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

+            && b.position.x + b.size.x <= self.position.x + self.size.x
+            && b.position.y + b.size.y <= self.position.y + self.size.y


could use get_end() here
also in a few of the following methods

bors · 2022-03-01T13:25:21Z

try

Build failed:

full-ci

parasyte · 2022-03-02T00:20:59Z

It seems I'm going to need a better understanding of how much you are willing to diverge from the Godot interface and implementation. I am aware of some preexisting inconsistencies between godot-rust and GDScript, but I don't know any of the history behind those decisions.

For instance, all of the warnings about inverted rectangles are emergent from the implementation, which is more or less a straight port of the C++. If you don't mind a compatibility hazard, we could always make improvements to either support inverted rectangles, or as you suggest not support them at all with an assertion and panic. I think I need to understand what exactly that balance between compatibility and "doing things better" is to have a meaningful conversation. My default is to point at the C++ to justify this code. That includes the documentation, code structure (De-Morgan was brought up), all of the duplication, etc.

So yeah, if you can let me know in general terms how much you are willing to break convention or compatibility, I'm all for making improvements above and beyond what Godot offers.

Bromeon · 2022-03-02T08:42:05Z

Keeping compatibility with Godot is a good general approach, however under these constraints:

In terms of semantics, not implementation -- example: if something can be done easier using underlying glam, let's do that. Of course, we can use the Godot implementation as a starting point and refine it in the future.
Improve error handling. GDScript is notorious for returning default values (and sometimes printing) whenever an error happens, making it very hard to the caller to detect what went wrong. Even worse are completely silent but unintuitive behaviors, which are places where I would use assert!.
Staying idiomatic with Rust. Our language is much more powerful than GDScript, meaning we can work with enums, tuples, Option<T>, Result<T, E>, and so on. Most APIs don't need to be fancy -- primarily those cases benefit, which are hard to express with GDScript's type system (like above-mentioned errors).

I guess you meant this specifically for the get_longest_axis*() variants? If you want, you can keep them as-is for now; let's maybe get this basic PR working, I can then also experiment a bit on my own. So let's focus on the other aspects 👍

parasyte · 2022-03-02T21:41:29Z

All of the items you provided answer many implied questions, so thank you for that. The get_...axis... methods are certainly within at least one of those questions. Broadly, I just wanted to know how much pressure there is to follow GDScript closely.

Some specifics and my opinion on what to do about them:

get_longest_axis, get_longest_axis_index, et al. These can return a tuple, or for more readable code a struct. Bikeshed on naming, but something like this:

pub struct AxisQueryResult {
    pub axis: Axis,
    pub size: Vector3,
}

The normal vector can be computed with size.normalized() if needed. We could also switch size to magnitude: f32 and implement methods on AxisQueryResult to compute the size and normal vectors if that makes more sense. In any case, returning a composite type of some sort means we can remove the 3x dupes for getting the longest and shortest axis on Aabb.
Returning default values is definitely surprising. Both Aabb::intersection and Rect2::clip return very different default values in the failure case. Option<T> would be better. Also I noted that the names of these methods are curious; they both do the same thing (shape intersection). Renaming Rect2::clip to Rect2::intersection seems like the right call in this case.
- I have found myself leaning on GDScript docs quite often while learning to use various classes. I think that existing tutorials are a major contributing force for this habit. Every time I consider breaking from the GDScript status quo, it makes me wonder how I would personally transfer knowledge from GDScript into godot-rust. The best I've come up with so far is the note in Aabb::get_volume; the math and semantics nerd in me disagrees that a cuboid has an "area", but it does have volume! Noting that this method is equivalent to GDScript's AABB.get_area() was my solution to this particular problem. I would love to hear other suggestions.
I can only speculate how often inverted shapes are actually used (and expected) in the wild. I was honestly surprised by the warning in the GDScript docs. Interestingly, if inverted shapes are disallowed, the abs method becomes even more important to avoid panics. It raises the question if we want to always call abs internally so that inverted shapes cannot be created in the first place? There is some overhead to that, so it's also perfectly reasonable to make the caller use abs when they need it.
I need to go over the CI failure(s), but the only issue might be the MSRV hazard using IntoIterator for array that was stabilized in 1.53.0.

Anyway, I think I have a better understanding what you're looking for. Even if my confusion is as broad as my questions.

Bromeon · 2022-03-02T22:25:09Z

Thanks for your elaborate answer!

get_longest_axis, get_longest_axis_index, et al. These can return a tuple, or for more readable code a struct. Bikeshed on naming, but something like this:
pub struct AxisQueryResult {
    pub axis: Axis,
    pub size: Vector3,
}

I think since this is only used really used in one place (with 6 variants), an extra type may not pay off.
What about returning a tuple?

let (vector, axis) = aabb.get_longest_axis();
let (vector, axis) = aabb.get_shortest_axis();

It's also easily possible to use (vector, _) if there is no interest in the axis.
I agree about normalize().

Returning default values is definitely surprising. Both Aabb::intersection and Rect2::clip return very different default values in the failure case. Option<T> would be better. Also I noted that the names of these methods are curious; they both do the same thing (shape intersection). Renaming Rect2::clip to Rect2::intersection seems like the right call in this case.

Good catch, and I agree with your conclusion. Godot 4 seems to have renamed it to intersection() as well (impl, doc).

I have found myself leaning on GDScript docs quite often while learning to use various classes. I think that existing tutorials are a major contributing force for this habit. Every time I consider breaking from the GDScript status quo, it makes me wonder how I would personally transfer knowledge from GDScript into godot-rust. The best I've come up with so far is the note in Aabb::get_volume; the math and semantics nerd in me disagrees that a cuboid has an "area", but it does have volume! Noting that this method is equivalent to GDScript's AABB.get_area() was my solution to this particular problem. I would love to hear other suggestions.

Fully agree -- and guess what, in Godot 4 it's also called get_volume() 😀

I can only speculate how often inverted shapes are actually used (and expected) in the wild. I was honestly surprised by the warning in the GDScript docs. Interestingly, if inverted shapes are disallowed, the abs method becomes even more important to avoid panics. It raises the question if we want to always call abs internally so that inverted shapes cannot be created in the first place? There is some overhead to that, so it's also perfectly reasonable to make the caller use abs when they need it.

This invariant is impossible to enforce with public fields. I think negative/inverted rectangles/AABBs are probably OK for certain use cases. The usage of certain methods on negative shapes however indicates usually a bug in my opinion -- if someone shows a valid use case in the future, we can allow it again. But I doubt that supporting negative shapes in this way was a conscious design choice on Godot's part.

I need to go over the CI failure(s), but the only issue might be the MSRV hazard using IntoIterator for array that was stabilized in 1.53.0.

Yes, it's a bit annoying, I already "fixed" this issue before. I might actually increase the MSRV (and maybe the Rust edition), otherwise we're stuck with this workaround for entire 0.10 -- incrementing MSRV is a breaking change. So don't change that part yet.

Bromeon

I updated the MSRV, let's see about the warnings now.
There are still a few suggestions open 🙂

bors try

Bromeon · 2022-03-06T22:56:13Z

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

+    pub fn get_support(&self, dir: Vector3) -> Vector3 {
+        let center = self.size * 0.5;
+        let offset = self.position + center;
+
+        Vector3::new(
+            if dir.x > 0.0 { -center.x } else { center.x },
+            if dir.y > 0.0 { -center.y } else { center.y },
+            if dir.z > 0.0 { -center.z } else { center.z },
+        ) + offset
+    }


The whole Godot implementation is strange. First, variables are badly named (center is not a position, offset is not a direction), second there's unnecessary back-and-forth.

With glam, can we not simply write this as the following?

pub fn get_support(&self, dir: Vector3) -> Vector3 { // for each component c: // c >= +0: self.position // c <= -0: self.position + self.size self.position + 0.5 * self.size * (1 - dir.glam().signum()) }

Might need to test it or at least double-check it, but the original impl seems quite confusing.
I'm not 100% sure about the border cases with signum() (positive/negative zero), but even with an explicit if, we can do better:

pub fn get_support(&self, dir: Vector3) -> Vector3 { self.position + Vector3::new( if dir.x > 0.0 { 0.0 } else { self.size.x }, if dir.y > 0.0 { 0.0 } else { self.size.y }, if dir.z > 0.0 { 0.0 } else { self.size.z }, ) }

Bromeon · 2022-03-06T23:00:00Z

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

+    /// Returns the support point in a given direction. This is useful for collision detection
+    /// algorithms.


Found an explanation here, or here.

Maybe something like:

Returns a point on the boundary of the AABB, which is the furthest in the given direction dir.
Mathematically, that corresponds to the point which maximizes its dot product with dir.

bors · 2022-03-06T23:20:34Z

try

Build succeeded:

full-ci

parasyte · 2022-03-07T00:34:08Z

There are still a few suggestions open 🙂

Yeah! I'm looking forward to getting back to this, soon. I just picked up a consulting gig last week and trying to wrap it up in the next few days. I agree with most (maybe all?) of the feedback you've provided so far, and I appreciate it.

Bromeon · 2022-04-13T13:11:38Z

@parasyte Have you had a chance to look into this again, or is there anything I can help with? 🙂

parasyte · 2022-04-16T00:30:57Z

In full disclosure, I'm just being lazy now. 😅 I'll try to find the motivation to pick it up again soon.

Bromeon · 2022-06-05T19:04:06Z

@parasyte Are you still interested in this? If you think you won't continue this PR, please let me know, I can gladly take over 🙂 but I'd like to clean up old PRs to avoid unnecessary code rot.

parasyte · 2022-06-10T20:55:51Z

@Bromeon You can take over on this. I have been focused on things that are very different.

… Quat, Plane etc.)

867: Implement methods on Rect2 and Aabb r=Bromeon a=parasyte The docs are based on the Godot documentation for these types, and the code is based on the C++ that implements them. Only documented methods are implemented here. This is especially notable for the `Aabb` type, which has many methods in C++ which are not documented for GDScript. I also took some liberties with a few method names and types. For instance, `Rect2::grow_margin` takes an enum argument, and `Aabb::get_endpoint` returns `Option<Vector3>`. These are different from the GDScript counterparts. `Aabb::get_area` was renamed to `Aabb::get_volume`, etc. Finally, I haven't added any tests for `Aabb` because I did that file last and got bored/lazy before writing any tests (this is what happens when you are not adamant about TDD). The `Rect2` tests did help find plenty of minor bugs, so it is worthwhile to test `Aabb`. Closes #864 Co-authored-by: Jay Oster <jay@kodewerx.org> Co-authored-by: Jan Haller <bromeon@gmail.com>

…o_unit_vector(); rename get_* methods

Bromeon · 2022-07-16T12:02:22Z

bors r+

867: Implement methods on Rect2 and Aabb r=Bromeon a=parasyte The docs are based on the Godot documentation for these types, and the code is based on the C++ that implements them. Only documented methods are implemented here. This is especially notable for the `Aabb` type, which has many methods in C++ which are not documented for GDScript. I also took some liberties with a few method names and types. For instance, `Rect2::grow_margin` takes an enum argument, and `Aabb::get_endpoint` returns `Option<Vector3>`. These are different from the GDScript counterparts. `Aabb::get_area` was renamed to `Aabb::get_volume`, etc. Finally, I haven't added any tests for `Aabb` because I did that file last and got bored/lazy before writing any tests (this is what happens when you are not adamant about TDD). The `Rect2` tests did help find plenty of minor bugs, so it is worthwhile to test `Aabb`. Closes #864 Co-authored-by: Jay Oster <jay@kodewerx.org> Co-authored-by: Jan Haller <bromeon@gmail.com>

bors · 2022-07-16T12:12:23Z

Build failed:

full-ci

See https://spdx.org/licenses/Unicode-DFS-2016.html for details

Bromeon · 2022-07-16T12:18:37Z

bors r+

bors · 2022-07-16T12:28:25Z

Build succeeded:

full-ci

Bromeon · 2022-07-16T12:29:03Z

Thanks @parasyte for all the work! 👍

parasyte · 2022-07-16T19:12:02Z

And thank you @Bromeon for taking it across the finish line!

parasyte force-pushed the feature/impl-rect2-aabb branch from 48e1d60 to eeb694f Compare February 28, 2022 23:31

parasyte force-pushed the feature/impl-rect2-aabb branch from a0ecc83 to bfd1b3e Compare March 1, 2022 09:38

Bromeon requested changes Mar 1, 2022

View reviewed changes

bors bot added a commit that referenced this pull request Mar 1, 2022

Try #867:

6662dcd

Bromeon added c: core Component: core (mod core_types, object, log, init, ...) feature Adds functionality to the library labels Mar 1, 2022

Bromeon reviewed Mar 6, 2022

View reviewed changes

bors bot added a commit that referenced this pull request Mar 6, 2022

Try #867:

d9c6eb5

Bromeon added this to the v0.10.1 milestone Mar 16, 2022

parasyte and others added 3 commits July 14, 2022 00:08

Add implementation and docs to Rect2 core type

7422e52

Add implementation and docs to Aabb core type

e3cb48c

Fix nightly clippy

6806394

Bromeon force-pushed the feature/impl-rect2-aabb branch from bfd1b3e to ef1bee8 Compare July 14, 2022 22:30

Apply PR suggestions for Aabb and Rect2

b02b6ce

Bromeon force-pushed the feature/impl-rect2-aabb branch from ef1bee8 to b02b6ce Compare July 14, 2022 22:34

Rect2, Aabb: pass self by value (they are Copy; also consistency with…

dfdcc4c

… Quat, Plane etc.)

Bromeon force-pushed the feature/impl-rect2-aabb branch 2 times, most recently from 7df8463 to 3e82357 Compare July 15, 2022 21:50

Rect2, Aabb: {longest|shortest}_axis() as single method + new Axis::t…

3344326

…o_unit_vector(); rename get_* methods

Bromeon force-pushed the feature/impl-rect2-aabb branch from 3e82357 to 3344326 Compare July 16, 2022 12:02

godot-rust deleted a comment from bors bot Jul 16, 2022

cargo-deny: allow Unicode-DFS-2016 license

88e3217

See https://spdx.org/licenses/Unicode-DFS-2016.html for details

bors bot merged commit 2163285 into godot-rust:master Jul 16, 2022

parasyte deleted the feature/impl-rect2-aabb branch July 16, 2022 19:11

		/// Returns the support point in a given direction. This is useful for collision detection
		/// algorithms.

		&& b.position.x + b.size.x <= self.position.x + self.size.x
		&& b.position.y + b.size.y <= self.position.y + self.size.y

Implement methods on Rect2 and Aabb #867

Implement methods on Rect2 and Aabb #867

Conversation

parasyte commented Feb 26, 2022

parasyte commented Feb 28, 2022

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

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

bors bot commented Mar 1, 2022

try

parasyte commented Mar 2, 2022

Bromeon commented Mar 2, 2022

parasyte commented Mar 2, 2022 • edited Loading

Bromeon commented Mar 2, 2022

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

bors bot commented Mar 6, 2022

try

parasyte commented Mar 7, 2022

Bromeon commented Apr 13, 2022

parasyte commented Apr 16, 2022

Bromeon commented Jun 5, 2022

parasyte commented Jun 10, 2022

Bromeon commented Jul 16, 2022

bors bot commented Jul 16, 2022

Bromeon commented Jul 16, 2022

bors bot commented Jul 16, 2022

Bromeon commented Jul 16, 2022

parasyte commented Jul 16, 2022

parasyte commented Mar 2, 2022 •

edited

Loading