#8910 closed patch
KYRA/SCUMM: Thumbnail support/improvement
| Reported by: | lordhoto | Owned by: | lordhoto |
|---|---|---|---|
| Priority: | normal | Component: | Engine: Kyra |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
Hi,
this patch adds thumbnail support to the kyra engine. To achieve this it moves out most of the former SCUMM specific thumbnail save format to a common API (found in graphics/thumbnail.h). It also changes SCUMM to use the new API to load/save thumbnails.
There are some things which should be discussed before this patch will be committed:
The patch contains support for optional thumbnail entries in save files, it is implemented via backward seeking if no valid header is found. Since our compressed save files do not support backward seeking, it will not be working properly right now. (See TODOs in engines/scumm/thumbnail.cpp) The question about this issue is: Do we want savegames without thumbnails? For example the Nintendo DS port does not create any thumbnails currently, it only saves a plain black rect as thumbnail. With the newly added code, the thumbnail could be simply left out for it.
Also do we really want thumbnail save/load to be done via a common API? I think it is easier to support thumbnails in other engines with it. Also we have some well tested and useable API for it then.
-
Another side note about the patch, I wasn't actually able to test the thumbnails saved from kyra in real world yet, since the kyra dialog does not use the overlay thus, it is not possible to display thumbnails. And I was too lazy to write a simple displayer yet ;-), I suspect it working though. At least there should be no issues with the newly added common API for thumbnails, I tested it with SCUMM and everything works fine for me.
Ticket imported from: #2050337. Ticket imported from: patches/1015.
Attachments (2)
Change History (14)
by , 16 years ago
| Attachment: | kyra_thumbnail_v1.patch added |
|---|
comment:2 by , 16 years ago
The attached patch compressed-save-seek.patch is supposed to enable backward seeking in compressed savegames (albeit very inefficiently -- code doing seek(-1) will suffer *a lot*). However, the patch is 100% untested -- but it might serve as a starting point at least :) File Added: compressed-save-seek.patch
comment:3 by , 16 years ago
Couldn't we just 'cache' some small memory block to allow efficient backseeking for small offsets?
Also any opinion about the patch itself? ;-)
comment:4 by , 16 years ago
| Owner: | removed |
|---|
comment:5 by , 16 years ago
We could cache some small memory block, aye -- but that will make the design even more complicated, I am afraid :/. Of course, one could always couple it with a BufferedReadStream wrapper, wrapping the wrapper... Hrm... Another thought: the "compressed savefile" wrapper could be turned into a generic compressed *stream* wrapper, now that SaveFiles are just plain streams...
But I am digressing :).
Regarding the patch: Looks mostly fine to me. One bit we maybe should change is the reference to MM NES in createThumbnail() in graphics/scaler/thumbnail.cpp. And explicitly describe the doxygen comment (not yet existing, I think, ahem ;) of this function how it will deal with input images of certain sizes. However, this is a deficiency of the existing code, not your patch, so... :).
As for backward seeking: As long as we do a full rewind (=seek back to start), the patch I attached is actually fine. No need for buffering there.
It might indeed be good if we could leave out thumbnails -- though best would be to explicitly ask Neil and other porters. How about writing to scummvm-devel.
Overall, a common thumbnail API sounds good to me, esp. since no engines but SCUMM support thumbnails right now, AFAIK (so there is no existing thumbnail code other than that in SCUMM we have to worry about adapting), and it might in fact encourage other engine maintainers to add thumbnail support -- at least it would make it a bit easier for them.
As for testing: We should soon have a "load" dialog in the launcher, which is able to display thumbnails for arbitrary saves (either as part of Chris RTL work, or with some extra effort by one of us after GSoC). I.e. the dialog is there, we just need to hook up Kyra, and could then test it.
comment:6 by , 16 years ago
I guess by removing the reference to MM NES you mean to allow other widths apart 320 and 640?
Also our Hercules mode check is rather hacky, and it should if it's supported by other engines (maybe AGI?) cut parts of the screen instead of just the menu as in SCUMM.
Of course all that is because we just want to have 160x100 or 160x120 thumbnails currently. We could either downscale any mode to a <= 160 width and <= 120 height and center it then afterwards or we change our load menus to allow dynamic resizing of the thumbnail preview screen and thus allow any size for thumbnails, of course we should define a max size again too.
comment:7 by , 16 years ago
Indeed, that's more or less what I wanted to get at: That the thumbnail code should be flexible enough to deal with games in various differing resolutions. Again, it doesn't have to be part of this patch, as this is clearly an existing problem in the code. But if we truly want to encourage other engine authors to add thumbnail support, we'll have to deal with that.
So, what I'd do is to indeed define a max-size -- namely 160x120 ;). And then allow storing arbitrary thumbnail sizes, as long as they are smaller than 160x120. The display code would simply center the pic. Resizing code should try to preserve the aspect ratio.
We might want to add code for arbitrary scale downsizing, but then again, that would possibly make things overly complex.
comment:8 by , 16 years ago
Oh well that shouldn't be impossible to achieve. Should we first check this one in and then worry about it, or should I work on some improvement before checking in?
comment:9 by , 16 years ago
I'd say, just check it in now, it doesn't make things worse, after all; and checked in, it will get more attention and testing.
comment:10 by , 16 years ago
Ok committed both patches after some minor testing. I'll close this item then.
comment:11 by , 16 years ago
| Owner: | set to |
|---|---|
| Status: | new → closed |
comment:12 by , 6 years ago
| Component: | → Engine: Kyra |
|---|
patch against r33849