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

En_Sw doc #1466

Open
wants to merge 48 commits into
base: main
Choose a base branch
from
Open

En_Sw doc #1466

wants to merge 48 commits into from

Conversation

blackgamma7
Copy link
Contributor

better document behavior of Skullwalltula and Gold Skulltula.

Copy link
Collaborator

@Dragorn421 Dragorn421 left a comment

Choose a reason for hiding this comment

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

Quick review

src/overlays/actors/ovl_En_Sw/z_en_sw.c Outdated Show resolved Hide resolved
src/overlays/actors/ovl_En_Sw/z_en_sw.c Outdated Show resolved Hide resolved
/* 0x0490 */ char unk_490[0x48];
} EnSw; // size = 0x04D8

#define SW_GOLDTYPE(params) ((params & 0xE000) >> 0xD)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should follow the ENSW_GET_GOLDTYPE(thisx) format instead, for example see BGICESHELTER_GET_TYPE or ENDOOR_GET_TYPE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

using params as the macro's arg rather than thisx appears to be more compatible, as there are 3 different use cases of thisx->params thisx->params - 0x8000 and this->actor.params

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actors don't always put all their params in Actor.params (mostly for "not everything fits there" reasons), so to abstract where the data goes we decided these macros should take a thisx

(for the this->actor.params uses it would mean passing &this->actor to the macro)

Admittedly ENSW_GET_GOLDTYPE((thisx->params - 0x8000)) is a weird one, didn't notice that... I would suggest keeping that one not using the macro for now, a comment could still indicate it's the type
The line below it uses its shift anyway

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be noted that the (thisx)->params convention breaks in MM (and it's in en_sw as well):

https://github.com/zeldaret/mm/blob/master/src/overlays/actors/ovl_En_Sw/z_en_sw.h#L10
https://github.com/zeldaret/mm/blob/master/src/overlays/actors/ovl_Obj_Tsubo/z_obj_tsubo.c#L106

There really isn't much reason for us to tie params to an actor instance, yet we do so because we are able to get away with it in most cases. I don't think this issue has ever really been discussed properly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you show in code where there is a macro being passed params instead of thisx?

I don't think this issue has ever really been discussed properly.

It has, Tharo even made #1359

src/overlays/actors/ovl_En_Sw/z_en_sw.h Outdated Show resolved Hide resolved
src/overlays/actors/ovl_En_Sw/z_en_sw.c Outdated Show resolved Hide resolved
src/overlays/actors/ovl_En_Sw/z_en_sw.c Outdated Show resolved Hide resolved
src/overlays/actors/ovl_En_Sw/z_en_sw.c Outdated Show resolved Hide resolved
src/overlays/actors/ovl_En_Sw/z_en_sw.c Outdated Show resolved Hide resolved
src/overlays/actors/ovl_En_Sw/z_en_sw.c Outdated Show resolved Hide resolved
src/overlays/actors/ovl_En_Sw/z_en_sw.h Outdated Show resolved Hide resolved
src/overlays/actors/ovl_En_Sw/z_en_sw.h Outdated Show resolved Hide resolved
@Dragorn421 Dragorn421 added the Waiting for author There are requested changes that have not been addressed label Dec 21, 2022
src/overlays/actors/ovl_En_Sw/z_en_sw.h Outdated Show resolved Hide resolved
src/overlays/actors/ovl_En_Sw/z_en_sw.h Outdated Show resolved Hide resolved
src/overlays/actors/ovl_En_Sw/z_en_sw.h Outdated Show resolved Hide resolved
src/overlays/actors/ovl_En_Sw/z_en_sw.c Outdated Show resolved Hide resolved
src/overlays/actors/ovl_En_Sw/z_en_sw.c Outdated Show resolved Hide resolved
src/overlays/actors/ovl_En_Sw/z_en_sw.c Outdated Show resolved Hide resolved
src/overlays/actors/ovl_En_Sw/z_en_sw.c Outdated Show resolved Hide resolved
src/overlays/actors/ovl_En_Sw/z_en_sw.c Outdated Show resolved Hide resolved
src/overlays/actors/ovl_En_Sw/z_en_sw.h Outdated Show resolved Hide resolved
src/overlays/actors/ovl_En_Sw/z_en_sw.h Outdated Show resolved Hide resolved
@Dragorn421 Dragorn421 removed the Waiting for author There are requested changes that have not been addressed label Jan 3, 2023
@fig02
Copy link
Collaborator

fig02 commented Sep 19, 2023

Hi
Just letting you know that I plan to review this very soon. I have things to comment, just havent found the time to sit down and go through the whole PR. Sorry again for the long wait.

@zeldaret zeldaret deleted a comment Jan 27, 2024
@blackgamma7
Copy link
Contributor Author

Hi Just letting you know that I plan to review this very soon. I have things to comment, just havent found the time to sit down and go through the whole PR. Sorry again for the long wait.

Bump - been sitting on these changes for over a year.

@fig02
Copy link
Collaborator

fig02 commented Sep 23, 2024

Hi, sorry again. Been busy with other things in the codebase.
If you would like to stop maintaining this yourself thats okay.

I think it would be more efficient for me personally to make the changes I have in mind myself, and push to your branch, rather than to have this go through a long review process. That will have to occur when I have the time and motivation to do so for this large actor.

Anyone else is free to review this though if they get to it before me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants