Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

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

FW old saves.zip (7.1 KB ) - added by SF/glorifindel 11 years ago.
FW - Old saves set
FW new saves.zip (4.6 KB ) - added by SF/glorifindel 11 years ago.
FW - New saves set
partial_fix_for_bug_2019355-v1.patch (19.9 KB ) - added by SF/buddha_ 11 years ago.
Partial fix for the bug (1st version). Uses Unix style newlines. At least fw-amiga-it.1 of the old savegame set still bugs.
animDataTable_dump_with_0-11-0-old_fw-amiga-it.0.txt (57.9 KB ) - added by SF/buddha_ 11 years ago.
animDataTable_dump_with_0-11-0-old_fw-amiga-it.1.txt (58.4 KB ) - added by SF/buddha_ 11 years ago.
animDataTable_dump_with_SVN-old_fw-amiga-it.0.txt (57.6 KB ) - added by SF/buddha_ 11 years ago.
animDataTable_dump_with_SVN-old_fw-amiga-it.1.txt (57.8 KB ) - added by SF/buddha_ 11 years ago.
animDataTable_dump_with_SVN-old_fw-amiga-it.1-updated_structs.txt (58.4 KB ) - added by SF/buddha_ 11 years ago.
animDataTable_dump_with_SVN-old_fw-amiga-it.0-updated_structs.txt (57.9 KB ) - added by SF/buddha_ 11 years ago.
fix_for_bug_2019355.patch (23.2 KB ) - added by SF/buddha_ 11 years ago.
Fix for the bug (Hopefully fully working now). Uses Unix style newlines. Appears to work with both the old and new savegame set.
fix_for_bug_2019355-v2.patch (23.3 KB ) - added by SF/buddha_ 11 years ago.
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.

Download all attachments as: .zip

Change History (36)

by SF/glorifindel, 11 years ago

Attachment: FW old saves.zip added

FW - Old saves set

by SF/glorifindel, 11 years ago

Attachment: FW new saves.zip added

FW - New saves set

comment:1 by SF/glorifindel, 11 years ago

File Added: FW new saves.zip

comment:2 by sev-, 11 years ago

Kari, now you really have data to experiment with :) This is a release critical.

comment:3 by sev-, 11 years ago

Owner: set to SF/buddha_
Priority: normalblocker

comment:4 by SF/buddha_, 11 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 SF/buddha_, 11 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 SF/buddha_, 11 years ago

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 SF/buddha_, 11 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

comment:7 by SF/buddha_, 11 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

comment:8 by SF/buddha_, 11 years ago

File Added: animDataTable_dump_with_0-11-0-old_fw-amiga-it.1.txt

comment:9 by SF/buddha_, 11 years ago

File Added: animDataTable_dump_with_SVN-old_fw-amiga-it.0.txt

comment:10 by SF/buddha_, 11 years ago

File Added: animDataTable_dump_with_SVN-old_fw-amiga-it.1.txt

comment:11 by SF/buddha_, 11 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.

comment:12 by SF/buddha_, 11 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

comment:13 by SF/buddha_, 11 years ago

File Added: animDataTable_dump_with_SVN-old_fw-amiga-it.0-updated_structs.txt

comment:14 by SF/buddha_, 11 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 SF/buddha_, 11 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 SF/buddha_, 11 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 wjp, 11 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 SF/buddha_, 11 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 SF/buddha_, 11 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 SF/buddha_, 11 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 SF/buddha_, 11 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 wjp, 11 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 SF/buddha_, 11 years ago

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 SF/buddha_, 11 years ago

Resolution: fixed
Status: newclosed

comment:23 by SF/buddha_, 11 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 SF/buddha_, 11 years ago

Fixed in the trunk in revision 33192.
Fixed in the branch-0-12-0 in revision 33194.

comment:25 by SF/buddha_, 11 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!

Note: See TracTickets for help on using tickets.