Redone fast file select by garrettjoecox · Pull Request #1854 · HarbourMasters/Shipwright

garrettjoecox · 2022-10-26T05:24:29Z

Should resolve #1850 and prevent much further inconsistencies when loading from fast file select vs from the file select screen.

Effectively this is just using the file choose screen and manually setting state to load the file, so that fast file select acts exactly as loading a file from the file choose screen should.

soh/src/overlays/gamestates/ovl_title/z_title.c

leggettc18 · 2022-10-26T05:45:34Z

Will this check if the file is loadable (i.e. will it not attempt to load an MQ file with only a vanilla OTR)? I looked through the code and didn't immediately see it but maybe I'm missing something.

briaguya0 · 2022-10-26T05:48:27Z

soh/src/overlays/gamestates/ovl_file_choose/z_file_choose.c

+        if (this->buttonIndex == 0xFF) {
+            Sram_InitDebugSave();


it's not clear what's happening here, will this not get hit if we already created a debug save in file 1?

This* debug save is not tied to a file slot, this for the 4th option specifically. The underlying code is already set up to handle fileNum = 255 (0xFF)

garrettjoecox · 2022-10-26T05:54:07Z

Will this check if the file is loadable (i.e. will it not attempt to load an MQ file with only a vanilla OTR)? I looked through the code and didn't immediately see it but maybe I'm missing something.

Good question, I guess I assume this wasn't handled previously by fast file select right? But I'll make sure it's handled 👍

briaguya0

this is much cleaner than before! i'd like to get another set of eyes on it before merging it though so i'm requesting a review from @dcvz

garrettjoecox · 2022-10-26T05:59:14Z

I probably should've put this in the description of the PR, but effectively this is just using the file choose screen and manually setting state to load the file, so that fast file select acts exactly as loading a file from the file choose screen should.

garrettjoecox · 2022-10-26T06:06:12Z

Will this check if the file is loadable (i.e. will it not attempt to load an MQ file with only a vanilla OTR)? I looked through the code and didn't immediately see it but maybe I'm missing something.

@leggettc18 thanks for making this so easy for me :D 8ed5cb8

leggettc18 · 2022-10-26T06:06:19Z

Good question, I guess I assume this wasn't handled previously by fast file select right? But I'll make sure it's handled 👍

Yeah fast file select predates this sort of thing where we actually wouldn't want to load a save file at all. Probably attempting to a load an incompatible save file should just spit you out to the file select screen (so still skips the title screen at least).

leggettc18 · 2022-10-26T06:11:28Z

soh/src/overlays/gamestates/ovl_file_choose/z_file_choose.c

    }

+    if (CVar_GetS32("gSkipLogoTitle", 0) && CVar_GetS32("gSaveFileID", 0) < 3) {
+        if (Save_Exist(CVar_GetS32("gSaveFileID", 0)) && FileChoose_IsSaveCompatible(Save_GetSaveMetaInfo(CVar_GetS32("gSaveFileID", 0)))) {


So it looks like if we have an incompatible save file we set the CVar to 4 and don't set any menuMode or selectMode, how does that actually look in-game?

Just loads you into the file select screen as if you already had 4 selected

leggettc18 · 2022-10-26T06:13:49Z

soh/src/overlays/gamestates/ovl_file_choose/z_file_choose.c

+            this->menuMode = FS_MENU_MODE_SELECT;
+            this->selectMode = SM_LOAD_GAME;
+        } else {
+            CVar_SetS32("gSaveFileID", 4);


This will change the CVar permanently and result in essentially disabling Fast File Select until it is manually re-enabled. Maybe we should save the CVar value into a local variable and operate based on that? That way if the player puts the right OTR in place to make the save file compatible, Fast File Select won't have to be manually re-enabled first.

Setting it back to a value that is valid seems okay for this situation IMO, but I can be convinced otherwise

Right, so my main issue with this is that this will change the user's configuration file without their consent, It seems like this CVar is only used in this file, meaning that changing it should be easily avoidable. I have a similar situation with the gMasterQuest CVar, and I may refactor it at some point, but the reason I haven't yet is that the CVar is used across multiple files that don't necessarily have shared state I can use. This CVar is only used in this one file, so it should be easy to grab a copy of the current CVar value and work with that rather than actually changing the CVar globally (which is actually what the previous iteration of this feature did).

That way, if the user fixes the issue with their OTRs or whatever other setting that was preventing Fast File Select from working, they won't have to re-configure anything else to get it working again. Just feels like a better UX to me.

so we need to account for a couple different situations here:

only vanilla otr only mq otr both vanilla and mq otr mq enabled fast file select save type behavior

✔️ ❌ ❌ ❌ vanilla load save

✔️ ❌ ❌ ❌ mq ?

✔️ ❌ ❌ ❌ rando w/ only vanilla dungeons load save

✔️ ❌ ❌ ❌ rando w/ only mq dungeons ?

✔️ ❌ ❌ ❌ rando w/ both vanilla and mq dungeons ?

❌ ✔️ ❌ ✔️ vanilla ?

❌ ✔️ ❌ ✔️ mq load save

❌ ✔️ ❌ ✔️ rando w/ only vanilla dungeons ?

❌ ✔️ ❌ ✔️ rando w/ only mq dungeons load save

❌ ✔️ ❌ ✔️ rando w/ both vanilla and mq dungeons ?

❌ ❌ ✔️ ❌ vanilla load save

❌ ❌ ✔️ ❌ mq ?

❌ ❌ ✔️ ❌ rando w/ only vanilla dungeons load save

❌ ❌ ✔️ ❌ rando w/ only mq dungeons load save

❌ ❌ ✔️ ❌ rando w/ both vanilla and mq dungeons load save

❌ ❌ ✔️ ✔️ vanilla ?

❌ ❌ ✔️ ✔️ mq load save

❌ ❌ ✔️ ✔️ rando w/ only vanilla dungeons load save

❌ ❌ ✔️ ✔️ rando w/ only mq dungeons load save

❌ ❌ ✔️ ✔️ rando w/ both vanilla and mq dungeons load save

Pretty much all of the question marks should result in going back to the file select screen, which should be correctly handled by this. There is one exception I can see right now, and that's the "both OTRs and MQ disabled" case.

The MQ checkbox only controls whether the save file gets created with the isMasterQuest value as true or false. As long as you have an MQ OTR you can play it even with the Checkbox disabled (basically the same way rando works, rando doesn't have to be enabled to play a previously created rando save file, MQ just has the extra wrinkle of needing the MQ OTR present to play).

EDIT: meant to say "both OTRs and MQ disabled"

tl;dr, the value of the gMasterQuest CVar is only relevant during save file creation, afterwards use a combo of gSaveContext.isMasterQuest, ResourceMgr_GameHasMasterQuest(), and ResourceMgr_GameHasOriginal()

I’ll get this change made to not update the cvar 👍

Also I think the weirdness with the gMasterQuest will go away once “the thing” is implemented

garrettjoecox · 2022-10-29T03:24:48Z

Ready to go I think 👍

briaguya0 · 2022-10-29T03:44:44Z

Ready to go I think +1

windows build is failing with a sdl link error, not sure what's going on there

leggettc18

Looks good to me.

briaguya0 · 2022-10-29T17:53:34Z

i'm going to merge this because the only issue with it was the sdl2-net stuff that was fixed by #1870

* Redone fast file select * Update soh/src/overlays/gamestates/ovl_title/z_title.c * Prevent loading incompatible saves * Dont change cvar on incompatible file

Redone fast file select

eac3c69

garrettjoecox commented Oct 26, 2022

View reviewed changes

soh/src/overlays/gamestates/ovl_title/z_title.c Outdated Show resolved Hide resolved

Update soh/src/overlays/gamestates/ovl_title/z_title.c

23c8915

briaguya0 reviewed Oct 26, 2022

View reviewed changes

briaguya0 approved these changes Oct 26, 2022

View reviewed changes

briaguya0 requested a review from dcvz October 26, 2022 05:57

Prevent loading incompatible saves

8ed5cb8

leggettc18 reviewed Oct 26, 2022

View reviewed changes

jbodner09 mentioned this pull request Oct 26, 2022

Prevent autosave when first loading a file #1792

Merged

Dont change cvar on incompatible file

3564166

briaguya0 approved these changes Oct 29, 2022

View reviewed changes

leggettc18 approved these changes Oct 29, 2022

View reviewed changes

briaguya0 merged commit 4a686cf into HarbourMasters:develop Oct 29, 2022

garrettjoecox deleted the fast-file-select-refactor branch November 2, 2022 19:09

jbodner09 mentioned this pull request Nov 13, 2023

Fix magic being zeroed out when using fast file select #3389

Merged

only vanilla otr	only mq otr	both vanilla and mq otr	mq enabled	fast file select save type	behavior
✔️	❌	❌	❌	vanilla	load save
✔️	❌	❌	❌	mq	?
✔️	❌	❌	❌	rando w/ only vanilla dungeons	load save
✔️	❌	❌	❌	rando w/ only mq dungeons	?
✔️	❌	❌	❌	rando w/ both vanilla and mq dungeons	?
❌	✔️	❌	✔️	vanilla	?
❌	✔️	❌	✔️	mq	load save
❌	✔️	❌	✔️	rando w/ only vanilla dungeons	?
❌	✔️	❌	✔️	rando w/ only mq dungeons	load save
❌	✔️	❌	✔️	rando w/ both vanilla and mq dungeons	?
❌	❌	✔️	❌	vanilla	load save
❌	❌	✔️	❌	mq	?
❌	❌	✔️	❌	rando w/ only vanilla dungeons	load save
❌	❌	✔️	❌	rando w/ only mq dungeons	load save
❌	❌	✔️	❌	rando w/ both vanilla and mq dungeons	load save
❌	❌	✔️	✔️	vanilla	?
❌	❌	✔️	✔️	mq	load save
❌	❌	✔️	✔️	rando w/ only vanilla dungeons	load save
❌	❌	✔️	✔️	rando w/ only mq dungeons	load save
❌	❌	✔️	✔️	rando w/ both vanilla and mq dungeons	load save

Conversation

garrettjoecox commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

leggettc18 commented Oct 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garrettjoecox Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garrettjoecox commented Oct 26, 2022

Uh oh!

briaguya0 left a comment

Choose a reason for hiding this comment

Uh oh!

garrettjoecox commented Oct 26, 2022

Uh oh!

garrettjoecox commented Oct 26, 2022

Uh oh!

leggettc18 commented Oct 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leggettc18 Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garrettjoecox commented Oct 29, 2022

Uh oh!

briaguya0 commented Oct 29, 2022

Uh oh!

leggettc18 left a comment

Choose a reason for hiding this comment

Uh oh!

briaguya0 commented Oct 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

garrettjoecox commented Oct 26, 2022 •

edited

Loading

garrettjoecox Oct 26, 2022 •

edited

Loading

leggettc18 Oct 26, 2022 •

edited

Loading