#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)
Change History (23)
by , 22 years ago
| Attachment: | gfx.cpp.diff added |
|---|
comment:1 by , 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 , 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 , 22 years ago
| Attachment: | palmanipulateinit.diff added |
|---|
Patch against 4 files from September 10 snapshot
comment:3 by , 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 , 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:6 by , 22 years ago
| Owner: | set to |
|---|---|
| Status: | new → closed |
comment:7 by , 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 , 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 , 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 , 22 years ago
| Status: | closed → new |
|---|
comment:11 by , 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.
comment:12 by , 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 , 22 years ago
| Attachment: | savepalmanipulate.2.diff added |
|---|
comment:13 by , 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 , 22 years ago
| Status: | new → closed |
|---|
comment:15 by , 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 , 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 , 6 years ago
| Component: | → Engine: SCUMM |
|---|
palManipulateInit implementation