#8986 closed patch
SCI: Rewrote saveload system
| Reported by: | fingolfin | Owned by: | fingolfin |
|---|---|---|---|
| Priority: | normal | Component: | Engine: SCI |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
(The SF.net tracker just made me loose the long elaborate text I wrote here originally. I am too tired and angry to rewrite it, so I'll be brief now).
The attached patch is a rewrite of the SCI saveload system. With it, we don't need CFSML anymore, so less voodoo, code is easier to modify, and more portable. The saves are smaller, too.
This is a big change, so it would be nice if people could run this through some heavy tests and look for regressions. I only tested it a bit with SQ1 and SQ3.
The new format is binary, and completely incompatible with the old one. Moreover, I plan to change the format at least once more, by adding some more data to the header. See also the FIXME and TODO comments.
Ticket imported from: #2687400. Ticket imported from: patches/1091.
Attachments (2)
Change History (12)
by , 16 years ago
| Attachment: | sci-save.patch.bz2 added |
|---|
comment:1 by , 16 years ago
comment:2 by , 16 years ago
I don't claim to understand the reg_t/string casts yet, but engine/stringfrag.cpp is the file to look at, I think.
comment:3 by , 16 years ago
Looks very good. Two notes:
When loading, sync_SegManager() doesn't reallocate memory for SegManager::heap when the heap_size in the savegame doesn't match the preallocated heap_size. (This crashes loading in KQ1sci.)
For the reg_t/char* things, the amount of memory allocated when loading doesn't match max_size anymore. I suspect this could break things when the string fragments will actually be used. However, I'm not sure what the relation with max_size should be... it's used differently in various places in engine/game.cpp (But from what I understood from Lars' comments, the string fragments code he committed so far is work in progress that isn't being fully used yet. I'm therefore not going to make assumptions about the exact purpose yet.)
by , 16 years ago
| Attachment: | sci-save-2.patch added |
|---|
Reallocate SegManager heap (apply on top of sci-save.patch)
comment:5 by , 16 years ago
thebluegr, it's not a reg_t that is stored as a string, it is an reg_t *array*, and being stored as a string here essentially means the same as being stored as a sequence of bytes -- the only diference is the way the length is computed.
That said, it's still a gross thing and fundamentally wrong, but the problem is, I have no idea what the "right" thing would be, as this "string fragments" code is quite weird. See also Willem's remarks.
@Willem: Good catch on the heap reallocation, thanks, will add this.
Some further notes: The stringfrag code is not at all endian safe. Saving/loading a reg_t* as a string is evil. The whole stringfrag code seems... evil. I wonder if there is any explanation on it somewhere... guess I'll have to dig into it (as Lars didn't reply to my email from 2 days ago yet :/).
comment:6 by , 16 years ago
Oh and did anybody perform any actual testing, besides the theoretical review? :)
comment:7 by , 16 years ago
Yes, I tested repeatedly loading and saving in KQ1, LSL3 and Iceman. No problems detected (with both patches applied).
By the way, this is my interpretation of the string fragment code: (I know I said I wouldn't speculate, but I couldn't resist...) The file engine/stringfrag.cpp has code to store strings in the offset fields of an array of reg_t's in a portable manner. (With well-defined semantics in which order things are stored.) The current usage in the rest of the code however simply dumps the string in the reg_t array. My guess is that this is a stopgap measure while Lars is rewriting the code to make use of engine/stringfrag.cpp.
comment:8 by , 16 years ago
Yeah, that is more or less what I think, too. But since we'll likely break backward compatibility a few more times before the dust settles, I'll wait for Lars to clarify.
So I have no commit this change, with some minor tweaks. Thanks for the feedback, guys!
comment:9 by , 16 years ago
| Owner: | set to |
|---|---|
| Status: | new → closed |
comment:10 by , 6 years ago
| Component: | → Engine: SCI |
|---|
The new code looks great :) No more CFSML! And it's good that you moved all the common bits to /common, so that there is less code duplication (e.g. with tinsel), and other engines may benefit from this code too in the future.
I also agree that there should be a version numbering scheme, as the save format will most likely change again.
I'm not sure why a reg_t is saved as a string. The old code did it, but this patch breaks compatibility anyway so... what's the point of this bit?
Other than that, it looks good :) Savegame compatibility is not an issue at this point, so it's a good time to break it now...