[Scummvm-devel] ATTN Porters: SaveFileManager API Adjustments

[Johannes Schickel]

On 03/20/2016 08:15 PM, Marcus Comstedt wrote:
> No, doing listSavefiles _once_ to get the casing of _all_ saves does 
> not lead to bad performance. It's performing the list operation _for 
> each save_, which is what would need to happen if the functionality is 
> supposed to exist inside openForReading, that leads to bad performance. 

See below.

>
> I included the time for actually reading the contents of the file
> (needed to get the display name of the save for the dialog).
> Technically, this happens already in openForLoading on the Dreamcast,
> but that makes no practical difference.

See below.

>
> No.  If the engine passes the filenames to openForLoading that it got
> from listSavefiles (which is enough for it to work witout specifying
> the case mode), the backend can do an open on the lower level
> directly, so it's
>
> Once:  list all files (listSavefile)
>
> For each save:  list files until a match is found (internally)
> For each save:  read file data from the block location found in the
>                  previous step
>
> versus (case insensitivity in openForLoading)
>
> Once:  list all files (listSavefile)
>
> For each save:  list files to find the correct case (case
>                                          insensitivity emultion)
> For each save:  list files until a match is found (internally)
> For each save:  read file data from the block location found in the
>                  previous step
>
> So it's  T(1+2N) versus T(1+3N) like I wrote.

Why is reading data from the block a operation taking time T (i.e. time 
to list save files + loading a small save in your scheme)?
As I outlined, the way I see it is it takes (N+1)T+ND or (2N+1)T+ND, 
where T is the time to list save files and D is the time it takes to 
extract the description. I don't really see that T and D can be used 
interchangably, nor what their relation is in general.

> The user do not have to either care about or ignore the case.  They
> just plug in the two memory cards, select the file to copy and the
> card to copy it to, and it is copied.
>

> You donÃ't copy files betweensystems on the Dreamcast, you move the
> memory card to the new system.

Sure, this might not be an issue on Dreamcast. But engine logic working 
on all supported systems has to do this anyway, even on Dreamcast. Thus, 
engines worrying about it will also affect performance on your backend.

> Changing target casing between versions sounds like a bad idea, yes. 
> We should not do that. 

We shouldn't. The user can. And there is not much we can do to avoid it.

> I do not agree with that conclusion.  Using the filenames returned by
> listSavegames should work for most engines.
>

See below.

> Well, you call it "caching", but it's really just processing the list. 
> It needs to go throgh the list one item at a time to populate its 
> internal represenation of the savegame list. The item _is_ the 
> filename, so it's right there, no caches needed. 

See below. You need real caching for some use cases or suffer 
performance issue there.

> If there is really significant logic that would need to be replicated,
> it could be put in common or something, but in general it would be
> something like
>
>    for(savename : listSavenames)
>       if (f = openForLoading(savename))
>         addSaveToMenu(savename, f)
>

Not a single engine implements it like this. This suggests either they 
assume it's case insensitive or they don't care.
Since most of these engines have been for systems with case insensitive 
file systems, it seems natural to assume we would feature the same 
behavior to provide an easy environment to re-implement these engines. 
This has been the case for Common::File.

Saving the filename for available saves inside a dialog is not the only 
issue we talk about here. We can re-work all engines with this use case 
to work like you suggested. However, not in all cases listSavefiles has 
been used before.

Other cases include:
1) Quick save/load via hotkeys.
2) Loading from command line/launcher.
3) Using save file manager as only way to write non-save files which 
store data for the engine. This is present also in our very first 
engine: SCUMM, but, for example, also used in SCI.
4) Engines not using a listSavefile at all for save handling, for 
example Gob (which is heavily original game script controlled IIRC), 
Composer, MADE, and Sword25.

Sure, we can probably put a lot of effort in it and implement all this 
working fine on case sensitive systems. However, the common theme seems 
to be that people want case insensitive access.

> But beacuse of the way the API looks, the SaveFileManager does not
> have access to the list generated by listSavefiles, that is owned by
> the engine.  The backend could cache the list (in a more proper sense
> of the term), but due to the lack of a session concept, it wouldn't
> know when to discard said cache (this should happen when the dialog is
> completely filled, to prevent stale entries).

Are stale entries the only issue we talk about which prevent you from 
caching available files inside your SaveFileManager implementation?

// Johannes