[Issue Report]: Devilution crashes and closes silently when trying to load a Diablo 1 save into Hellfire

Operating System

Windows x64

DevilutionX version

1.5.2

Describe

(Windows 10) I played Diablo 1, that I bought off Battle Net using Devilution. The game was started using their downloaded version in compatibility mode, but it got stuck at a point and just kept crashing so I switched over. My last save is right before the battle with Diablo. It didn't automatically pick up my saved character and game when I switched to Hellfire, so I copied the single_0.sv file and renamed it single_0.hsv. Now when I load Hellfire I can see my character and the saved game. If I select save game it the progress bar goes to about a 1/4 to 1/3 of the way and then I'm looking at Windows Explorer. If I start a new game with the same character it loads with my character completely intact - gear, gold, etc. (Although if gold was in the stash it didn't save that.) And if I look at the event view I see the attached.

Here is the XML version of the details from the event view
xml-error-details.txt
I don't even see an error log created by the application.
Here is my Diablo save file - I just added txt to the end so I could uploaded. The Hellfire version was just a copy renamed as mentioned before.
single_0 - Copy.sv.txt

To Reproduce

As previously noted. I copy my save file and renamed it from .sv to .hsv. I went to the start up screen set the game to the Hellfire start. Went into Hellfire. Used my saved character "Max" and then clicked Saved Game. Deviloution crashed.

Expected Behavior

My expectation based upon everything I read that the save would load and the world would have the expanded Hellfire content.

Additional context

If you want exact screenshots I can take them.

You probably shouldn't load games switching between Diablo and Hellfire. Make a new game.

I tested the save file, crashed for me on load game, but new game worked fine, as far as I can tell. My understanding is the character transfers fine, loading a game saved in the other version isn't supported.

Works fine on master, fixed by 0d19c1b

it's not feasable to backport that, but I can try and se if it can be easily patched for the next patch release.

Note: I did think loading a saved game was possible. Starting a new one with the same character I know I can do.

If it can be done, great, if not I get it if the legacy behavior was to not support that behavior.

To that point, if it's not possible, my recommendation at that point would be:

a) Show a message saying you can't load this saved game when someone tries
b) or don't offer the option when the character is being imported from Diablo so you can't get to the point I got to.

c) Gracefully crash without closing out the whole game. Catch the exception, whatever it is, and say error loading save file or something like that and return them to the last menu.

That's my 2 cents :)

I could be wrong - this came up in Discord and but I didn't see anyone say it was definitely intended to be able to load a single player save from one version to another.

Rather than not being able to load the game, we probably are loading the game just fine and ~~then failing to render some asset~~. It's like tricking the game into thinking the save is valid. I think it would be difficult to fail early with an error message. Or, at the very least, more difficult than simply catching an exception.

EDIT: I just saw Qndel's screenshot. I guess the issue occurs during level conversion, not during rendering. But it happens when saving the converted level, which means the level loaded just fine. I guess the issue is we don't call GetLevelMTypes() so it's unable to load the monster type from LevelMonsterTypes.

I could be wrong - this came up in Discord and but I didn't see anyone say it was definitely intended to be able to load a single player save from one version to another.

Maybe people didn't convert to hellfire that often or maybe they saved in dungeon or something, this started to happen in 1.5.0 after dereferencing null pointer got added and glebm's PR fixed it - we are still reading incorrect data (for zombie instead of golem) but at least it doesn't crash ;)

The following fields in SaveMonster() access data from the MonsterData array which can't be referenced without loading data into LevelMonsterTypes.

devilutionX/Source/loadsave.cpp

Line 1449 in 039a6c9

    
           file->WriteLE<int8_t>(static_cast<int8_t>(monster.level(sgGameInitInfo.nDifficulty)));

devilutionX/Source/loadsave.cpp

Line 1451 in 039a6c9

    
           file->WriteLE<uint16_t>(static_cast<uint16_t>(std::min<unsigned>(std::numeric_limits<uint16_t>::max(), monster.exp(sgGameInitInfo.nDifficulty))));

devilutionX/Source/loadsave.cpp

Line 1456 in 039a6c9

    
           file->WriteLE<uint8_t>(static_cast<uint8_t>(std::min<uint16_t>(monster.toHitSpecial(sgGameInitInfo.nDifficulty), std::numeric_limits<uint8_t>::max()))); // For backwards compatibility

Nowadays, each of these fields is being skipped during LoadMonster() in favor of computing them on the fly.

devilutionX/Source/loadsave.cpp

Line 642 in 039a6c9

file->Skip(1); // Skip level - now calculated on the fly

devilutionX/Source/loadsave.cpp

Line 644 in 039a6c9

file->Skip(2); // Skip exp - now calculated from monstdat when the monster dies

devilutionX/Source/loadsave.cpp

Line 652 in 039a6c9

file->Skip(1); // Skip toHitSpecial as it's already initialized

It would be pretty convenient if we could just copy these fields over from the loaded level to the converted level rather than trying to compute them. Otherwise, we'd have to do something like this for normal dungeon levels...

InitLevelMonsters();
SetRndSeed(glSeedTbl[currlevel]);
GetLevelMTypes();

And for setlevels, maybe something like this...

InitLevelMonsters();
LoadSetMap();
GetLevelMTypes();

maybe just load them and add a comment that it's only needed for conversion // load default for use with save game conversion or something like that.