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

sys_math.c Rename #1258

Merged
merged 11 commits into from
Jun 6, 2023
Merged

sys_math.c Rename #1258

merged 11 commits into from
Jun 6, 2023

Conversation

engineer124
Copy link
Collaborator

Every function of sys_math.c had a comment with a name of each function. This simply follows those suggestions. If there's a reason for waiting to name these, let me know!

Copy link
Collaborator

@AngheloAlf AngheloAlf left a comment

Choose a reason for hiding this comment

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

I wonder why we haven't renamed those functions before.
Thanks

include/functions.h Outdated Show resolved Hide resolved
@engineer124 engineer124 removed the Needs-second-approval Second approval label May 30, 2023
Copy link
Collaborator

@hensldm hensldm left a comment

Choose a reason for hiding this comment

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

Also thoughts on doing the same for boot_80086760.c?

src/code/sys_math.c Outdated Show resolved Hide resolved
src/overlays/actors/ovl_Eff_Dust/z_eff_dust.c Outdated Show resolved Hide resolved
src/overlays/actors/ovl_Eff_Dust/z_eff_dust.c Outdated Show resolved Hide resolved
@engineer124
Copy link
Collaborator Author

Also thoughts on doing the same for boot_80086760.c?

Sounds good to me. Done.

Copy link
Contributor

@EllipticEllipsis EllipticEllipsis left a comment

Choose a reason for hiding this comment

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

Hooray, this one's been a long time coming, thanks for doing it. I put it off for a long time mainly because I thought it'd actually be much worse than this, I'm surprised it's "only" about 130 files.

The comments on the mathematics functions I'd like addressed now, the rest can be added to a cleanup PR later if preferred.

return func_80086C70(x);
}

// Unused
// Math_FCeilF
f32 func_800867B4(f32 x) {
f32 Math_FCeilF(f32 x) {
return func_80086CA8(x);
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're doing these, you might as well name the inner ones as well (and use the same names as OoT, since this is the only place they actually occur).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nearbyint is used in z_bgcheck.c, so there's 1 use case outside this file

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's the weird one, wonder if it's close to the wrapper name or something.

src/code/sys_math.c Show resolved Hide resolved
/**
* Returns a pseudo-random floating-point number between (- scale / 2) and (scale / 2). Originally in z_actor in OoT.
*/
f32 randPlusMinusPoint5Scaled(f32 scale) {
f32 Rand_CenteredFloat(f32 scale) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately these don't exist in DnM+ in any of the obvious places, at least, although at least we know the names of the random functions in qrand.c (which we should rename at some point):
qrand
sqrand
fqrand
fqrand2
sqrand_r
qrand_r
fqrand_r
fqrand2_r

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not seeing a qrand.c in the repo, what do you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's currently called boot_80xxxxxx something, qrand is its actual name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh apparently it's rand.c now? No wonder I couldn't find it... it seems to be a common library file.

Comment on lines +4860 to +4862
currentMatrix->mf[3][0] = Rand_CenteredFloat((f32)sREG(24) + 30.0f) + limbPos->x;
currentMatrix->mf[3][1] = Rand_CenteredFloat((f32)sREG(24) + 30.0f) + limbPos->y;
currentMatrix->mf[3][2] = Rand_CenteredFloat((f32)sREG(24) + 30.0f) + limbPos->z;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to keep these explicit casts?

Copy link
Collaborator Author

@engineer124 engineer124 Jun 5, 2023

Choose a reason for hiding this comment

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

There's an open PR looking at explicit casts (albeit not for floats) (#1266), I don't have a preference either way

src/code/z_camera.c Show resolved Hide resolved
@@ -1173,7 +1173,7 @@ void EnPp_BodyPart_SetupMove(EnPp* this) {
this->deadBodyPartRotationalVelocity.z = (this->deadBodyPartIndex * 0x2E) + 0xFF00;
if (EN_PP_GET_TYPE(&this->actor) != EN_PP_TYPE_BODY_PART_BODY) {
this->actor.speed = Rand_ZeroFloat(4.0f) + 4.0f;
this->actor.world.rot.y = ((s32)randPlusMinusPoint5Scaled(223.0f) + 0x1999) * this->deadBodyPartIndex;
this->actor.world.rot.y = ((s32)Rand_CenteredFloat(223.0f) + 0x1999) * this->deadBodyPartIndex;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a weird one, maybe 0xDF but probably not...

Comment on lines +1187 to +1189
wobbleScale.x = 1.0f - (Math_SinF((f32)this->timer * (M_PI / 2.0f)) * 0.3f);
wobbleScale.y = (Math_SinF((f32)this->timer * (M_PI / 2.0f)) * 0.3f) + 1.0f;
wobbleScale.z = 1.0f - (Math_CosF((f32)this->timer * (M_PI / 2.0f)) * 0.3f);
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, do we care about the explicit cast here?

src/overlays/actors/ovl_En_Tanron4/z_en_tanron4.c Outdated Show resolved Hide resolved
src/overlays/actors/ovl_En_Wiz_Fire/z_en_wiz_fire.c Outdated Show resolved Hide resolved
src/overlays/actors/ovl_Obj_Bigicicle/z_obj_bigicicle.c Outdated Show resolved Hide resolved
Copy link
Contributor

@EllipticEllipsis EllipticEllipsis left a comment

Choose a reason for hiding this comment

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

I'm still a bit suspicious about that s16 cast but it's not that important to deal with it now.

@engineer124 engineer124 added Merge-ready All reviewers satisfied, just waiting for CI and removed Needs-first-approval First approval labels Jun 6, 2023
@AngheloAlf AngheloAlf merged commit 7f5087d into zeldaret:master Jun 6, 2023
@engineer124 engineer124 deleted the sysMath branch June 6, 2023 02:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean up Merge-ready All reviewers satisfied, just waiting for CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants