#8405 closed patch
Thumbnails for ScummEngine
| Reported by: | lordhoto | Owned by: | fingolfin |
|---|---|---|---|
| Priority: | normal | Component: | Engine: SCUMM |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
Hi,
after 1 and a half day hard work noe here is it, a first version of my thumbnail patch. It contains a small bugfix for ListWidget also and a basic implementation of a GraphicsWidget.
Currently TODO: - save thumbnails into the save file and _NOT_ to a seperate file - extend ScummEngine::createThumbnail to include text in COMI (maybe in HE games too?) - maybe move some code to different files - get the thumbnail code also to work with non 16 bit overlays - extend GraphicsWidget to convert gfx data automaticly to current OverlayColor - maybe current thumbnail save isn't endian save (i think it isn't but can't test it)
Currently DONE: - a basic thumbnail 'scaler' - saving thumbnail to a file with .tmb at the end - extending the save/load dialog of the scummengine - fixing a bug with ListWidget (now in the save dialog you can input the savegamename and after that you can press "save" and the savegamename is saved before you have to press enter to get it to work)
I hope haven't forgotten anything :)
cya and have fun while using the patch
Ticket imported from: #1163026. Ticket imported from: patches/510.
Attachments (10)
Change History (41)
by , 19 years ago
| Attachment: | thumbnails-v1.patch added |
|---|
comment:1 by , 19 years ago
comment:2 by , 19 years ago
hmm k i will look at them today or tomorrow thx for testing
but i don't know why we shouldn't display thumbnails at 320x200 i tried and there is enough size for the rest IHMO
comment:3 by , 19 years ago
so new version!
Currently TODO: - eliminate some bugs like: drawBlastObject: getObjectIndex on BlastObject 191 failed! while making thumbnails - testing with HE games - get the thumbnail code also to work with non 16 bit overlays - maybe move some code to different files - save thumbnails into the save file and _NOT_ to a seperate file - maybe current thumbnail save isn't endian save (i think it isn't but can't test it) - extend GraphicsWidget to convert gfx data automaticly to current OverlayColor - create a GraphicsList for loading savegames while in 640x400 mode
Added to DONE: - extend ScummEngine::createThumbnail to include text in COMI (maybe in HE games too?) - In --force-1x-overlay mode it does not show anything. - "No preview" disappears after first click
comment:4 by , 19 years ago
ah now i see it( drawBlastObject: getObjectIndex on BlastObject XXX failed! ) happens only when a video is playing... so don't try to save while a video is playing ;)
comment:5 by , 19 years ago
I wonder why do you name functions as backUpBlastObjectQueue() and varaibles as _blastTextQueueBackUP? Note Up vs UP. Besides you could write simple 'Backup' wihtout additional caps.
Just realized why I told you that it would be more proper not to show thumbnail with 1x overlay. Problem is that you scale2x thumb at 2x overlay mode. This shouldn't be done. Current thumbnail is too big and eats a lot of space, making long names even narrower.
It is bad to allow F5 dialog at video playing anyway, so this should be disabled in the engine. If you know how to restrict that feel free to submit a separate patch.
I don't think we should gear for non-16bit overlays. Those should use current 320x200 GUI which should be left intact.
To make thumbnails save/restore endian-safe use simple loop instead single write with FROM_LE_16/TO_LE_16 functions.
I just tested it with HE games. Works as expected and text is saved too.
comment:6 by , 19 years ago
I just returned to this tracker item to see why nobody ever responded to my comment on it, only to discover my comment on it is, erhm, nonexistant... Probably because once again SF.net logged me out, and I didn't notice the "submission failed because you do not have the access rights" <sigh>.
Anyway, I don't recall everything I wrote, and some has been said now already, but: - createThumbnail_4 and createThumbnail_8 really could be simplified a lot, by using loops. That could of course hurt the speed a bit, but OTOH, a good loop unroller will probably produce identical (or better) results than your manual code...
- the ListWidget changes/fixes sound like a seperate issue, so they should be in a separate patch, which we can review and apply independant of the thumbnail work
- Eugene, regarding 320x200 vs 640x400 versions of the dialogs: My plan is to let dialog subclasses specify with some flag or so whether they support only 320x200 (and thus needs to be scaled by the GUI), or whether it can do the scaling itself. E.g. by adding a virtual bool Dialog::needsAutoscaling() method. After that, we can migrate all dialog step by step to support high res "natively". As how to do that: essentially, you only have to modify the GUI element placement for the active resolution, and possibly some GUI elements are only visible in hig res mode (like, a ScummVM logo on the launcher). Anyway, I guess the discussion of that should take place someplace else :-)
- The way the thumbnail is currently made is rather "bad". I realiize it's the easiest / least intrusive way to do it right now, but it causes all sorts of potential problems, because it destroys the draw state. That's also the reason why you had to add the backUpBlastObjectQueue() hack, and why you have problems with text etc.. Not good. So, instead of doing a redraw, the actual screen content need to be captured. There are at least two ways we can achieve that: 1) add a new method to OSystem to grab the current (8bit) content of the screen surface. Pro: very clean. Con: need all backends to be updateded again 2) add another buffer in the SCUMM engine which we duplicate all blits to the screen on. Pro: Local change only. Con: Takes up more memory/time for a very small benefit; also if other engines have the same problem, they need to duplicate code
- If I understand the code right, _thumbnail simply contains 80*60 pixels of 16 bit data. Since you write it out with a single write() call, the data is written in the current endianess, which is indeed not portable. Portable way to read/write: Loop over _thumbnail, and all saveUint16/loadUint16 on each entry.
- Actually, just writing out the raw thumb data is not ideal. I'd much prefer if we also included at least some minimal info about it; like the dimension, and length of data. That way we can easily extend/change the thumbnail format if we ever have a reason to. Actually, I think it would be best if we used some standard format for that, e.g. BMP or so (something simple which is easy to read/write. Even if we eventually store the thumb inside the savegame, I would favor that aproach.
comment:7 by , 19 years ago
(grr, and once again, SF.net has logged me out while I wasn't looking... luckily I wrote my comment in an editor and still had it in the clipboard).
comment:8 by , 19 years ago
yeah i think i should create a seperate patch for the list wigdet code ;)
>> - If I understand the code right, _thumbnail simply contains 80*60 pixels of 16 bit data. Since you write it out with a single write() call, the data is written in the current endianess, which is indeed not portable. Portable way to read/write: Loop over _thumbnail, and all saveUint16/loadUint16 on each entry. <<
yeah i know :)
so you should now still work in process
sry for the short answer but not much time
comment:9 by , 19 years ago
so new version of the patch
DONE: - now endian save - working with all scumm games (only 320x200 or bigger, so no NES games) - added grabSurface to the OSystem, currently only implementation in the SDL backend - thumbnail size now 160x100, 160x120 - 2 save/load dialogs, one without thumbnails for 320x200 overlays and one with thumbnails for 640x400 or higher
have fun with this patch
comment:10 by , 19 years ago
Notes: o remove config.h from your patch o as Finglofin mentioned, there should be thumbnail size stored in it, because now thunmbnails generated by earlier patch do not work o when you exit from save/load dialog and then launch it again, there is displayed last shown thumbnail, when there is no savegame selected, but this is a minor thing because it will go away after you implement list of thumbnails o I wonder how safe current grabScreen() is. Maybe add size parameter there? o would be nice to use symbolic constants instead of hardcoded values for thumbnail size
comment:11 by , 19 years ago
hups i used make depclean before, so i though there shouldn't a config.h or something like that...
yeah but currently the extra file is only a test, it should be integrated in the savefile once so i don't think it has much sense to add the values now.
yeah maybe i should extend grabScreen a bit but it's only for grabbing the hole screen like grabOverlay so i don't know why there should be a size parameter.
instead of wich hardcoded values?
comment:12 by , 19 years ago
> instead of wich hardcoded values? Thumbnail dimensions you use everywhere, i.e. 160 and 120
comment:13 by , 19 years ago
Lordhoto, the thumb dimensions should be inclded in the data *now*. Essentially, if it is integrated into the savefile, the *exact same* data would be written, only into the savefile instead of a seperate thumb file. Hence it makes no sense to wait with implementing that required functionality. Plus I still think it would be a good idea to use an existing file format, like BMP or Targe, for this.
comment:14 by , 19 years ago
Some further remarks:
* There are config.h and newgui.cpp.orig files in your patch, don't belong there * grabScreen() probably shouldn't have an empty default implementation. And it needs better documentation / specification, but that can happen later. * As Eugene says, it's not good to hardcode 160/120 in lots of places. * The ListWidget stuff still isn't in a separate patch.
comment:15 by , 19 years ago
So new patch. Now the width/height of the thumbnail is saved in the file and all hardcoded 160/120 should be away! also the ListWidget stuff isn't in this patch anymore. It's not a really new thing but it's a bit cleaned up.
Have fun with this patch!
comment:16 by , 19 years ago
v4 is out now!
now the thumbnail is saved into the savefile, with a nice header (idea from _sev). also fixed is the bug with displaying a thumbnail without clicking on a savefile. i hope it's clean.
Have fun with this patch!
comment:17 by , 19 years ago
The "grabScreen" method isn't really sufficient. It would be too easy for it to cause memory overflow. Much better would be to have it take a Graphics::Surface as a parameter, to which the screen contents are copied.
Likewise, GraphicsWidget::setGfx should take a Graphics::Surface.
Also I don't understand why you added various _thumbnail* members to ScummEngine -- any reason these are not simply parameters/return values of the various functions they are used in right now? In the worst case (there is such a reason), they should be replaced by a single Graphics::Surface. Better would be to just get completly rid of them (using global variables to pass on data between select functions is bad, as it makes the code much harder to understand and maintain)
Actually, the current ScummEngine::createThumbnail doesn't need to be in that class either, AFAICT, the only thing it needs is _system, which could be passed to it via a param, too. So it could be turned into a "createThumbFromScreen" method in thumbnail.h which other engines could then use, too. And it would return a newly allocated Surface.
(This makes me wonder about memory managment; maybe we should add a virtual destructor to class Surface, so that we can add subclasses of it which automatically free the video memory)
comment:18 by , 19 years ago
Also, 160 is still hardcoded in loadThumbnailFromData()
Note that in the meantime, CURRENT_VER was incremented, so the patch will have to be adjusted accordingly.
In loadThumbnail, if you detect a new thumb version, you just load it into a buffer and dispose that buffer. A seek would be nicer, but SaveFile doesn't allow that right now. Maybe put a TODO comment there, we'll update SaveFile for this eventually.
The thumb data in the savefile has a "Bpp" field for "bytes per pixel". I recommend switching this to "bpp", "bits per pixel", which is more flexible and already used in ScummVM.
getThumbnail() and loadThumbnail() seem to duplicate a lot of code, and could be unified (getThumbnail could just use loadThumbnail)
comment:19 by , 19 years ago
so new pre version of the patch. is more for looking if it's ok with the new OSystem extensions and the new structure, also now it should work with MM NES and it applies to current CVS again!
comment:21 by , 19 years ago
* discribed -> described
* grabScreen / grabRawScreen comments should clarify that it's the callers responsibility to dealloc the surface buffer
* The 24 bit code in OSystem::grabScreen just looks wrong to me. Even on BE systems, it is RGB, not BGR ... The 32 bit code there is wrong, too (not endian safe). As a rule of the thumb: if you access per-byte, you do not have to do additional endian conversion. If you store a whole uint16/uint32 in one go, you *have* to do endian conversion.
* GraphicsWidget::setGfx still should take a Surface as param...
* NewGui::drawSurface should do clipping; at the very least, it should have an assert to prevent code to accidentally overwrite memory outside the graphics buffer; even better would be if it just properly clipped the rect before the blit.
* ScummSaveLoadChooser is strange. First off, it should be called BaseSaveLoadChooser, to follow the implicit naming convention we use for common base classes throughout the code. Secondly, why doesn't it inherit from GUI::ChooserDialog ? It seems awkward to use multiple inheritance when there is absolutely no need for it.
* As I mentioned before, CURRENT_VER was incremented recently. You need to update your patch accordingly (and use CURRENT_VER 50 in it, not 49). This also means your patch won't apply to CVS currently.
* "hardcastet" isn't an english word (it's not a german word either, of course :-)
comment:22 by , 19 years ago
"* "hardcastet" isn't an english word (it's not a german word either, of course :-)" huuuuuupppppppssssssss ;)
ScummSaveLoadChooser is notinherit from GUI::ChooserDialog, because the displayer for the thumbnail is created in a bit different way.
ok i will look at the grabScreen function.
and as i said it's a prepatch for looking, so don't wonder why i changed the gui part by now.
comment:23 by , 19 years ago
| Owner: | set to |
|---|
comment:24 by , 19 years ago
When you say this is a "prepatch", does that mean you are working on a new version of it or what?
comment:25 by , 19 years ago
yeah there will be a new version in the next time, but currently i have to learn for school and i'm doing a bit for another project, so it could take some time.
comment:26 by , 19 years ago
so now a new version of the prepetch, still a few things have to be 'fixed', so Graphics::Widget should convert the surface to the overlaycolor and a special scaler for Hercules games should be added.
comment:27 by , 19 years ago
some remarks: Graphcis::Widget should be GraphicsWidget::setGfx ;) also grabScreen has now no default param for bpp and only 15 and 16 bpp are supported. yeah an the new NewGui::drawSurface is now used
comment:28 by , 19 years ago
Now here is a new version, it includes support for MM thumbnails in hercules mode (no menu and speechline included). But it will only work with the SDL Backend (compiling with other backends will fail, because of the virtual methods added a few patches ago).
Have fun with this patch.
comment:29 by , 19 years ago
| Status: | new → closed |
|---|
comment:30 by , 19 years ago
I just checked in the last parts of the patch (a lot was modified by me).
Note that I had to change the saveformat slightly between the patch attached to this tracker item and CVS -- you were generating a wrong 'header.size' value upon saving.
comment:31 by , 5 years ago
| Component: | → Engine: SCUMM |
|---|
Besides problems you already mentioned, I'll add few more.
o It would be nice that thumbnail border either ajusts to thumb size or at least thumb is centered vertically in it. o In --force-1x-overlay mode it does not show anything. o "No preview" disappears after first click
Speaking about second item, I think we should fork current GUI. I.e current 320x200-oriented should stay as is, and another one we will alter as much as we like. That means that for 320x200 mode we should not show thumbnails at all.
Also it would be nice if thumbnails would be in another list, i.e. that you can scroll between them and find savegame by it. Right now you have to click on each savegame to see the preview.