Opened 14 years ago

Closed 14 years ago

Last modified 13 months ago

#8452 closed patch

Scumm Savegame Informations (Feature Request ID: 1217640)

Reported by: lordhoto Owned by: fingolfin
Priority: normal Component: GUI
Keywords: Cc:
Game:

Description

Hi,

here it is a implementation of the feature request
1217640 (UI Extensions). Maybe it is sometime a bit
'overcoded' but it is extendable for the future this
way, also it is not breaking backward compatiblity.
Hope it is working for all of you, I didn't have any
problem with it. But maybe it has some bugs left.

Ticket imported from: #1259034. Ticket imported from: patches/557.

Attachments (7)

scumm_info_v1.patch (9.8 KB ) - added by lordhoto 14 years ago.
Patch V1 against todays anon CVS
scumm_info_v2.patch (10.0 KB ) - added by lordhoto 14 years ago.
Patch V2 against todays anon CVS
scumm_info_v3.patch (18.0 KB ) - added by lordhoto 14 years ago.
Patch V3 against todays anon CVS
scumm_info_v4.patch (18.2 KB ) - added by lordhoto 14 years ago.
Patch V4 against todays anon CVS
scumm_info_v5.patch (10.3 KB ) - added by lordhoto 14 years ago.
Patch V5 against todays anon CVS
scumm_info_v6.patch (10.4 KB ) - added by lordhoto 14 years ago.
Patch V6 against todays anon CVS
scumm_info_v7.patch (10.7 KB ) - added by lordhoto 14 years ago.
Patch V7 against anon CVS from yesterday

Download all attachments as: .zip

Change History (25)

by lordhoto, 14 years ago

Attachment: scumm_info_v1.patch added

Patch V1 against todays anon CVS

comment:1 by lordhoto, 14 years ago

sry forgett to mention that "- possibility to delete
savegames from within the save/load dialog" is not
implemented in this patch.

comment:2 by sev-, 14 years ago

Code looks ok, but there are 2 places where it gives wrong time.

in loadInfos() you should add _dialogStartTime =
_system->getMillis() / 1000; I.e. that amount of time
between dialog launch and actual game load will not be
substracted from running time.

And in saveInfos() you should use
header.playtime = _dialogStartTime - _engineStartTime;

so again, time while user crawls in dialogs will not be
counted as gameplay.

by lordhoto, 14 years ago

Attachment: scumm_info_v2.patch added

Patch V2 against todays anon CVS

comment:3 by lordhoto, 14 years ago

> in loadInfos() you should add _dialogStartTime =
> _system->getMillis() / 1000; I.e. that amount of time
> between dialog launch and actual game load will not be
> substracted from running time.

the loading function is not used while the dialog and the
time between the dialog and the actual game load shouldn't
be a real big value (i think about 1 or 2 secs at maximum).

> And in saveInfos() you should use
> header.playtime = _dialogStartTime - _engineStartTime;

> so again, time while user crawls in dialogs will not be
> counted as gameplay.

since it is not used while the dialog this is not needed.

I added a patch which reduces time difference while loading,
if the loading would have taken a long time the game time
would be counted else.

comment:4 by lordhoto, 14 years ago

ability to remove saved games is added in this patch. it
works with 1x overlay mode and with 'normal' overlay.

by lordhoto, 14 years ago

Attachment: scumm_info_v3.patch added

Patch V3 against todays anon CVS

comment:5 by sev-, 14 years ago

I'd suggest not to show Remove button in save mode at all,
since it does not make sence and clicking on game save edits
the title, not just selects it.

comment:6 by lordhoto, 14 years ago

k That is done now in the new patch, any other ideas?

by lordhoto, 14 years ago

Attachment: scumm_info_v4.patch added

Patch V4 against todays anon CVS

comment:7 by sev-, 14 years ago

Owner: set to fingolfin

comment:8 by sev-, 14 years ago

No, I think now we need to hear from Fingolfin :).

comment:9 by fingolfin, 14 years ago

I see a problem with the remove code, which isn't portable. Maybe it would
be wise to factor it out and move it to a seperate patch which builds upon
this one (after all the primary purpose of this patch isn't to allow users to
remove save games).

Removing files needs to be properly factored out, either into the file
system node class, or some other suitable location in backends/fs/fs.h.

by lordhoto, 14 years ago

Attachment: scumm_info_v5.patch added

Patch V5 against todays anon CVS

comment:10 by lordhoto, 14 years ago

k. I deleted all remove things for now and later I will make
another patch for that.

comment:11 by fingolfin, 14 years ago

Thanks. And don't get me wrong, a "remove" feature makes
sense, we just shouldn't this patch get delayed because of
discussion about that other feature :-).

The new patch replaces some comment text in saveload.cpp
("We account for that by retrying once with swapped byte
order") by garbage. No need to upload a new patch for this,
you just might want to fix this in your local copy.

I would recommend turning the SaveInfoHeader.version into at
least an uint16. Or better just another uint32.

The SaveInfoHeader.size file is totally useless if you don't
use it, as you currently do! What really should happen is
that loadInfos checks it; if the header is too short, it
should abort reading. If it is longer, it should ignore the
extra bytes, but properly skip over them.

"Infoheader version is too high" is a not a good
warning/error message. Alas, we can ignore that problem for
now -- the whole save/load system currently is pretty bad
regarding user feedback, we should look into this seperatly
at some point.

Also, if the info header version is too high, well --- fine,
but no reason to abort loading the game! Just use the "size"
field to skip over the rest of the header (see above).

Using localtime is probably not a good idea -- time zone
issues... better to use gmtime or so, and then convert it
for display to the local time zone (i.e. all internal times
should be stored in GMT/UTC, while the display should occur
with the local time zone).

I haven't yet tried your patch, actually, I'll do that later
today when I am back home.

comment:12 by lordhoto, 14 years ago

new version, should be allright now, but i think we should
use a uint64 for saving of the time_t value, but that could
be corrected later...

by lordhoto, 14 years ago

Attachment: scumm_info_v6.patch added

Patch V6 against todays anon CVS

comment:13 by fingolfin, 14 years ago

Sorry for being silent so long again, I am quite busy in real life :-/.

For saving a time, you can either use seconds since a certain starting
date (e.g. 1. Jan 1970), or you can store it in BCD: year, month, day,
hour, minute. The former is what time() gives you, but it suffers from
possible overflows (which is why one may want to use 64bit ints). The
latter doesn't suffer from this problem, but is less compact and allows
for invalid entries.

I am a bit reluctant to add a uint64 just for this purpose. Besides a
suitable datatype isn't available everywhere. Code for reading/writing
64bit values would have to be added. And converting that 64 bit value to
be usable by the time/gmtime/etc. functions (in a portable fashion) may
be tricky, too.

Considering that 32bit gives us a time range of about 136 years, I think
it's sufficient we store the date using 32bits. Of course we'll run into a
"year 2106" problem this way; but I think we should rewrite the
savegame format till then anyway ;-).

comment:14 by fingolfin, 14 years ago

if (section.version > INFOSECTION_VERSION) {
file->skip(section.size - sizeof(SaveInfoSection));
}

This isn't quite as I would do it. It performs no sanity
check on section.size; if a bad savegame has e.g. a 0 value
here, you do a negative skip...

So, I'd do this: Do not check the header version, instead
check if section.size > sizeof(SaveInfoSection), and if that
is the case, do the skip. Also, you might want to add a
check in kind of "if version == 1 and size !=
sizeof(SaveInfoSection) then something is wrong, deal with
it appropriately..."

by lordhoto, 14 years ago

Attachment: scumm_info_v7.patch added

Patch V7 against anon CVS from yesterday

comment:15 by lordhoto, 14 years ago

so here it is. it implements the descriptet things and
removes the comments on a uint64, which was meanded for
LATER as i said, i thought you will be against it :). Since
anon cvs does not work for me it is against anon cvs from
yesterday.

comment:16 by fingolfin, 14 years ago

Status: newclosed

comment:17 by fingolfin, 14 years ago

This looks pretty good now.

One thing I am wondering about: Why does InfoStuff use a
(not documented :-/) 'stuffed' format for the date and time?
Why not use explicit fields for day, month, year, etc., or
even *gasp* a tm struct ? :-) This seems to serve no purpose
except making the code a little bit more complicated... ?
(Granted, a small detail).

Since this doesn't affect the save game format, it can be
changed later, if desired, so I'll commit this patch now.

Thanks for your efforts!

comment:18 by digitall, 13 months ago

Component: GUI
Note: See TracTickets for help on using tickets.