Double quickload slot selection bug

[phantombeta]

When you select a quickload slot, the first time seems to fail, so you have to do it twice. No idea what might cause this bug.

Steps to reproduce:

• Start a new game.
• Press quickload.
• Try to load a saved game. GZDoom will successfully load the saved game.
• Press quickload again.
• GZDoom will now show you the "Load Game" menu again.
• Selecting a game now will actually set the quicksave slot. (Or, if you quit the Load Game menu, it'll crash GZDoom.)

[Graf Zahl]

Does anybody actually use that semi-broken feature? I'd rather get rid of it entirely in favor of the new quicksave rotation than to fix something this shitty feature.

[phantombeta]

I very much do, myself, and I'm sure others use the normal quicksave/load too. I'd rather not be forced to use that quicksave rotation thing, either, myself.

[Graf Zahl]

Ok then. I'll leave it as it is. This feature as implemented makes no sense to me so I'd rather leave the fixes to someone for whom it does.

[drfrag]

But when did the bug appear? I doubt it was there right from the start since you're reporting it now.

[phantombeta]

The bug has existed since at least GZDoom 2.3.2 1.7.1. Not being reported doesn't mean it's new. It can simply mean that either no one noticed or no one bothered reporting it.
[Edit]I decided to search for the first version with this bug. This bug seems to have first appeared in ZDoom 2.6.0.

[drfrag]

To reproduce it you need to quit after creating the savegames else the first one you created is selected. May be it's actually there since the feature was added/from the start.

[Graf Zahl]

Didn't I say that the feature looks broken by design?

[Zhs2]

If this implementation is broken by design, then would it be simpler to add on to the "quicksave rotation" feature a way for a user to set a specific file to save to when the quicksave button is hit for the session? It always peeved me that once you've selected which file to quicksave to, that's the file that gets written for the whole duration GZDoom stays open, with no outward facing feedback about how to change it at will, too bad if you selected the wrong file accidentally. I'd imagine fans of the current implementation's concept (like myself) would be more than happy to see the code dumped if able to specify some file or name to save to in a menu. (And it wouldn't step on use of Autoautosave...)

[Graf Zahl]

"Broken by design" is a subjective thing. To me the feature and "quick" do not mix.

[drfrag]

The feature is completely broken, may be it's even an unfinished feature. The question is what's the design here if any.
The behavior is inconsistent. First we need to investigate what makes sense, always selecting it seems?
If there are no savegames you create one for quicksaving, but obviously that's not a dedicated quicksave slot but a normal one.
If you create two saves the first one is used, if you quit and relaunch you select.
But if you relaunch and create a third one, the last one is used directly.

To me sorting alphabetically also doesn't make much sense so i changed the order (optional) to reverse slot number (filename). Right now the new dedicated quicksave slots appear in the middle of the list. In my version autos go first, then quicks and finally normal slots.

Edit: quicksave rotation uses dedicated slots and allows to use just one slot.

[Graf Zahl]

The feature is an original Doom feature, it's a hack from ancient times and I agree with all your findings. I personally think it should be removed entirely, those who are used to classic one-slot quicksaving may just use a quicksave rotation with only one slot.

[drfrag]

Okay, i've tested it with RUDE and now i know how it's supposed to work. You always need to select a quicksave slot (with quicksave) on every session, else you get an error message saying that you haven't picked a quicksave slot yet.
Edit: according to phantombeta the bug appeared in ZDoom 2.6.0.

[drfrag]

I confirm that 2.5.0 is not affected but that's a minor thing, the feature is still broken. Note that in ZDoom you can set the quicksave slot using quickload and even regular save, that thing was condemned to fail.
Edit: it's the same in 1.23 beta, 1.22 still had the original Doom empty slots but instead of getting the error message it would perform a regular load.

Edit2: after reading the changelog now i think i know how it's supposed to work in ZDoom.
You can't select the quicksave slot with quicksave (unlike in original Doom).
For every session if you save the first save will select the quicksave slot, if not you must select with quickload.
So now i think it actually makes some sense.

[drfrag]

The bug was not present in 2.5.0 but there was a major refactoring and the code is very different, those changes are not in the repo.
I've tried to debug it and savegameManager.quickSaveSlot is still null when it enters CCMD (quickload) the second time.

Code: Select all

	if (savegameManager.quickSaveSlot == NULL || savegameManager.quickSaveSlot == (FSaveGameNode*)1)
	{
		M_StartControlPanel(true);
		// signal that whatever gets loaded should be the new quicksave
		savegameManager.quickSaveSlot = (FSaveGameNode *)1;
		M_SetMenu(NAME_Loadgamemenu);
		return;
	}

quickSaveSlot is set in FSavegameManager::LoadSavegame but i cannot debug it, it enters in DEFINE_ACTION_FUNCTION(FSavegameManager, LoadSavegame) instead and it already enters there the first time.

Code: Select all

void FSavegameManager::LoadSavegame(int Selected)
{
	G_LoadGame(SaveGames[Selected]->Filename.GetChars(), true);
	if (quickSaveSlot == (FSaveGameNode*)1)
	{
		quickSaveSlot = SaveGames[Selected];
	}
	M_ClearMenus();
	V_SetBorderNeedRefresh();
	LastAccessed = Selected;
}