save/load bug in map02 AD_MORTEM.wad

[rfomin]

WAD: [doomworld]
How to reproduce: at the beginning of map02 grab the Rocket Launcher which activates Cacodemos, save the game. Loading this save crashes the port.
Same bug in DSDA-Doom 0.21.3 and the latest build of Woof.
I think it's related to Pushers. @kraflab

[fabiangreffrath]

Could this probably be fixed by properly saving mobj pointer indices for pushers as is already done for other thinkers?

woof/Source/p_saveg.c

Line 2079 in c9455a7

// killough 2/14/98: convert pointers into indices.

[rfomin]

It restores source mobj with P_GetPushThing:

woof/Source/p_saveg.c

Line 2641 in c9455a7

pusher->source = P_GetPushThing(pusher->affectee);

[kraflab]

Is this bug also in prboom+? If it is just pushers and not mbf21-specific, we should probably patch it there as well.

[rfomin]

Could this probably be fixed by properly saving mobj pointer indices for pushers as is already done for other thinkers?

This approach seems to be better

[rfomin]

Is this bug also in prboom+?

Probably, I haven't tested.

[rfomin]

AD_MORTEM.wad has been updated, the bug is no longer reproducible: [doomworld]
They seem to have removed the pushers.

[fabiangreffrath]

Okay, we don't need an over-engineered fix for a mapping error then. It would still be useful, though, to report a warning if P_GetPushThing() returns NULL. Something like "Pusher thinker without pusher object" or similar.

[fabiangreffrath]

Shouldn't this rather mean continue? Else this function will return NULL if the first thing in the sector's thing list is neither a pusher nor a puller.

woof/Source/p_spec.c

Lines 3238 to 3239 in c9455a7

	default:
	break;

[rfomin]

Shouldn't this rather mean continue? Else this function will return NULL if the first thing in the sector's thing list is neither a pusher nor a puller.

woof/Source/p_spec.c

Lines 3238 to 3239 in c9455a7

	default:
	break;

AD_MORTENv2.wad map02 crashes on load with this change.

So, are we call I_Error if P_GetPushThing() returns NULL when loading the save?

[fabiangreffrath]

I guess we can do better than erroring out. Could you please upload the crashing WAD here, now that it has been replaced in the DW thread?

[rfomin]

AD_MORTEMv2.zip

[fabiangreffrath]

I still don't quite get it. When spawning pushers, all sectors with the same tag as the corresponding special linedef are scanned for pusher things. And only if such a think is found, a thinker is spawned and the sector which holds the thing is stored in its affectee field. So, why can't we find the pusher thing again in the very same sector when restoring from a savegame?

woof/Source/p_spec.c

Line 3273 in 4a4917c

if (thing) // No MT_P* means no effect

Is it possible for pusher objects to move to a different sector? Or is it possible to remove them off the map or crush them, without their connected thinker getting removed?

[fabiangreffrath]

--- a/Source/p_saveg.c
+++ b/Source/p_saveg.c
@@ -1792,7 +1792,7 @@ static void saveg_write_pusher_t(pusher_t *str)
     saveg_write32(str->y);

     // int affectee;
-    saveg_write32(str->affectee);
+    saveg_write32(str->source ? str->source->subsector->sector - sectors : str->affectee);
 }

 //

[rfomin]

--- a/Source/p_saveg.c
+++ b/Source/p_saveg.c
@@ -1792,7 +1792,7 @@ static void saveg_write_pusher_t(pusher_t *str)
     saveg_write32(str->y);

     // int affectee;
-    saveg_write32(str->affectee);
+    saveg_write32(str->source ? str->source->subsector->sector - sectors : str->affectee);
 }

 //

This patch doesn't work for me. Try this save: woofsav0.zip

[fabiangreffrath]

Well, it's in the writer function, so it doesn't fix broken savegames. But newly saved games will be fine - at least as far as my testing went.

[rfomin]

That is a new save. If you save right in the moment of activation, pushers don't work after load.

[fabiangreffrath]

But isn't this rather because pushers got moved into a different sector that doesn't have PUSH_MASK set?

[rfomin]

I don't know, but my PR works for saves like that.

[fabiangreffrath]

With your PR, savegames saved with an old version and loaded with a patched one will still crash, right? So, we will still need a check if pusher->source == NULL before calling P_AddThinker() and error out in that case - indeed.

[rfomin]

Eh, you're probably right about overengineering. This is an unconventional use of pushers, so maybe we just erroring out?

[fabiangreffrath]

Another idea: If P_IndexToThinker() is unable to translate str->source into a proper thinker in saveg_read_pusher_t(), then that's probably because it hasn't been converted into an index before but is still a saved pointer address, i.e. it has been saved with an old version of the engine. In that case, P_IndexToThinker() returns NULL and overrides that field, so we know just as much as before. Then, we just need to check if pusher->source == NULL in P_UnArchiveSpecials() and only call P_GetPushThing() in that case. This way, we can get away without bumping savegame version but can just "sneek" this improvement in.

[fabiangreffrath]

Is it possible for pusher objects to move to a different sector?

Apparently, this is before and after collecting the rocket launcher:

[rfomin]

Another idea: ...

I have implemented this in e580f49