PR #3/5 Astolfo feature/builtin-vector #67

RealAstolfo · 2023-01-16T20:35:46Z

This PR builds upon ttencates macro and my math functions to create a rust implementation of what godot has in its C++ as close as i think, meant to be merged after #66

Bromeon · 2023-01-16T21:09:48Z

Thanks a lot!

As mentioned here:

I think we should provide x and y as public fields, like in GDNative. There are no invariants to upheld on a vector and dealing with setters/getters makes user code extremely verbose. Then, we can have two private functions that would mem::transmute() between godot-rust and glam representations. Of course Vector2 needs to be #[repr(C)] for that.

ttencate · 2023-01-17T11:50:24Z

Oh wow, I didn't realize how far you'd gotten with this.

If #71 goes through instead of #66, you'll have some nice entry points for deduplication. All functions that work directly on glam types and don't access individual components can be moved into vector_macros.rs, into these macros:

impl_common_vector_fns if it works for float and int vectors
impl_float_vector_fns if it only works for float vectors

I hope you'll find that these macros actually reduce the cognitive burden, since they don't have a lot of parameters.

Bromeon · 2023-01-21T08:11:11Z

Thanks a lot for this great collection of methods!

To progress here, could you maybe:

make PR #1/5 Astolfo feature/builtin-scalar #65 ready (only license header missing)
rebase this PR onto master (which includes Implement the basics of built-in vector types #71 now)

RealAstolfo · 2023-01-28T00:39:57Z

clippy complains about having too many parameters. i think this is a "too bad, so sad" situation. since all of these parameters are required to emulate the C++ godot equivelent

Bromeon · 2023-01-28T10:43:33Z

@RealAstolfo yeah, later we can think about providing some parameters as ranges, e.g. a..=b.

For now, just add a #[allow::clippy(...)] with ... being the ID of the clippy error you get.

RealAstolfo · 2023-01-28T21:35:50Z

didnt know about that. ill add the change then upload it

Bromeon · 2023-01-28T21:52:25Z

Don't worry about ..= now, it's not yet definitive. I would just add the #[allow::clippy(...)], so we can finally merge this PR 🙂

Maybe you can squash all the commits to one? I think you can remove the co-author, as ttencate's additions have been merged in a separate PR in the meantime.

Bromeon · 2023-01-28T21:58:36Z

bors try

RealAstolfo · 2023-01-28T22:05:27Z

added the clippy change. no idea why vector2 wanted to revert itself but i handled it

bors · 2023-01-28T22:17:39Z

try

Build succeeded:

full-ci

Bromeon

Thanks a lot for this huge list of vector functions!

Gave some first feedback 🙂

bors try

Bromeon · 2023-01-28T22:05:30Z

godot-core/src/builtin/vector2.rs

+    pub fn sign(self) -> Self {
+        -self
+    }


This is not the sign...

apologies i should have looked closer. was origonally under the guise that it was a sign flip, not extracting 1 or -1

Bromeon · 2023-01-28T22:09:27Z

godot-core/src/builtin/vector2.rs

+        Self::from_glam(glam::Affine2::from_angle(angle).transform_vector2(self.to_glam()))
+    }
+
+    #[cfg(not(any(gdext_test, doctest)))]


The #[cfg(not(any(gdext_test, doctest)))] is no longer necessary (as of today, if you rebase on master) 🙂

Bromeon · 2023-01-28T22:11:14Z

godot-core/src/builtin/vector3.rs

+    pub fn distance_squared_to(self, to: Self) -> f32 {
+        self.to_glam().distance_squared(to.to_glam())
+    }
+
+    pub fn distance_to(self, to: Self) -> f32 {
+        self.to_glam().distance(to.to_glam())
+    }


Would be easier to just use (to - self).length_squared() and (to - self).length().

Bromeon · 2023-01-28T22:17:26Z

godot-core/src/builtin/vector3.rs

+    pub fn max_axis_index(self) -> i32 {
+        let me = self.to_glam().max_element();
+        if me == self.x {
+            0
+        } else if me == self.y {
+            1
+        } else {
+            2
+        }
+    }
+
+    pub fn min_axis_index(self) -> i32 {
+        let me = self.to_glam().min_element();
+        if me == self.x {
+            0
+        } else if me == self.y {
+            1
+        } else {
+            2
+        }
+    }


I'm not sure we should implement it like that (first computing the maximum, then comparing it to each axis).
Maybe check the gdnative implementation.

We should also make use of the Vector3Axis type, defined further below in this file.

copy, fixing to use the given type

Bromeon · 2023-01-28T22:23:17Z

godot-core/src/builtin/vector2.rs

+    pub fn slerp(self, to: Self, weight: f32) -> Self {
+        let start_length_sq = self.length_squared();
+        let end_length_sq = to.length_squared();
+        if start_length_sq == 0.0 || end_length_sq == 0.0 {
+            return self.lerp(to, weight);
+        }
+        let start_length = start_length_sq.sqrt();
+        let result_length = lerp(start_length, end_length_sq.sqrt(), weight);
+        let angle = self.angle_to(to);
+        self.rotated(angle * weight) * (result_length / start_length)
+    }


The gdnative implementation is significantly easier; I wonder if it's correct, too?

/// Returns the result of spherical linear interpolation between this vector and b, by amount t. /// t is on the range of 0.0 to 1.0, representing the amount of interpolation. /// /// **Note**: Both vectors must be normalized. #[inline] pub fn slerp(self, b: Self, t: f32) -> Self { let theta = self.angle_to(b); self.rotated(self.cross(b).normalized(), theta * t) }

perhaps the gdnative implementation is valid as well, i simply looked at upstream godot and converted the language. up to you if you prefer the gdnative version as opposed to upstream.

Vector2 Vector2::slerp(const Vector2 &p_to, const real_t p_weight) const { real_t start_length_sq = length_squared(); real_t end_length_sq = p_to.length_squared(); if (unlikely(start_length_sq == 0.0f || end_length_sq == 0.0f)) { // Zero length vectors have no angle, so the best we can do is either lerp or throw an error. return lerp(p_to, p_weight); } real_t start_length = Math::sqrt(start_length_sq); real_t result_length = Math::lerp(start_length, Math::sqrt(end_length_sq), p_weight); real_t angle = angle_to(p_to); return rotated(angle * p_weight) * (result_length / start_length); }

Maybe keep your implementation for now. In the first line, can you add this comment?

// TODO compare with gdnative implementation: // https://github.com/godot-rust/gdnative/blob/master/gdnative-core/src/core_types/vector3.rs#L335-L343

Then we can check it later. We anyway need to work out a testing strategy at some point 🙂

bors · 2023-01-28T22:36:23Z

try

Build succeeded:

full-ci

RealAstolfo · 2023-01-28T23:02:42Z

btw i tried squashing any two commits but the github client refuses too for everything i try. so unless you have a good workaround we might have to just live with it

Bromeon · 2023-01-28T23:10:12Z

You should be able to combine commits with git rebase -i, and then mark all except the oldest as "squash".
Then, you can force-push using git push --force-with-lease.

If you can't get it to work, I can also do it before the merge.

bors try

RealAstolfo · 2023-01-28T23:14:06Z

You should be able to combine commits with git rebase -i, and then mark all except the oldest as "squash". Then, you can force-push using git push --force-with-lease.

If you can't get it to work, I can also do it before the merge.

i might just need you to tell me how you did it later, because all i get is
Unable to squash. Squashing replays all commits up to the last one required for the squash. A merge commit cannot exist among those commits. (of which many of the commits were)

bors · 2023-01-28T23:19:38Z

try

Build succeeded:

full-ci

Bromeon · 2023-02-01T19:02:22Z

Thanks for the contribution! 👍

Squashed and rebased onto master. Would be nice if you could do that in the future 🙂 maybe search StackOverflow etc. for necessary git workflows.

bors r+

bors · 2023-02-01T19:16:06Z

Build succeeded:

full-ci

68: PR #4/5 Astolfo feature/builtin-quaternion r=Bromeon a=RealAstolfo Co-Authored-By: Thomas ten Cate <ttencate@gmail.com> Implemented Quaternion to the best of my current ability to resemble godot's, meant to be merged after #67 Co-authored-by: RealAstolfo <astolfo.gman@gmail.com> Co-authored-by: Jan Haller <bromeon@gmail.com>

RealAstolfo mentioned this pull request Jan 16, 2023

PR #4/5 Astolfo feature/builtin-quaternion #68

Merged

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

RealAstolfo force-pushed the astolfo-feature/builtin-vector branch 2 times, most recently from dc81475 to 6560738 Compare January 28, 2023 00:33

RealAstolfo force-pushed the astolfo-feature/builtin-vector branch from 6560738 to b98c870 Compare January 28, 2023 22:03

bors bot added a commit that referenced this pull request Jan 28, 2023

Try #67:

dfe400d

Bromeon reviewed Jan 28, 2023

View reviewed changes

bors bot added a commit that referenced this pull request Jan 28, 2023

Try #67:

4ebffeb

Bromeon force-pushed the master branch from 6ae8ffd to 188aa69 Compare January 28, 2023 22:40

RealAstolfo force-pushed the astolfo-feature/builtin-vector branch 2 times, most recently from 861506c to 39d8022 Compare January 28, 2023 23:02

RealAstolfo force-pushed the astolfo-feature/builtin-vector branch from 39d8022 to 542805c Compare January 28, 2023 23:05

bors bot added a commit that referenced this pull request Jan 28, 2023

Try #67:

998eb76

RealAstolfo force-pushed the astolfo-feature/builtin-vector branch from 542805c to 292cada Compare January 28, 2023 23:16

Bromeon force-pushed the master branch from 7d4582d to c1a36da Compare February 1, 2023 18:12

Add more vector functionality

c9996c3

Bromeon force-pushed the astolfo-feature/builtin-vector branch from 292cada to c9996c3 Compare February 1, 2023 18:52

bors bot merged commit 895283f into godot-rust:master Feb 1, 2023

RealAstolfo deleted the astolfo-feature/builtin-vector branch February 5, 2023 03:07

Bromeon mentioned this pull request Feb 11, 2023

Project status overview #24

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR #3/5 Astolfo feature/builtin-vector #67

PR #3/5 Astolfo feature/builtin-vector #67

RealAstolfo commented Jan 16, 2023 •

edited

Bromeon commented Jan 16, 2023

ttencate commented Jan 17, 2023

Bromeon commented Jan 21, 2023

RealAstolfo commented Jan 28, 2023

Bromeon commented Jan 28, 2023

RealAstolfo commented Jan 28, 2023

Bromeon commented Jan 28, 2023

Bromeon commented Jan 28, 2023

RealAstolfo commented Jan 28, 2023

bors bot commented Jan 28, 2023

Bromeon left a comment

Bromeon Jan 28, 2023

RealAstolfo Jan 28, 2023

Bromeon Jan 28, 2023

Bromeon Jan 28, 2023

Bromeon Jan 28, 2023

RealAstolfo Jan 28, 2023

Bromeon Jan 28, 2023

RealAstolfo Jan 28, 2023

Bromeon Jan 28, 2023

bors bot commented Jan 28, 2023

RealAstolfo commented Jan 28, 2023

Bromeon commented Jan 28, 2023

RealAstolfo commented Jan 28, 2023

bors bot commented Jan 28, 2023

Bromeon commented Feb 1, 2023

bors bot commented Feb 1, 2023

PR #3/5 Astolfo feature/builtin-vector #67

PR #3/5 Astolfo feature/builtin-vector #67

Conversation

RealAstolfo commented Jan 16, 2023 • edited

Bromeon commented Jan 16, 2023

ttencate commented Jan 17, 2023

Bromeon commented Jan 21, 2023

RealAstolfo commented Jan 28, 2023

Bromeon commented Jan 28, 2023

RealAstolfo commented Jan 28, 2023

Bromeon commented Jan 28, 2023

Bromeon commented Jan 28, 2023

RealAstolfo commented Jan 28, 2023

bors bot commented Jan 28, 2023

try

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

bors bot commented Jan 28, 2023

try

RealAstolfo commented Jan 28, 2023

Bromeon commented Jan 28, 2023

RealAstolfo commented Jan 28, 2023

bors bot commented Jan 28, 2023

try

Bromeon commented Feb 1, 2023

bors bot commented Feb 1, 2023

RealAstolfo commented Jan 16, 2023 •

edited