Improving savegames

[mankrip]

As you may know, Makaqu uses a bugfixed savegame format, because the original one doesn't store the initial state of the serverflags for the level.

Here's an example: Get the rune in E1M7, defeat Chthon, and get to the portal that will lead you back to the start map.
Now that you're in the start map, and you have started it with the rune in hand, save the game.
Now go to the main menu and start a new game, so you start the start map without runes.
Load the saved game. The rune is in your inventary.
When saving that game, you already had the rune when starting the map, so go to the console and enter the "restart" command.
The rune is gone!

Here's the fix; it just saves/loads the serverflags to the file before saving/loading the lightstyles. Bugfixed saves won't load in engines that doesn't support them, but by enabling savegame_compatibility this bugfix will be disabled, thus allowing us to load the game and save it again to convert it to the vanilla Quake format.

host_cmd.c:

Code: Select all

/*
===============================================================================

LOAD / SAVE GAME

===============================================================================
*/

// mankrip - serverflags bugfix - begin
#define	VANILLA_SAVESTATE_VERSION	5
#ifdef _arch_dreamcast
	#define	SAVESTATE_VERSION	VANILLA_SAVESTATE_VERSION // VMU savestates always had the serverflags bugfix
#else
	#define	SAVESTATE_VERSION	6 // only used for full saves
#endif
#define	SAVEGAME_VERSION	5 // only used for small saves

cvar_t
	savegame_compatibility = {"savegame_compatibility", "0", true}
	;
// mankrip - serverflags bugfix - end



[...]



void Host_Savegame_f (void)
{
	[...]

	fprintf (f, "%i\n", savegame_compatibility.value ? VANILLA_SAVESTATE_VERSION : SAVESTATE_VERSION); // mankrip - serverflags bugfix
	Host_SavegameComment (comment);

	[...]

	if (!savegame_compatibility.value) // mankrip - serverflags bugfix
		fprintf (f, "%i\n", svs.serverflags); // mankrip - serverflags bugfix

// write the light styles

	for (i=0 ; i<MAX_LIGHTSTYLES ; i++)
	{
		if (sv.lightstyles[i])
			fprintf (f, "%s\n", sv.lightstyles[i]);
		else
			fprintf (f,"m\n");
	}

	[...]
}



void Host_Loadgame_f (void)
{
	[...]

	fscanf (f, "%i\n", &version);
	if (version != SAVESTATE_VERSION && version != VANILLA_SAVESTATE_VERSION) // mankrip - serverflags bugfix
	{
		fclose (f);
		Con_Printf ("Savegame is version %i, not %i or %i\n", version, VANILLA_SAVESTATE_VERSION, SAVESTATE_VERSION); // mankrip - serverflags bugfix - edited
		return;
	}

	[...]

	fscanf (f, "%f\n",&time);

	if (version != VANILLA_SAVESTATE_VERSION) // mankrip - serverflags bugfix
		fscanf (f, "%i\n", &svs.serverflags); // mankrip - serverflags bugfix

	CL_Disconnect_f ();

#ifdef QUAKE2
	SV_SpawnServer (mapname, NULL);
#else
	SV_SpawnServer (mapname);
#endif

	[...]
}



[...]



void Host_InitCommands (void)
{
	Cvar_RegisterVariable (&savegame_compatibility); // mankrip - serverflags bugfix

	[...]
}

Up to version 1.5, Makaqu implemented this fix without changing the version of the saves. So, here's a hack to load saves from those versions of Makaqu:

Code: Select all

void Host_Loadgame_f (void)
{
	[...]

	fscanf (f, "%i\n", &version);
	if (version != SAVESTATE_VERSION && version != VANILLA_SAVESTATE_VERSION) // mankrip - serverflags bugfix
	{
		fclose (f);
		Con_Printf ("Savegame is version %i, not %i or %i\n", version, VANILLA_SAVESTATE_VERSION, SAVESTATE_VERSION); // mankrip - serverflags bugfix - edited
		return;
	}

	[...]

	fscanf (f, "%f\n",&time);

	// mankrip - serverflags bugfix - begin
	// hack to load Makaqu's bugfixed saves that used the old savegame version - begin
	#ifndef _arch_dreamcast
	if (version == VANILLA_SAVESTATE_VERSION)
	{
		// load everything up to the line where the first opening brace should be.
		// if it's not there, and is on the following line instead, this is a Makaqu bugfixed save.
		for (i = 0 ; i < MAX_LIGHTSTYLES ; i++)
			fscanf (f, "%s\n", str); // load the lightstyles
		fscanf (f, "%s\n", str); // try to load the first opening brace
		if (str[0] != '{') // no brace here, so it's not a vanilla version
		{
			fscanf (f, "%s\n", str); // see if the first opening brace is on the next line
			if (str[0] == '{') // the brace is here, so it's not a vanilla version
				version = SAVESTATE_VERSION; // not a vanilla version
		}
		// close and reopen the file
		fclose (f);
		f = fopen (name, "r");
		fscanf (f, "%s\n", str); // skip the version
		fscanf (f, "%s\n", str); // skip the comment
		for (i=0 ; i<NUM_SPAWN_PARMS ; i++)
			fscanf (f, "%s\n", str); // skip spawnparms
		fscanf (f, "%s\n", str); // skip the skill
		fscanf (f, "%s\n", str); // skip the mapname
		fscanf (f, "%s\n", str); // skip the time
		// TODO: overwrite the file automatically, updating its version to SAVESTATE_VERSION to fix it?
	}
	#endif
	// hack to load Makaqu's bugfixed saves that used the old savegame version - end

	if (version != VANILLA_SAVESTATE_VERSION)
		fscanf (f, "%i\n", &svs.serverflags);
	// mankrip - serverflags bugfix - end

	CL_Disconnect_f ();

#ifdef QUAKE2
	SV_SpawnServer (mapname, NULL);
#else
	SV_SpawnServer (mapname);
#endif

	[...]
}

I've also added a menu option to set the version of the saves that will be created. Since Makaqu uses a different Single Player menu, I've added the option to the Single Player menu instead of to the Options menu.

menu.c:

Code: Select all

extern cvar_t
	savegame_compatibility // mankrip - serverflags bugfix
	;



[...]



//=============================================================================
/* SINGLE PLAYER MENU */

// mankrip - serverflags bugfix - begin
#ifdef _arch_dreamcast
	#define	SINGLEPLAYER_ITEMS	5
#else
	#define	SINGLEPLAYER_ITEMS	6
#endif
// mankrip - serverflags bugfix - end



[...]



void M_SinglePlayer_Draw (void)
{
	// mankrip - begin
	int y = 20;

	M_DrawPlaque ("gfx/ttl_sgl.lmp", true);

	M_Print (16, y += 8, "              New game");
	M_Print (16, y += 8, "             Load game    ...");
	M_Print (16, y += 8, "             Save game    ...");
	M_Print (16, y += 8, "            Load state    ...");
	M_Print (16, y += 8, "            Save state    ...");
	#ifndef _arch_dreamcast
	M_Print (16, y += 8, "     Savestate version"); M_Print (220, y, (savegame_compatibility.value) ? "Old" : "New"); // serverflags bugfix
	#endif

	M_DrawCursor (200, 28, m_cursor[m_state]);
	// mankrip - end
}


void M_SinglePlayer_Key (int key)
{
	if (m_inp_cancel)
		M_Main_f ();
	else if (m_inp_up)
	{
		S_LocalSound ("misc/menu1.wav");
		if (--m_cursor[m_state] < 0)
			m_cursor[m_state] = SINGLEPLAYER_ITEMS - 1;
	}
	else if (m_inp_down)
	{
		S_LocalSound ("misc/menu1.wav");
		if (++m_cursor[m_state] >= SINGLEPLAYER_ITEMS)
			m_cursor[m_state] = 0;
	}
	// mankrip - serverflags bugfix - begin
	#ifndef _arch_dreamcast
	else if (m_cursor[m_state] == 5)
	{
		if (m_inp_left || m_inp_right || m_inp_ok)
		{
			S_LocalSound ("misc/menu3.wav");
			Cvar_SetValue ("savegame_compatibility", !savegame_compatibility.value);
		}
	}
	#endif
	// mankrip - serverflags bugfix - end
	else if (m_inp_ok)
	{
		m_entersound = true;
		switch (m_cursor[m_state]) // mankrip - replaced cursor variables
		{
		case 0:
			if (sv.active)
				M_PopUp_f("Are you sure you want to\nstart a new game?", "disconnect\nmaxplayers 1\nmap start\n"); // mankrip
			else
				Cbuf_AddText("disconnect\nmaxplayers 1\nmap start\n"); // mankrip
			break;
		// mankrip - small saves - begin
		case 1:
			// mankrip - VMU saves - begin
			#ifdef _arch_dreamcast
			M_VMUloadsmall_f ();
			#else
			// mankrip - VMU saves - end
			M_LoadSmall_f ();
			#endif // mankrip - VMU saves
			break;

		case 2:
			// mankrip - VMU saves - begin
			#ifdef _arch_dreamcast
			M_VMUsavesmall_f ();
			#else
			// mankrip - VMU saves - end
			M_SaveSmall_f ();
			#endif // mankrip - VMU saves
			break;
		// mankrip - small saves - end
		case 3:
			// mankrip - VMU saves - begin
			#ifdef _arch_dreamcast
			M_VMUload_f ();
			#else
			// mankrip - VMU saves - end
			M_Load_f ();
			#endif // mankrip - VMU saves
			break;

		case 4:
			// mankrip - VMU saves - begin
			#ifdef _arch_dreamcast
			M_VMUsave_f ();
			#else
			// mankrip - VMU saves - end
			M_Save_f ();
			#endif // mankrip - VMU saves
			break;

		default:
			break;
		}
	}
}

I don't know of any other engine that is aware of that bug. qbism's Super8 has the fix I've implemented in Makaqu, Engoo still has that bug, and I haven't tested it in any hardware-accelerated engine recently, but back then I couldn't find any other engine that fixed that.

This fix requires a small change in the savegame format. So, since Makaqu's saves already have this difference and older engines won't be able to read its saves properly anyway, I'm thinking of implementing more features in the saves.

One that I really need is to save the actual viewangles of the player. It's really annoying when we want to save the game with the camera pointing at a very specific point, only to have the camera turned to a completely different direction upon loading the save.

Another thing I'm thinking about are the particles. Shoot a rocket, and save the game exactly after it explodes. When you close the menu and go back to the game the particles will be there, but after that, when you load the game the particles are gone. However, saving the state of all those particles could inflate the size of the saves quite a lot, so I'm not sure whether fixing that would be desirable.

What are other bugfixes and improvements that could be added to the saves?

[mh]

I've noticed the runebug before but only after issuing a "kill" command, and hacked around that case in DirectQ - just saving out the serverflags, running the command, and restoring them when done. Wasn't aware that there was a more general case of it.

DirectQ has powerup counters that depend on cl.itemgetime and I recently had a query from a player about those not being restored properly on a save/load cycle.

Many things send to the client via stuffcmd won't be saved/restored properly. These would include fog commands, loadsky commands, v_cshift stuff, r_wateralpha, anything that is a cmd_t or a non-archived cvar. Lots of mods do this.

[Baker]

Quake should use SVCs for everything. And svc for bonus flash, fog, skyboxes, etc. etc.

But that isn't the real world.

Some things use client-state cvars, like vcshift, and other things use server-state cvars. Like a single player map that toggles sv_gravity.

http://www.quaddicted.com/reviews/starship.html

Really all the client-state stuff should be stored in "HUD-like fields".

Quite a messy topic. Plus the "client-side worldspawn hacks" like skybox/fog.

[Spike]

Why?
Why should there be a separate SVC for _every_ _single_ _piece_ _of_ _game_ _state_.
Underscores annoying you? That's what supporting 500 svcs is like.
Each svc needs checking for modification, it needs the code to trigger it in qc, it needs the code to write it out, it needs the code to parse it, it needs to use different svc numbers in different engines or even just with different protocols in a single engine... etc...
Frankly, specialist svcs are no less evil than stuffcmds. At least stuffcmds don't result in clients erroring out due to illegible messages.
Stats, on the other hand, are stored in fields. Saved games save those directly. They're trivially restored without having to do any special stuff purely because its a saved game, etc, heck - they also work with starting recording a demo mid-map without any extra changes (however, I should point out that stats can conflict in numbers just as much as svcs do, and csqc is 'allowed' to use stats 32-127 freely without conflict - dp already uses stats starting at 255- for player movement properties like gravity).
With CSQC and saved games, stats are the *only* clean way. Which is why nexuiz used temp entities to send client settings... grr.

fog should be part of the map format ala q3 if only because that gives nice volumetric fog instead of simple global fog (although mobile fog regions attached to entities would also be cool although impractical). skyboxes are the same, again q3 uses a client-side shader for those. ignoring the texture in the bsp and using some skybox instead is a hack just as much as anything else.

client-side parsing of worldspawn fields is required for mods that don't know about those fields, for mods that don't know if the engine uses an svc, or a stuffcmd, or what number or order of arguments to use. Worldspawn hacks are generally more standardized as they're typically based upon existing content, kinda required for halflife bsps anyway, and guarenteed to be reloaded on saved games/demos etc.
Either way, visuals should follow content, not qc. Mappers don't want to be at the mercy of the qc modders in addition to the engine modders, its a double whammy.


Anyway, back on topic.
A couple of engines support mid-map precaching of sounds/models. Engines that do need to store the list of those in the map. The same goes for anything that traditionally goes into MSG_INIT, including makestatic, staticsounds, etc, though I'm not enturely sure how those are useful mid-map.
A couple of engines support hub systems (including rmqe now, apparently). Such engines need to save the other maps in addition to the current map! As well as allowing restart to work properly.
Regarding the runes... I can see the issue, yeah... Can't say I ever noticed it. You can probably just update svs.serverflags to match the global and ignore if they get too many runes until there's a more significant savegame change that warrents making it incompatible.

[mankrip]

My intention is basically to improve the quality of the engine. I don't care much about supporting specific mods, so stuff like hub systems, although cool, doesn't spark my interest.

Also, cvars and stuffcmds are things that could be re-set again by the QC code itself, so I don't see a strong argument in favor of it. For example, my JoyMenu mod already has code for detecting when a save is loaded, and sets everything accordingly.

If something improves the original game, I'm much more inclined to implement it. Another example would be to store the previous model frames used for each entity, so the interpolation for the current frames can be properly displayed upon loading a save.

Regarding the runes... I can see the issue, yeah... Can't say I ever noticed it. You can probably just update svs.serverflags to match the global

That wouldn't work. If you save the game in E1M7 after getting the rune, and then load the game and restart the map, the rune shouldn't be there, because you didn't start the map with it.

My solution was basically to make the serverflags mimic the behavior of the spawnparms. I should probably turn it into a tutorial, but afaik I've commented the changes well enough for anyone to be able to copy them.

[mh]

I won't pretend to know what the right solution is here. svc_'s are no better than stuffcmd - worse in fact for the reasons that Spike outlined. They just add another layer of complexity without actually solving anything that's really broken about stuffcmd. Add a .cfg file to each save game, capture each stuffcmd as it happens, and write it to the .cfg file on save? Don't know. Unfortunately we're in a position where stuffcmd has become the accepted way of doing this kind of thing, warts and all, and even otherwise really really good and well-implemented stuff (like ne_tower) stuffs r_wateralpha at the start and never bothers to save/restore it through a save/quit/restart/load cycle.

[Spike]

r_wateralpha is another of them content things that would have had a worldspawn key if only it had not existed in vanilla glquake.

mankrip, if you're really bored, you could try saving coop games?

[qbism]

Vanilla only has 30 or so svc_ commands. 5 or 10 more for typical "things that could be triggered" would not be so bad. Even Fitzquake has svc_fog and svc_skybox.

Unfortunately, where lacking a precedent, the numbering of these new commands is left to the whims of the engine coder. The true unfortunate thing is that the ordering even matters.

For skyboxes, fog, etc. could there not be added keypairs in mapping that the engine would support? A super basic shader system. Alpha is already out there in use.

[mankrip]

mankrip, if you're really bored, you could try saving coop games?

That's actually a great suggestion, and something that's really needed. I'll add that to my todo list.

[Spike]

Vanilla only has 30 or so svc_ commands. 5 or 10 more for typical "things that could be triggered" would not be so bad. Even Fitzquake has svc_fog and svc_skybox.

Unfortunately, where lacking a precedent, the numbering of these new commands is left to the whims of the engine coder. The true unfortunate thing is that the ordering even matters.

For skyboxes, fog, etc. could there not be added keypairs in mapping that the engine would support? A super basic shader system. Alpha is already out there in use.

FTE has svcs, it has stuffcmds, it has parsing of worldspawn, it has serverinfo. How many more ways are you going to create to send part of a .bsp to clients from the server?!?
Of course svc numbering is left to the whims of the engine coder. Generally because people make new svcs that are already used for something. Silly coders using stuff that conflicts with everything else...
Tbh, the skybox/fog stuff in worldspawn is the most sane way to do it. Stuff that is part of the map stays part of the map. Stuff like mapgroups with user-specified sky overriding 'just works' (assuming its coded properly anyway), without mods trying to break things (what with mods knowing user desires better than the client that actually knows all the settings the user chose to set and all).
Anyway, I'm trying to make this post more comic than it really needs to be. My personal feeling is that sky+fog options are too specific in a world where mods are stuffcmding all sorts of things 'because the user will have the wrong defaults'. Content should control how the content looks, not the mod, and tbh not the engine (unless the engine simply doesn't support said feature, in which case there's not really any choice anyway).
I hereby propose some "_cl_exec" worldspawn key for mappers to use without having to depend upon mods, which ideally latches the cvars it specifies somewhow like FTE does with stuffcmds, so that they're reset for the start of the following map. Tracking the named cvars initial value before the map and resetting those cvars to that value on map end would do the job. This would allow control of all sorts of stuff, rtlight_world enables, etc, everything, scoped to just that map, without having to worry if the mod supports various fields etc. If the engine doesn't know the cvar, it can just silence (or dprint) the warning. Fixes saved games, removes network incompatibility, etc.

[mankrip]

This "_cl_exec" most likely wouldn't be a simple replacement for stuffcmds then, because if you parse it like a commandline, it would allow the map to execute external config files, and that could make latching cvars even more complicated because cvars could be changed externally.

Aliases, for example, are unpredictable, as there's no sure way to predict when they'll be called, so we have no way to predict and control things such as alias +attack "fov 30;impulse 69"; alias -attack "fov 111; impulse 88" overriding cvars for that map only. And similarly, latching stuff such as keybindings could make things really hairy. Not to mention "what if the author decides to put a changelevel command into that?".

Something like worldspawn keys beginning with "_map_cvar_*" and limited to a single cvar per entry could be safer.

[Baker]

Vanilla only has 30 or so svc_ commands. 5 or 10 more for typical "things that could be triggered" would not be so bad. Even Fitzquake has svc_fog and svc_skybox.

Unfortunately, where lacking a precedent, the numbering of these new commands is left to the whims of the engine coder. The true unfortunate thing is that the ordering even matters.

For skyboxes, fog, etc. could there not be added keypairs in mapping that the engine would support? A super basic shader system. Alpha is already out there in use.

FTE has svcs, it has stuffcmds, it has parsing of worldspawn, it has serverinfo. How many more ways are you going to create to send part of a .bsp to clients from the server?!?
Of course svc numbering is left to the whims of the engine coder. Generally because people make new svcs that are already used for something. Silly coders using stuff that conflicts with everything else...
Tbh, the skybox/fog stuff in worldspawn is the most sane way to do it. Stuff that is part of the map stays part of the map. Stuff like mapgroups with user-specified sky overriding 'just works' (assuming its coded properly anyway), without mods trying to break things (what with mods knowing user desires better than the client that actually knows all the settings the user chose to set and all).
Anyway, I'm trying to make this post more comic than it really needs to be. My personal feeling is that sky+fog options are too specific in a world where mods are stuffcmding all sorts of things 'because the user will have the wrong defaults'. Content should control how the content looks, not the mod, and tbh not the engine (unless the engine simply doesn't support said feature, in which case there's not really any choice anyway).
I hereby propose some "_cl_exec" worldspawn key for mappers to use without having to depend upon mods, which ideally latches the cvars it specifies somewhow like FTE does with stuffcmds, so that they're reset for the start of the following map. Tracking the named cvars initial value before the map and resetting those cvars to that value on map end would do the job. This would allow control of all sorts of stuff, rtlight_world enables, etc, everything, scoped to just that map, without having to worry if the mod supports various fields etc. If the engine doesn't know the cvar, it can just silence (or dprint) the warning. Fixes saved games, removes network incompatibility, etc.

With external ent support being easy to implement that really isn't a bad idea.

[Spike]

individual _map_cvar_* fields works.
The mapper might try setting r_texturemode with this so beware of that potential expectation (its already a cvar in many QW engines).
Being worried about exec commands is probably not a major issue as a server/mod can already stuffcmd anything malitious anyway, this should thus follow the same rules/restrictions as those stuffcmds.
Individual fields are indeed better, if only because you don't get huge great big strings with mappers who are very specific about how their map should look.
cvars that are not known should probably be ignored silently, and as the cvars ought to be restored before the next map is loaded it has to be a somewhat separate parser anyway.
Dedicated servers should probably also exec them, if only for consistancy (or maps that insist on things like pm_airstep in qw).

[mh]

Worldspawn keys is the only sane way. I proposed this before for r_wateralpha but it got shot down. It's good to see it coming up for discussion again.

Bonus is that engines that don't support the feature will just happily and safely ignore it, giving a better gameplay experience to the user (no console spamming with "unknown command" crap every frame).

You could also hack surface flags using this method - add a "surfaceflag" "texturename value" key or something (done this way in case a texture name conflicts with an existing known key).

[qbism]

Worldspawn keys is the only sane way. I proposed this before for r_wateralpha but it got shot down. It's good to see it coming up for discussion again.

It would be easy to extend Fog_ParseWorldspawn, make it a more general case