Skip to content

Re-adjust % to & operation#262

Open
JaceCear wants to merge 4 commits intomainfrom
screen_shake_adjust
Open

Re-adjust % to & operation#262
JaceCear wants to merge 4 commits intomainfrom
screen_shake_adjust

Conversation

@JaceCear
Copy link
Copy Markdown
Collaborator

@JaceCear JaceCear commented May 3, 2026

'% 2u' doesn't match in SA3, implying that this was always '& 0x1'.

}

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

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

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

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

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

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

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

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

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.

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.

2 participants