i didn't mention the other reason i'd like to land this (which is the reason #4650 was made in the first place) - relying on out of bounds indexing of arrays is bad. even though byteswapping the inventory "works" - it's still relying on memory layout in a very fragile way.

ITEM_MASK_GERUDO, ITEM_MASK_TRUTH, ITEM_SONG_NOCTURNE & ITEM_SONG_PRELUDE do land in padding bytes, but in n64, they're still stored, meaning it's possible to write something to the bytes with RBA to then read them with BA. I don't know if we should reproduce this or if it's too niche, but good to mention it anyways.

nocturne and prelude were mentioned in the original PR's description #4650

There is a slight deviation from vanilla behavior. ITEM_SONG_NOCTURNE and ITEM_SONG_PRELUDE read and write to padding bytes in the save struct. Theoretically, you could use BA to write a value to them, then read it with RBA. This would be a more complicated thing to implement since it'll add new fields in the save struct. I couldn't find any actual use of this behavior in speedruns or rando tricks, so I've elected to not fully implement it.

didn't see anything about ITEM_MASK_GERUDO or ITEM_MASK_TRUTH - @Rozelette did you look into those at all?

It is my mistake, I forgot to mention ITEM_MASK_GERUDO or ITEM_MASK_TRUTH in my original PR. In DoBA/DoRBA, they are called out as also being in padding.

To support it, I'd recommend adding additional fields in the save struct. But I do think it's rather niche.

Also, thanks for taking the time to convert this to ShipInit!

Is there any particular reason you converted the if/else chain, as well as the switch statement, to be early returns? In my experience, while early returns are appropriate for preconditions, they are not beneficial for the main function logic. If anything needs to be added at the end of the function, it is inconvenient if there are a bunch of early returns that then need to be changed. I feel like the early returns make the function more confusing. In addition, they aren't going to make the function any more efficient since any modern compiler will collapse the jumps anyways.

Sorry if the comment is unexpected, but since you've essentially taken over my PR, I felt it was appropriate.

i updated the PR description after changing that, where i said

i switched them away from being giant if/else blocks into ifs with early returns. that at least makes it easier to tell from a glance that only one of the things in there should happen, and when it happens we're done.

re:

If anything needs to be added at the end of the function, it is inconvenient if there are a bunch of early returns that then need to be changed.

do you expect to add things to the end of the function? if so, why would it go in there instead of in a new function?

re:

I feel like the early returns make the function more confusing

i feel the opposite. when i first saw the giant if / else block i found myself scrolling to the bottom to try to figure out if something else was going to happen. by using early returns it's clear that what is in that if block is the only thing that happens in that scenario.

i'd like to get other opinions on this. if the general sense is the if / else chain is clearer i can revert that change

do you expect to add things to the end of the function?

I have no idea, but I shouldn't have to worry about it. Perhaps I am debugging an issue and want to add some logging at the end of the function. It would be much more convenient if the function didn't have a ton of unneeded early returns.

by using early returns it's clear that what is in that if block is the only thing that happens in that scenario

I disagree. At least with an if/else chain the intent is obvious that only one scenario could happen. With individual ifs, theoritically each condition could happen and you have to consider each one to see if there's a return in there to see if they're actually mutually exclusive.

I know this is the eternal early return debate but I am proselytizing that this is a situation where it is not appropriate.

also @Rozelette

re:

you've essentially taken over my PR

i'm sorry for stepping on your toes like that. it started as just moving the existing stuff over to use shipinit, but then i "self reviewed" the PR and remembered my thoughts from when you first made the PR

my thoughts then were:

• it doesn't seem easy to read/understand
• there's giant combo of if/else and switch/case
• it includes a bunch of bitwise operations without documenting them
• there are giant functions just thrown straight into z_param

with this PR i started by addressing the "thrown straight into z_param" part, but then started on the "giant combo of if/else and switch/case" part

i'm happy to close this PR if you'd like to just pull the shipinit part (changes up through c009bc8) over into your PR

i'm sorry for stepping on your toes like that

I don't have a problem with it at all! In fact I think the best course of action would be to merge this PR and close my original one, since this uses the modern ShipInit methodology. I just said that as a justification to throw in my 2c, since I don't usually review PRs and, to be honest, I felt a little bit of ownership. RBA was always going to be implementing a complicated table into code so it's bound to be a little messy.

I don't have a problem with it at all! In fact I think the best course of action would be to merge this PR and close my original one, since this uses the modern ShipInit methodology

good deal! i think it sounds like we both want to get to a place where this is at least somewhat easy to read/understand. i'll revert the early return changes and see if i can come up with something that better aligns with your original intent

@Rozelette i pushed a commit breaking up the logic into functions, separating the "which cases are we handling" and the "how do we handle those cases" logic

i think it makes it a lot easier to glance at DoBA and DoRBA and get a sense of what they're doing without being overwhelmed

i'd love to hear your thoughts!

I think this is a much better way at handling the BA/RBA behaviors. It chunks it into digestible portions. And, it isolates the padding bytes, which is where we may want to improve our behavior in the future. I have no qualms. I apprentice your considerations!

I looked at the logic and it seems to be correct. I know you mentioned that there is some bit magic and there indeed is, but it seems to be correct to me. At the very least, if we do get bug reports that it is incorrect, the logic is isolated so that it is easy to correct.

Merge pending conflicts