QFG1VGA - picture format problem?

[ZvikaZ]

Another problem with QFG1VGA.

I have just exported a single picture as a patch (300.p56), without changing anything, and it fails.
On ScummVM:

Code: [Select]

Unsupported pic-operation 0!On DOSBox it just gets stuck.

Am I missing (again...) some other file that I should export as well?
Or is it another strange format used by QFG1VGA, and not supported (yet  ) by SciCompanion?

[Kawa]

Indeed. For a background picture, all you'd need is the .p56 resource, much like with a view. It's the scripts that eventually got funky.

The strangest thing is, I just went through the effort to remake the picture from scratch (export bitmap, create new, import bitmap, remake priority and control screens by hand) and that also crashed the game. And a totally blank picture, with no commands in it at all? Also crashes the game. Even a blank made in Sierra's own picture editor (start, save, exit) does.

I'm starting to think it's not a format thing.

[ZvikaZ]

It seems to me to be something with SCICompanion picture export.
I've tried to take an arbitrary .p56 file from SQ5 (81.P56), and copy it over to QFG1VGA, and renamed it to 300.p56.
It loads successfully. (looks ugly, probably because of wrong color pallete, but it doesn't matter...)

Also, before doing that, I've run ScummVM in debug mode, and traced the problem with the .p56 file to this code at picture.cpp:

Code: [Select]

void GfxPicture::draw(bool mirroredFlag, bool addToFlag, int16 EGApaletteNo) {
...
headerSize = _resource->getUint16LEAt(0);
switch (headerSize) {
case 0x26: // SCI 1.1 VGA picture
_resourceType = SCI_PICTURE_TYPE_SCI11;
drawSci11Vga();
break;
default:
// VGA, EGA or Amiga vector data
_resourceType = SCI_PICTURE_TYPE_REGULAR;
drawVectorData(*_resource);
}

It expects for SCI1.1 games to get the byte 0x26, but it got 0x00, therefore it treated it as vector data, received illegal vector command, and failed.
With the "fake" file from SQ5, it indeed got 0x26.

In hex editor I can see 0x26 at the problematic file, but earlier than its location on SQ5 file.

SQ5 file:

Code: [Select]

00000000  81 80 00 00 40 01 C8 00 05 00 06 00 00 01 00 00  .€..@.ָ.........
00000010  00 00 02 00 CA FF 2F 00 00 00 26 00 10 0E 01 00  ....ֺ/...&.....

Problematic file:

Code: [Select]

00000000  81 81 00 00 26 00 10 0E 01 00 A0 00 F0 D8 46 00  ....&..... .נ״F.
00000010  83 01 00 00 8C BD 00 00 00 00 00 00 00 00 00 00  ƒ....½..........

So, is there some header missing maybe?

[Kawa]

It's funny.

If I force SCI Companion to assume the pictures are in SCI1 format, they end up displayed as blanks, because they're not that format. Set the version detection to 1.1, and they appear fine. Same with SV's picture viewer. So mark that one down.

Looking at any random QFG1 picture's raw bytes in SV set to hex view, they all start with 0x26, marking them as SCI11 VGA, as per your code snippet, including #300.

Looking at my remade 300.p56 as a loose resource file, in a plain hex editor, I see this:

Code: [Select]

00000000: 81 81 00 00 26 00 10 0E 01 00 A0 00 F0 D8 46 00
00000010: 9E 01 00 00 87 BE 00 00 00 00 00 00 00 00 00 00The 0x26 appears at offset 0x04.
An SV-exported, otherwise clean 300.p56 has this:

Code: [Select]

00000000: 81 80 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00000010: 00 00 00 00 00 00 00 00 00 00 26 00 10 0E 01 00Note that the 0x26 doesn't appear until offset 0x1A.

I've always found the loose resources to be a little odd in that supposedly, the big difference between a loose resource and a packed one is that the loose one has a type identifier at the start, which is only two bytes. The header size is supposedly at 0x00, as per your ScummVM snippet, but much like with views it does not actually start at that point in the file, unlike for example vocabs, text, messages... Both of these are SCI11 pictures, too, so why do neither of them have the 0x26 where you'd expect it to be?

Y'know what actually fixed it and let me have a 300.p56 (the SV-exported one) without hanging? Changing the 81 80 at the start to 81 81.

But what's particularly interesting is that SV-exporting 84.p56 and renaming that to 300.p56 works fine without further trouble. For reference, it has 81 80 and the 0x26 byte is at offset 0x1A. Just like the hanging SV-exported 300.p56 has. Weird, huh?

[Kawa]

Taking a clean picture 300 and using SCI Companion to slightly mess up the palette, then exporting that to 300.p56 again had the game hang. This file started with 81 81 00 00 26. So I took the header (81 80, 0x26 at offset 0x1A) from 84.p56 and put it in 300.p56 with the altered palette... and guess what?

[ZvikaZ]

So, what's the conclusion?

If I understand correctly, the header that SciCompanion creates to the .p56 patch should be changed for QFG1VGA?

I've created a tiny script to modify the header, according to your experiments: https://github.com/adventurebrew/HebrewAdventure/blob/master/tools/p56_fix.py
and it indeed worked well for 2 pictures that I've checked.

I debugged it a little in ScummVM, in order to try to better understand the situation, and how the patches headers are being treated.
Got to resource.cpp

Code: [Select]

void ResourceManager::processPatch(ResourceSource *source, ResourceType resourceType, uint16 resourceNr, uint32 tuple) {
...
case kResourceTypePic:
if (_volVersion < kResVersionSci2) {
fileStream->seek(3, SEEK_SET);
patchDataOffset += fileStream->readByte() + kViewHeaderSize + kExtraHeaderSize;
} else {
patchDataOffset += kExtraHeaderSize;
}
break;
patchDataOffset was 2 before that `if` was executed.
_volVersion is 5 (= kResVersionSci11), therefore the `if` is true.
It reads 0x00 from the file, kViewHeaderSize  is 0x00000016, kExtraHeaderSize is 0x00000002
Therefore, after the `if`, patchDataOffset is 0x1A [which is where the expected 0x26 resides in our modified good .p56 resources]

It seems that the .p56 created by SciCompanion match that `if` being false, which would lead to patchDataOffset being 0x4.

[Kawa]

But that if you posted is to tell SCI16 and SCI32 data apart. For any SCI16 game (SCI0, 01, 1, 11) the if would be true. It's SCI2, 21, and 3 games whose pictures always start at the same offset.

[ZvikaZ]

OK.
Anyway, where does it stand in your opinion?

Do you think that SciCompanion should be fixed?
(I think so...)

[Kawa]

Of course it should be fixed. I'm just not sure what's going on.

[doomlazer]

Sorry to necro this old thread, but I've run into this problem exporting .p56 pics as patch files again recently. Looking At the SCICompanion source I think I've tracked things back to some relevant code in ResourceBlob.cpp:

Code: [Select]

{
if (fNoHeader)
{
// No resource header - this is in its own file.  It is just an indicator of what type it is.
uint16_t w = 0x80 | (int)header.Type;
// For SCI1.1 and above, views and pics appear to have 0x8? as the second byte. The high bit is a
// marker that extra data is there, and 0x00 indicates 24 bytes of extra data.
uint8_t secondByte = 0x00;
if (header.Version.PackageFormat >= ResourcePackageFormat::SCI11)
{
if (header.Type == ResourceType::View)
{
secondByte = 0x80;
}
else if (header.Type == ResourceType::Pic)
{
secondByte = 0x81;
}
}
w |= ((uint16_t)secondByte) << 8;
fWrote = WriteFile(hFile, &w, sizeof(w), &cbWrittenHeader, nullptr) && (cbWrittenHeader == sizeof(w));

uint32_t sizeOfExtraStuff = GetResourceOffsetInFile(secondByte);
if (fWrote && sizeOfExtraStuff)
{
std::vector<uint8_t> emptyData(sizeOfExtraStuff);
fWrote = WriteFile(hFile, &emptyData[0], emptyData.size(), &cbWrittenHeader, nullptr) && (cbWrittenHeader == emptyData.size());
}

// When we're writing a resource to a separate file, we don't have a header, so we can't have any compression methods.
fWriteCompressedIfPossible = FALSE;
}
else
{
if (header.Version.MapFormat > ResourceMapFormat::SCI0_LayoutSCI1)
{
fWrote = _WriteHeaderToFile<RESOURCEHEADER_SCI1>(hFile, header, &cbWrittenHeader);
}
else
{
fWrote = _WriteHeaderToFile<RESOURCEHEADER_SCI0>(hFile, header, &cbWrittenHeader);
}
fWriteCompressedIfPossible = TRUE;
}
}

I'm wondering if code needs to be added for SCI1 pic patches that have a header to add the "empty data", like it seems to do when (fNoHeader) returns true. Maybe code can be added to move 0x26 to offset 0x1A?

I'll see if I can figure out something on my own, but I figured I'd post here in case anyone can see an obvious solution.

[doomlazer]

Well, changing the second byte for pic resources to also be 0x80 exports working .p56 patch files. It adds all the padding to move 0x26 back to offset 0x1A. Now, is this the right solution or is it introducing additional bugs? I have no clue.

Edit: After reviewing all the info, this change appears to be correct.

[Kawa]

Tested it myself and found it working. Thanks, your changes are on the git repo now.