Add extension trait for floats

[lilizoey]

Adds an extension trait FloatExt that implements the various utility methods we have directly on f32 and f64.

This is implemented for both f64 and f32 instead of just real to make it easier to use them everywhere. Since godot float values are always f64 and real may be either f32 or f64 depending on configuration.

Also moves a bunch of methods in Vector2 and Vector3 that used these methods into a macro, as well as their ApproxEq impls. And used that macro to implement those methods for Vector2, Vector3 and Vector4. (Vector4 in godot doesn't actually contain the bezier functions but i figured it's fine to add them.)

Also split up the tests for eq_approx for floats into f32 and f64 variants, which also makes it so we dont need to use cfg in the one test.

labelled QOL despite this technically adding a couple of functions to Vector4, since it's mainly just about improving ergonomics of float functions.

[GodotRust]

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

[Bromeon]

Thanks! While this makes sense from a technical perspective, there are some smaller things to discuss about the resulting API.

Some names are now a bit confusing or make it hard to see that they're from Godot: f.sign() vs. f.signum() from the standard library.

Others already have a trait: f.approx_eq(other) -- one should be removed, possibly with #[doc(alias)] in the ApproxEq trait itself.

For many functions, the receiver syntax is a bit less clear as it consumes the from argument, introducing asymmetry: lerp_angle(from, to, weight) vs. from.lerp_angle(to, weight). It's also different from GDScript for floats. At least it's more consistent with vectors, and we could theoretically use the fully qualified syntax... I guess this is just a matter of getting used to?

Also, if we refactor those, are there ergonomic improvements we could do? Some methods take a big number of parameters, but I guess encapsulating just makes matters worse. Other ideas? (This could of course be done separately...)

---

Apart from that, did you change any of the implementations? It's impossible for me to see because you moved everything to another file and changed indentation, so git cannot detect changes. Just to know what I should review 🙂

[Bromeon]

This use of the warning sign is incorrect, according to

gdext/godot/src/lib.rs

Lines 81 to 85 in 565bf10

	//! To help you identify panicking methods, we use the symbol "⚠️" at the beginning of the documentation;
	//! this should also appear immediately in the auto-completion of your IDE. Note that this warning sign is
	//! not used as a general panic indicator, but particularly for methods which have a `Option`/`Result`-based
	//! overload. If you want to know whether and how a method can panic, check if its documentation has a
	//! _Panics_ section.

[lilizoey]

oh i wasn't aware of that distinction. fair enough.

[Bromeon]

We can also discuss it of course. I thought it would be redundant if we already have a "Panics" section that had the exact same meaning. But maybe it helps in IDE suggestions... 🤷

[Bromeon]

Also:

This is implemented for both f64 and f32 instead of just real to make it easier to use them everywhere. Since godot float values are always f64 and real may be either f32 or f64 depending on configuration.

That issue is not really solved by this PR, but made a bit more convenient to deal with.

The default configuration for godot is real == f32, yet all Godot APIs use f64. This means there is constant friction between the two points, already starting at fn process(f64: delta) and passing it to anything that takes real.

In some cases, real is critical for FFI safety (e.g. as a component of vectors), but in others it could maybe be passed as f64, narrowing where needed.

[lilizoey]

Thanks! While this makes sense from a technical perspective, there are some smaller things to discuss about the resulting API.

Some names are now a bit confusing or make it hard to see that they're from Godot: f.sign() vs. f.signum() from the standard library.

Yeah i wasn't sure what to do there, since signum and sign have different semantics.

Others already have a trait: f.approx_eq(other) -- one should be removed, possibly with #[doc(alias)] in the ApproxEq trait itself.

fair

For many functions, the receiver syntax is a bit less clear as it consumes the from argument, introducing asymmetry: lerp_angle(from, to, weight) vs. from.lerp_angle(to, weight). It's also different from GDScript for floats. At least it's more consistent with vectors, and we could theoretically use the fully qualified syntax... I guess this is just a matter of getting used to?

You can also just do f32::lerp_angle(from, to, weight). And i can also just implement these as not taking a self.

Also, if we refactor those, are there ergonomic improvements we could do? Some methods take a big number of parameters, but I guess encapsulating just makes matters worse. Other ideas? (This could of course be done separately...)

I'll have a look

Apart from that, did you change any of the implementations? It's impossible for me to see because you moved everything to another file and changed indentation, so git cannot detect changes. Just to know what I should review 🙂

sign:

// old
pub fn sign(value: real) -> real {
    if value == 0.0 {
        0.0
    } else if value < 0.0 {
        -1.0
    } else {
        1.0
    }
}

// new
fn sign(self) -> Self {
    use std::cmp::Ordering;

    match self.partial_cmp(&0.0) {
        Some(Ordering::Equal) => 0.0,
        Some(Ordering::Greater) => 1.0,
        Some(Ordering::Less) => -1.0,
        // NAN, Godot return `1` in this case.
        None => 1.0,
    }
}

fposmod:

// old
pub fn fposmod(x: real, y: real) -> real {
    let mut value = x % y;
    if ((value < 0.0) && (y > 0.0)) || ((value > 0.0) && (y < 0.0)) {
        value += y;
    }
    value += 0.0;
    value
}

// new
fn fposmod(self, pmod: Self) -> Self {
    let mut value = self % pmod;
    if ((value < 0.0) && (pmod > 0.0)) || ((value > 0.0) && (pmod < 0.0)) {
        value += pmod;
    }
    value
}

is_angle_equal_approx:

// old
pub fn is_angle_equal_approx(a: &real, b: &real) -> bool {
    let (x1, y1) = a.sin_cos();
    let (x2, y2) = b.sin_cos();

    println!("({x1}, {y1}) ({x2}, {y2})");

    is_equal_approx(
        Vector2::distance_to(Vector2::new(x1, y1), Vector2::new(x2, y2)),
        0.0,
    )
}

// new
fn is_angle_equal_approx(self, other: Self) -> bool {
    let (x1, y1) = self.sin_cos();
    let (x2, y2) = other.sin_cos();

    let point_1 = Vector2::new(real::$to_real(x1), real::$to_real(y1));
    let point_2 = Vector2::new(real::$to_real(x2), real::$to_real(y2));

    point_1.distance_to(point_2).is_zero_approx()
}

The rest should be identical with just some renaming (mainly real -> Self)

[Bromeon]

Yeah i wasn't sure what to do there, since signum and sign have different semantics.

I think it's OK. Maybe a short doc that appears in IDEs helps:

/// Godot sign; returns 0 for input 0.
///
/// See also [`signum`][Self::signum], which always returns -1 or +1 (or NaN).
fn sign(self) -> Self {

Is Godot's "return 1 for NaN" a feature? It looks like a bug for me, one that our previous implementation also had: the else branch returns 1.

Either way, this is completely unintuitive -- we should return NaN in that case, too.

---

The fposmod has useless parentheses around the if (already from the old code), could you remove them? Precedence of || and && should be well-known to any developer.

---

And i can also just implement these as not taking a self.

It's probably fine.

---

The rest should be identical with just some renaming (mainly real -> Self)

Thanks a lot for the side-by-side! Changes look good, comments about sign and fposmod see above.

For the future, please keep refactoring/API changes and semantics in separate commits (at least when easily possible), that allows to track such changes better 🙂

[lilizoey]

Yeah i wasn't sure what to do there, since signum and sign have different semantics.

I think it's OK. Maybe a short doc that appears in IDEs helps:

/// Godot sign; returns 0 for input 0.
///
/// See also [`signum`][Self::signum], which always returns -1 or +1 (or NaN).
fn sign(self) -> Self {

Is Godot's "return 1 for NaN" a feature? It looks like a bug for me, one that our previous implementation also had: the else branch returns 1.

Either way, this is completely unintuitive -- we should return NaN in that case, too.

seems godot somewhat agrees godotengine/godot#79036

The fposmod has useless parentheses around the if (already from the old code), could you remove them? Precedence of || and && should be well-known to any developer.

Honestly i somewhat disagree. yes i know the precedence of && and || but it's such an easy thing to accidentally get wrong, and it's easy to miss when it's wrong. so i usually add some reduntant parentheses there anyway. but i dont think we need parentheses around the < and >.

For the future, please keep refactoring/API changes and semantics in separate commits (at least when easily possible), that allows to track such changes better slightly_smiling_face

oki

[lilizoey]

Regarding other refactoring, only thing i can really think of would be to create a like bezier struct, or cubic struct of some kind to store some of the arguments to cubic/bezier interpolation. but im not sure if that'd really be worth it. maybe in the future we could make a better abstraction for doing these more advanced interpolation functions. something that we could reuse for the vectors too since they can also be interpolated.

[Bromeon]

Yep, we can always think about other abstractions :) but also, in many cases, polymorphism may not always be needed -- it's also OK if the same methods exist for different types without having an underlying trait.

There are some remaining uses of is_equal_approx that render the CI red; is the PR ready apart from that?

[lilizoey]

Yep, we can always think about other abstractions :) but also, in many cases, polymorphism may not always be needed -- it's also OK if the same methods exist for different types without having an underlying trait.

There are some remaining uses of is_equal_approx that render the CI red; is the PR ready apart from that?

oh, yeah it should be. i guess i forgot to run check locally.