Skip to content

Document z_eff_ss_stone1.[ch]#2670

Open
GabrielRavier wants to merge 2 commits intozeldaret:mainfrom
GabrielRavier:feat/document-z-eff-ss-stone1
Open

Document z_eff_ss_stone1.[ch]#2670
GabrielRavier wants to merge 2 commits intozeldaret:mainfrom
GabrielRavier:feat/document-z-eff-ss-stone1

Conversation

@GabrielRavier
Copy link
Contributor

No description provided.

/* 0x00 */ Vec3f pos;
/* 0x00 */ s32 unk_C;
} EffectSsStone1InitParams; // size = 0x
/* 0x0C */ s32 suppressRTransFadeFlashAlphaStepAfterTwoFrames; // If this is non-zero, then on the 2nd frame after being spawned, the effect will suppress any flash effect from R_TRANS_FADE_FLASH_ALPHA_STEP. All current users of EffectSsStone1InitParams set this to 0. Given the only use of R_TRANS_FADE_FLASH_ALPHA_STEP is made by arrows, when they're deku nut "arrows", and that arrows are the only thing that spawn this effect, it might be that this parameter was used at some point in development in relation to deku nuts or arrows.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/* 0x0C */ s32 suppressRTransFadeFlashAlphaStepAfterTwoFrames; // If this is non-zero, then on the 2nd frame after being spawned, the effect will suppress any flash effect from R_TRANS_FADE_FLASH_ALPHA_STEP. All current users of EffectSsStone1InitParams set this to 0. Given the only use of R_TRANS_FADE_FLASH_ALPHA_STEP is made by arrows, when they're deku nut "arrows", and that arrows are the only thing that spawn this effect, it might be that this parameter was used at some point in development in relation to deku nuts or arrows.
/* 0x00 */ Vec3f pos;
// If this is non-zero, then on the 2nd frame after being spawned, the effect will suppress any flash
// effect from R_TRANS_FADE_FLASH_ALPHA_STEP. All current users of EffectSsStone1InitParams set this
// to 0. Given the only use of R_TRANS_FADE_FLASH_ALPHA_STEP is made by arrows, when they're deku nut
// "arrows", and that arrows are the only thing that spawn this effect, it might be that this parameter
// was used at some point in development in relation to deku nuts or arrows.
/* 0x0C */ s32 suppressRTransFadeFlashAlphaStepAfterTwoFrames;

Not sure if the formatter didn't pick this up, but our 120 character line limit should be used here to split the comment over multiple lines

Copy link
Contributor Author

@GabrielRavier GabrielRavier Feb 6, 2026

Choose a reason for hiding this comment

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

I've made the comment fit within the column limit - I will note that I had written the original comment with the style of other headers in the project in mind - those I found with a similarly long comment for a struct or enum member had it formatted in the same way (a // comment to the right of the declaration, going way beyond the 120-character limit), e.g. for instance in src/overlays/actors/ovl_Demo_Gj/z_demo_gj.h.

clang-format does catch this correctly - I just hadn't noticed the .clang-format file was there, and there doesn't seem to be anything in the contribution documentation that mentions it, so I hadn't thought of using it until now.

Copy link
Contributor Author

@GabrielRavier GabrielRavier Feb 6, 2026

Choose a reason for hiding this comment

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

PS: clang-format does catch this correctly, but if I actually applied it to every file modified here, it would add 12 modifications to the diff (in parts of include/effect.h that are unrelated to my changes), so I haven't ran it on all of them - do tell me if I should

Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't clang-format .h files typically, and historically don't enforce the line limit for single-line comments following struct variables
We could/should change this maybe, I don't really have an opinion

From zeldaret#2670:
- Shortened excessively verbose
  suppressRTransFadeFlashAlphaStepAfterTwoFrames name to
  suppressFadeFlash
- Fixed up comment that went waaaaaay beyond the column limit in
  z_eff_ss_stone1.h
- Shortened comparison with 0 of variable bool semantics in
  z_eff_ss_stone1.c

Signed-off-by: Gabriel Ravier <gabravier@gmail.com>
@GabrielRavier GabrielRavier force-pushed the feat/document-z-eff-ss-stone1 branch from bbeafe8 to 20dba2c Compare February 6, 2026 16:20
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.

3 participants