NovelRT icon indicating copy to clipboard operation
NovelRT copied to clipboard

Proposal: Modify GeoVector classes to be more feature complete

Open Pheubel opened this issue 2 years ago • 1 comments

The GeoVector classes currently lack an out of the box solution to some commonly used functions

Squared length

GeoVectors already provide an API to get a vector's length. However, in some applications you can leave out the square root step at the end of the equation. For example, to determine if a point is in range of a vector. By comparing the squared distance between the vector and the point to the squared range you can tell if the point is within the range of a vector.

Example Implementation

// GeoVector2F.h

inline float getSquaredMagnitude() const noexcept
{
    return glm::length2(*reinterpret_cast<const glm::vec2*>(this));
}

inline float getSquaredLength() const noexcept
{
    return getSquaredMagnitude();
}

Usage of glm::length2 will require that glm/gtx/norm.hpp is included, as it is currently

Dot product

The dot product is used in places like vector reflection, for example to bounce a ball against a wall, and calculating the angle between two vectors.

Example Implementation

// GeoVector2F.h

static inline float dot(GeoVector2F lhs, GeoVector2F rhs) noexcept
{
    return glm::dot(*reinterpret_cast<const glm::vec2*>(&lhs), *reinterpret_cast<const glm::vec2*>(&rhs));
}

Cross product

A cross product allows for finding a vector that lies perpendicular on two other three-dimensional vectors.

Example Implementation

// GeoVector3F.h

static inline GeoVector3F cross(GeoVector3F lhs, GeoVector3F rhs)
{
    return glm::cross(*reinterpret_cast<glm::vec3*>(&lhs), *reinterpret_cast<glm::vec3*>(&rhs));
}

Question:

  • Should Vector2F also implement the cross product? Some codebases allow for calculating the cross product between two-dimensional vector by extending them to three-dimensional vectors with the Z component set to 0. As a consequence, the resulting vector would have its X and Y components set to 0

Besides adding new functions, some changes to existing code might be required as well.

Make rotateToAngleAroundPoint use radians instead of degrees

As stated in https://github.com/novelrt/NovelRT/pull/483#discussion_r922731797, the current behavior of using degrees for rotation is not desired.

Example Implementation

// GeoVector2F.h

void rotateToAngleAroundPoint(float angleRotationValue, GeoVector2F point) noexcept
{
    *reinterpret_cast<glm::vec2*>(this) =
        glm::rotate((*reinterpret_cast<glm::vec2*>(this) = *reinterpret_cast<const glm::vec2*>(this) -
                                                           *reinterpret_cast<const glm::vec2*>(&point)),
                    angleRotationValue) +
        *reinterpret_cast<const glm::vec2*>(&point);
}

As rotateToAngleAroundPoint now expects angleRotationValue to be in radians, a conversion is no longer required to use glm::rotate

Question:

  • Should there be an API that allows for rotating in degrees alongside one that allows for radians? It would reduce the need for end users to do the conversion from degrees to radians if they are used to working with degrees.

Add missing initializers in GeoVector4F

The constructor GeoVector4F() lacks any initializers and causes uninitialized member variable warnings on Visual Studio, this might extend to other compilers as well. Adding initializers would bring it in line with the other dimension vectors, as they provide zero initializers for its members in the zero argument constructor.

Example Implementation

GeoVector4F() noexcept : x(0.0f), y(0.0f), z(0.0f), w(0.0f)
{
}

General Question:

  • Are there any other functions that would be worth including?

Pheubel avatar Aug 25 '22 15:08 Pheubel

I think my 2c here is that the member methods should follow our new convention, and existing methods should have their naming updated accordingly from camelCase to PascalCase.

I can't think of any additional functions at the moment, but we can keep this ticket open just in case?

RubyNova avatar Sep 01 '22 13:09 RubyNova

Marking this as approved and removing "Proposal" as... well, it's been approved :)

capnkenny avatar Oct 25 '22 13:10 capnkenny

Eh, Didn't I already add the approved label?

The approved label makes no sense if we remove the proposals label either.

RubyNova avatar Oct 25 '22 14:10 RubyNova

Fair enough - can we agree to remove "Proposal" from the title since the issues grown to be approved?

capnkenny avatar Oct 25 '22 14:10 capnkenny

Yeah that's fine

RubyNova avatar Oct 25 '22 14:10 RubyNova