#8452 closed patch
Scumm Savegame Informations (Feature Request ID: 1217640)
| Reported by: | lordhoto | Owned by: | fingolfin |
|---|---|---|---|
| Priority: | normal | Component: | GUI |
| Version: | 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)
Change History (25)
by , 19 years ago
| Attachment: | scumm_info_v1.patch added |
|---|
comment:1 by , 19 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 , 19 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.
comment:3 by , 19 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 , 19 years ago
ability to remove saved games is added in this patch. it works with 1x overlay mode and with 'normal' overlay.
comment:5 by , 19 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:7 by , 19 years ago
| Owner: | set to |
|---|
comment:9 by , 19 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.
comment:10 by , 19 years ago
k. I deleted all remove things for now and later I will make another patch for that.
comment:11 by , 19 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 , 19 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...
comment:13 by , 19 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 , 19 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..."
comment:15 by , 19 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 , 19 years ago
| Status: | new → closed |
|---|
comment:17 by , 19 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 , 6 years ago
| Component: | → GUI |
|---|
Patch V1 against todays anon CVS