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

Kick Quat out of VectorSpace #12762

Closed
mweatherley opened this issue Mar 28, 2024 · 5 comments
Closed

Kick Quat out of VectorSpace #12762

mweatherley opened this issue Mar 28, 2024 · 5 comments
Labels
A-Math Fundamental domain-agnostic mathematical operations C-Bug An unexpected or incorrect behavior D-Trivial Nice and easy! A great choice to get started with Bevy S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!

Comments

@mweatherley
Copy link
Contributor

Follow-up for #12747

I have filed this as a bug, but it's really more of a matter of philosophy than anything (and it's not really an enhancement...).

What is the issue?

Currently, Quat implements VectorSpace; however, glam's Quat type is supposed to represent unit quaternions (i.e. versors), which do not meaningfully form a vector space over the reals, even though the full quaternion algebra does form a real vector space.

The reason that this is bad is because a lot of vector space operations really don't make sense for Quat; chief among these is VectorSpace::ZERO, which always produces an invalid quaternion, but in general, vector space operations will cause unit quaternions to denormalize. In other words, VectorSpace is at best a collection of convenience methods for quaternions, but they have extremely poor semantics.

What is the alternative?

Let's fall in line with the philosophy we landed on for colors: Unit quaternions are not a vector space, and if you want to pretend they are, you can embed them in Vec4 and perform explicit conversions. e.g.:

let sum_quat = Quat::from_vec4(Vec4::from(quat1) + Vec4::from(quat2)).normalize()
@mweatherley mweatherley added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Mar 28, 2024
@BD103 BD103 added A-Math Fundamental domain-agnostic mathematical operations and removed S-Needs-Triage This issue needs to be labelled labels Mar 28, 2024
@BD103
Copy link
Member

BD103 commented Mar 28, 2024

Not sure how to label this kind of design issue, so I'm going to leave it marked as bug. :)

@rlidwka
Copy link
Contributor

rlidwka commented Mar 28, 2024

glam's Quat type is supposed to represent unit quaternions (i.e. versors), which do not meaningfully form a vector space over the reals

Unit quaternions are isomorphic to rotations, and rotations do form a perfectly valid vector space.

edit: sorry, I said dumb thing, 3d rotations don't commute

@mweatherley
Copy link
Contributor Author

mweatherley commented Mar 28, 2024

In particular, unit quaternions form a vector space over R, where zero vector is x=0, y=0, z=0, w=1, vector addition is quaternion multiplication, and multiplication of vector by scalar is exponentiation of quaternion by said scalar.

For this to produce anything like a vector space, the quaternions would have to be Abelian. They are not; in fact, SU(2) has rank 1, and its Lie algebra su(2) doesn't even have any Lie subalgebras of dimension 2 (hence no Abelian ones). In other words: it has to be a miracle for addition between two elements to even commute.

@SolarLiner
Copy link
Contributor

So the action here is to remove the VectorSpace impl from Quat, right? If so, I'm marking this as a good first issue as the changes are straightforward, and the impl hasn't been there long enough for it to be an effective breaking change.

@SolarLiner SolarLiner added D-Trivial Nice and easy! A great choice to get started with Bevy S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! labels Mar 29, 2024
@mweatherley
Copy link
Contributor Author

So the action here is to remove the VectorSpace impl from Quat, right? If so, I'm marking this as a good first issue as the changes are straightforward, and the impl hasn't been there long enough for it to be an effective breaking change.

Yep! Technically NormedVectorSpace too, but it's all in one place. We did release a version with Quat as a Point, so it's breaking in that sense (at least, either way it should probably be mentioned in release notes).

github-merge-queue bot pushed a commit that referenced this issue Mar 30, 2024
- Fixes #[12762](#12762).

## Migration Guide

- `Quat` no longer implements `VectorSpace` as unit quaternions don't
actually form proper vector spaces. If you're absolutely certain that
what you're doing is correct, convert the `Quat` into a `Vec4` and
perform the operations before converting back.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Math Fundamental domain-agnostic mathematical operations C-Bug An unexpected or incorrect behavior D-Trivial Nice and easy! A great choice to get started with Bevy S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!
Projects
None yet
Development

No branches or pull requests

5 participants