Skip to content
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

Implement the matrix types #124

Merged
merged 4 commits into from Mar 4, 2023
Merged

Conversation

lilizoey
Copy link
Member

@lilizoey lilizoey commented Feb 13, 2023

Implement the four matrix types godot has: Basis, Transform2D, Transform3D, Projection.

Transform2D in godot consists of an array of 3 Vector2s, 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

@Bromeon Bromeon added feature Adds functionality to the library c: core Core components labels Feb 14, 2023
@Bromeon
Copy link
Member

Bromeon commented Feb 16, 2023

Thanks a lot for all these great additions! 👍

This PR is huge with 1.5 kLOC diff. Would it be possible to split changes to existing parts (vectors, glam conv) from the new features?

@lilizoey
Copy link
Member Author

(...) Would it be possible to split changes to existing parts (vectors, glam conv) from the new features?

Absolutely

@lilizoey lilizoey force-pushed the feature/matrix branch 2 times, most recently from 53ba518 to 3b88a0a Compare February 19, 2023 17:18
@lilizoey lilizoey marked this pull request as ready for review February 19, 2023 17:26
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a fantastic contribution, thank you very much for the great effort! 👍

I haven't been able to review everything yet, but added already several comments.


There's an asymmetry in the Euler angle conversions, in both naming an argument types:

fn from_euler(order: EulerOrder, a: f32, b: f32, c: f32) -> Self;
fn euler_angles(&self, order: EulerOrder) -> Vector3;

I would probably do it like this:

fn from_euler(order: EulerOrder, angles: Vector3) -> Self;
fn to_euler(&self, order: EulerOrder) -> Vector3;

godot-core/src/builtin/basis.rs Outdated Show resolved Hide resolved
godot-core/src/builtin/basis.rs Outdated Show resolved Hide resolved
godot-core/src/builtin/basis.rs Outdated Show resolved Hide resolved
godot-core/src/builtin/basis.rs Outdated Show resolved Hide resolved
Comment on lines 228 to 252
let sy: f32 = self[0].z;
if sy < 1.0 - CMP_EPSILON {
if sy > -(1.0 - CMP_EPSILON) {
// is this a pure Y rotation?
if self[1].x == 0.0
&& self[0].y == 0.0
&& self[1].z == 0.0
&& self[2].y == 0.0
&& self[1].y == 1.0
{
// return the simplest form (human friendlier in editor and scripts)
Vector3::new(0.0, f32::atan2(self[0].z, self[0].x), 0.0)
} else {
Vector3::new(
f32::atan2(-self[1].z, self[2].z),
f32::asin(sy),
f32::atan2(-self[0].y, self[0].x),
)
}
} else {
Vector3::new(f32::atan2(self[2].y, self[1].y), -FRAC_PI_2, 0.0)
}
} else {
Vector3::new(f32::atan2(self[2].y, self[1].y), FRAC_PI_2, 0.0)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This exists 6 times in a slight variantion. Is it not possible to extract the core operations to a function, by reordering elements before and after the operation? In principle:

use glam::swizzles::Vec3Swizzles as _;

// Get input in right order
let input: Vector3 = ...;
let zxy: glam::Vec3 = input.to_glam().zxy();

// Actual operation
let zxy: glam::Vec3 = transmogrify(zxy);

// Transform back
let xyz: Vector3 = zxy.yzx().to_front();

Might need more than 1 vector, but you get the idea.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'll look into it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok i was able to do something close, but not exact. There are like 3 edge cases that make it not work perfectly. but at least for 4/6 of the euler orderings it's basically just that.

godot-core/src/builtin/basis.rs Outdated Show resolved Hide resolved
godot-core/src/builtin/basis.rs Outdated Show resolved Hide resolved
(Basis::from_axis_angle(Vector3::FORWARD, 0.0) * Vector3::RIGHT)
.is_equal_approx(Vector3::RIGHT),
"got {got}, expected {expected}",
got = Basis::from_axis_angle(Vector3::FORWARD, 0.0) * Vector3::RIGHT,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're repeating longer expressions, it makes sense to extract them. This would also have avoided the mistakes with FLIP_X above:

let actual = Basis::from_axis_angle(Vector3::FORWARD, 0.0) * Vector3::RIGHT;
let expected = Vector3::RIGHT;
assert!(actual, expected, "... what is being tested ...");

itest/rust/src/transform3d_test.rs Outdated Show resolved Hide resolved
Comment on lines +876 to +789
let vectors_to_test: Vec<Vector3> = vec![
Vector3::new(0.0, 0.0, 0.0),
Vector3::new(0.5, 0.5, 0.5),
Vector3::new(-0.5, -0.5, -0.5),
Vector3::new(40.0, 40.0, 40.0),
Vector3::new(-40.0, -40.0, -40.0),
Vector3::new(0.0, 0.0, -90.0),
Vector3::new(0.0, -90.0, 0.0),
Vector3::new(-90.0, 0.0, 0.0),
Vector3::new(0.0, 0.0, 90.0),
Vector3::new(0.0, 90.0, 0.0),
Vector3::new(90.0, 0.0, 0.0),
Vector3::new(0.0, 0.0, -30.0),
Vector3::new(0.0, -30.0, 0.0),
Vector3::new(-30.0, 0.0, 0.0),
Vector3::new(0.0, 0.0, 30.0),
Vector3::new(0.0, 30.0, 0.0),
Vector3::new(30.0, 0.0, 0.0),
Vector3::new(0.5, 50.0, 20.0),
Vector3::new(-0.5, -50.0, -20.0),
Vector3::new(0.5, 0.0, 90.0),
Vector3::new(0.5, 0.0, -90.0),
Vector3::new(360.0, 360.0, 360.0),
Vector3::new(-360.0, -360.0, -360.0),
Vector3::new(-90.0, 60.0, -90.0),
Vector3::new(90.0, 60.0, -90.0),
Vector3::new(90.0, -60.0, -90.0),
Vector3::new(-90.0, -60.0, -90.0),
Vector3::new(-90.0, 60.0, 90.0),
Vector3::new(90.0, 60.0, 90.0),
Vector3::new(90.0, -60.0, 90.0),
Vector3::new(-90.0, -60.0, 90.0),
Vector3::new(60.0, 90.0, -40.0),
Vector3::new(60.0, -90.0, -40.0),
Vector3::new(-60.0, -90.0, -40.0),
Vector3::new(-60.0, 90.0, 40.0),
Vector3::new(60.0, 90.0, 40.0),
Vector3::new(60.0, -90.0, 40.0),
Vector3::new(-60.0, -90.0, 40.0),
Vector3::new(-90.0, 90.0, -90.0),
Vector3::new(90.0, 90.0, -90.0),
Vector3::new(90.0, -90.0, -90.0),
Vector3::new(-90.0, -90.0, -90.0),
Vector3::new(-90.0, 90.0, 90.0),
Vector3::new(90.0, 90.0, 90.0),
Vector3::new(90.0, -90.0, 90.0),
Vector3::new(20.0, 150.0, 30.0),
Vector3::new(20.0, -150.0, 30.0),
Vector3::new(-120.0, -150.0, 30.0),
Vector3::new(-120.0, -150.0, -130.0),
Vector3::new(120.0, -150.0, -130.0),
Vector3::new(120.0, 150.0, -130.0),
Vector3::new(120.0, 150.0, 130.0),
];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This constant is used in itest as well, unfortunately there's currently no way to reuse it, without adding it to the user-facing crate. Maybe in the future I could add a cargo feature for this.

);
assert_inner_equivalent!(
InnerTransform3D,
TEST_TRANSFORM.translated_local(Vector3::new(1.0, 2.0, 3.0)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity: Is it on purpose that e.g. translated_local is used twice (same in the 2d case, where also rotated was omitted)? From just briefly glancing over the code I haven't really understood where the "expected result" is actually coming from.

In any case great to see that you have adapted all the translated/scaled/rotated test cases from Godot already on the unit test level 👍 (I had added them to Godot as part of aligning their semantics.) So that aspect should be fully covered anyhow.

Copy link
Member Author

@lilizoey lilizoey Feb 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope that was an accident.

These tests just check that calling the function through our rust-impl vs calling into godot does roughly the same thing. Basically by calling the provided function on Transform3D vs InnerTransform3D.

Also Transform2D does have rotated it's just that rustfmt formatted it differently than the rest so it looks like it's missing.

bors bot added a commit that referenced this pull request Feb 24, 2023
128: Tweaks to glamconv and indexing for vectors r=Bromeon a=sayaks

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

Split from #124 

Co-authored-by: Lili Zoey <lili.andersen@nrk.no>
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had another look, but this PR is so big it will probably take more rounds 😅 that said, really great work and attention to detail!

Below some general comments, you'll find more inline.


f32 vs. f64

One thing which is a bit inconsistent is the usage of f32 vs. f64. The code-generated builtin instances use f64 simply because that's the type exposed via GDExtension. On the other hand, function using glam return f32, e.g. Projection::determinant.

We might want to stick to f64 for consistency, but I'm open for suggestions. Something like a real type depending on build options is out-of-scope for this already huge PR, and it's also not something I'd like to address very soon, as there are much bigger construction sites at the moment.


Basis2D

I do see the appeal of simplifying some of the Transform2D implementation and making it a bit closer to Transform3D + Basis. But the more I think about this, the more I see potential problems with having a separate type:

  • The API is more distant from Godot, making it harder to translate Rust <-> GDScript code or reading GDScript resources to learn godot-rust. This can sometimes be OK if we can make an interface more type-safe and expressive, but I'm not sure this is a clear case.
  • It adds an extra indirection which seems not that useful to me. Basis (3D) is at least used in a few Godot engine properties and methods, e.g. GridMap::get_cell_item_basis() or PhysicsDirectBodyState3D::get_principal_inertia_axes().
  • This is now the only case where we have Basis2D/Basis asymmetric naming.
  • It is also the only case where a geometric quantity is not efficiently representable as a Variant, since there is no VariantType::Basis2D. This is yet another asymmetry with Basis (3D), which does have such a representation.

In general, we add a lot of extra API surface. Godot 4 is already significantly more complex than Godot 3 regarding geometric types, with the addition of integer vectors, 4D vectors or projections. I don't think we're doing users a favor by adding yet another type, which has no Godot support and thus not that much use outside the Rust-facing Transform2D class.

I would like to hear a bit about your motivation to add Basis2 though, I'm sure you put some thought into it 🙂


Testing

You have a lot of tests of the form assert_somehow_eq!(a, b, how) which is then repeated for dozens of different a, b and hows.

Would it make sense to use a table here? This is for example done in godot-codegen/tests.rs. Simple left-aligned formatting like test_enumerator_names might help readability. An option is to have simply one table per different how variable. Then you could just loop through the array and have the how directly in the loop body.

The main gain would be to avoid passing the 3rd parameter all the time. If the a and b expressions are super long, the benefits may be limited though...

.gitignore Outdated Show resolved Hide resolved
Comment on lines 208 to 235
assert_eq_approx!(
lerp_angle(-5.0 * TAU, angle + 3.0 * TAU, 0.5).sin(),
(angle / 2.0).sin(),
is_equal_approx
);
assert_eq_approx!(
lerp_angle(-5.0 * TAU, angle + 3.0 * TAU, 0.5).cos(),
(angle / 2.0).cos(),
is_equal_approx
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why compare sine and cosine separately? Didn't we have a function to compare angles? 🙂

Copy link
Member Author

@lilizoey lilizoey Feb 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we talked about it but i dont think we ever made one. If we do though i can change it. I could replace this with a sin_cos() call instead tho.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say it's worth a separate function/macro for angle comparison, already for expressivity. It's for sure not the last time we're going to use it 😉

Comment on lines 11 to 15
use super::{
glam_helpers::{GlamConv, GlamType},
inner::InnerProjection,
Plane, Transform3D, Vector2, Vector4,
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the flat import style.

/// A Projection with all values initialized to 0. When applied to other
/// data structures, they will be zeroed.
///
/// _Godot equivalent: Projection.ZERO
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing end of italics

pub const ZERO: Self = Self::from_diagonal(0.0, 0.0, 0.0, 0.0);

/// Create a new projection from a list of column vectors.
#[must_use]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned elsewhere, only use #[must_use] in cases of ambiguity due to naming. Constructors don't need it.

}
}

impl GlamType for Mat4 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we keep glam types fully-qualified, i.e. glam::Mat4? This helps recognizability of foreign types, and there are not too many places where we need to name them, thanks to the convenience methods.

}

/// A projections clipping plane.
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick, but try to use consistent order of #[derive] arguments (everywhere, not just this line).

This is not 100% uniform across the existing code, but maybe we could use the following order (pick only the ones you need)?

#[derive(Copy, Clone, Eq, PartialEq, Hash, Ord, PartialOrd, Debug)]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from what i can tell the most common order is:

#[derive(Default, Copy, Clone, Eq, PartialEq, Debug, Ord, PartialOrd, Hash)]

I can do what you said, though i dont like either ordering.
Personally i'd probably prefer:

#[derive(Debug, Default, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)]

Especially i think it makes sense to have traits derived in the order they're required, for instance PartialEq before Eq because Eq depends on PartialEq.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order Eq, PartialEq has the advantage that you see the important traits first. Which was also the rationale of having Debug at the end.

You would then also have grouping by:

  • Ways to create an object: Default, Copy, Clone
  • Equivalence and ordering: Eq, PartialEq, Hash, Ord, PartialOrd
  • Printing: Debug

So maybe:

#[derive(Default, Copy, Clone, Eq, PartialEq, Hash, Ord, PartialOrd, Debug)]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me it just seems strange to have Eq before PartialEq, for three main reasons:

  1. rust-analyzer suggests the ordering PartialEq, Eq

  2. The main behavior of Eq/Ord/Clone actually comes from the trait they depend on. In order to compare two functions with ==, it needs to implement PartialEq. Eq does not enable you to use ==, it just makes it usable in more contexts that require equality. Similar for the other two.

  3. when i read code, especially a single line, it usually is the case that things are ordered in the order they happen.

like if i do some kind of iterator chain i'd do

thing.iter().map(something).filter(another_thing).collect()

Which here describes the operation happening from left to right.

Now supertrait relationships are something that is ordered, just like iterator operations. For something to have Eq it must first have PartialEq. And while the order in the code doesn't matter to rust, to me it still makes sense to order things in the order they mentally "happen" anyway.

Ordering things by importance instead of the order in which they "happen" makes sense if you're encapsulating information. Like if im creating a struct relationship like this:

pub struct Foo {
    field: Bar
}

struct Bar { ... }

It makes sense that Bar comes after Foo, because it is Foo that is most important here, even though it depends on Bar. And if we were to do it the other way around, we'd get cluttered and more difficult to read code. However here we have a couple of differences still. for one a struct definition is generally speaking multiple lines of code, whereas a derive is a single word. and also here we do still name Bar inside of Foo to say that Foo depends on Bar before we actually do anything else that depends on Bar, like implementing a function that uses it.

So my general my point is that things should at least be named before something that depends on it is named, as examples:

  • type definitions come before type-impls
  • constructors usually come before functions that act on an object constructed by the constructor
  • constants are declared before they're used
  • variables are declared before they're used
  • use-statements come before the imported names are used

etc.

But ordering derives like Eq, PartialEq to me violates that principle, as Eq depends on PartialEq.
The times when this principle is in some way violated is usually because doing it the other way creates a lot of clutter and difficulty reading the code. like in the Foo/Bar example above. but that's not really the case for Eq/PartialEq.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the elaborate rationale!

I also came across rustfmt discussions, although they were never agreed upon and split closely-related traits like Clone and Default, which is not nice. Since there is no tooling to do this, we also don't benefit much from adopting it...


rust-analyzer suggests the ordering PartialEq, Eq

Well, and CLion suggests Eq, PartialEq 😉
grafik
grafik

Does RA really consistently suggest the other one, or did it learn from how you tend to list the two?


So my general my point is that things should at least be named before something that depends on it is named, as examples:

I mostly agree in the examples you mentioned, but there are also cases where we don't do that, like functions calls in the same file (often "downward") or macro rules that are recursive. Anyway, I'm not sure if this is applicable here. First and foremost, a reader should recognize the semantics of the type as quickly as possible, and in my opinion ordering by importance achieves that better than by dependency. I'll explain why:

Here, we have a case where some traits are implied by others. In other words, no one cares about PartialEq, PartialOrd or Clone if Eq, Ord or Copy are derived. It's a limitation of the language that implied traits cannot be inferred, because the extra information we get by specifying them is zero. It's boilerplate we have to write, because Rust says so.

And this is why I would list the ones which convey more information first: Copy before Clone, Eq before PartialEq, Ord before PartialOrd.

Copy link
Member Author

@lilizoey lilizoey Mar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does RA really consistently suggest the other one, or did it learn from how you tend to list the two?

It has always suggested PartialX before X:
Screenshot_20230302_193502

Here, we have a case where some traits are implied by others.

The way i see it, PartialEq and Eq are connected but i dont consider them subtyped really. Because rust doesn't actually have subtypes. PartialEq does one thing, and Eq another. PartialEq is what allows you to do ==, whereas Eq allows you to use it in more places.

So the more i think about it, i honestly feel like PartialEq is more important to me than Eq. Because i usually just care about if i can do == or not. Whether it's a total equality vs partial equality is usually not that important, and that's what the presence/absence of Eq indicates.

Comment on lines 436 to 428
/// The eye to create a projection for, when creating a projection adjusted
/// for head-mounted displays.
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
#[repr(C)]
pub enum Eye {
Left = 1,
Right = 2,
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great abstraction!

Keep in mind that we re-export all those identifiers via prelude, so it might be worth adding context. This could be named ProjectionEye for example.

Comment on lines 101 to 103
pub fn get_euler(self, order: EulerOrder) -> Vector3 {
Basis::from_quat(self).to_euler(order)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yeah, i think that's what it was like before so i didn't think to change it, i just replaced the ordering type. but yeah i can do that

Comment on lines 129 to 131
assert_inner_equivalent!(InnerBasis, TEST_BASIS.inverse(), |a, b| {
Basis::is_equal_approx(&a, &b)
});
Copy link
Member

@Bromeon Bromeon Feb 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: this change may be superseded by my suggestion to use tables in the main comment thread.

Can we not pass Basis::is_equal_approx directly? Maybe due to the extra referencing?
If not, we should at least consider storing the closure in a variable rather than repeating it again and again.

@Bromeon
Copy link
Member

Bromeon commented Feb 27, 2023

bors try

bors bot added a commit that referenced this pull request Feb 27, 2023
@bors
Copy link
Contributor

bors bot commented Feb 27, 2023

try

Build succeeded:

Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About f32/f64, we didn't conclude that yet, and I don't think accepting f32 is necessarily the better option. Godot typicall uses f64 (double) for their FFI, so we artificially limit precision. In addition, we enforce lots of conversions, both internally and for the user (they get values as f64 but need to pass them back as f32).

Either case, this asymmetry seems inconsistent. Maybe we should just stick to f64 for now, and later consider replacing it with a real type?

Comment on lines 268 to 271
major: f32,
fst: glam::Vec2,
snd: glam::Vec2,
rest: glam::Vec2,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These parameter names are not very clear. I assume fst/snd mean 1st/2nd? What about the others?

Copy link
Member Author

@lilizoey lilizoey Mar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm honestly not sure what to name these. But my idea for these were roughly:
There are two main cases
Case one: where -1 < major < 1. so i named it major since it decides where to branch
Then in the equal case, we have one element be based off of major, and then we have fst and snd in order (though i realize now after some refactoring i've managed to flip those two around)
Otherwise, use the values from the rest vector

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the algorithm is based on this mapping, so im not sure what to name it entirely. the wikipedia article just uses s1s2 etc. for the different values/rows.

Comment on lines +28 to +40
#[rustfmt::skip]
let mappings_transform = [
("affine_inverse", inner.affine_inverse(), outer.affine_inverse() ),
("orthonormalized", inner.orthonormalized(), outer.orthonormalized() ),
("rotated", inner.rotated(1.0), outer.rotated(1.0) ),
("rotated_local", inner.rotated_local(1.0), outer.rotated_local(1.0) ),
("scaled", inner.scaled(vec), outer.scaled(vec) ),
("scaled_local", inner.scaled_local(vec), outer.scaled_local(vec) ),
("translated", inner.translated(vec), outer.translated(vec) ),
("translated_local", inner.translated_local(vec), outer.translated_local(vec) ),
("interpolate_with", inner.interpolate_with(Transform2D::IDENTITY, 0.5), outer.interpolate_with(Transform2D::IDENTITY, 0.5))
];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! 🤩

inner.get_rotation(),
outer.rotation(),
|a, b| is_equal_approx(a as f32, b),
"function = get_rotation\n"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe also use function: get_rotation (colon instead of equals, like above)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh woops

@lilizoey
Copy link
Member Author

lilizoey commented Mar 2, 2023

Godot typicall uses f64 (double) for their FFI (...)

yeah, but none of the functions actually use doubles on godot's side unless you enable precision=double, which isn't enabled by default (also i think all our other code will break if you do that since we're assuming single precision). I can make them f64 tho for now if you want.

@Bromeon
Copy link
Member

Bromeon commented Mar 2, 2023

but none of the functions actually use doubles on godot's side unless you enable precision=double, which isn't enabled by default

Hm, maybe that's then another topic to tackle soon... Yes, f64 for now would be nice.

@lilizoey
Copy link
Member Author

lilizoey commented Mar 2, 2023

but none of the functions actually use doubles on godot's side unless you enable precision=double, which isn't enabled by default

Hm, maybe that's then another topic to tackle soon... Yes, f64 for now would be nice.

only for Projection or should i do so for all the classes? keeping in mind that all the float-vectors also use f32s for now which idk if i should do anything about rn.

@Bromeon
Copy link
Member

Bromeon commented Mar 2, 2023

Yeah, you can keep the rest for now. I checked godot-cpp today, and indeed they use real_t for Projection methods, which is defined through conditional compilation.

@Bromeon
Copy link
Member

Bromeon commented Mar 4, 2023

Thanks a lot for this huge PR! Some minor things (like #[derive] order) are not yet concluded, but I don't think this justifies delaying the PR -- I'll address them in a future cleanup.

Could you maybe combine some of the commits? Doesn't have to be a single one, but more separated by logical parts rather than how the review was implemented. Or just 1 if that's the easiest.

let row_b = self.rows[1].to_glam();
let row_c = self.rows[2].to_glam();

match order {
Copy link
Member

@Bromeon Bromeon Mar 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume the the remaining two branches XYZ and YXZ cannot be integrated into to_euler_inner()? I see the issues with the code, but from a conceptual level, shouldn't those be somewhat symmetric?

Otherwise it should at least be possible to make a different function that handles both XYZ and YXZ.

@Bromeon
Copy link
Member

Bromeon commented Mar 4, 2023

bors try

bors bot added a commit that referenced this pull request Mar 4, 2023
@bors
Copy link
Contributor

bors bot commented Mar 4, 2023

try

Build succeeded:

@Bromeon
Copy link
Member

Bromeon commented Mar 4, 2023

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 4, 2023

Build succeeded:

@bors bors bot merged commit 72870d1 into godot-rust:master Mar 4, 2023
@Bromeon Bromeon mentioned this pull request Mar 6, 2023
@lilizoey lilizoey deleted the feature/matrix branch April 16, 2023 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: core Core components feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants