Opened 10 years ago

Closed 8 years ago

Last modified 13 months ago

#4880 closed defect (fixed)

IPHONE: No grabPalette() implementation

Reported by: SF/mthreepwood Owned by: lordhoto
Priority: normal Component: Port: iOS
Keywords: Cc:
Game:

Description

The iPhone backend doesn't implement the grabPalette() function, which could possibly lead to problems in the engines that use that OSystem function: Tucker, Kyra, M4, Groovie, and SCI

Ticket imported from: #2999153. Ticket imported from: bugs/4880.

Attachments (1)

iphone-grabPalette.patch (975 bytes ) - added by fingolfin 10 years ago.
Attempt to implement grabPalette

Download all attachments as: .zip

Change History (15)

by fingolfin, 10 years ago

Attachment: iphone-grabPalette.patch added

Attempt to implement grabPalette

comment:1 by fingolfin, 10 years ago

I just attached a patch which tries to implement grabPalette. Right after that, I realized that I actually recently installed the iPhone SDK, so I can test it -- did just that, and found that I made a mistake.

Anyway, since it compiles, I feel confident enough to just commit it ... ;). But I am not closing this item yet, as vinterstum should review (and ideally test?) this change.

comment:2 by SF/mthreepwood, 10 years ago

Doesn't that lose precision on the RGB values? The iPhone backend converts from 24-bit to 16-bit on setPalette() and now 16-bit to 24-bit on grabPalette().

comment:3 by fingolfin, 10 years ago

It is true that this "looses" bits compared to what is passed in via setPalette. But nowhere in the OSystem specs does it say that grabPalette will return the exact same data that was set via setPalette, It only promises to return the "currently active palette", and that it does: If you call grabPalette and then setPalette with the data returned by grabPalette, the palette stays unchanged (we don't say this explicitly, but it is implicit from the wording of the grabPalette specs, I'd say).

Note that the dc, ds, n64, ps2 and wii backends do the exact same thing (and also the psp backend, unless used in 32bit mode).

Only the SDL backend, and backends derived from it (WinCE, Symbian, SamsungTV, ..., and indirectly GP2x) actually always prefer the data passed in.

comment:4 by SF/mthreepwood, 10 years ago

It seems wrong that this is 'assumed' to return different than what is passed in. I also don't believe this is implicitly stated in the documentation, there's nothing that says either way; it just says "currently active palette" which makes sense to be assumed to be the same palette that was set from the engines.

I could come up with a patch that would switch them all to storing the palette in 32-bits (of course, I wouldn't have a way of testing this on any of the other ports as of this writing).

comment:5 by SF/mthreepwood, 10 years ago

In the very least, the documentation for grabPalette() could use an update ;)

comment:6 by bluegr, 9 years ago

This seems to have been implemented already by fingolfin, right? Can it be closed now?

comment:7 by bluegr, 9 years ago

Owner: changed from vinterstum to fingolfin

comment:8 by fingolfin, 9 years ago

I made a change there, yes, but I can't test it myself, and I don't know if it works at all. So I'd really prefer if somebody would verify whether this actually works now.

comment:9 by fingolfin, 9 years ago

Owner: changed from fingolfin to vinterstum

comment:10 by vinterstum, 9 years ago

What's a good testcase for this?

comment:11 by bluegr, 8 years ago

A good test case would be a game that uses grabPalette(). An example that comes to mind is SCI, whenever a fadein/fadeout or a palette variation effect is made, like, for example, in Freddy Pharkas, when starting a new game. There's an effect which changes the screen colors from sepia to full color, which uses grabPalette().

comment:12 by digitall, 8 years ago

Owner: changed from vinterstum to lordhoto
Resolution: fixed
Status: newclosed

comment:13 by digitall, 8 years ago

LordHoto has been working on the IPhone backend and has reviewed the relevant code. He is happy that it looks good so closing. If testing reveals any issues, this can be reopened.

comment:14 by digitall, 13 months ago

Component: Port: iOS
Note: See TracTickets for help on using tickets.