Skip to content

Re-adjust % to & operation #262

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.

Sign up for GitHub

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

Open
JaceCear wants to merge 4 commits into
base: main
Choose a base branch
from
+1 −1
Open

Re-adjust % to & operation #262

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
05b204d
'% 2u' doesn't match in SA3, implying that this was always '& 0x1'.
JaceCear May 3, 2026
5f37391
Merge branch 'main' into screen_shake_adjust
JaceCear May 5, 2026
df3c067
Merge branch 'main' into screen_shake_adjust
JaceCear May 6, 2026
b8bb59e
Merge branch 'main' into screen_shake_adjust
JaceCear May 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/game/shared/stage/screen_shake.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ void Task_ScreenShake(void)
} break;
}

if (!(shake->flags & 0x20) || ((gStageTime % 2u) == 0)) {
if (!(shake->flags & 0x20) || ((gStageTime & 0x1) == 0)) {
Copy link
Copy Markdown
Collaborator

@freshollie freshollie May 4, 2026

Choose a reason for hiding this comment

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

Wouldn't & 1 be better? 0x1 is like it's a flag?

Copy link
Copy Markdown
Collaborator Author

@JaceCear JaceCear May 4, 2026

Choose a reason for hiding this comment

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

Uhh... well, if it was value % 16u, I'd write & 0xF as well, so this is kind of more consistent?

And if it was a flag... we'd a flag macro, right? 😋

Copy link
Copy Markdown
Collaborator

@freshollie freshollie May 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I wouldn't write & 0xF if I knew the point of this code was to trigger every 16 increments, it would be easier to understand as & 15?

Copy link
Copy Markdown
Collaborator Author

@JaceCear JaceCear May 5, 2026

Choose a reason for hiding this comment

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

I'm a bit confused why we're arguing about it.
There's hundreds of cases of this in the codebase already.
And I do not think I'd ever prefer to read a decimal value in a bit-operation over hex. 😅
That's just not intuitive, imo.

Copy link
Copy Markdown
Collaborator

@freshollie freshollie May 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

To your point, yes there are lots of cases in the code. I'm aiming to improve the cases where the number as a decimal makes more sense. I would likely re-write this as & 1 hence I'm feeding back.

Copy link
Copy Markdown
Collaborator Author

@JaceCear JaceCear May 5, 2026

Choose a reason for hiding this comment

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

I just personally don't find it being an improvement when using bitwise ops on decimals.

The one case I understand is for word-alignment (like (value + (4-1)) & ~(4-1)) but those would usually take the base as a parameter in a macro, not be handwritten. 😅

Copy link
Copy Markdown
Collaborator

@freshollie freshollie May 6, 2026

Choose a reason for hiding this comment

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

But the intention of this was % 2u, but they wrote it as & 1, so it does make sense?

Copy link
Copy Markdown
Collaborator Author

@JaceCear JaceCear May 6, 2026

Choose a reason for hiding this comment

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

The thing is, with hex, you immediately know how many bits are being &-ed against.
Sure, it's easy with 1, 3 and 7, but for higher numbers you need to convert decimal numbers first (either in your head or via calc), which is less intuitive, I would say.

I also think, and that might be a bit "by the gut" so to speak, that if you have a decimal value, you're more likely to change it to something that is not a bit mask when you edit it, introducing bugs, unlike a hex value, where you can intuitively infer what it does and why you should not just change it e.g. from 15 to 16.

if (shake->flags & SCREENSHAKE_HORIZONTAL) {
cam->shakeOffsetX = r2;
}
Expand Down
Loading