-
-
Notifications
You must be signed in to change notification settings - Fork 269
Small Aabb cleanup #1447
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Small Aabb cleanup #1447
Conversation
- Add `get_endpoint` method to Aabb. - Fix & simplify`support` method (account for 0). - Test godot-rust implementation of Aabb against its Godot counterpart.
41394eb to
fad6923
Compare
|
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1447 |
Bromeon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks a lot! 👍
| /// Returns the position of one of the 8 vertices that compose this bounding box. With an `idx` of 0 this is the same as position, and an `idx` of 7 is the same as end. | ||
| /// | ||
| /// # Panics | ||
| /// If `idx` is greater than 7. | ||
| #[inline] | ||
| pub fn endpoint(&self, idx: usize) -> Vector3 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep the "header" limited to 1 line, just describing what the function does. Details can be part of the "body":
| /// Returns the position of one of the 8 vertices that compose this bounding box. With an `idx` of 0 this is the same as position, and an `idx` of 7 is the same as end. | |
| /// | |
| /// # Panics | |
| /// If `idx` is greater than 7. | |
| #[inline] | |
| pub fn endpoint(&self, idx: usize) -> Vector3 { | |
| /// Returns the position of one of the 8 vertices that compose this bounding box. | |
| /// | |
| /// With an `idx` of 0 this is the same as [`position`][Self::position], and an `idx` of 7 is the same as [`end()`][Self::end]. | |
| /// | |
| /// # Panics | |
| /// If `idx` is greater than 7. | |
| /// | |
| /// _Godot equivalent: `get_endpoint`_ | |
| #[doc(alias = "get_endpoint")] | |
| #[inline] | |
| pub fn get_corner(self, idx: usize) -> Vector3 { |
I also added links to position/end().
Function should probably be named get_corner:
- "get" prefix as it takes a parameter -- so consistent with new
get_support - "endpoint" is really a bad name, and confusing with "end"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would stick to get_endpoint due to discoverability – https://docs.godotengine.org/en/stable/classes/class_aabb.html#class-aabb-method-get-endpoint
| pub fn intersects_plane(self, plane: Plane) -> bool { | ||
| // The set of the edges of the AABB. | ||
| let points = [ | ||
| pub fn points(&self) -> [Vector3; 8] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| pub fn points(&self) -> [Vector3; 8] { | |
| pub fn corners(&self) -> [Vector3; 8] { |
Would also be good if get_corner + corners were defined next to each other, and had mutual intra-doc links.
As this function doesn't exist in Godot, it doesn't need an "equivalent" doc line.
I wonder if the order of corners should be documented? Is this some canonical ordering?
Do we expect it to change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ordering is the same as in Aabb::get_endpoint https://github.com/godotengine/godot/blob/master/core/math/aabb.h#L244 which has been unchanged since the very beginning (back when it was called Rect3).
Equivalent Gdscript code is something along the lines of:
let my_points = [];
for i in range(8):
my_points.push(my_aabb.get_endpoint(i))Weird that Godot doesn't expose that – it is pretty common usecase 🤔.
| let test_aabb = Aabb::new( | ||
| TEST_AABB.position - Vector3::new(1.0, 0.25, 0.55), | ||
| TEST_AABB.size, | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confusing to name test_aabb like TEST_AABB in different case, when it is a different one. I don't know if one of the two can have more specific names? Or at least the constant could be BASE_AABB or so...
Add
get_endpointmethod to Aabb.Pretty obvious. I also exposed "points" method to get all the points (this is fairly common usecase); as far as I can tell it is being nicely optimized by the compiler.
Fix & simplify
supportmethod (account for 0).Less obvious – our implementation of support was wrong, since it was not properly accounting for
0. It is still based on Godot implementation, just the latest one: Simplify Rect2/AABBget_supportfunction godotengine/godot#95790 .Additionally
supporthas been renamed toget_supportnot only because it's closer to Godot, but also because it's not really a "getter" in Rust's sense. Not a breaking change per se, I added the deprecation.Not a full cleanup – we still have ominous
/// Currently most methods are only available through [InnerAabb](super::inner::InnerAabb).in Aabb's docs.