Skip to content
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

Closed
wants to merge 3 commits into from

Conversation

XaserAcheron
Copy link
Contributor

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 ;).

@kraflab
Copy link
Owner

kraflab commented Aug 22, 2024

@fabiangreffrath @rfomin please take a look as well, since we'll need to match behavior in woof

@rfomin
Copy link
Contributor

rfomin commented Aug 23, 2024

@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.

@fabiangreffrath
Copy link
Contributor

Any chance we get away without another comp flag?

@XaserAcheron
Copy link
Contributor Author

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.

@kraflab
Copy link
Owner

kraflab commented Aug 30, 2024

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.

@kraflab
Copy link
Owner

kraflab commented Sep 26, 2024

Putting this through regression testing right now. If there are no issues with existing demos we'll go without the comp flag.

@kraflab
Copy link
Owner

kraflab commented Sep 27, 2024

No desyncs, just taking this then without the flag: c892178
@rfomin @fabiangreffrath

@kraflab kraflab closed this Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants