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

[qndel]

Works fine on master, fixed by 0d19c1b

[StephenCWills]

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.

[qndel]

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

[StephenCWills]

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

[DakkJaniels]

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.

[AJenbo]

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

[mildar7]

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 :)

[DakkJaniels]

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.

[AJenbo]

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.