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

Move Point out of cubic splines module and expand it #12747

Merged
merged 7 commits into from Mar 28, 2024

Conversation

mweatherley
Copy link
Contributor

@mweatherley mweatherley commented Mar 27, 2024

Objective

Previously, the Point trait, which abstracts all of the operations of a real vector space, was sitting in the submodule of bevy_math for cubic splines. However, the trait has broader applications than merely cubic splines, and we should use it when possible to avoid code duplication when performing vector operations.

Solution

Point has been moved into a new submodule in bevy_math named common_traits. Furthermore, it has been renamed to VectorSpace, which is more descriptive, and an additional trait NormedVectorSpace has been introduced to expand the API to cover situations involving geometry in addition to algebra. Additionally, VectorSpace itself now requires a ZERO constant and Neg. It also supports a lerp function as an automatic trait method.

Here is what that looks like:

/// A type that supports the mathematical operations of a real vector space, irrespective of dimension.
/// In particular, this means that the implementing type supports:
/// - Scalar multiplication and division on the right by elements of `f32`
/// - Negation
/// - Addition and subtraction
/// - Zero
///
/// Within the limitations of floating point arithmetic, all the following are required to hold:
/// - (Associativity of addition) For all `u, v, w: Self`, `(u + v) + w == u + (v + w)`.
/// - (Commutativity of addition) For all `u, v: Self`, `u + v == v + u`.
/// - (Additive identity) For all `v: Self`, `v + Self::ZERO == v`.
/// - (Additive inverse) For all `v: Self`, `v - v == v + (-v) == Self::ZERO`.
/// - (Compatibility of multiplication) For all `a, b: f32`, `v: Self`, `v * (a * b) == (v * a) * b`.
/// - (Multiplicative identity) For all `v: Self`, `v * 1.0 == v`.
/// - (Distributivity for vector addition) For all `a: f32`, `u, v: Self`, `(u + v) * a == u * a + v * a`.
/// - (Distributivity for scalar addition) For all `a, b: f32`, `v: Self`, `v * (a + b) == v * a + v * b`.
///
/// Note that, because implementing types use floating point arithmetic, they are not required to actually
/// implement `PartialEq` or `Eq`.
pub trait VectorSpace:
    Mul<f32, Output = Self>
    + Div<f32, Output = Self>
    + Add<Self, Output = Self>
    + Sub<Self, Output = Self>
    + Neg
    + Default
    + Debug
    + Clone
    + Copy
{
    /// The zero vector, which is the identity of addition for the vector space type.
    const ZERO: Self;

    /// Perform vector space linear interpolation between this element and another, based
    /// on the parameter `t`. When `t` is `0`, `self` is recovered. When `t` is `1`, `rhs`
    /// is recovered.
    ///
    /// Note that the value of `t` is not clamped by this function, so interpolating outside
    /// of the interval `[0,1]` is allowed.
    #[inline]
    fn lerp(&self, rhs: Self, t: f32) -> Self {
        *self * (1. - t) + rhs * t
    }
}
/// A type that supports the operations of a normed vector space; i.e. a norm operation in addition
/// to those of [`VectorSpace`]. Specifically, the implementor must guarantee that the following
/// relationships hold, within the limitations of floating point arithmetic:
/// - (Nonnegativity) For all `v: Self`, `v.norm() >= 0.0`.
/// - (Positive definiteness) For all `v: Self`, `v.norm() == 0.0` implies `v == Self::ZERO`.
/// - (Absolute homogeneity) For all `c: f32`, `v: Self`, `(v * c).norm() == v.norm() * c.abs()`.
/// - (Triangle inequality) For all `v, w: Self`, `(v + w).norm() <= v.norm() + w.norm()`.
///
/// Note that, because implementing types use floating point arithmetic, they are not required to actually
/// implement `PartialEq` or `Eq`.
pub trait NormedVectorSpace: VectorSpace {
    /// The size of this element. The return value should always be nonnegative.
    fn norm(self) -> f32;

    /// The squared norm of this element. Computing this is often faster than computing
    /// [`NormedVectorSpace::norm`].
    #[inline]
    fn norm_squared(self) -> f32 {
        self.norm() * self.norm()
    }

    /// The distance between this element and another, as determined by the norm.
    #[inline]
    fn distance(self, rhs: Self) -> f32 {
        (rhs - self).norm()
    }

    /// The squared distance between this element and another, as determined by the norm. Note that
    /// this is often faster to compute in practice than [`NormedVectorSpace::distance`].
    #[inline]
    fn distance_squared(self, rhs: Self) -> f32 {
        (rhs - self).norm_squared()
    }
}

Furthermore, this PR also demonstrates the use of the NormedVectorSpace combined API to implement ShapeSample for Triangle2d and Triangle3d simultaneously. Such deduplication is one of the drivers for developing these APIs.


Changelog

  • Point from cubic_splines becomes VectorSpace, exported as bevy::math::VectorSpace.
  • VectorSpace requires Neg and VectorSpace::ZERO in addition to its existing prerequisites.
  • Introduced public traits bevy::math::NormedVectorSpace for generic geometry tasks involving vectors.
  • Implemented ShapeSample for Triangle2d and Triangle3d.

Migration Guide

Since Point no longer exists, any projects using it must switch to bevy::math::VectorSpace. Additionally, third-party implementations of this trait now require the Neg trait; the constant VectorSpace::ZERO must be provided as well.


Discussion

Design considerations

Originally, the NormedVectorSpace::norm method was part of a separate trait Normed. However, I think that was probably too broad and, more importantly, the semantics of having it in NormedVectorSpace are much clearer.

As it currently stands, the API exposed here is pretty minimal, and there is definitely a lot more that we could do, but there are more questions to answer along the way. As a silly example, we could implement NormedVectorSpace::length as an alias for NormedVectorSpace::norm, but this overlaps with methods in all of the glam types, so we would want to make sure that the implementations are effectively identical (for what it's worth, I think they are already).

Future directions

One example of something that could belong in the NormedVectorSpace API is normalization. Actually, such a thing previously existed on this branch before I decided to shelve it because of concerns with namespace collision. It looked like this:

/// This element, but normalized to norm 1 if possible. Returns an error when the reciprocal of
/// the element's norm is not finite.
#[inline]
#[must_use]
fn normalize(&self) -> Result<Self, NonNormalizableError> {
    let reciprocal = 1.0 / self.norm();
    if reciprocal.is_finite() {
        Ok(*self * reciprocal)
    } else {
        Err(NonNormalizableError { reciprocal })
    }
}

/// An error indicating that an element of a [`NormedVectorSpace`] was non-normalizable due to having 
/// non-finite norm-reciprocal.
#[derive(Debug, Error)]
#[error("Element with norm reciprocal {reciprocal} cannot be normalized")]
pub struct NonNormalizableError {
    reciprocal: f32
}

With this kind of thing in hand, it might be worth considering eventually making the passage from vectors to directions fully generic by employing a wrapper type. (Of course, for our concrete types, we would leave the existing names in place as aliases.) That is, something like:

pub struct NormOne<T>
where T: NormedVectorSpace { //... }

Utterly separately, the reason that I implemented ShapeSample for Triangle2d/Triangle3d was to prototype uniform sampling of abstract meshes, so that's also a future direction.

@alice-i-cecile alice-i-cecile added C-Enhancement A new feature A-Math Fundamental domain-agnostic mathematical operations C-Code-Quality A section of code that is hard to understand or change C-Needs-Release-Note Work that should be called out in the blog due to impact labels Mar 27, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Much clearer code organization, better docs, and a much clearer / more motivated trait name. I also like NormedVectorSpace: it's genuinely useful and a good standard abstraction.

I'd be open to a Normed trait instead (better separation!), but I do like the scoped and well-motivated nature of NormedVectorSpace. We can revisit this if we ever need a normed non-vector space I guess.

/// A type that supports the operations of a normed vector space; i.e. a norm operation in addition
/// to those of [`VectorSpace`]. The implementor must guarantee that the axioms of a normed vector
/// space are satisfied.
pub trait NormedVectorSpace: VectorSpace {
Copy link
Contributor

Choose a reason for hiding this comment

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

The mathematician in me also wants to see MetricSpace and InnerProductSpace traits with corresponding blanket impls, but I agree with Alice that we shouldn't add traits until the need arises.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree! I suspect it will come up before too long if someone tries to use NormedVectorSpace and ends up needing a dot product.

Copy link
Contributor

@NthTensor NthTensor left a comment

Choose a reason for hiding this comment

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

Love this!

As discussed in discord, Quat doesn't really belong here. I also wonder if we should implement VectorSpace on the Glam matrix types. Either way, changing which types implement VectorSpace deserves it's own PR.

I would prefer to see an expanded trait doc before I approve.

@mweatherley
Copy link
Contributor Author

Love this!

As discussed in discord, Quat doesn't really belong here. I also wonder if we should implement VectorSpace on the Glam matrix types. Either way, changing which types implement VectorSpace deserves it's own PR.

I would prefer to see an expanded trait doc before I approve.

Makes sense! I've now rewritten the trait docs for both VectorSpace and NormedVectorSpace. I found it kind of awkward to be axiomatic without actually having zero, so I also ended up adding that to the trait (and hence to the colors' implementation macro as well).

As a result, I'm rewriting the Migration Guide a little bit as well, just to mention the presence of zero.

@alice-i-cecile
Copy link
Member

I agree with the addition of a zero element: it will make this trait much more useful.

Copy link
Contributor

@NthTensor NthTensor left a comment

Choose a reason for hiding this comment

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

Excellent! Especially nice work on the docs, I'm glad you didn't get into the weeds about approximation error. Thanks for expanding those.

The zero constant seems pretty natural. I wonder if we can remove Default from VectorSpace now. IIRC we may have used it implicitly as the zero vector somewhere and it's now the only parent trait with somewhat unspecified behavior.

Every elegant. Now lets add FiniteGroup, AproxRing, AproxModule and AproxField traits! /s

@NthTensor NthTensor added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 27, 2024
Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

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

Looks fantastic! I have some very pedantic nits as suggestions, but none are worth blocking over if anyone disagrees with me.

/// - (Associativity of addition) For all `u, v, w: Self`, `(u + v) + w == u + (v + w)`.
/// - (Commutativity of addition) For all `u, v: Self`, `u + v == v + u`.
/// - (Additive identity) For all `v: Self`, `v + Self::ZERO == v`.
/// - (Additive inverse) For all `v: Self`, `v - v == v + v * (-1) == Self::ZERO`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// - (Additive inverse) For all `v: Self`, `v - v == v + v * (-1) == Self::ZERO`.
/// - (Additive inverse) For all `u: Self` there exists a `v: Self` such that, `u + v == Self::ZERO`.

The existence of an additive inverse doesn't necessarily imply that it will always equal -1*v. The "Compatibility of multiplication" rule below will when combined with this requirement.

As an aside, should the trait Neg be included? Feels strange to require scalar multiplication without it, especially if we require the additive inverse be defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Existential quantifiers really don't belong in algebraic constraints in my opinion. I agree that it's phrased differently from usual, but there are a couple of things at play:

  1. The "right" thing is probably v + (-v) == ZERO, but we don't demand the Neg trait despite the fact that we have enough data to implement it ourselves (we could change this)
  2. Subtraction is, in practice, one of the most important operations in the API, even if mathematically speaking it originates from addition and negation

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just add Neg and define it as the inverse.

crates/bevy_math/src/shape_sampling.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/shape_sampling.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/common_traits.rs Outdated Show resolved Hide resolved
/// - (Commutativity of addition) For all `u, v: Self`, `u + v == v + u`.
/// - (Additive identity) For all `v: Self`, `v + Self::ZERO == v`.
/// - (Additive inverse) For all `v: Self`, `v - v == v + v * (-1) == Self::ZERO`.
/// - (Compatibility of multiplication) For all `a, b: f32`, `v: Self`, `v * (a * b) == (v * a) * b`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// - (Compatibility of multiplication) For all `a, b: f32`, `v: Self`, `v * (a * b) == (v * a) * b`.
/// - (Compatibility of scalar multiplication) For all `a, b: f32`, `v: Self`, `v * (a * b) == (v * a) * b`.

Worth calling this explicitly scalar multiplication to allow for possible future definition of vector multiplication in another trait.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrote "multiplication" because really it's "compatibility of scalar multiplication with field multiplication", which was too many words. I'm also not sure how confusing it would be in the future anyway, since vector multiplication is almost always called something else anyway (e.g. inner product, dot product, etc.).

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly, that's fair. Until this trait is generic over the scalar field (probably never) I think that's specific enough language already.

/// - (Additive identity) For all `v: Self`, `v + Self::ZERO == v`.
/// - (Additive inverse) For all `v: Self`, `v - v == v + v * (-1) == Self::ZERO`.
/// - (Compatibility of multiplication) For all `a, b: f32`, `v: Self`, `v * (a * b) == (v * a) * b`.
/// - (Multiplicative identity) For all `v: Self`, `v * 1.0 == v`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// - (Multiplicative identity) For all `v: Self`, `v * 1.0 == v`.
/// - (Multiplicative scalar identity) For all `v: Self`, `v * 1.0 == v`.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 28, 2024
Merged via the queue into bevyengine:main with commit f924b4d Mar 28, 2024
27 checks passed
@mweatherley mweatherley deleted the vector-spaces branch March 28, 2024 14:36
github-merge-queue bot pushed a commit that referenced this pull request Mar 29, 2024
…riangle (#12766)

# Objective

When I wrote #12747 I neglected to translate random samples from
triangles back to the point where they originated, so they would be
sampled near the origin instead of at the actual triangle location.

## Solution

Translate by the first vertex location so that the samples follow the
actual triangle.
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-Code-Quality A section of code that is hard to understand or change C-Enhancement A new feature C-Needs-Release-Note Work that should be called out in the blog due to impact S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants