Opened 12 years ago

Closed 12 years ago

Last modified 23 months ago

#8916 closed patch

Enhance OSystem_SDL::setupIcon

Reported by: SF/canavan Owned by: fingolfin
Priority: normal Component: --Other--
Keywords: Cc:
Game:

Description

OSystem_SDL::setupIcon imposes unneccesary size limitations on the icon to be installed. Furthermore, it computes a mask for the icon, although SDL_WM_SetIcon will do that automatically when mask==NULL and and the RGBSurface has an alpha channel.

Ticket imported from: #2072006. Ticket imported from: patches/1021.

Attachments (1)

SDLsetupIcon.patch (1.8 KB ) - added by SF/canavan 12 years ago.
patch

Download all attachments as: .zip

Change History (12)

comment:1 by sev-, 12 years ago

Was a patch supposed to be attached here?

comment:2 by fingolfin, 12 years ago

Hrm, I wasn't able to attach files to another tracker item, too, and so were several other people. See this support request I posted yesterday: <https://sourceforge.net/tracker/?func=detail&aid=2070259&group_id=1&atid=200001>. canavan, if you got the same error, you may want to chime up on that SR.

comment:3 by SF/canavan, 12 years ago

Priority: normalhigh

comment:4 by SF/canavan, 12 years ago

--- sdl.cpp.orig Fri Aug 22 21:43:29 CEST 2008 +++ sdl.cpp Sun Aug 24 17:22:20 CEST 2008 @@ -276,15 +276,21 @@ }

void OSystem_SDL::setupIcon() { - int w, h, ncols, nbytes, i; - unsigned int rgba[256], icon[32 * 32]; - unsigned char mask[32][4]; + int x, y, w, h, ncols, nbytes, i; + unsigned int rgba[256]; + unsigned int *icon;

sscanf(scummvm_icon[0], "%d %d %d %d", &w, &h, &ncols, &nbytes); - if ((w != 32) || (h != 32) || (ncols > 255) || (nbytes > 1)) { + if ((w > 512) || (h > 512) || (ncols > 255) || (nbytes > 1)) { warning("Could not load the icon (%d %d %d %d)", w, h, ncols, nbytes); return; } + icon=(unsigned int*)malloc(w*h*sizeof(unsigned int)); + if (icon==NULL) { + warning("Could not allocate the icon"); + return; + } + for (i = 0; i < ncols; i++) { unsigned char code; char color[32]; @@ -304,20 +310,20 @@

rgba[code] = col; } - memset(mask, 0, sizeof(mask)); - for (h = 0; h < 32; h++) { - const char *line = scummvm_icon[1 + ncols + h]; - for (w = 0; w < 32; w++) { - icon[w + 32 * h] = rgba[(int)line[w]]; - if (rgba[(int)line[w]] & 0xFF000000) { - mask[h][w >> 3] |= 1 << (7 - (w & 0x07)); - } + for (y = 0; y < h; y++) { + const char *line = scummvm_icon[1 + ncols + y]; + for (x = 0; x < w; x++) { + icon[x + w * y] = rgba[(int)line[x]]; } }

- SDL_Surface *sdl_surf = SDL_CreateRGBSurfaceFrom(icon, 32, 32, 32, 32 * 4, 0xFF0000, 0x00FF00, 0x0000FF, 0xFF000000); - SDL_WM_SetIcon(sdl_surf, (unsigned char *) mask); + SDL_Surface *sdl_surf = SDL_CreateRGBSurfaceFrom(icon, w, h, 32, w * 4, 0xFF0000, 0x00FF00, 0x0000FF, 0xFF000000); + if (NULL==(sdl_surf)) { + warning("SDL_CreateRGBSurfaceFrom(icon) failed"); + } + SDL_WM_SetIcon(sdl_surf, NULL); SDL_FreeSurface(sdl_surf); + free(icon); }

OSystem::MutexRef OSystem_SDL::createMutex(void) {

comment:5 by SF/canavan, 12 years ago

Priority: highnormal

comment:6 by SF/canavan, 12 years ago

Since uploads don't currently work and cut-and-paste apparently mangles tabs, I've put a clean version of the patch on http://scummvm.canavan.de/SDLsetupIcon.patch

comment:7 by fingolfin, 12 years ago

Attachment problems should be fixed now.

So, back to my questions: While in theory it's nice to support icons bigger than 32x32, in reality our built-in icon has a fixed size of 32x32, so why should we add a malloc to support bigger icons? Should we switch to a bigger icon, we could just increase the temp buffer in setupIcon to match, no?

Dropping the "mask" bit seems like a nice move, though.

by SF/canavan, 12 years ago

Attachment: SDLsetupIcon.patch added

patch

comment:8 by SF/canavan, 12 years ago

While it should be trivial to update the constants scattered about setupIcon() in the unliekely event that the official scummvm icon should ever be changed to some other size, this artificial restriction makes it unneccesarily difficult to change to another icon format on platforms where 32x32 just doesn't look good enough.

I actually started inspecting/modifying setupIcon() due to a bug in SDL_WM_SetIcon(), but the added flexibility allows me to provide a suitable icon for IRIX's default window manager 4DWm, which has a constant icon size of 85x67. File Added: SDLsetupIcon.patch

comment:9 by fingolfin, 12 years ago

OK, fair enough. I commited a slightly cleaned up version of this patch.

comment:10 by fingolfin, 12 years ago

Owner: set to fingolfin
Status: newclosed
Summary: simplify OSystem_SDL::setupIconEnhance OSystem_SDL::setupIcon

comment:11 by digitall, 23 months ago

Component: --Other--
Note: See TracTickets for help on using tickets.