#310 Quaternion Function Completeness #565

0awful · 2024-01-13T00:54:56Z

Implements Quaternion functions for #310.
Added tests for from_angle_axis(). Discovered from_angle_axis() was inconsistent with gdscript.

See discord thread.

We may want to alter the entire quaternion API surface based on thread resolution.

Thread has more context, but in short:

Quaternions in godot silently return default when functions or initialization fail
Several Quaternion Functions require normalized quaternions and will fail if they are not normalized.

What should we do?
Possible solutions:

Do what gdscript does and silently return default
panic
return optional/result

Current implementation opts for 1 but I'd prefer 3.

GodotRust · 2024-01-13T00:59:59Z

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

Bromeon

As discussed on Discord, I would vote for 2) panic -- reasoning is here. It should have a # Panic section.

Option 1) is bad because it hides logic errors, and 3) doesn't seem that useful because not normalizing inputs is almost always a bug and not something to react meaningfully at runtime.

godot-core/src/builtin/quaternion.rs

itest/rust/src/builtin_tests/geometry/quaternion_test.rs

0awful · 2024-01-13T17:35:05Z

As discussed on Discord, I would vote for 2) panic -- reasoning is here. It should have a # Panic section.

I agree. Are you alright with maintaining the overloading pattern called out in that doc here. One panicking method and one that returns optional/result?

Bromeon · 2024-01-13T18:12:46Z

I agree. Are you alright with maintaining the overloading pattern called out in that doc here. One panicking method and one that returns optional/result?

Unlike for try_cast, I don't see the use case for fallible construction here -- how can an application meaningfully react to this error at runtime?

0awful · 2024-01-13T18:19:52Z

Unlike for try_cast, I don't see the use case for fallible construction here -- how can an application meaningfully react to this error at runtime?

I understand. Yeah there really isn't a path for recovery. If you have hit that error you obviously have a bug. 👍

Based on that normalized() and the math functions I called out should also have panic! paths.

godot-core/src/builtin/quaternion.rs

0awful · 2024-01-14T20:36:12Z

godot-core/src/builtin/quaternion.rs

+    /// Performs slerp
+    ///
+    /// # Panics
+    /// If either quaternion is not normalized
    pub fn slerp(self, to: Self, weight: real) -> Self {


Abandoned the hand-rolled solution when I discovered that it didn't handle non-normalized vectors by erroring as it should.

0awful · 2024-01-14T20:36:47Z

godot-core/src/builtin/quaternion.rs

+    /// Performs slerpni
+    ///
+    /// # Panics
+    /// If either quaternion is not normalized
    pub fn slerpni(self, to: Self, weight: real) -> Self {


Same story as slerp. Abandoned because it did not appropriately error on non-normalized vectors.

0awful · 2024-01-15T19:57:17Z

As an aside. I'm personally not a fan of how the scope of this kept expanding between review cycles. I'll look to mitigate that going forward as it strikes me as an unfair burden to reviewers (aka @Bromeon 😂)

godot-core/src/builtin/quaternion.rs

Bromeon

Thanks a lot! Great tests as well 🙂

I'll look to mitigate that going forward as it strikes me as an unfair burden to reviewers

I see it as an unfair burden for authors to get extra work on their plate 😅

But yes, maybe don't add anymore (like constructors) in this PR. It also makes it a bit harder to see what's already reviewed and what's new.

godot-core/src/builtin/quaternion.rs

Bromeon · 2024-01-15T20:12:01Z

godot-core/src/builtin/quaternion.rs

    pub fn normalized(self) -> Self {
-        self / self.length()
+        let length = self.length();
+        assert!(!length.approx_eq(&0.0), "Quaternion has length 0");


Note to self: we could maybe consider a method ApproxEq::approx_zero(&self). But not in this PR 😉

godot-core/src/builtin/quaternion.rs

itest/rust/src/builtin_tests/geometry/quaternion_test.rs

godot-core/src/builtin/quaternion.rs

itest/rust/src/builtin_tests/geometry/quaternion_test.rs

Bromeon

Thanks a lot!

Bromeon · 2024-01-20T08:49:58Z

Several tests failed in release mode:
https://github.com/godot-rust/gdext/actions/runs/7593093187/job/20683183275

0awful · 2024-01-20T18:23:23Z

Several tests failed in release mode:

https://github.com/godot-rust/gdext/actions/runs/7593093187/job/20683183275

I'll take a look. Sorry I've been caught up in stuff this week. I'll get those changes as well.

0awful · 2024-01-20T18:48:24Z

-- quaternion_slerp ... ERROR: Rust function panicked in file itest/rust/src/framework/mod.rs at line 120. Context: itest `quaternion_slerp` failed
at: <function unset> (godot-core/src/lib.rs:161)
ERROR: Panic msg:  code should have panicked but did not: Slerp requires normalized quaternions
at: <function unset> (godot-core/src/lib.rs:109)
-- quaternion_slerp ... FAILED
-- quaternion_slerpni ... ERROR: Rust function panicked in file itest/rust/src/framework/mod.rs at line 120. Context: itest `quaternion_slerpni` failed
at: <function unset> (godot-core/src/lib.rs:161)
ERROR: Panic msg:  code should have panicked but did not: Slerpni requires normalized quaternions
at: <function unset> (godot-core/src/lib.rs:109)
-- quaternion_slerpni ... FAILED
-- quaternion_spherical_cubic_interpolate ... ERROR: Rust function panicked in file itest/rust/src/framework/mod.rs at line 120. Context: itest `quaternion_spherical_cubic_interpolate` failed
at: <function unset> (godot-core/src/lib.rs:161)
ERROR: Panic msg:  code should have panicked but did not: Spherical cubic interpolation requires normalized quaternions
at: <function unset> (godot-core/src/lib.rs:109)
-- quaternion_spherical_cubic_interpolate ... FAILED
-- quaternion_spherical_cubic_interpolate_in_time ... ERROR: Rust function panicked in file itest/rust/src/framework/mod.rs at line 120. Context: itest `quaternion_spherical_cubic_interpolate_in_time` failed
at: <function unset> (godot-core/src/lib.rs:161)
ERROR: Panic msg:  code should have panicked but did not: Spherical cubic interpolation in time requires normalized quaternions
at: <function unset> (godot-core/src/lib.rs:109)
-- quaternion_spherical_cubic_interpolate_in_time ... FAILED

Related error. Panics are not happening within release mode for linux only. ~~Did it test other platforms in release mode?~~

0awful · 2024-01-20T18:59:31Z

Did it test other platforms in release mode?

Digging suggests it only tests release mode on linux. I run linux so I can replicate that locally. assert! does still panic in release mode. (assert_debug! does not).

0awful · 2024-01-20T19:41:17Z

Hunch is release mode handles quaternion error states differently. Evaluating.

0awful · 2024-01-20T20:05:56Z

Ugh. So it returns self if you call any interpolation function without normalized quaternions in release mode.

The solution of checking is_default() is too brittle. @Bromeon what do you think about always calling ensure_normalized() within interpolation functions?

Bromeon · 2024-01-21T09:22:39Z

Ugh. So it returns self if you call any interpolation function without normalized quaternions in release mode.

Godot does that, right?
Yes, in that case, calling ensure_normalized is probably better.

I'm glad I introduced release builds to CI, was only a month ago!

Thanks a lot for fixing it!

Bromeon reviewed Jan 13, 2024

View reviewed changes

godot-core/src/builtin/quaternion.rs Outdated Show resolved Hide resolved

godot-core/src/builtin/quaternion.rs Outdated Show resolved Hide resolved

itest/rust/src/builtin_tests/geometry/quaternion_test.rs Outdated Show resolved Hide resolved

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

0awful force-pushed the #310/Quaternion branch 3 times, most recently from a196c08 to 31d7c7d Compare January 13, 2024 19:19

0awful marked this pull request as ready for review January 13, 2024 19:19

0awful force-pushed the #310/Quaternion branch from 31d7c7d to d65adfa Compare January 13, 2024 19:20

0awful requested a review from Bromeon January 13, 2024 19:24

Bromeon reviewed Jan 13, 2024

View reviewed changes

0awful force-pushed the #310/Quaternion branch from d65adfa to ad34119 Compare January 14, 2024 20:33

0awful commented Jan 14, 2024

View reviewed changes

0awful requested a review from Bromeon January 14, 2024 20:40

0awful commented Jan 15, 2024

View reviewed changes

godot-core/src/builtin/quaternion.rs Outdated Show resolved Hide resolved

Bromeon reviewed Jan 15, 2024

View reviewed changes

0awful force-pushed the #310/Quaternion branch from ad34119 to 4c2d189 Compare January 17, 2024 02:46

Bromeon reviewed Jan 18, 2024

View reviewed changes

Bromeon approved these changes Jan 20, 2024

View reviewed changes

Bromeon added this pull request to the merge queue Jan 20, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 20, 2024

0awful force-pushed the #310/Quaternion branch 2 times, most recently from bbde3cd to 3a1e061 Compare January 20, 2024 19:13

function-completeness

fd68cb1

0awful force-pushed the #310/Quaternion branch from 3a1e061 to fd68cb1 Compare January 20, 2024 21:09

0awful requested a review from Bromeon January 20, 2024 21:10

Bromeon added this pull request to the merge queue Jan 21, 2024

Merged via the queue into godot-rust:master with commit 48e1d65 Jan 21, 2024
16 checks passed

0awful mentioned this pull request Jan 21, 2024

Tracking issue for builtin feature-completeness #310

Open

33 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#310 Quaternion Function Completeness #565

#310 Quaternion Function Completeness #565

0awful commented Jan 13, 2024

GodotRust commented Jan 13, 2024

Bromeon left a comment •

edited

Loading

0awful commented Jan 13, 2024 •

edited

Loading

Bromeon commented Jan 13, 2024

0awful commented Jan 13, 2024 •

edited

Loading

0awful Jan 14, 2024

0awful Jan 14, 2024

0awful commented Jan 15, 2024

Bromeon left a comment

Bromeon Jan 15, 2024

Bromeon left a comment

Bromeon commented Jan 20, 2024

0awful commented Jan 20, 2024

0awful commented Jan 20, 2024 •

edited

Loading

0awful commented Jan 20, 2024 •

edited

Loading

0awful commented Jan 20, 2024

0awful commented Jan 20, 2024

Bromeon commented Jan 21, 2024 •

edited

Loading

#310 Quaternion Function Completeness #565

#310 Quaternion Function Completeness #565

Conversation

0awful commented Jan 13, 2024

GodotRust commented Jan 13, 2024

Bromeon left a comment • edited Loading

Choose a reason for hiding this comment

0awful commented Jan 13, 2024 • edited Loading

Bromeon commented Jan 13, 2024

0awful commented Jan 13, 2024 • edited Loading

0awful Jan 14, 2024

Choose a reason for hiding this comment

0awful Jan 14, 2024

Choose a reason for hiding this comment

0awful commented Jan 15, 2024

Bromeon left a comment

Choose a reason for hiding this comment

Bromeon Jan 15, 2024

Choose a reason for hiding this comment

Bromeon left a comment

Choose a reason for hiding this comment

Bromeon commented Jan 20, 2024

0awful commented Jan 20, 2024

0awful commented Jan 20, 2024 • edited Loading

0awful commented Jan 20, 2024 • edited Loading

0awful commented Jan 20, 2024

0awful commented Jan 20, 2024

Bromeon commented Jan 21, 2024 • edited Loading

Bromeon left a comment •

edited

Loading

0awful commented Jan 13, 2024 •

edited

Loading

0awful commented Jan 13, 2024 •

edited

Loading

0awful commented Jan 20, 2024 •

edited

Loading

0awful commented Jan 20, 2024 •

edited

Loading

Bromeon commented Jan 21, 2024 •

edited

Loading