Doom: a11y Flickering Option Improvements

[Noseey]

Changes Summary

• Fixed issue having static strobe/glow/flash lightlevel != maxlight when changing the option to disable sector flickering and loading a previously stored save-file.
• Fixed issue having flickering-glitches in some levels when it should be disabled.

Verified on Doom1 - E1M5 and E2M4 & Doom2 Lvl 09 - no further flickering-glitches seen. Checked vanilla compatibility using 3 regular Demos (start of maps Doom2 Lvl 02, 05, 24) and 2 Demos loading from savegames (Doom 2 LVL 05), no deviation seen. No indication in the source-code found that lightlevel impacts player-detection by enemies.

[Noseey]

Two hints:

1. I saw there is a quickcheck script for doom, but I don't really know how to get it working. It would be appreciated if someone could run it on this change.
2. I kept the read/write of rlightlevel in the extsaveg.c. This is because of the swipescreen taking only the loaded lightlevel into account and the flasher-funtions are not called at this point yet.

So to sum it up:

• The maxlight reconstruction in the flasher-function is required if the user changes option from flickering -> no flickering and loads a previous savegame. After the level has been loaded, the lighting will be corrected. [Solved]
• The read/write in extsaveg.c is used to show the static light in the swipescreen correctly at loading, as long as the user saved with static light already. [Solved]
• So the last remaining issue is, that the swipescreen can show the incorrect lightlevel when the user changed option from flickering -> no flickering and loads a previous savegame. I see that as negligible, since it gets immediatelly corrected after the swipescreen ends and wouldn't be visible anymore in subsequent saves. Fixing this would imho require a bigger change on how rlightlevel is used, so I would vote for leaving it as is. [Open]

[fabiangreffrath]

Thank you for this! Could the third (open) issue be solved by early initializing the rlightlevel values already in the spawn functions?

[fabiangreffrath]

Could you please try this?

diff --git a/src/doom/p_extsaveg.c b/src/doom/p_extsaveg.c
index 95641703..be500977 100644
--- a/src/doom/p_extsaveg.c
+++ b/src/doom/p_extsaveg.c
@@ -32,6 +32,7 @@
 #include "s_sound.h"
 #include "s_musinfo.h"
 #include "z_zone.h"
+#include "a11y.h"
 
 #define MAX_LINE_LEN 260
 #define MAX_STRING_LEN 80
@@ -164,6 +165,9 @@ static void P_ReadFireFlicker (const char *key)
 
 		flick->thinker.function.acp1 = (actionf_p1)T_FireFlicker;
 
+		if (!a11y_sector_lighting)
+			flick->sector->rlightlevel = flick->maxlight;
+
 		P_AddThinker(&flick->thinker);
 	}
 }
@@ -236,40 +240,6 @@ static void P_ReadOldSpecial (const char *key)
 	}
 }
 
-// sector->rlightlevel
-
-static void P_WriteRLightlevel (const char *key)
-{
-	int i;
-	sector_t *sector;
-
-	for (i = 0, sector = sectors; i < numsectors; i++, sector++)
-	{
-		if (sector->rlightlevel != sector->lightlevel)
-		{
-			M_snprintf(line, MAX_LINE_LEN, "%s %d %d\n",
-			           key,
-			           i,
-			           (int)sector->rlightlevel);
-			fputs(line, save_stream);
-		}
-	}
-}
-
-static void P_ReadRLightlevel (const char *key)
-{
-	int sector, rlightlevel;
-
-	if (sscanf(line, "%s %d %d\n",
-	           string,
-	           &sector,
-	           &rlightlevel) == 3 &&
-	    !strncmp(string, key, MAX_STRING_LEN))
-	{
-		sectors[sector].rlightlevel = (short)rlightlevel;
-	}
-}
-
 // buttonlist[]
 
 extern void P_StartButton (line_t *line, bwhere_e w, int texture, int time);
@@ -484,7 +454,6 @@ static const extsavegdata_t extsavegdata[] =
 	{"fireflicker", P_WriteFireFlicker, P_ReadFireFlicker, 1},
 	{"soundtarget", P_WriteSoundTarget, P_ReadSoundTarget, 1},
 	{"oldspecial", P_WriteOldSpecial, P_ReadOldSpecial, 1},
-	{"rlightlevel", P_WriteRLightlevel, P_ReadRLightlevel, 1},
 	{"button", P_WriteButton, P_ReadButton, 1},
 	{"braintarget", P_WriteBrainTarget, P_ReadBrainTarget, 1},
 	{"markpoints", P_WriteMarkPoints, P_ReadMarkPoints, 1},
diff --git a/src/doom/p_saveg.c b/src/doom/p_saveg.c
index ed9fbac8..d6813bde 100644
--- a/src/doom/p_saveg.c
+++ b/src/doom/p_saveg.c
@@ -26,6 +26,7 @@
 #include "z_zone.h"
 #include "p_local.h"
 #include "p_saveg.h"
+#include "a11y.h"
 
 // State.
 #include "doomstat.h"
@@ -1283,6 +1284,9 @@ static void saveg_read_lightflash_t(lightflash_t *str)
 
     // int mintime;
     str->mintime = saveg_read32();
+
+    if (!a11y_sector_lighting)
+        str->sector->rlightlevel = str->maxlight;
 }
 
 static void saveg_write_lightflash_t(lightflash_t *str)
@@ -1338,6 +1342,9 @@ static void saveg_read_strobe_t(strobe_t *str)
 
     // int brighttime;
     str->brighttime = saveg_read32();
+
+    if (!a11y_sector_lighting)
+        str->sector->rlightlevel = str->maxlight;
 }
 
 static void saveg_write_strobe_t(strobe_t *str)
@@ -1387,6 +1394,9 @@ static void saveg_read_glow_t(glow_t *str)
 
     // int direction;
     str->direction = saveg_read32();
+
+    if (!a11y_sector_lighting)
+        str->sector->rlightlevel = str->maxlight;
 }
 
 static void saveg_write_glow_t(glow_t *str)

[Noseey]

Could you please try this?

Good Idea! On the first glance, it seems to have fixed all the three points. I will do some further checks and update the PR if everything goes well. 😄

[Noseey]

After re-running the previous demos and checking a new Demo on Doom2 Lvl 10 (turns out has the fireflicker), I see it is still compatible. So I updated the PR, thanks alot for the tip! 👍

One philosophical side-question for my sanity: I saw in the sources that vanilla doesn't seem to store the fireflicker (count) at all and doesn't seem to reconstruct the flashlight (count) when loading. Since both of them have an effect on the PRandom, am I correct to assume that loading a vanilla save-game vs. loading chocolate/crispy leads to different results and is not compatible? Ofc this is not demo-relevant, as vanilla didn't have the feature of recording / playing from savegames correctly implemented, according to doom-wiki.

[fabiangreffrath]

One philosophical side-question for my sanity: I saw in the sources that vanilla doesn't seem to store the fireflicker (count) at all and doesn't seem to reconstruct the flashlight (count) when loading.

Yes, fireflicker was introduced in Doom 2 and they simply forgot to consider it in the savegame code, which was still taken unmodified from Doom 1.

Since both of them have an effect on the PRandom, am I correct to assume that loading a vanilla save-game vs. loading chocolate/crispy leads to different results and is not compatible? Ofc this is not demo-relevant, as vanilla didn't have the feature of recording / playing from savegames correctly implemented, according to doom-wiki.

Yes, that's one of the points where the fragile construct that demo compatibility is would likely break. In general, save/restore cycles should not be assumed to be demo compatible, since e.g. thinkers get reordered upon restoring.

[fabiangreffrath]

Thank you again!

[Noseey]

Yes, that's one of the points where the fragile construct that demo compatibility is would likely break. In general, save/restore cycles should not be assumed to be demo compatible, since e.g. thinkers get reordered upon restoring.

Ok, I see, there are even further aspects to it than just the lights. Thanks for the info!