Skip to content
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.

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

Organize ship specific save context additions #4597

Merged
merged 11 commits into from
Jan 15, 2025

Conversation

Pepe20129
Copy link
Contributor

@Pepe20129 Pepe20129 commented Nov 29, 2024

I've left randomizerInf alone for now, will probably do a followup pr soon.

Build Artifacts

@Pepper0ni
Copy link
Contributor

The failed linux check here looks real, if you aren't aware. seems to be some LUS thing not liking your changes

Copy link
Contributor

@Malkierian Malkierian left a comment

Choose a reason for hiding this comment

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

I'm not entirely sold on the sheer amount of nested members getting to quest-specific data or the union itself, but it's not a hard block for me. The code looks sound. Would like to get a few more opinions on it.

@Malkierian
Copy link
Contributor

I think that's a straightforward conflict resolution, but just in case I'll have you take care of it, then we'll get it merged.

Copy link
Contributor

@aMannus aMannus left a comment

Choose a reason for hiding this comment

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

1 question pertaining some rando inf flag code, otherwise it looks good to me after merge conflicts are resolved. I'm not a huge fan of how nested some of these get either, but very happy with the overal separation off of the regular savecontext.

Comment on lines 401 to 405
// Sets all rando flags to false
for (s32 i = 0; i < ARRAY_COUNT(gSaveContext.randomizerInf); i++) {
gSaveContext.randomizerInf[i] = 0;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this removed?

Comment on lines 822 to 826
for (int flag = 0; flag < ARRAY_COUNT(gSaveContext.randomizerInf); flag++) {
gSaveContext.randomizerInf[flag] = 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

And this one

Comment on lines 4999 to 5004
if (!IS_RANDO) {
LUSLOG_ERROR("Tried to set randomizerInf flag \"%d\" outside of rando", flag);
assert(false);
return;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand why this was added while you were already around this code, but I'd suggest to separate this kinda stuff into a different PR down the line.

@Pepe20129
Copy link
Contributor Author

Funnily enough, all those changes were stuff that I forgot to revert after moving the sohInf/randomizerInf split to a future PR.

@aMannus
Copy link
Contributor

aMannus commented Jan 11, 2025

Once the build succeeds again and conflicts are resolved, I'll look to get this in again.

@aMannus aMannus added this to the 9.0.0 milestone Jan 11, 2025
@aMannus aMannus merged commit 7f31fd2 into HarbourMasters:develop Jan 15, 2025
5 checks passed
@Pepe20129 Pepe20129 deleted the save_context branch January 15, 2025 14:53
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