godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
85.17k stars 18.71k forks source link

Core: Reimplement math structs as `constexpr` #91992

Open Repiteo opened 2 weeks ago

Repiteo commented 2 weeks ago

Supersedes #91089

In constrast to the above commit having constexpr as a sort of bonus addition from initializer lists, this commit focuses squarely on incorporating a constexpr foundation for various math structs. Because making everything about these structs constexpr would be extremely noisy & make micromanaging the more explicit changes a headache, this instead implements the concept for constructors and operators (and any constructor-helper funcs).

The most obvious change revolves around operator[] for structs with union members. This is because, to my knowledge, constexpr environments cannot dynamically change an active union member. Because construction values are explicitly assigned to named members, the usual approach of calling to the array union member will simply fail. This can be worked around by explicitly checking for the named values via a switch statement first.

constexpr Vector3 vec3 = Vector3();

// Normal environment, any extraction method works
const real_t val_name = vec3.x;
const real_t val_coord = vec3.coord[0];
const real_t val_index = vec3[0];

// Constexpr environment
constexpr real_t const_name = vec3.x; // Works, struct containing `x` is active union member.
constexpr real_t const_coord = vec3.coord[0]; // Error, accessing inactive union member.
constexpr real_t const_index = vec3[0]; // Error with old implementation, works on PR. See below:

// Old implementation
_FORCE_INLINE_ real_t &operator[](int p_axis) const {
    DEV_ASSERT((unsigned int)p_axis < 3);
    return coord[p_axis]; // Tries to access inactive member, fails.
}

// New implementation
constexpr const real_t &Vector3::operator[](size_t p_axis) const {
    switch (p_axis) {
        case AXIS_X: // Explicitly parse for valid values, assign to named equivalents.
            return x;
        case AXIS_Y:
            return y;
        case AXIS_Z:
            return z;
        default: // Only remaining values are oob, will be logged as such.
            return coord[p_axis];
    }
}

Some other aspects of the code that were adjusted are:

clayjohn commented 2 weeks ago

You've done a great job explaining the technical considerations you faced when implementing this PR. Can you also provide an explanation about why these changes are beneficial? At face value, this looks like a stylistic change that ended up necessitating some functional changes that may have performance tradeoffs and may impact compatibility with older compiler versions (in addition to introducing risk throughout the engine).

If performance is a factor, can you post the results of the benchmarking that you did while making this change? If the benefit is stylistic, can you explain why the benefit outweighs the cost?

fire commented 2 weeks ago

Style improvements

One point of favour constexpr allows the compiler to remove dead code (unused variables) from the constructors.

This conversion also removed duplicated methods.

clayjohn commented 2 weeks ago

One point of favour constexpr allows the compiler to remove dead code (unused variables) from the constructors.

This conversion also removed duplicated methods.

The compiler does that anyway, see https://github.com/godotengine/godot/pull/91089#issuecomment-2074220542

fire commented 2 weeks ago

Yes, that's why I'm approaching it from an improved style point of view and not performance.

Like the principle of don't repeat yourself. https://en.wikipedia.org/wiki/Don%27t_repeat_yourself

Repiteo commented 2 weeks ago

The primary functional benefits are those inherent to constexpr: the ability to evaluate variables at compile-time. Relatedly, benefits from constexpr aren't inherently applied to non-constexpr environments, so practically speaking this won't change the current codebase's performance. However, this does open the door for constexpr to be assigned to variables of these types; benchmarking will only have a real impact once variables start implementing this. Older compiler versions shouldn't be a concern so long as they have proper C++17 support; after #91833 bumped the GCC minimum version to 9, I don't think any current minimums feature breaking regressions with C++17, but more testing of those minimum versions couldn't hurt.

clayjohn commented 2 weeks ago

In that case, I'm not sure what to say. We can't really justify changing 1700 lines of deeply core code for no benefit. The ability to evaluate variables at compile-time is really nice for development since it helps you catch type errors while writing code. But you are applying this to code that has already been written, tested, and used in production for years. Effectively creating a risk of introducing new bugs into a code that works in the name of making the code easier to write.

To be perfectly blunt, I think this falls into the "if its not broken, don't fix it" territory.

If there were some other benefit to this change, then it would be a different story. So perhaps it makes sense to look into those performance benefits that we could take advantage of and form a picture of whether that is something worth pursuing.

Repiteo commented 2 weeks ago

I'm used to separating foundational implementation from codebase integration in PRs, but it sounds like that's not viable here. One obvious option would be the ability to convert several runtime tests to static_assert equivalents, catching breaking changes immediately:

TEST_CASE("[AABB] Constructor methods") {
    const AABB aabb = AABB(Vector3(-1.5, 2, -2.5), Vector3(4, 5, 6));
    const AABB aabb_copy = AABB(aabb);

    CHECK_MESSAGE(
            aabb == aabb_copy,
            "AABBs created with the same dimensions but by different methods should be equal.");
}

namespace AABBConstructorMethods {
inline constexpr AABB aabb = AABB(Vector3(-1.5, 2, -2.5), Vector3(4, 5, 6));
inline constexpr AABB aabb_copy = AABB(aabb);

static_assert(
        aabb == aabb_copy,
        "AABBs created with the same dimensions but by different methods should be equal.");
} //namespace AABBConstructorMethods
Repiteo commented 2 weeks ago

The scope of this is much greater than I expected, even after trying to trim things down. Gonna be converting this to a draft while I work on piecemealing the concepts across other PRs, starting with #92059.