novelrt / NovelRT

A cross-platform 2D game engine accompanied by a strong toolset for visual novels.
MIT License
184 stars 42 forks source link

Modify GeoVector classes to be more feature complete #491

Closed Pheubel closed 1 year ago

Pheubel commented 2 years ago

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:

Resolution:

GeoVector2F will currently not have an out of the box implementation of calculating a cross product. To calculate the cross product with a GeoVector2F it can be expanded to a GeoVector3F.


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:

Resolution:

There will be an API for rotating in both degrees and radians.

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:

Resolution:

APIs for checking the distance and square distance between two points should be included.

RubyNova commented 2 years ago

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?

capnkenny commented 2 years ago

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

RubyNova commented 2 years ago

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

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

capnkenny commented 2 years ago

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

RubyNova commented 2 years ago

Yeah that's fine