woof icon indicating copy to clipboard operation
woof copied to clipboard

SSG-flash correction causes desyncs

Open MrAlaux opened this issue 2 months ago • 4 comments

Tested in the latest master artifact (Win-x64).

Before anything, I'd like to say that this seems to be of very-low priority; I'm not aware of any real cases.

This is the correction in question: https://github.com/fabiangreffrath/woof/blob/58c957b715f5f79a5976ef462e7c2ef6e996001f/src/info.c#L162-L165

I could tell right away that it could cause issues, so I put together a little DeHackEd patch that adds a state to the SSG flash animation which fires a rocket, and recorded a demo with it in Chocolate. Both the patch and demo are here: SSG-Flash_Desync.zip.

Sure enough, the demo desynced in the Woof artifact mentioned above. I checked the DSDA 0.27.5 sources and it doesn't have the correction, and as expected, the demo does sync in it.

Judging by the comment, it would appear that the correction was implemented into either Boom or MBF very long ago. If that's truly the case, it should probably be complevel-dependent, meaning that neither Woof nor DSDA handle it correctly.

Admittedly, this is something very obscure; assuming that it is a Boom/MBF-specific change, it has been around for over 25 years, and yet I've never heard about it causing desyncs, and judging by the fact that neither Woof nor DSDA handle it as I believe they should, I suspect that it was never reviewed because it never caused any issues.

What do you think? Maybe I missed something.

MrAlaux avatar Apr 29 '24 07:04 MrAlaux

Seems this was introduced between Boom and MBF. Since every behavior that's different from PrBoom+, even in complevel 11 and even if that'd be real MBF code, we should revert this and guide it with the MBF_STRICT macro.

fabiangreffrath avatar Apr 29 '24 07:04 fabiangreffrath

I must admit that I'd miss the fix for casual play (and others may notice and complain about it), which is why I suggested the "complevel-dependent" approach; if not in demo or net play, and if Strict Mode were disabled, the correction would remain applied regardless of complevel. Then again, that approach would require changing the state even after startup, given that Strict Mode can be toggled. Perhaps an autoloaded DeHackEd patch to correct it is enough.

MrAlaux avatar Apr 29 '24 07:04 MrAlaux

We don't support conditional autoloading, so a dehacked patch in the autoload directory would be just the same as hard-coding the fix. I agree that a run-time fix would be best, but then we'd need to track that the state hasn't been modified by any other dehacked patch in between.

fabiangreffrath avatar Apr 29 '24 08:04 fabiangreffrath

We don't support conditional autoloading, so a dehacked patch in the autoload directory would be just the same as hard-coding the fix.

Sorry, I should've clarified: I meant on my end.

I agree that a run-time fix would be best, but then we'd need to track that the state hasn't been modified by any other dehacked patch in between.

Well, I believe that checking just once at startup would be good enough for demo and net play, since complevel and Strict Mode shouldn't change during that. It wouldn't cover the case of toggling Strict Mode, but I'd say that's passable, and I guess it wouldn't conflict with patches loaded afterwards, because D_InitTables() manages to do something similar to apply some MBF21 stuff.

MrAlaux avatar Apr 29 '24 08:04 MrAlaux

Well, the complevel may still change between the demo reel playback and actual play. And even regardless of Strict Mode, if a WAD loads a DEHACKED lump that relies on the exact length of a state, that would still cause issues.

fabiangreffrath avatar May 07 '24 09:05 fabiangreffrath

Maybe the correction of the SSG flash state duration is something that's worth suggesting for the upcoming successor of the MBF21 complevel.

fabiangreffrath avatar May 09 '24 21:05 fabiangreffrath