Devilutionx: Game crashes in level 6 corrupting save

[rsn8887]

This is after the monster save fix using fresh game:
“
ugh... I returned to diablo today and i just think these crashes are haunting me at this point. I think i'm done with trying to complete it on switch lol. So i got to level 5 again, cleaned it, returend to town with a portal and then was able to get back and continue palying (there i thought i was done with crashes). So then i found an exit to town from lvl 5 (or 6, i don't even remember now) so i entered it, saved in town and here we go again... After trying to go into this shortcut back to dungeon game crashes, AND IT ALSO CRASHES when trying to enter the dungeon even through church... So... No way to progress again, huh.
“
single_0.zip

Discussion: https://gbatemp.net/threads/diablo-nx-nintendo-switch-port-of-devilutionx-diablo.542414/page-7#post-8730405

[rsn8887]

Might be related to #194 or so.

[AJenbo]

was this from a clean start in latest nightly, or based of a previous save game from the buggy version?

[pgielda]

I've loaded this save and looked where it crashes and it seems to be hitting all the places with "TODO: change less equal to less", "TODO: may go out of boundary", etc. Unfortunately when I fix one place it hits another. In all those places a "TODO" is present.

[rsn8887]

This was done using the latest nightly but with a save from the buggy version, maybe that’s the problem.
“
i did updated but i didn't start a new game with the new version, i just continued my playthrough from my last save, which was not supposed to be broken, all seemed fine until i opened a shortcut and saved in town...
“

[AJenbo]

I suspect that that would be the issue.

[pgielda]

Actually, since this project is really interesting and I wanted to dig deeper -- I've managed to load the attached save with some modifications to the game.

What I had to fix is:

• add a check for (x+ii) < DMAXX in a loop in gendung.cpp
• add checks for xx >= 0, yy >= 0, xx < DMAXX, yy < DMAXY in DRLG_L2PlaceRndSet
  in drlg_l2.cpp
• change "<=" (less equal) to "<" (less) in two lines in drlg_l2.cpp (there is a BUGFIX comment there suggesting that)
• comment tbuf += sizeof(DWORD) * 3

The first three is obvious that those checks should be there instead of just crashing the game (not that I know if that is because of a faulty save or just a bug).
The last (commenting out tbuf += sizeof(DWORD) * 3 is interesting for two reasons. One is it might make saves incompatible between 32- and 64-bit hosts, unless of course DWORD is specifically defined to be always 4 bytes. In that case I would still just write 4*3 instead of using sizeof * 3...
Also I had a save that was done using new devilutionx (cloned yesterday?) and then loaded it and it crashed on exactly same problem. I had to temporarily comment out this tbuf += sizeof + 3 line, load the save, save it again, uncomment the line and then my save worked fine without modifying the game.
Btw this corruption is easy to detect if additional checks during save game loading were performed...
Are the save games compatible with "original" diablo? If not then maybe adding some CRC checks here and there would make sense to immediately detect such a corruption.

[pgielda]

Now I looked at previous commit and see that this tbuf += sizeof(DWORD) * 3 was added there. So my save (as well as @rsn8887) was created before this commit most probably.
@rsn8887 problem showed up not during load just because the save was made in town.
Nevertheless my first three fixes still are valid most probably, as is my suggestion to add some CRC - now load save will most probably load any corrupted data it is asked to load.

[AJenbo]

DWORD is uint32_t in DevilutionX.
I think most of the BUGFIXes that you mention are fixed here: https://github.com/diasurgical/devilutionX/pull/169
Yes save games are compatible between DevilutionX and the original Diablo.

[pgielda]

Yes, it seems that this "bugfixes" pull request indeed addresses those issues.
What is blocking merging of #169 ? Just testing it?

[AJenbo]

Yeah we need to test that it doesn't change level generation or AI behavior (especially around firewalls in any big way.

[AJenbo]

Could you test your save game with vanilla 1.09?

[AJenbo]

I can confirm that the savegame works upstream, the issues must there for be with the DevilutionX load game code.

[AJenbo]

This issue is now fixed in master, it will be fixed in the switch branch, either on the next rebase or if it is cherry-picked (@rsn8887 maybe you would like to do so?).
https://github.com/diasurgical/devilutionX/commit/b90ef1336d0fa23288768b420fb2cbe5356fbfdd

[AJenbo]

@rsn8887 / @pgielda would one of you be willing to test my effort to rebase the Switch code on the latest master?

You can find the test build here https://1368-143324737-gh.circle-artifacts.com/0/diablo-nx.nro