Better Savefile Versioning (Define Macros Edition) (#91898)

Savefile versioning is quite robust, but it relied a lot on comments to
explain why exactly it worked (such as needing to explain that
error/empty states were negative integers only because any valid
savefile version would be a positive integer). Anyways, let's spell it
out a lot more using `#define` values and dmdoccing said values in order
to make the whole system a bit more cohesive.

This also corrects an issue that has irked me for a while, which is that
we call and go through `update_preferences()` or `update_character()`
_every single time_. `SAVEFILE_VERSION_MAX` is essentially just a
placeholder, it's not checked against anything! Let's use that in one of
the new macros we created so we can slim down on the proc call overhead
(as well as whole branches of code) that we iterate through every single
time we load a savefile. Like we were generating a backup json every
single run without fail, let's trim that out when it's not necessary.

More clear for people operating on this code in the future as well as
does a lot less work. Win win.
Tested it and it migrated my savefile from 48 -> 49 without any ill
effect. Let me know if we want to have the redundancy behavior baked in
where we call these procs/enter code branches every single time though-
I would rather just bake the code in that way rather than live in a
world of confusion on why it operates that way.

🆑
server: Savefile migration has been tweaked a bit, though there
realistically shouldn't be any issues with this. Keep a wary eye out
though.
/🆑

code/modules/client/preferences_savefile.dm

@@ -1,12 +1,23 @@
-//This is the lowest supported version, anything below this is completely obsolete and the entire savefile will be wiped.
+/// Placeholder to check against the SAVE_DATA_EMPTY and SAVE_DATA_OBSOLETE values.
+/// _Any_ generated save data version will be zero or a positive integer, so it's only necessary to check against this value for anything negative (error states).
+#define SAVE_DATA_NO_ERROR 0
+/// Typically signifies an empty list, where the savefile is not loaded or the character is new. Will just trigger a regeneration.
+#define SAVE_DATA_EMPTY -1
+/// The save data is below the accepted minimum and should be reset.
+#define SAVE_DATA_OBSOLETE -2
+
+/// This is the lowest supported version, anything below this is completely obsolete and the entire savefile will be wiped.
 #define SAVEFILE_VERSION_MIN 32
 
-//This is the current version, anything below this will attempt to update (if it's not obsolete)
-// You do not need to raise this if you are adding new values that have sane defaults.
-// Only raise this value when changing the meaning/format/name/layout of an existing value
-// where you would want the updater procs below to run
+/// This is the current version, anything below this will attempt to update (if it's not obsolete)
+/// You do not need to raise this if you are adding new values that have sane defaults.
+/// Only raise this value when changing the meaning/format/name/layout of an existing value
+/// where you would want the updater procs below to run
 #define SAVEFILE_VERSION_MAX 49
 
+#define IS_DATA_OBSOLETE(version) (version == SAVE_DATA_OBSOLETE)
+#define SHOULD_UPDATE_DATA(version) (version >= SAVE_DATA_NO_ERROR && version < SAVEFILE_VERSION_MAX)
+
 /*
 SAVEFILE UPDATING/VERSIONING - 'Simplified', or rather, more coder-friendly ~Carn
 	This proc checks if the current directory of the savefile S needs updating
@@ -23,14 +34,16 @@ SAVEFILE UPDATING/VERSIONING - 'Simplified', or rather, more coder-friendly ~Car
 	Failing all that, the standard sanity checks are performed. They simply check the data is suitable, reverting to
 	initial() values if necessary.
 */
-/datum/preferences/proc/save_data_needs_update(list/save_data)
-	if(!save_data) // empty list, either savefile isnt loaded or its a new char
-		return -3 // BUBBER EDIT
-	if(save_data["version"] < SAVEFILE_VERSION_MIN)
-		return -2
-	if(save_data["version"] < SAVEFILE_VERSION_MAX)
-		return save_data["version"]
-	return -1
+/datum/preferences/proc/check_savedata_version(list/save_data)
+	if(!save_data)
+		return -3 // BUBBER EDIT - ORIGINAL: return SAVE_DATA_EMPTY
+	var/save_version = save_data["version"]
+
+	if(save_version < SAVEFILE_VERSION_MIN)
+		return SAVE_DATA_OBSOLETE
+	if(save_version < SAVEFILE_VERSION_MAX)
+		return save_version
+	return SAVE_DATA_EMPTY
 
 //should these procs get fairly long
 //just increase SAVEFILE_VERSION_MIN so it's not as far behind
@@ -186,8 +199,8 @@ SAVEFILE UPDATING/VERSIONING - 'Simplified', or rather, more coder-friendly ~Car
 			stack_trace("Failed to load the savefile for [parent] after manually calling load_savefile; something is very wrong.")
 			return FALSE
 
-	var/needs_update = save_data_needs_update(savefile.get_entry())
-	if(load_and_save && (needs_update == -2)) //fatal, can't load any data
+	var/data_validity_integer = check_savedata_version(savefile.get_entry())
+	if(load_and_save && IS_DATA_OBSOLETE(data_validity_integer)) //fatal, can't load any data
 		var/bacpath = "[path].updatebac" //todo: if the savefile version is higher then the server, check the backup, and give the player a prompt to load the backup
 		if (fexists(bacpath))
 			fdel(bacpath) //only keep 1 version of backup
@@ -222,12 +235,12 @@ SAVEFILE UPDATING/VERSIONING - 'Simplified', or rather, more coder-friendly ~Car
 	key_bindings = savefile.get_entry("key_bindings", key_bindings)
 
 	//try to fix any outdated data if necessary
-	if(needs_update >= 0)
+	if(SHOULD_UPDATE_DATA(data_validity_integer))
 		var/bacpath = "[path].updatebac" //todo: if the savefile version is higher then the server, check the backup, and give the player a prompt to load the backup
 		if (fexists(bacpath))
 			fdel(bacpath) //only keep 1 version of backup
 		fcopy(savefile.path, bacpath) //byond helpfully lets you use a savefile for the first arg.
-		update_preferences(needs_update, savefile) //needs_update = savefile_version if we need an update (positive integer)
+		update_preferences(data_validity_integer, savefile)
 
 	check_keybindings() // this apparently fails every time and overwrites any unloaded prefs with the default values, so don't load anything after this line or it won't actually save
 	key_bindings_by_key = get_key_bindings_by_key(key_bindings)
@@ -240,7 +253,7 @@ SAVEFILE UPDATING/VERSIONING - 'Simplified', or rather, more coder-friendly ~Car
 	key_bindings = sanitize_keybindings(key_bindings)
 	favorite_outfits = SANITIZE_LIST(favorite_outfits)
 
-	if(needs_update >= 0) //save the updated version
+	if(SHOULD_UPDATE_DATA(data_validity_integer)) //save the updated version
 		var/old_default_slot = default_slot
 		var/old_max_save_slots = max_save_slots
 
@@ -301,8 +314,8 @@ SAVEFILE UPDATING/VERSIONING - 'Simplified', or rather, more coder-friendly ~Car
 
 	var/tree_key = "character[slot]"
 	var/list/save_data = savefile.get_entry(tree_key)
-	var/needs_update = save_data_needs_update(save_data)
-	if(needs_update == -2) //fatal, can't load any data
+	var/data_validity_integer = check_savedata_version(save_data)
+	if(IS_DATA_OBSOLETE(data_validity_integer)) //fatal, can't load any data
 		return FALSE
 
 	// Read everything into cache
@@ -326,8 +339,8 @@ SAVEFILE UPDATING/VERSIONING - 'Simplified', or rather, more coder-friendly ~Car
 
 	//try to fix any outdated data if necessary
 	//preference updating will handle saving the updated data for us.
-	if(needs_update >= 0)
-		update_character(needs_update, save_data) //needs_update == savefile_version if we need an update (positive integer)
+	if(SHOULD_UPDATE_DATA(data_validity_integer))
+		update_character(data_validity_integer, save_data)
 
 	//Sanitize
 	randomise = SANITIZE_LIST(randomise)
@@ -440,3 +453,8 @@ SAVEFILE UPDATING/VERSIONING - 'Simplified', or rather, more coder-friendly ~Car
 
 #undef SAVEFILE_VERSION_MAX
 #undef SAVEFILE_VERSION_MIN
+#undef SAVE_DATA_NO_ERROR
+#undef SAVE_DATA_EMPTY
+#undef SAVE_DATA_OBSOLETE
+#undef IS_DATA_OBSOLETE
+#undef SHOULD_UPDATE_DATA