Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CalcCommon additions, minor fixes #132

Merged
merged 3 commits into from
Mar 27, 2024
Merged

Conversation

Mars2032
Copy link
Contributor

@Mars2032 Mars2032 commented Aug 31, 2023

MathCalcCommon::max was changed due to being the exact same implementation as ::clampMin, which is unnecessary, and the new implementation was needed. QuatCalcCommon::setAxisAngle converts and axis and an angle into a quaternion, and a new Vector3CalcCommon::rotate was added for quaternion rotation on a vector. the implementation is matching in some functions, but has a register swap mismatch in others. it's as good as its gonna get for now.


This change is Reviewable

MathCalcCommon::max was changed due to being the exact same implementation as ::clampMin, which is unnecessary, and the new implementation was needed.
QuatCalcCommon::setAxisAngle converts and axis and an angle into a quaternion, and a new Vector3CalcCommon::rotate was added for quaternion rotation on a vector. the implementation is matching in some functions, but has a register swap mismatch in others. it's as good as its gonna get for now.
@MonsterDruide1
Copy link
Contributor

Could you maybe add the two example functions we looked at to get the rotate as close as possible as decomp.me in here? Maybe someone else has another idea to improve it, and it can instantly be tested.

@Mars2032
Copy link
Contributor Author

https://decomp.me/scratch/3hHWD instance of both functions matching

https://decomp.me/scratch/hXU5w instance of Vector3CalcCommon<f32>::rotate not perfectly matching, all registers are just swapped for some reason.

@ThePixelGamer
Copy link
Contributor

@aboood40091 opinions on this?

Copy link
Contributor

@leoetlino leoetlino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, just one minor nit

{
T angleRad = sead::Mathf::deg2rad(angle);
angleRad *= 0.5f;
q.w = cosf(angleRad);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be MathCalcCommon<T>::cos instead? failing that, std::cos

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sead::Mathf::cos and sead::Mathf::sin both work perfectly fine and still match. If you would prefer to have sead::MathCalcCommon<f32>::cos written instead then I can do that as well.

Copy link
Contributor

@ThePixelGamer ThePixelGamer Oct 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer MathCalcCommon<T>

Changed to use standardized MathCalcCommon functions with the template rather than assuming template type.
oops im blind
@ThePixelGamer
Copy link
Contributor

LGTM

@ThePixelGamer ThePixelGamer merged commit 7e78702 into open-ead:master Mar 27, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants