-
Notifications
You must be signed in to change notification settings - Fork 86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix crash when adding or removing NOBLOCKMAP and NOSECTOR flags #504
Conversation
…P or NOSECTOR flags
…/remove behavior in demos that were recorded before the fix
@fabiangreffrath @rfomin please take a look as well, since we'll need to match behavior in woof |
Looks good to me. Thanks, we'll add this. |
Any chance we get away without another comp flag? |
Maybe -- kraflab would know best here, but the comp flag is only there to make sure that any previously-existing MBF21 demos still sync, even if they ran into the bug. Since this is a crash fix, there's no value at all in letting users set the comp flag, we're just being extra-strict on demo compatibility because that be how dsda do. ;) That said, I don't even know if there are any such demos out there in the wild -- this goes way out of my area of knowledge though; the dsda crew are the oracles here. If the desync risk is low and everyone's fine with Woof! and company maybe not syncing said demos if they exist, then yoinking the fix without the comp flag will work just fine. The flag still oughta go in the MBF21 spec regardless, since it does take up a "slot" in the table. The demo playback code will at a minimum need to read the flag and ignore it. |
I think the comp flag is needed in case demos get recorded in the future on versions that don't yet have the fix. This doesn't always crash, so it's entirely possible that future wads come out that have this behavior change but don't crash, which would mean someone may record demos that don't sync in later releases. On the other hand, those demos would be playing the wad with broken behavior unintended by the author, so would they even be important to keep in sync with? 🤔 If we go without a comp flag, we'll have to run a sync check against all existing mbf21 demos. |
Putting this through regression testing right now. If there are no issues with existing demos we'll go without the comp flag. |
No desyncs, just taking this then without the flag: c892178 |
If MBF21's A_AddFlags or A_RemoveFlags is used to add or set the NOBLOCKMAP or NOSECTOR flags, the game can crash due to a dangling actor pointer. This can occasionally be seen with Legacy of Rust (id1.wad)'s Vassago fireballs when they impact a wal -- it's luck of the draw whether it decides to crash, since it's invalid memory shenanigans, but either way it's buggy. :P
This PR fixes the crash, and adds a new
comp_blockmapflags
option (not settable via OPTIONS lump since it's strictly a bugfix) that re-enables the buggy behavior for any demos that were recorded before the fix. Attached a quick demo (requires id1.wad) recorded on the release build, just as a demo of the comp functionality.comp_blockmapflags.lmp.zip
Lemme know if this all looks kosher and I'll make a PR to the MBF21 repo documenting the new comp option (sort of a minor implementation detail in a way, since it's not user-settable, but the docs oughta be accurate ;).