Tweaks to glamconv and indexing for vectors #128

lilizoey · 2023-02-16T15:21:54Z

Add bool as glam type
Add glamconv/glamtype for Vector2/3/4
Add assert_eq/ne_approx
Add lerp_angle

Split from #124

Bromeon

Thanks for the split!

Are the Index<usize> and IndexMut<usize> traits really necessary? We have already Index<Vector2Axis>, and the regular .x/.y/.z access. I don't want to provide 3 ways to achieve the same thing unless there's a very good reason.

If this is truly needed, we should look at the concrete use case. E.g. if it's for a for loop, we could provide something like Vector2Axis::values() instead.

Bromeon · 2023-02-16T15:35:07Z

godot-core/src/builtin/glam_helpers.rs

+    ($(,)?) => {};
+    ($($T:ty),+ $(,)?) => {$(


What's the reason behind this? I'd rather repeat the macro invocation than making the macro more complex.

Repetition patterns are mostly useful for token ranges inside the macro body, not for repeating the entire macro.

i can change it if you want, but it's not that uncommon afaik. the standard library does it a fair bit, see for instance https://doc.rust-lang.org/src/core/cmp.rs.html#1370.

Interesting, thanks for that link! 👍

I think it's easier if we use separate calls though, as we do that already in several other places (the impl macros for vectors, From/ToVariant, etc)

Bromeon · 2023-02-16T15:36:02Z

godot-core/src/builtin/math.rs

+    let difference = (to - from) % (2.0 * PI);
+    let distance = (2.0 * difference) % (2.0 * PI) - difference;


Bromeon · 2023-02-16T15:47:07Z

godot-core/src/builtin/vector3.rs

+impl GlamType for Vec3A {
+    type Mapped = Vector3;
+
+    fn to_front(&self) -> Self::Mapped {
+        Vector3::new(self.x, self.y, self.z)
+    }
+
+    fn from_front(mapped: &Self::Mapped) -> Self {
+        Vec3A::new(mapped.x, mapped.y, mapped.z)
+    }
+}


We are not using Vec3A at the moment, so this is dead code.

we will be because the Transform3D will use Affine3A, since there is no Affine3, which uses Vec3A internally

Ok, we then need to see if Vec3A <-> Vector3 conversions are not necessary all over the place 🤔 otherwise we might not gain much from using glam. But I guess it's a good start.

Bromeon · 2023-02-16T15:47:34Z

godot-core/src/builtin/vector3.rs

+    }
+}
+
+impl super::glam_helpers::GlamConv for Vector3 {


GlamConv could be imported.

Bromeon · 2023-02-16T15:48:46Z

godot-core/src/builtin/vector4.rs

+use std::{
+    fmt,
+    ops::{Index, IndexMut},
+};


IIRC we don't use this import style elsewhere, maybe keep it flat for now (i.e. two use statements).

I think there are nightly rustfmt configs to tweak that, but so far we don't have automation on this 🙂

i didn't do this manually so i think this is something rust-analyzer + rustfmt concocted

lilizoey · 2023-02-16T17:59:45Z

Are the Index and IndexMut traits really necessary?

The main usecase is to index into matrices like mat[0][2] instead of something like mat[0].z or mat.x.z.

As an example:

pub fn transposed(self) -> Self {
    Self {
        rows: [
            Vector3::new(self[0][0], self[1][0], self[2][0]),
            Vector3::new(self[0][1], self[1][1], self[2][1]),
            Vector3::new(self[0][2], self[1][2], self[2][2]),
        ],
    }
}

Bromeon · 2023-02-17T16:11:51Z

The main usecase is to index into matrices like mat[0][2] instead of something like mat[0].z or mat.x.z.

In this case you only use literals, so you don't benefit from having the numbers (except for syntactical reasons). For example, the code you showed adds 9 extra bound checks -- which might be optimized away, or might not.

If we really need random access syntax later, we can still consider something at the matrix level. But a general philosophy I'd like to follow is to start with a slim, non-redundant API and only expand when there is a significant use case that cannot be easily solved with existing APIs🙂

Regarding transposed() in particular, we could also give access to column vectors like in GDNative's Basis:

pub fn transposed(self) -> Self {
    Self {
        rows: [self.a(), self.b(), self.c()]
    }
}

lilizoey · 2023-02-17T17:14:10Z

I think i just find myself often getting frustrated when i'm not able to just index normally into things that are just lists of homogeneous values. Like i can't really think of a case from the std where something that is mentally just a list of values doesn't have a way to index into them without a good reason. Closest are probably tuples, but there it's just .index instead of [index], and also tuples are heterogeneous. String also doesn't have straightforward indexing, but that's because of UTF-8. Glam itself has indexing defined for its vectors too.

I'll remove them for now but that's where it comes from i think.

For matrices at least i think we should have indexing though, mainly because godot does it.

Bromeon · 2023-02-17T18:38:02Z

I think i just find myself often getting frustrated when i'm not able to just index normally into things that are just lists of homogeneous values. Like i can't really think of a case from the std where something that is mentally just a list of values doesn't have a way to index into them without a good reason.

But a 2D/3D vector is not just a list of values -- it's a geometric object.

Slices in Rust are located at a much lower abstraction level. Nothing would prevent us from modeling Vector3 as [f32; 3] + extension trait, but it's impractical and less intuitive.

And there is a good reason. I mentioned two of them already, but there are more:

you don't pay for bound checks when you don't need them. *
you don't have 3x redundant APIs v.x, v[Axis::X] and v[0]. 2x is enough, I think.
accessing via the other 2 ways does not put you at risk of panic, i.e. is more type-safe.

I understand the desire to have that syntax, but for the use case described it seems to be based mostly on syntactical preference.

* Edit: we conducted some further analysis on performance in this Godbolt example. With #[inline] and in release mode (rustc -O), the index access is optimized away indeed. Without #[inline] the function is retained in the binary but still inlined, this might not work across crate boundaries though.

Bromeon

Thanks for the update!

bors try

Bromeon · 2023-02-19T07:15:07Z

godot-core/src/builtin/math.rs

+pub fn lerp_angle(from: f32, to: f32, weight: f32) -> f32 {
+    let difference = (to - from) % TAU;
+    let distance = (2.0 * difference) % TAU - difference;
+    from + distance * weight
+}


Is this wrapping correctly?
Is the output in [0, TAU] or [-PI, PI] range or something else?

Some docs would be nice 🙂

i just copied the code from godot, so i didn't super look into it closely.
https://github.com/godotengine/godot/blob/master/core/math/math_funcs.h#L389

bors · 2023-02-19T07:31:41Z

try

Build succeeded:

full-ci

Add glamconv/glamtype for Vector2/3/4 Add lerp_angle Add assert_eq_approx/assert_ne_approx

Bromeon · 2023-02-19T17:20:10Z

Thanks for the docs! Is it necessary to always pass is_equal_approx to the macro, due to macro hygiene? 🤔

lilizoey · 2023-02-19T17:30:53Z

Thanks for the docs! Is it necessary to always pass is_equal_approx to the macro, due to macro hygiene? thinking

If not it wont know what function to use. We could make it use the one for floats by default, but i decided not to.

The main alternative would be to create a trait for is_equal_approx and have the macro use that, similar to how assert_eq uses PartialEq. But that'd require a lot more work than this current impl. And it'd likely be better to just use approx at that point.

Bromeon · 2023-02-19T19:21:08Z

If not it wont know what function to use. We could make it use the one for floats by default, but i decided not to.

But it's only about the name, no? As long as an is_equal_approx function is in scope, it can call that.
Since it's a macro, it can do "poor man's ducktyping" and a trait is not really needed.

lilizoey · 2023-02-19T19:46:45Z

If not it wont know what function to use. We could make it use the one for floats by default, but i decided not to.

But it's only about the name, no? As long as an is_equal_approx function is in scope, it can call that. Since it's a macro, it can do "poor man's ducktyping" and a trait is not really needed.

Not really, because the is_equal_approx function for f32 isn't implemented on f32 but is a standalone function, whereas for all the other types it is an inherent function.

Bromeon · 2023-02-24T17:26:22Z

Thanks!

bors r+

bors · 2023-02-24T17:43:25Z

Build succeeded:

full-ci

124: Implement the matrix types r=Bromeon a=sayaks Implement the four matrix types godot has: `Basis`, `Transform2D`, `Transform3D`, `Projection`. `Transform2D` in godot consists of an array of 3 `Vector2`s, where the first two are the basis. This means godot has some methods named `Transform2D::basis_<something>()`. I decided to instead make a struct `Basis2D`. so now any method like the above would be called `trans.basis.<something>()` instead. `Projection` has a lot of intricate math for many of its functions so it is largely a wrapper around `InnerProjection`, except for some methods that were easy to reimplement. Depends on #128 Co-authored-by: Lili Zoey <[email protected]>

Bromeon added quality-of-life No new functionality, but improves ergonomics/internals c: core Core components labels Feb 16, 2023

Bromeon reviewed Feb 16, 2023

View reviewed changes

lilizoey force-pushed the glam-vector-tweaks branch 2 times, most recently from 293fe3f to e1aaa3f Compare February 17, 2023 17:34

Bromeon reviewed Feb 19, 2023

View reviewed changes

bors bot added a commit that referenced this pull request Feb 19, 2023

Try #128:

cc721c8

Add bool as glam type

91cc0a3

Add glamconv/glamtype for Vector2/3/4 Add lerp_angle Add assert_eq_approx/assert_ne_approx

lilizoey force-pushed the glam-vector-tweaks branch from e1aaa3f to 91cc0a3 Compare February 19, 2023 16:57

lilizoey mentioned this pull request Feb 19, 2023

Implement the matrix types #124

Merged

bors bot merged commit b3b9e0e into godot-rust:master Feb 24, 2023

lilizoey deleted the glam-vector-tweaks branch April 16, 2023 16:23

		let difference = (to - from) % (2.0 * PI);
		let distance = (2.0 * difference) % (2.0 * PI) - difference;

Uh oh!

Tweaks to glamconv and indexing for vectors #128

Tweaks to glamconv and indexing for vectors #128

Uh oh!

Conversation

lilizoey commented Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bromeon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lilizoey Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lilizoey commented Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bromeon commented Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lilizoey commented Feb 17, 2023

Uh oh!

Bromeon commented Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bromeon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors bot commented Feb 19, 2023

try

Uh oh!

Bromeon commented Feb 19, 2023

Uh oh!

lilizoey commented Feb 19, 2023

Uh oh!

Bromeon commented Feb 19, 2023

Uh oh!

lilizoey commented Feb 19, 2023

Uh oh!

Bromeon commented Feb 24, 2023

Uh oh!

bors bot commented Feb 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lilizoey commented Feb 16, 2023 •

edited

Loading

lilizoey Feb 16, 2023 •

edited

Loading

lilizoey commented Feb 16, 2023 •

edited

Loading

Bromeon commented Feb 17, 2023 •

edited

Loading

Bromeon commented Feb 17, 2023 •

edited

Loading