#3815 closed defect (fixed)
FW: broken compatibility with 0.11.1 saves
| Reported by: | SF/glorifindel | Owned by: | SF/buddha_ |
|---|---|---|---|
| Priority: | blocker | Component: | Engine: Cine |
| Version: | Keywords: | ||
| Cc: | Game: | Future Wars |
Description
The current SVN build of ScummVM exits if I try to load a Future Wars savegame made with version 0.11.1 of ScummVM; the console displays this warning:
File::open: No filename was specified!
If I create a fresh save it works. I attach both a set of new and old saves.
Ticket imported from: #2019355. Ticket imported from: bugs/3815.
Attachments (11)
Change History (36)
by , 16 years ago
| Attachment: | FW old saves.zip added |
|---|
comment:2 by , 16 years ago
Kari, now you really have data to experiment with :) This is a release critical.
comment:3 by , 16 years ago
| Owner: | set to |
|---|---|
| Priority: | normal → blocker |
comment:4 by , 16 years ago
Alright, I found a critical change between the 0.11.1 release version's Cine engine's savegame saving code and the current trunk's saving code.
//---------------------------------------------- - Here's the part for saving a animDataTable entry in 0.11.1:
fHandle->writeUint16BE(animDataTable[i].width); fHandle->writeUint16BE(animDataTable[i].var1); fHandle->writeUint16BE(animDataTable[i].bpp); fHandle->writeUint16BE(animDataTable[i].height); fHandle->writeSint16BE(animDataTable[i].fileIdx); fHandle->writeSint16BE(animDataTable[i].frameIdx); fHandle->write(animDataTable[i].name, 10); // Horrifyingly, cinE used to dump the entire struct to the // save file, including the data pointers. While these pointers // would be invalid after loading, the loadResourcesFromSave() // function would still test if ptr1 was non-NULL, presumably // to see if the object was present in the room. fHandle->writeByte(animDataTable[i].ptr1 ? 1 : 0); //---------------------------------------------- - And here's the corresponding code from the current trunk:
fHandle.writeUint16BE(_width); fHandle.writeUint16BE(_var1); fHandle.writeUint16BE(_bpp); fHandle.writeUint16BE(_height); fHandle.writeUint32BE(_data != NULL); // _data fHandle.writeUint32BE(_mask != NULL); // _mask fHandle.writeUint16BE(_fileIdx); fHandle.writeUint16BE(_frameIdx); fHandle.write(_name, sizeof(_name)); // 10 bytes //----------------------------------------------
So there it is, they simply output different stuff. 0.11.1 outputs 23 bytes per animDataTable entry. Current trunk outputs 30 bytes per animDataTable entry. Yay! :) Let's see what I can do about it...
comment:5 by , 16 years ago
Here's some info I dug up from the SVN logs:
- In revision 21772 (Before official support for the Cine engine): http://scummvm.svn.sourceforge.net/viewvc/scummvm?view=rev&revision=21772 the saved bytes per animDataTable entry first started to be 23.
- At revision 27495 (Release 0.11.0) official support for Future Wars was introduced: http://scummvm.svn.sourceforge.net/viewvc/scummvm/scummvm/tags/release-0-10-0/engines/cine/various.cpp?view=markup
- Revision 31444 (Patch #1913862: CinE Script system) changed the saving code: http://scummvm.svn.sourceforge.net/viewvc/scummvm?view=rev&revision=31444 That's where the saved bytes per animDataTable entry changed from 23 to 30. Here's a link to the animDataTable entry's saving code in revision 31444: http://scummvm.svn.sourceforge.net/viewvc/scummvm/scummvm/trunk/engines/cine/anim.cpp?view=markup&pathrev=31444#l_376 It saves the data and mask pointers into the savegame file.
- Revision 31453 changed the data and mask pointers to be saved as zeroes: http://scummvm.svn.sourceforge.net/viewvc/scummvm/scummvm/trunk/engines/cine/anim.cpp?r1=31453&r2=31452&pathrev=31453
- Revision 32073 changed the data and mask pointers to be saved as booleans (i.e. 0 was saved if the pointer was NULL, 1 if it wasn't): http://scummvm.svn.sourceforge.net/viewvc/scummvm/scummvm/trunk/engines/cine/anim.cpp?r1=31701&r2=32073
So here's a recap of the effects affecting versions starting from 0.11.0: - Revisions 21772-31443: 23 bytes per animDataTable entry in the savegames. - Revisions 31444-31452: 30 bytes per animDataTable entry in the savegames and pointers saved as they are. - Revisions 31453-32072: 30 bytes per animDataTable entry in the savegames and pointers saved as zeroes. Loading of new savegames was broken in this revision range. - Revisions 32073 to current (33084 is current at the moment of writing): 30 bytes per animDataTable entry in the savegames and pointers saved as booleans.
So there are two different major versions of the savegames plus a third one which is minor (A broken version of the latter major version).
Looks like there's some code already in place (The brokenSave function) for testing whether the savegame format is of the older or the newer major format: http://scummvm.svn.sourceforge.net/viewvc/scummvm/scummvm/trunk/engines/cine/various.cpp?revision=33068&view=markup#l_302 But it's not used at all - it always returns false and there's a comment: // Backward seeking not supported in compressed savefiles // if you really want it, finish it yourself
I'll look into using the brokenSave function, maybe unpacking the savegame first into memory or something like that.
by , 16 years ago
| Attachment: | partial_fix_for_bug_2019355-v1.patch added |
|---|
Partial fix for the bug (1st version). Uses Unix style newlines. At least fw-amiga-it.1 of the old savegame set still bugs.
comment:6 by , 16 years ago
I haven't yet been able to get all the provided old savegames to work correctly (If I load the old savegame fw-amiga-it.1 the hero is invisible and only appears at times when walking around). But all the other savegames provided in this bug report this far work now.
I've attached a patch for a partial fix to the bug, it doesn't yet fix the bug totally because at least the old savegame fw-amiga-it.1 does not work correctly with it. But I thought it might be good to let others see the patch too for possible peer review. File Added: partial_fix_for_bug_2019355-v1.patch
by , 16 years ago
| Attachment: | animDataTable_dump_with_0-11-0-old_fw-amiga-it.0.txt added |
|---|
comment:7 by , 16 years ago
Here are some animDataTable dumps I made with branch-0-11-0 revision 31347 + debug print code and current SVN revision 33117 + debug print code + partial bugfix for this bug.
The AnimData entries were dumped right at the end of makeLoad-function which loads the savegame, just before returning from the function.
I didn't include the data and mask pointers in the debug prints so using diff between the files would be more revealing. And I also made the refresh variable to be calculated from the data pointer in the SVN version (i.e. refresh is equal to _data != NULL).
The *old_fw-amiga-it.0.txt are the seemingly working old savegame dumps.
The *old_fw-amiga-it.1.txt are the dumps of the old savegame that doesn't yet work correctly. File Added: animDataTable_dump_with_0-11-0-old_fw-amiga-it.0.txt
by , 16 years ago
| Attachment: | animDataTable_dump_with_0-11-0-old_fw-amiga-it.1.txt added |
|---|
by , 16 years ago
| Attachment: | animDataTable_dump_with_SVN-old_fw-amiga-it.0.txt added |
|---|
by , 16 years ago
| Attachment: | animDataTable_dump_with_SVN-old_fw-amiga-it.1.txt added |
|---|
comment:11 by , 16 years ago
Some info about the animDataTable dumps:
The entries with refresh == false in both the 0.11.0 and SVN dumps can be safely ignored AFAIK.
But if refresh == true in either or both of the 0.11.0 and SVN dumps then differences should matter. The entries with refresh == true in both dumps but with fileIdx = -1 in one and fileIdx >= 0 in the other seem strange to me as the fileIdx is used for finding the data that's to be loaded from an archive file. So if fileIdx is -1 then the entry shouldn't be loaded at all AFAIK.
by , 16 years ago
| Attachment: | animDataTable_dump_with_SVN-old_fw-amiga-it.1-updated_structs.txt added |
|---|
comment:12 by , 16 years ago
As wjp pointed out on #scummvm (Thanks btw) the AnimData entries' variables weren't being updated with the current SVN each and every time an animDataTable entry was loaded, although in 0.11.0 and 0.11.1 they were.
So after modifying the SVN version locally to do that here are some dumps with the animDataTable entries updated each and every time. These dumps should be more revealing when used with diff against the 0.11.0 dumps.
Adding the updating of animDataTable entries' variables each and every time an entry is loaded didn't fix this bug yet though, the hero is still invisible with the old fw-amiga-it.1 save... File Added: animDataTable_dump_with_SVN-old_fw-amiga-it.1-updated_structs.txt
by , 16 years ago
| Attachment: | animDataTable_dump_with_SVN-old_fw-amiga-it.0-updated_structs.txt added |
|---|
comment:13 by , 16 years ago
File Added: animDataTable_dump_with_SVN-old_fw-amiga-it.0-updated_structs.txt
comment:14 by , 16 years ago
Some info about the hero's object (Coordinates etc):
objectTable[1] seems to be the hero in all the given saves, so e.g. objectTable[1].x and objectTable[1].y give the coordinates of the hero.
objectTable[1] begins at offset 0x0083 in a savegame file. objectTable[1].x is a big endian 16-bit value at 0x0083-0x0084. objectTable[1].y is a big endian 16-bit value at 0x0085-0x0086.
The hero's object info is identical between the old and new set's fw-amiga-it.1 saves excluding the x-coordinate which is 0x9f (159) in the new save and 0xAD (173) in the old save. Both have the hero at vertical position 0x63 (99).
So the hero is at (159, 99) in the new save (fw-amiga-it.1), but at (173, 99) in the old save (fw-amiga-it.1).
Nothing remarkable there, the hero's just a bit to the right in the old save. So at least the object structs don't seem to be the source of the problem at all...
comment:15 by , 16 years ago
I found at least a way to consistently make the hero appear and disappear after loading the still bugging savegame (fw-amiga-it.1 of the old savegame set).
The hero appears in the fw-amiga-it.1 of the old savegame set when the hero's frame is 0 (i.e. objectTable[1].frame == 0), otherwise he remains invisible. The frame tells which frame of the animation is currently being shown. Frame 0 shows the hero stationary facing to the right.
By walking to the right in extremely small steps (By clicking slightly to the right of the hero's position) you can sometimes get the hero's frame to become 0 and see him.
comment:16 by , 16 years ago
Forcing file index to 67 for frames 1-33 when loading those resources in loadResourcesFromSave makes the hero visible in the still bugging savegame. But it also made 7 copies of the hero around the screen that don't move around but seemingly perpetually walk in place :). This is at least something! :D
comment:17 by , 16 years ago
[copy-paste from IRC:]
20:46 <@wjp> Buddha^: I see a potential problem. In r31443 there is a for (i = 0; i < animHeader.numFrames; i++) that disappeared in 31444
20:48 <@wjp> so it might that if animDataTable[0] is a multi-frame animation, the rest of the frames is skipped somehow?
20:49 <@wjp> (OTOH, it doesn't seem to break other savegames, so maybe this is not it...)
20:59 <@wjp> Buddha^: actually that might explain why the fileIdx changed from -1 in 0.11 to >0 in SVN. The code inside that loop can change the animTable for upcoming entries, while the current SVN code reads fresh data from the savegame at that point
21:07 <@wjp> aha
21:07 <@wjp> that actually also explains the different between .0 and .1: you get savegames that look like the second one if you load a savegame and then save
21:07 <@wjp> if you save after starting a new game, you get one like .0
21:08 <@wjp> (where by 'like' I mean how the first bunch of entries in animDataTable[] looks)
comment:18 by , 16 years ago
Actually it was just my incorrect code that made the 7 copies of the hero appear. When loading the animDataTable entries I forced fileIdx to 67 for entries that had the frame variable in range 1-33. That's when the 7 copies of the hero appeared.
But then I changed the code so that when loading the animDataTable entries I just forced fileIdx to 67 for animDataTable entries number 0-33 (i.e. animDataTable[index] where 0 <= index <= 33). And that worked, now the fw-amiga-it.1 of the old savegame set shows the hero and one can walk around without blinking in and out of visibility.
comment:19 by , 16 years ago
Some info about the hero's walking animation:
The hero's walking animation is "MARCHE.ANI" from the PART01 archive file and it has 34 frames. It is the first resource being loaded in loadResourcesFromSave. As the animation is multiframe it should be loaded into the animDataTable entries 0-33 but that's not currently happening with the SVN version as multiframe animation loading is broken somehow as wjp noticed.
by , 16 years ago
| Attachment: | fix_for_bug_2019355.patch added |
|---|
Fix for the bug (Hopefully fully working now). Uses Unix style newlines. Appears to work with both the old and new savegame set.
comment:20 by , 16 years ago
Here's a bit modified version of the partial fix patch I submitted earlier. It seems to work with all the savegames attached to this bug report so far.
I'll commit this fix to the trunk and branch-0-12-0 later during Tuesday if I don't hear any objections. File Added: fix_for_bug_2019355.patch
comment:21 by , 16 years ago
That looks good. You do need to reset animHeader.numFrames to 1 before doing "if (foundFileIdx < 0 || !validPtr) { continue; }", though. (Since it may still be higher than 1 from the previous animation at that point.)
by , 16 years ago
| Attachment: | fix_for_bug_2019355-v2.patch added |
|---|
Fix for the bug with the number of frames set to 1 before each loop (Not doing so could've causes problems). Uses Unix style newlines.
comment:22 by , 16 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
comment:23 by , 16 years ago
A good catch that one, thanks. Here's the fix with the animHeader.numFrames set to 1 before handling each animation entry. I'm also committing this to the trunk and to branch-0-12-0 and closing this bug as fixed. Thanks to wjp for all his help with this one. File Added: fix_for_bug_2019355-v2.patch
comment:24 by , 16 years ago
Fixed in the trunk in revision 33192. Fixed in the branch-0-12-0 in revision 33194.
comment:25 by , 16 years ago
Had to fix CineSaveGameFormat enumeration's include order which caused problems at least with GCC (For some reason it compiled fine with MSVC8). So added fix for that to the trunk in revision 33196 (Just moved the enum around, nothing more): http://scummvm.svn.sourceforge.net/scummvm/?rev=33196&view=rev And to branch-0-12-0 in revision 33197 (Exactly the same thing here): http://scummvm.svn.sourceforge.net/scummvm/?rev=33197&view=rev Thanks to Kirben for noticing the problem in the first place!
FW - Old saves set