novelrt / NovelRT

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

Performance improvement: Maths APIs #530

Closed RubyNova closed 1 year ago

RubyNova commented 1 year ago

Note: for support questions, please use the #engine-user-help channel in our Discord or create a discussion. This repository's issues are reserved for feature requests and bug reports.

Describe the issue: Currently all maths operations within NovelRT::Maths are handled at runtime, when many could also be marked as constexpr to make the performance cost a compile-time cost, meaning that runtime would be faster.

Please provide the steps to reproduce if possible:

  1. clone the repo
  2. switch to main
  3. call AlignUp from the Maths utilities header
  4. Observe the issue

Expected behaviour: Certain functions should be marked as constexpr and have their bodies updated accordingly for free perf gain.

Please tell us about your environment:
N/A

Additional context: I am already adjusting some headers as part of the imgui update as needed - please check if #522 has been merged before picking this up.

Pheubel commented 1 year ago

Since #522 has been closed, I assume this can be opened up again. But the work that you've done can still be used.

PopCount(uint32_t), Log2(uint32_t), LeadingZeroCount32(uint32_t) and LeadingZeroCount64(uint64_t) are pretty straight forward when it comes to adding constexpr. However, for degree <-> radian conversion, GLM makes it a little bit less straight forward. For a regular use case they would most likely be constexpr. But if a developer were to make GLM use hardware intrinsics, then it would not be able to be marked constexpr.

The way it decides is through this macro magic:

// N2235 Generalized Constant Expressions http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2235.pdf
// N3652 Extended Constant Expressions http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3652.html
#if (GLM_ARCH & GLM_ARCH_SIMD_BIT) // Compiler SIMD intrinsics don't support constexpr...
#   define GLM_HAS_CONSTEXPR 0
#elif (GLM_COMPILER & GLM_COMPILER_CLANG)
#   define GLM_HAS_CONSTEXPR __has_feature(cxx_relaxed_constexpr)
#elif (GLM_LANG & GLM_LANG_CXX14_FLAG)
#   define GLM_HAS_CONSTEXPR 1
#else
#   define GLM_HAS_CONSTEXPR ((GLM_LANG & GLM_LANG_CXX0X_FLAG) && GLM_HAS_INITIALIZER_LISTS && (\
        ((GLM_COMPILER & GLM_COMPILER_INTEL) && (GLM_COMPILER >= GLM_COMPILER_INTEL17)) || \
        ((GLM_COMPILER & GLM_COMPILER_VC) && (GLM_COMPILER >= GLM_COMPILER_VC15))))
#endif

#if GLM_HAS_CONSTEXPR
#   define GLM_CONSTEXPR constexpr
#else
#   define GLM_CONSTEXPR
#endif

The a solution for this might be to mark DegreesToRadians(float) and RadiansToDegrees(float) with GLM_CONSTEXPR, but that leaks a bit of the implementation detail. So i would not mind a second opinion on that.

Pheubel commented 1 year ago

On another note, some of the functions in NovelRT::Math::Utilities could make use of already made implementations, PopCount(uint32_t) noted that it could be replaced by std::popcount(int) when compiling for C++ 20. But since the current plans are to keep C++ 17 support for the foreseeable future. It might be able to make use of __builtin_popcount(int) for clang/GCC and fall back on the current implementation on MSVC, like how it is done with NovelRT::Utilities::BitCast<TTo,TTFrom>(const TFrom&). This would make it a little bit more complicated, as there will be more preprocessors making it visually more cluttered.

Might be something for a future issue (or this one depending on if it fits within the scope).

RubyNova commented 1 year ago

My take here is we should just do the safe ones - i.e. guaranteed wins without finnicky flag changes - and go from there.