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 Godot methods for Quat #720

Merged
merged 8 commits into from Aug 23, 2021
Merged

Implement Godot methods for Quat #720

merged 8 commits into from Aug 23, 2021

Conversation

Demindiro
Copy link
Contributor

@Demindiro Demindiro commented Apr 4, 2021

No description provided.

@Demindiro
Copy link
Contributor Author

glam's from_quat method calls this internally:

    #[inline]
    fn from_quaternion(rotation: XYZW<T>) -> Self {
        glam_assert!(rotation.is_normalized());
        let x2 = rotation.x + rotation.x;
        let y2 = rotation.y + rotation.y;
        let z2 = rotation.z + rotation.z;
        let xx = rotation.x * x2;
        let xy = rotation.x * y2;
        let xz = rotation.x * z2;
        let yy = rotation.y * y2;
        let yz = rotation.y * z2;
        let zz = rotation.z * z2;
        let wx = rotation.w * x2;
        let wy = rotation.w * y2;
        let wz = rotation.w * z2;

        Self::from_cols(
            V3::new(T::ONE - (yy + zz), xy + wz, xz - wy),
            V3::new(xy - wz, T::ONE - (xx + zz), yz + wx),
            V3::new(xz + wy, yz - wx, T::ONE - (xx + yy)),
        )
    }

Godot's Basis constructor uses set_quat internally and looks like this:

void Basis::set_quat(const Quat &p_quat) {

        real_t d = p_quat.length_squared();
        real_t s = 2.0 / d;
        real_t xs = p_quat.x * s, ys = p_quat.y * s, zs = p_quat.z * s;
        real_t wx = p_quat.w * xs, wy = p_quat.w * ys, wz = p_quat.w * zs;
        real_t xx = p_quat.x * xs, xy = p_quat.x * ys, xz = p_quat.x * zs;
        real_t yy = p_quat.y * ys, yz = p_quat.y * zs, zz = p_quat.z * zs;
        set(1.0 - (yy + zz), xy - wz, xz + wy,
                xy + wz, 1.0 - (xx + zz), yz - wx,
                xz - wy, yz + wx, 1.0 - (xx + yy));
}

The implementations are different, which could be the cause of the inaccuracy. Then again, the to_basis test does pass...

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.

Thanks a lot for the contribution! 🙂
I left some comments.

gdnative-core/src/core_types/quat.rs Outdated Show resolved Hide resolved
gdnative-core/src/core_types/quat.rs Show resolved Hide resolved
gdnative-core/src/core_types/quat.rs Outdated Show resolved Hide resolved
gdnative-core/src/core_types/quat.rs Outdated Show resolved Hide resolved
gdnative-core/src/core_types/quat.rs Show resolved Hide resolved
gdnative-core/src/core_types/quat.rs Show resolved Hide resolved
gdnative-core/src/core_types/quat.rs Outdated Show resolved Hide resolved
gdnative-core/src/core_types/quat.rs Outdated Show resolved Hide resolved
@Bromeon Bromeon added c: core Component: core (mod core_types, object, log, init, ...) feature Adds functionality to the library labels Apr 4, 2021
@Bromeon Bromeon added this to the 0.10 milestone Apr 4, 2021
@Demindiro Demindiro marked this pull request as ready for review April 5, 2021 18:40
@Demindiro Demindiro requested a review from Bromeon April 5, 2021 18:44
setzer22 added a commit to setzer22/godot-rust that referenced this pull request Jun 27, 2021
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.

Looks mostly good, added a last few cosmetic remarks.
In my opinion this can be merged afterwards 🙂

gdnative-core/src/core_types/quat.rs Outdated Show resolved Hide resolved
gdnative-core/src/core_types/quat.rs Outdated Show resolved Hide resolved
gdnative-core/src/core_types/quat.rs Show resolved Hide resolved
gdnative-core/src/core_types/quat.rs Show resolved Hide resolved
gdnative-core/src/core_types/quat.rs Show resolved Hide resolved
@Demindiro Demindiro force-pushed the quat branch 2 times, most recently from aa9be58 to 1c0918e Compare July 6, 2021 18:46
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.

Missed your comments at first. Hope I got them all now 🙂

gdnative-core/src/core_types/quat.rs Show resolved Hide resolved
gdnative-core/src/core_types/quat.rs Show resolved Hide resolved
gdnative-core/src/core_types/quat.rs Show resolved Hide resolved
@Demindiro Demindiro requested a review from Bromeon July 12, 2021 09:59
@Bromeon
Copy link
Member

Bromeon commented Aug 7, 2021

bors try

@bors
Copy link
Contributor

bors bot commented Aug 7, 2021

try

Merge conflict.

There are still some tests lacking & `get_euler` has a large error for
some reason.
* s/core::/std::
* Add documentation to IDENTITY constant
* s/get_euler/to_euler
* Use Basis::to_euler for to_euler instead. Incidentally, this fixes the
  precision issue in to_the euler test.
* Remove set_axis_angle and set_euler
* Make Basis::to_quat private. The implementation could perhaps be moved
  to Quat::from_basis instead?
The slerp test fails because the result diverges a lot. Using `slerp`
instead of `lerp` causes the result to be completely different.
@Bromeon
Copy link
Member

Bromeon commented Aug 7, 2021

Thanks for the quick adaptation! I'm waiting for @setzer22's feedback (who is a big user of quaternions), after that we can merge 🙂

bors try

bors bot added a commit that referenced this pull request Aug 7, 2021
@bors
Copy link
Contributor

bors bot commented Aug 7, 2021

try

Build failed:

@Bromeon
Copy link
Member

Bromeon commented Aug 7, 2021

bors try

bors bot added a commit that referenced this pull request Aug 7, 2021
@bors
Copy link
Contributor

bors bot commented Aug 7, 2021

try

Build succeeded:

@Bromeon
Copy link
Member

Bromeon commented Aug 23, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Aug 23, 2021

Build succeeded:

@bors bors bot merged commit 7ffe859 into godot-rust:master Aug 23, 2021
@Bromeon
Copy link
Member

Bromeon commented Aug 23, 2021

Thanks a lot for the work @Demindiro and for the (off-site) feedback @setzer22!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: core Component: core (mod core_types, object, log, init, ...) feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants