Heretic: Adding a11y-Option - Flickering Sector Lighting #1246
Conversation
Option added based upon the crispy doom implementation.
… lines and triggers
| if (a11y_sector_lighting) | ||
| flash->sector->rlightlevel = flash->sector->lightlevel; | ||
| else | ||
| flash->sector->rlightlevel = flash->maxlight; |
There was a problem hiding this comment.
Required to restore maxlight for those sectors after loading a save file. Should be fixed in crispy-doom aswell?
There was a problem hiding this comment.
Speaking of savegames, I had to add some code to doom/p_extsave.c to make sure that both values are restored for sectors for which sector->lightlevel and sector->rlightlevel differ. I don't see any such code in your PR. Does this mean that the code I added for Doom becomes unnecessary with the changes you introduced here?
There was a problem hiding this comment.
I experimented with storing, but eventually undid that. Since all the flash-lights are thinkers and they are already stored, I took the maxlight level from there to restore it on startup/load of a game. Without further testing, I presume the same could be done in Doom aswell without separate handling when storing the lightlevel.
In fact, the code in p_extsave.c doesn't appear to prevent inconsitent light-levels when changing the flickering-option between saves. You can verify this by:
- Enable Sector Light Flickering in Doom.
- Go up to a flickering light and save when the lightlevel is minimum/off (might require some timing)
- Change Setup to Disable Sector Light Flickering.
- Load the previous save
Expected Result:
Light should be a static maxlight
Actual Result:
Light remains static on last saved flickering value.
However, there is still one finding as I also undid by mistake the copy of lightlevel to rlightlevel when unarchiving. I have corrected that now. This is required to ensure that changes to lights ( e. g. caused by triggers) are correctly restored when loading a save.
There was a problem hiding this comment.
Alright, I see now. Seems you have got better insight into the topic than I had when implementing it for Doom. 😁 Could you please do me a favour and fix all the things you learned here for the Doom code base as well? I guess the P_WriteRLightlevel() and P_ReadRLightlevel() functions in doom/p_extsaveg.c could be turned into NOPs then.
There was a problem hiding this comment.
Alright, I see now. Seems you have got better insight into the topic than I had when implementing it for Doom. 😁 Could you please do me a favour and fix all the things you learned here for the Doom code base as well? I guess the
P_WriteRLightlevel()andP_ReadRLightlevel()functions indoom/p_extsaveg.ccould be turned into NOPs then.
In a separate RP, that is. Forgot to mention this. 😬
There was a problem hiding this comment.
Alright, I see now. Seems you have got better insight into the topic than I had when implementing it for Doom. 😁 Could you please do me a favour and fix all the things you learned here for the Doom code base as well? I guess the
P_WriteRLightlevel()andP_ReadRLightlevel()functions indoom/p_extsaveg.ccould be turned into NOPs then.
No worries! Like mentioned in the issue post, this also caused me quite some headache when doing the first proposal. 😀
Sure, I will take a look at doom and open a seperate pull-request!
| @@ -313,7 +313,7 @@ void R_AddLine(seg_t * line) | |||
| // reject empty lines used for triggers and special events | |||
| if (backsector->ceilingpic == frontsector->ceilingpic | |||
| && backsector->floorpic == frontsector->floorpic | |||
| && backsector->lightlevel == frontsector->lightlevel | |||
| && backsector->rlightlevel == frontsector->rlightlevel | |||
There was a problem hiding this comment.
Required to avoid flickering glitch in E1M1 Hallway where Gauntlets are located. Should be fixed in crispy-doom aswell?
There was a problem hiding this comment.
It's probably not even an oversight, as I wanted to reduce changes to apparently unrelated code paths to a minimum. My initial idea was to let the rest of the game act on the unmodified light levels, but if this leads to glitches, these should of course get fixed. Have you found similar glitches in Doom already?
There was a problem hiding this comment.
Have you found similar glitches in Doom already?
Yes, e. g. in Doom2 Level 09 near the yellow key-card:
There was a problem hiding this comment.
Oh, wow! Yes, this needs to get fixed, please!
There was a problem hiding this comment.
These glitches however only happen when the flickering is disabled. When it is enabled, the rlightlevel and lightlevel are coupled and the condition leads to correct results.
There was a problem hiding this comment.
Yes, sure, but a feature targeted at accessibility shouldn't end up in a visual glitch anyway!
| @@ -675,15 +676,15 @@ void R_StoreWallRange(int start, int stop) | |||
|
|
|||
| if (worldlow != worldbottom | |||
| || backsector->floorpic != frontsector->floorpic | |||
| || backsector->lightlevel != frontsector->lightlevel | |||
| || backsector->rlightlevel != frontsector->rlightlevel | |||
There was a problem hiding this comment.
Required to avoid flickering glitch in E2M7 Hub Area. Should be fixed in crispy-doom aswell?
.. to reflect light-changes done throughout the map.
Related Issue:
Closes #1244
Changes Summary
Verified on E1M1 and E2M7 that no further flickering-glitches remain. Checked vanilla compatibility using 4 Demos (E1M1, E2M7, E4M1, E4M7), no deviation seen. No indication in the source-code found that lightlevel impacts player-detection by enemies.