Add missing integer vector methods #351

kulkalkul · 2023-07-19T06:56:02Z

Saw these unimplemented on #310. I haven't tested any as it is straightforward. Let me know if rest of this PR is verbose for the little amount of work I did. 😅

Coverage

abs: already implemented by impl_common_vector_fns
aspect: Vector2i only
clamp: Ord provides it
length
length_squared
max_axis_index
min_axis_index
sign
snapped

Extra

I created two new macros for common implementations. I don't know if that's the preferable way of doing stuff, but I saw macros implemented some methods, so I did the same.

Vector4i::max_axis_index and Vector4i::min_axis_index are rustified version of C++ implementation. I used an unwrap there while using Vector4Axis::try_from_ord. But it has no chance of failing so I just added a comment without documenting it as "panics".

Added a utility method similar to to_glam named to_glam_real. Using it for length implementation on RVec's.

Due to trailing addition (+) causing issues with the vector component macro, I created a variable and increased it for each component. It results in a similar assembly with optimization level 0, and the same assembly with optimization level 1 and above.

GodotRust · 2023-07-19T14:10:26Z

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

Bromeon

Thanks a lot! Added some comments, mostly nitpicking wrt docs.

What I'm not really sure is the {min|max}_axis_index API. I'm aware this is precisely what GDScript offers (and we already have for float vectors), but the semantics are quite weird:

max_axis_index returns X on equality
min_axis_index returns Y on equality

The implementations of 2D, 3D and 4D vectors also looks completely different, which combined with absence of bugs makes it quite easy to introduce bugs. If not now, then in future.

Is there a good use case for the GDScript semantics of returning an arbitrary axis on equality? If not, we may consider Option as a return type, like Rust's max() functions...

Bromeon · 2023-07-19T14:14:20Z

godot-core/src/builtin/vectors/vector2i.rs

@@ -56,6 +56,31 @@ impl Vector2i {
        Self { x, y }
    }

+    ///Returns the aspect ratio of this vector, the ratio of x to y.


I would keep docs as concise as possible and without repetition. x / y is probably slightly easier to understand 🙂

Suggested change

///Returns the aspect ratio of this vector, the ratio of x to y.

/// Aspect ratio: x / y, as a `real` value.

Agreed, I just copied that from Godot docs😅

Bromeon · 2023-07-19T14:18:56Z

godot-core/src/builtin/vectors/vector2i.rs

+    /// Returns the axis of the vector's highest value. See [`Vector2Axis`] enum. If all components
+    /// are equal, this method returns [`Vector2Axis::X`].


In addition to previous points, the See [`Vector2Axis`] enum is not needed, as the user can click on any involved type (such as the return type).

Bromeon · 2023-07-19T14:22:42Z

godot-core/src/builtin/vectors/vector2i.rs

@@ -75,10 +100,15 @@ impl Vector2i {
    }

    /// Converts `self` to the corresponding `glam` type.
-    fn to_glam(self) -> glam::IVec2 {
+    fn to_glam(self) -> IVec2 {


Please keep glam types qualified, so the code is more expressive.

I've checked how other integer vector modules used glam types and they all omitted glam module. I'll include it for other integer modules too so it can be uniform.

Makes sense, thanks!

Bromeon · 2023-07-19T14:31:26Z

godot-core/src/builtin/vectors/vector_macros.rs

+            /// Returns a new vector with each component set to 1 if it's positive, -1 if it's
+            /// negative, and 0 if it's zero.
+            #[inline]
+            pub fn sign(self) -> Self {
+                Self::from_glam(self.to_glam().signum())
+            }


The implementation is wrong; signum() never returns 0.

signum does return 0 for integers. it's only for floats signum is never 0. see https://doc.rust-lang.org/std/primitive.i64.html#method.signum

Are we sure? Both IVec2::signum (i32::signum) and godot docs says it does return 0.

I see, thanks! Now I'm even more confused why Rust chose two different behaviors here. Btw, a unit test wouldn't have raised this question in the first place 😛

I never wrote an unit test before, only integration tests; so I don't know the workflow. I've checked multiple files but couldn't find any tests that I can figure out from. Would you mind suggesting a file with good written tests so I can do something similar? 😅

I see, thanks! Now I'm even more confused why Rust chose two different behaviors here. Btw, a unit test wouldn't have raised this question in the first place 😛

well signum represents the sign of a value with numbers with: 1 being positive, -1 being negative, 0 being unsigned, and NAN for NAN.

so since 0 is unsigned for integers, signum returns 0. but since floats have signed 0s, i.e +0 and -0 are distinct values, signum should return 1 or -1 respectively.

floats are weird but it's consistent

@kulkalkul it was a joke, I didn't mean to burden you with extra work 😉 as you said, most of your additions are simple enough to not need testing. In this case I was just surprised about the difference from floats, but with @lilizoey's explanation, it actually makes sense (from a IEEE but not mathematical perspective at least).

Maybe for the future, you could have a look at rect2.rs, which contains both unit tests (#[test]) and doctests (for Display impl). Integration tests #[itest] are in separate files, see here for explanation. For now it's really OK though 🙂

kulkalkul · 2023-07-19T16:14:37Z

Is there a good use case for the GDScript semantics of returning an arbitrary axis on equality?

Both ways are OK, imo. I think someone experienced with Godot would expect it would have the same implementation as Godot. On the other hand, Option gives extra possibilities without regressing the previous use case. And it is the rusty way of doing things. I don't think there is a clear reason for it except for C++ developers are not used to using algebraic data types. So, making it an Option makes sense.

Bromeon · 2023-07-20T07:12:10Z

I think someone experienced with Godot would expect it would have the same implementation as Godot. On the other hand, Option gives extra possibilities without regressing the previous use case.

Yes, that's my thinking as well. Godot doesn't have Option and their design is probably based on a frequent use case

var axis = vec.max_axis_index()
var value = vec[axis]

which then "works" in terms of "just getting the largest component".

In Rust, this may be a tiny bit more verbose, but explicit:

let axis = vec.max_axis().unwrap_or(Vector2Axis::X);
let value = vec[axis];

We can probably call it max_axis()/min_axis(), as it's a type-safe enum, not just an index.

Implementation could probably be done using max_by_key() with some tricks 🤔

kulkalkul · 2023-07-20T07:33:00Z

Ah, looks like I forgot running ./check.sh before the second commit, my bad. I'll make clippy happy next time, no worries

kulkalkul · 2023-07-21T22:55:37Z

Well, I thought a lot about how to solve it best, figured going straightforward would be the most mainteinable. I haven't used max_by_key for that reason. I also added some unit tests for axis functions (several lines are verbose, but kept them for the sake of completeness), which proved pretty useful to avert some pitfalls, especially with Vector4.

And I just noticed that, files I was working with already had tests and I saw them previously... But my mind didn't register them as unit tests for some reason 😅

Bromeon

Thanks for updating!

I like the use of cmp. Still not 100% happy about 3 different impls for 2D/3D/4D and duplication for max/min, but since we have tests now, it's mostly a cosmetic issue that can be addressed later 🙂

As mentioned in the code review, I'd keep tests a bit shorter and assertions more local. At the moment a reader needs to jump to declaration of a variable quite far to see what's happening -- the idea of tests is however to make as obvious as possible what should be tested (the "spec").

Bromeon · 2023-07-23T21:30:34Z

godot-core/src/builtin/vectors/vector2i.rs

@@ -56,6 +57,29 @@ impl Vector2i {
        Self { x, y }
    }

+    ///Aspect ratio: x / y, as a `real` value.


Space after ///. I even provided a verbatim suggestion 😉

Bromeon · 2023-07-23T21:36:56Z

godot-core/src/builtin/vectors/vector2i.rs

+    fn axis_min_max() {
+        let x_max = Vector2i::new(10, 5);
+        let y_max = Vector2i::new(5, 10);
+
+        let x_min = Vector2i::new(-5, 5);
+        let y_min = Vector2i::new(5, -5);
+
+        let max_eq = Vector2i::new(15, 15);
+        let min_eq = Vector2i::new(15, 15);
+
+        assert_eq!(x_max.max_axis(), Some(Vector2Axis::X));
+        assert_eq!(y_max.max_axis(), Some(Vector2Axis::Y));
+
+        assert_eq!(x_min.min_axis(), Some(Vector2Axis::X));
+        assert_eq!(y_min.min_axis(), Some(Vector2Axis::Y));
+
+        assert_eq!(max_eq.max_axis(), None);
+        assert_eq!(min_eq.min_axis(), None);
+    }


Since you only use each variable once, it would be easier to inline them, as the condition would be self-contained. I.e. this test would just have 6 assertions and no other statements.

Same for the others.

kulkalkul · 2023-07-23T22:47:26Z

Still not 100% happy about 3 different impls for 2D/3D/4D and duplication for max/min

It bugs me too, but I refrained from abstracting it away to something similar like 4D to not overcomplicate it.

About tests, I'm more on verbose side but replaced it as you asked. I also put a minor comment on 3D and 4D tests about "early equality traps" that I noticed while implementing so those two tests wouldn't be removed in the future (because they look like they are not testing anything important).

Bromeon · 2023-07-24T15:50:51Z

Thanks a lot!

I also put a minor comment on 3D and 4D tests about "early equality traps" that I noticed while implementing so those two tests wouldn't be removed in the future (because they look like they are not testing anything important).

Great forethought, I appreciate it.

Bromeon reviewed Jul 19, 2023

View reviewed changes

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

kulkalkul force-pushed the integer_vector_methods branch from 9a7d69c to 8be1b68 Compare July 19, 2023 15:58

kulkalkul force-pushed the integer_vector_methods branch from 8be1b68 to 421fad5 Compare July 21, 2023 22:49

Bromeon reviewed Jul 23, 2023

View reviewed changes

Add missing integer vector methods

974af2d

kulkalkul force-pushed the integer_vector_methods branch from 421fad5 to 974af2d Compare July 23, 2023 22:44

Bromeon added this pull request to the merge queue Jul 24, 2023

Merged via the queue into godot-rust:master with commit 18c04d0 Jul 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing integer vector methods #351

Add missing integer vector methods #351

kulkalkul commented Jul 19, 2023

GodotRust commented Jul 19, 2023

Bromeon left a comment

Bromeon Jul 19, 2023

kulkalkul Jul 19, 2023

Bromeon Jul 19, 2023

Bromeon Jul 19, 2023

kulkalkul Jul 19, 2023

Bromeon Jul 20, 2023

Bromeon Jul 19, 2023

lilizoey Jul 19, 2023

kulkalkul Jul 19, 2023

Bromeon Jul 20, 2023

kulkalkul Jul 20, 2023

lilizoey Jul 20, 2023 •

edited

Loading

Bromeon Jul 20, 2023

kulkalkul commented Jul 19, 2023 •

edited

Loading

Bromeon commented Jul 20, 2023

kulkalkul commented Jul 20, 2023

kulkalkul commented Jul 21, 2023 •

edited

Loading

Bromeon left a comment

Bromeon Jul 23, 2023

Bromeon Jul 23, 2023 •

edited

Loading

kulkalkul commented Jul 23, 2023 •

edited

Loading

Bromeon commented Jul 24, 2023

	///Returns the aspect ratio of this vector, the ratio of x to y.
	/// Aspect ratio: x / y, as a `real` value.

		/// Returns the axis of the vector's highest value. See [`Vector2Axis`] enum. If all components
		/// are equal, this method returns [`Vector2Axis::X`].

Add missing integer vector methods #351

Add missing integer vector methods #351

Conversation

kulkalkul commented Jul 19, 2023

Coverage

Extra

GodotRust commented Jul 19, 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

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 Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kulkalkul commented Jul 19, 2023 • edited Loading

Bromeon commented Jul 20, 2023

kulkalkul commented Jul 20, 2023

kulkalkul commented Jul 21, 2023 • edited Loading

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon Jul 23, 2023 • edited Loading

Choose a reason for hiding this comment

kulkalkul commented Jul 23, 2023 • edited Loading

Bromeon commented Jul 24, 2023

lilizoey Jul 20, 2023 •

edited

Loading

kulkalkul commented Jul 19, 2023 •

edited

Loading

kulkalkul commented Jul 21, 2023 •

edited

Loading

Bromeon Jul 23, 2023 •

edited

Loading

kulkalkul commented Jul 23, 2023 •

edited

Loading