Skip to content

Conversation

@amelenty
Copy link
Contributor

There's still a lot to improve/document in these functions. I intend to continue working on them in separate MRs to keep the scope manageable.

Improvements

  • Reused naming from MarEntityPlayerBlinkWhite/RicEntityPlayerBlinkWhite in all BlinkWhite functions.
  • Found many other places where this particular way of modulating color values with sine waves is used; added FLT macros to them to hopefully make it clearer that rsin(angle) + FLT(1.0) is a value from 0 to 2.
  • Named the color intensity field in ET_PlayerBlink.

Future plans

  • Ideally, I want to create a macro for this color modulation, e.g. #define MODULATE_SIN(angle, intensity, divide) ((rsin(angle) + FLT(1.0)) >> 6) * intensity / divide;, but I'm not sure if it's a good idea at this point.
  • Continue investigating similarities with other visual effect functions such as Lightning and Warp.
  • Figure out what's happening with x/y offsets (there are some discrepancies in naming between similar functions)
  • Fix all gotos (I fixed some of them, but the remaining ones break the match when I remove them)
  • Continue investigating upperParams. I started cross-referencing the params these functions are called with, and it looks like most ifs in this function would be unused. (Maybe in other overlays?)
  • Document logic in comments and figure out the meaning of variables not present in the Maria/Richter version.

All names reused from MarEntityPlayerBlinkWhite.
rsin/rcos return FLT (1.0 = 4096) as per the Psy-Q manual.
Name taken from EntityTeleport.
This field is used as a multiplier
for color values in color-changing effects.
@amelenty amelenty requested a review from Xeeynamo as a code owner September 29, 2025 23:35
g_Player.unk6C = 0;
goto block_231;
DestroyEntity(self);
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Did not look too closely but I think removing these gotos breaks the PSP match?
You might need to bring in your changes to the PSP scratch and play around with it some more
https://decomp.me/scratch/GoMQZ

angle = D_us_801805EC[(i + 0) % 16];
prim->r0 =
((rsin(angle) + FIX(1.0 / 16)) >> 6) * D_us_801D3104 / 256;
((rsin(angle) + FLT(1.0)) >> 6) * D_us_801D3104 / 256;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since they don't affect functionality your PlayerBlinkWhite changes, it would be better to split off these FIX to FLT changes into a separate PR.
Since there are so many of them, it drowns out the other changes and makes it more difficult to review.

@JoshSchreuder
Copy link
Contributor

Run make format to fix linter

Looks like PSP side is also broken - you'll need to find or create the PSP scratches and pull in the changes there to confirm what the exact mismatches are.

@ProjectOblivion
Copy link
Contributor

ProjectOblivion commented Sep 30, 2025

Running make format will either fix your linter test failures, or report a message about what needs to be fixed.

I got sniped...

@Xeeynamo
Copy link
Owner

Hey @amelenty 👋 the changes you're proposing are not generating a full match with the PSP build. Do you need any help to make this pull request mergeable?

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