Opened 15 years ago

Closed 14 years ago

Last modified 11 months ago

#8405 closed patch

Thumbnails for ScummEngine

Reported by: lordhoto Owned by: fingolfin
Priority: normal Component: Engine: SCUMM
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)

thumbnails-v1.patch (27.8 KB ) - added by lordhoto 15 years ago.
thumbnails-v2.patch (30.6 KB ) - added by lordhoto 15 years ago.
new standalone patch
thumbnails-v3.patch (38.3 KB ) - added by lordhoto 15 years ago.
new standalone patch
thumbnail-v3-2.patch (24.2 KB ) - added by lordhoto 15 years ago.
new standalone patch
thumbnail-v4.patch (27.9 KB ) - added by lordhoto 15 years ago.
new standalone patch
thumbnail-v4-2pre1.patch (33.6 KB ) - added by lordhoto 15 years ago.
new standalone patch
thumbnail-v4-2.patch (33.7 KB ) - added by lordhoto 14 years ago.
new standalone patch
thumbnail-v4-2-new.patch (33.7 KB ) - added by lordhoto 14 years ago.
new standalone patch
thumbnail-v4-2-08-05-2005.patch (33.5 KB ) - added by lordhoto 14 years ago.
applies to cvs code again..
thumbnail-v4-2-08-05-2005-r2.patch (33.3 KB ) - added by lordhoto 14 years ago.
after some words in irc…

Download all attachments as: .zip

Change History (41)

by lordhoto, 15 years ago

Attachment: thumbnails-v1.patch added

comment:1 by sev-, 15 years ago

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.

comment:2 by lordhoto, 15 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 lordhoto, 15 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

by lordhoto, 15 years ago

Attachment: thumbnails-v2.patch added

new standalone patch

comment:4 by lordhoto, 15 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 sev-, 15 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 fingolfin, 15 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 fingolfin, 15 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 lordhoto, 15 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

by lordhoto, 15 years ago

Attachment: thumbnails-v3.patch added

new standalone patch

comment:9 by lordhoto, 15 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 sev-, 15 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 lordhoto, 15 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 sev-, 15 years ago

> instead of wich hardcoded values?
Thumbnail dimensions you use everywhere, i.e. 160 and 120

comment:13 by fingolfin, 15 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 fingolfin, 15 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.

by lordhoto, 15 years ago

Attachment: thumbnail-v3-2.patch added

new standalone patch

comment:15 by lordhoto, 15 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 lordhoto, 15 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!

by lordhoto, 15 years ago

Attachment: thumbnail-v4.patch added

new standalone patch

comment:17 by fingolfin, 15 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 fingolfin, 15 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)

by lordhoto, 15 years ago

Attachment: thumbnail-v4-2pre1.patch added

new standalone patch

comment:19 by lordhoto, 15 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:20 by sev-, 15 years ago

Works ok with MM NES

comment:21 by fingolfin, 15 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 lordhoto, 15 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 fingolfin, 15 years ago

Owner: set to fingolfin

comment:24 by fingolfin, 15 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 lordhoto, 14 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 lordhoto, 14 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 lordhoto, 14 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 lordhoto, 14 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.

by lordhoto, 14 years ago

Attachment: thumbnail-v4-2.patch added

new standalone patch

by lordhoto, 14 years ago

Attachment: thumbnail-v4-2-new.patch added

new standalone patch

by lordhoto, 14 years ago

applies to cvs code again..

by lordhoto, 14 years ago

after some words in irc...

comment:29 by fingolfin, 14 years ago

Status: newclosed

comment:30 by fingolfin, 14 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 digitall, 11 months ago

Component: Engine: SCUMM
Note: See TracTickets for help on using tickets.