Opened 22 years ago

Closed 22 years ago

Last modified 6 years ago

#8042 closed patch

palManipulateInit code (bug #558245)

Reported by: SF/jamieson630 Owned by: fingolfin
Priority: normal Component: Engine: SCUMM
Version: Keywords:
Cc: Game:

Description

The included modifications to gfx.cpp implement palManipulateInit with the necessary information for palManipulate to correctly execute SCUMM opcode 0x33 subcode 0x0F (palManipulate). The lack of implementation of this opcode is addressed in bug #558245, in which the knowledge basis for this implementation was developed.

Some notes:

* The implementation of palManipulate was left untouched, since according to fingolfin it is the result of direct reverse engineering on the LEC code. However, it has a resolution problem: the slowest transition rate it supports is 1 luminescence value every 256 ticks. FOA has at least one occurrence of a palManipulate over 800 ticks, and I believe the lack of resolution shows. (I also believe this is why in that instance, there is a trailing palManipulate to immediately change to the target palette and clean up any inaccuracies that arose over the course of the transition.)

I originally showed fingolfin a slightly modified palManipulate implementation that has no such resolution limitations, at the cost of 3 extra divide operations per tick. If there is any interest in this variation, I would be happy to submit a patch for that as well. (There are associated mods to palManipulateInit to make it work.)

* I have assumed in my code that each tick corrseponds to 1/10 second; consequently, _palManipCounter is set equal to the "time" parameter without any transformation. In timing tests, this appeared to work out properly. I'm sure someone can confirm or deny the validity of this assumption for me off the top of their head.

* I tested the palManipulate usage in FOA Room 23 (the Inner Sanctum) and Room 96 (the extro). Everything seems to be working quite well. However, I only tested it in the Win32 non-MP3 debug build. For some reason, I kept getting page faults immediately when trying to run the Win32 release build. I may be linking to a debug version of SDL.lib; I'll keep trying to get it to work.

* The patch submitted was generated by diff - GNU diffutils version 2.7, the Win32 port available from SourceForge. I was using the "diff" command on CVS 1.11.2, but its output appeared to be substantially different from the patches that I've seen here. Let me know if this patch is not in the form needed.

I hope this patch helps bring that gorgeous Indy sunset back to life. :-)

Ticket imported from: #608138. Ticket imported from: patches/147.

Attachments (6)

gfx.cpp.diff (4.7 KB ) - added by SF/jamieson630 22 years ago.
palManipulateInit implementation
palmanipulateinit.diff (6.4 KB ) - added by SF/jamieson630 22 years ago.
Patch against 4 files from September 10 snapshot
savepalmanipulate.diff (9.0 KB ) - added by SF/jamieson630 22 years ago.
Patch adds palManipulate data save/load
atlantis.s05 (61.2 KB ) - added by SF/jamieson630 22 years ago.
FOA Big Machine - uses new save/load
atlantis.s06 (34.2 KB ) - added by SF/jamieson630 22 years ago.
FOA Extro - uses new save/load
savepalmanipulate.2.diff (9.0 KB ) - added by SF/jamieson630 22 years ago.

Download all attachments as: .zip

Change History (23)

by SF/jamieson630, 22 years ago

Attachment: gfx.cpp.diff added

palManipulateInit implementation

comment:1 by SF/jamieson630, 22 years ago

BTW, palManipulateInit was stubbed with a parameter that has absolutely no meaning and does not map to any parameter in the script opcode. Why is that? In both script_v1.cpp and script_v2.cpp, it is being called with a constant value 1 for that parameter. Should this function prototype be revised, and calling code modified, to remove the unnecessary parameter?

comment:2 by SF/trinity78, 22 years ago

as far as i know and as i talked with fingolfin about it, the "1"- parameter is totally useless and can be removed.

by SF/jamieson630, 22 years ago

Attachment: palmanipulateinit.diff added

Patch against 4 files from September 10 snapshot

comment:3 by SF/jamieson630, 22 years ago

New patch file added, which contains a couple things:

1) Removal of the undefined final parameter for palManipulateInit, in the header file, source file, and both calling modules.

2) Optimization of the incremental color adjustment algorithm to prevent resolution limitations. This required slight mods to palManipulate, but seems to work better for palette shifts over long periods of time (i.e. very minute changes per tick).

This patch is against the following snapshots:

gfx.cpp - September 9 (v 1.15) scumm.h - September 10 (v 1.13) script_v1.cpp - September 9 (v 1.8) script_v2.cpp - September 7 (v 1.7)

These were all the latest snapshots at the time of this post.

comment:4 by SF/trinity78, 22 years ago

just wanted to say, that the problem with the Win32 release build and the page faults/crashing is a general problem and you're not the only one who has this problem. i have it too and most likely every other msvc++ user. i use win2k adv. server and msvc++ 6.0a.

btw.: Great job with the patch! *thumbs up* :)

comment:5 by fingolfin, 22 years ago

Looks great, now in CVS. Thanks!

comment:6 by fingolfin, 22 years ago

Owner: set to fingolfin
Status: newclosed

comment:7 by SF/trinity78, 22 years ago

i've noticed a bug related to this patch! if you load a savegame while the palManipulation takes place, the screen of the loaded savegame is messed up with red colours!

comment:8 by SF/trinity78, 22 years ago

i've uploaded a screenshot here:

http://domfree.de/trin/uploads/indy.png

on further examinations, i've noticed that it only happens, if you loaded a save game at a particular time (somewhere in the middle of the palManip).

comment:9 by SF/jamieson630, 22 years ago

Loadgame palette problem verified. In addition, a game saved while a palManipulate is in progress does not properly complete the palManipulate on load (indeed, as far as I could tell, even the palette changes effected so far do not get restored). I wonder if one or more palManipulate-related variables or buffers are not getting saved/restored/reset properly.

Off we go to do some more poking....

comment:10 by SF/jamieson630, 22 years ago

Status: closednew

comment:11 by SF/jamieson630, 22 years ago

Okay, it turns out that NONE of the palManipulate information is being saved or loaded. The variables are not serialized, and rtTemp buffers are apparently never saved or loaded. (They're supposed to be temporary, right?)

I'm moving the code away from rtTemp buffers and creating a new pointer similar to _shadowPalette. While I'm at it, I'm making sure this new pointer is appropriately initialized and free'd at all relevant locations. (This seems to be an oversight with other pointers, such as _shadowPalette, and may be causing memory leaks.)

The patch for this problem WILL end up breaking previous savegames. I guess there are several other mods the developers want to make to the serialization code whenever the format changes, as noted in various places in saveload.cpp.

Hopefully I will have a patch completely tested for upload by the end of the day. I have to go back through FOA to rebuild my relevant savegames.

by SF/jamieson630, 22 years ago

Attachment: savepalmanipulate.diff added

Patch adds palManipulate data save/load

comment:12 by SF/jamieson630, 22 years ago

The included patch savepalmanipulate.diff allows palette manipulations in progress to be saved and loaded, thus correcting the following problems:

1) When loading a game that was saved during a palette shift, the shift does not continue to completion.

2) When loading a game while the current game is in the middle of a palette shift, the palette for the newly loaded game may get corrupted.

This patch does the following:

1) It removes use of rtTemp resources to store palManipulate information, since such resources are never loaded or saved. Instead, all information is stored in new variables created in the Scumm class.

2) It adds serialization logic to include all necessary palManipulate information in savegames. (It uses a "smart" method whereby it does not save the largest chunks of information if there is no palette shift in progress.)

I tested this patch with FOA Room 23 (the Big Machine) and Room 96 (the extro). I loaded games while palManipulate was in progress, loaded games I had saved with palManipulate in progress, and "ping-pong'd" between two games where palManipulate was doing different stuff in each. All of the jumps from one game to another seemed to work with no palette corruption and an appropriate resumption of palette transitions.

THIS PATCH IS NOT COMPATIBLE WITH GAMES SAVED USING CVS VERSIONS PRIOR TO THAT IN WHICH THIS PATCH IS APPLIED.

To provide support for testing of this patch, I will attach a couple of savegames generated with the new palManipulate data. One will be just before turning on the Big Machine (I've already aligned the spheres, just push the spindle), and one will be moments before the start of the extro.

This patch was made against the following snapshots:

scumm.h - v 1.15 - September 13 resource_v2.cpp - v 1.2 - September 10* resource_v2.cpp - v 1.2 - September 10 saveload.cpp - v 1.8 - August 31 gfx.cpp - v 1.16 - September 13

These were the most recent snapshots at the time of this post.

by SF/jamieson630, 22 years ago

Attachment: atlantis.s05 added

FOA Big Machine - uses new save/load

by SF/jamieson630, 22 years ago

Attachment: atlantis.s06 added

FOA Extro - uses new save/load

by SF/jamieson630, 22 years ago

Attachment: savepalmanipulate.2.diff added

comment:13 by SF/jamieson630, 22 years ago

Use the latest savepalmanipulate.diff file. The other one had a problem in that it was trying to set the palManipulateInit function declaration back to include that unused parameter. Gotta keep things straight on my end!

Also the footnote I forgot to include: resource_v2.cpp does not include a CVS build version and date in the opening comments.

comment:14 by fingolfin, 22 years ago

Status: newclosed

comment:15 by fingolfin, 22 years ago

Please please please, do *not* add patches to already closed items! This is a new bug, and a new patch for it, so open a seperate patch and/ or bug tracker item for it, rather than reusing this old one.

Also note: patches that break save game compatibility are of course much less likely to get accepted. Endy will have to review it, and before he accepts it, Endy will have to ask us other developers if there are changes we need to get in.

comment:16 by SF/jamieson630, 22 years ago

Sorry about the mixup. I've created a new patch (609334) to address palManipulate load/save issues. This version of the patch will NOT break savegames, but also does not restore palette shifts that were in progress when a game was saved.

It does, hoever, address Trinity's problem of getting corrupted palettes when a game load interrupts a palette transition.

P.S. Frankly, I was indeed surprised to find that SourceForge would let me modify post parameters even after the patch had been closed. But being the incorrigibly curious person that I am, I figured, "Well, might as well give it a try." ;-)

comment:17 by digitall, 6 years ago

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