search

open-ead / sead

Decompilation of sead: the standard C++ library for first-party Nintendo games
190 stars 26 forks source link

math: Pull latest changes #87

Closed ThePixelGamer closed 3 years ago

ThePixelGamer commented 3 years ago

Closes #86

This change is Reviewable

aboood40091 commented 3 years ago

This misses changes to BoundBox in this comment: https://github.com/aboood40091/sead/commit/8fce6b69eb9f1d1944a11a22ecef855431ec91b7

Also, Nintendo did not specialize MathCalcCommon::abs for float. It uses std::abs. (It is up to the compiler to place an fabs call or do something different)

ThePixelGamer commented 3 years ago

Hi thanks for the review, the reason why this commit includes specialization for float was because of the previous absf found here: https://github.com/open-ead/sead/blob/master/include/math/seadMathCalcCommon.h#L81-L84 If you think this function deserves to be in it's own and not a specialization for abs let me know!

leoetlino commented 3 years ago

std::abs and the current implementation of sead::absf (which was replaced with a specialisation in this PR) compile down to very different code: https://godbolt.org/z/dYv8bM1v1

Considering the second pattern is what we see in sead::QuatCalcCommon<float>::slerpTo, I think it's likely that MathCalcCommon's abs was specialised for float or that the generic implementation uses the ternary expression, not std::abs.

aboood40091 commented 3 years ago

std::abs and the current implementation of sead::absf (which was replaced with a specialisation in this PR) compile down to very different code: https://godbolt.org/z/dYv8bM1v1

Considering the second pattern is what we see in sead::QuatCalcCommon<float>::slerpTo, I think it's likely that MathCalcCommon's abs was specialised for float or that the generic implementation uses the ternary expression, not std::abs.

Hm... I needed to use std::abs for it to match on Wii U.

This could mean one of two things:

  1. Nintendo changed the implementation for sead::MathCalcCommon<T>::abs (or specialized it for float) for newer versions specifically to the implementation in this pull request.
  2. The specialized implementation for float in this pull request was actually the default implementaion all along and Nintendo specialized it for float on Wii U specifically to use std::abs.

To me, both sound equally likely (... or rather, equally unlikely because both cases are weird).

leoetlino commented 3 years ago
  1. Nintendo changed the implementation for sead::MathCalcCommon<T>::abs (or specialized it for float) for newer versions specifically to the implementation in this pull request.
  2. The specialized implementation for float in this pull request was actually the default implementaion all along and Nintendo specialized it for float on Wii U specifically to use std::abs.

Yeah, it's super strange. That said, we (aboood40091 and I) did notice that the same slerpTo function was using fabs in the Wii U target and the custom comparison on Switch...

ThePixelGamer commented 3 years ago

In this case would it be better to have both definitions but combined with the preprocessor?

ThePixelGamer commented 3 years ago

Also this is something I'm unable to check but for classes like VectorCalcCommon wouldn't it be better to use the actual base structures for these functions as they don't really utilize any functions from actual class and we can just rely on implicit upcasting for the calls

leoetlino commented 3 years ago

In this case would it be better to have both definitions but combined with the preprocessor?

Yeah, maybe. Perhaps we should even add a comment to this discussion so that somebody doesn't try to replace the comparison with an std::abs in the future

Also this is something I'm unable to check but for classes like VectorCalcCommon wouldn't it be better to use the actual base structures for these functions as they don't really utilize any functions from actual class and we can just rely on implicit upcasting for the calls

You mean have VectorCalcCommon take e.g. Vector3 instead of BaseVec3? I actually don't know why the base class is used here — maybe @aboood40091 would know?