Skip to content
Permalink
Browse files

SCI: Fix SQ4CD hintbook text layout

Fixes #11070
  • Loading branch information...
sluicebox authored and bluegr committed Aug 12, 2019
1 parent 8d648b5 commit 0069281c0901d194035c937d221a1386d1fe8386
Showing with 8 additions and 3 deletions.
  1. +8 −3 engines/sci/engine/message.cpp
@@ -453,9 +453,14 @@ Common::String MessageState::processString(const char *s, uint32 maxLength) {
uint index = 0;

while (index < inStr.size() && index < maxLength) {
// Check for hex escape sequence
if (stringHex(outStr, inStr, index))
continue;
// Check for hex escape sequence.
// SQ4CD predates this interpreter feature but has a message on the
// hintbook screen which appears to contain hex strings and renders
// incorrectly if converted, so exclude it. Fixes #11070
if (g_sci->getGameId() != GID_SQ4) {

This comment has been minimized.

Copy link
@m-kiewitz

m-kiewitz Aug 12, 2019

Contributor

Sorry to disturb, but I don't like this at all.
In my opinion we should figure out when exactly that feature was added and then add explicit checks for that point.
It seems this feature was at least used for foreign Space Quest 5 versions.
Maybe it was only added for foreign SCI1.1 games.
Will look into this, if you want me to.

if (stringHex(outStr, inStr, index))
continue;
}

// Check for literal escape sequence
if (stringLit(outStr, inStr, index))

4 comments on commit 0069281

@Kawa-oneechan

This comment has been minimized.

Copy link
Contributor

replied Aug 12, 2019

Personally I'd detect not just that it's SQ4CD but that it's that specific line and sneak in a replacement, like how the "Choas" typo in Loom is done... if the message workaround system can't otherwise cover such a change.

@sluicebox

This comment has been minimized.

Copy link
Member Author

replied Aug 13, 2019

I'm glad we're talking about this one. I explored a couple paths and ended up writing it this way to get issue on everyone's radar, and bonus, to fix the three year bug. Missions accomplished! =)

I did a bit of research on this and came to the conclusion that this is the least worst thing to do, slightly, and I should have laid that out in the PR.

I would love to have a function that returns if a game has this feature and apply parsing based on that. But...

Unless I'm missing something, this feature can't be detected by examining a game's data files, as it's an isolated interpreter parsing feature with no corresponding scripts, so "detecting" this is going to end up being a hard coded list of all SCI 1.1 games that have this or a big list of SCI 1.1 games that don't. I think it would have to be the latter, otherwise this list would have to be maintained with fan SCI 1.1 games as they appear.

If anyone has a way to detect this feature's presence from resource files, even a heuristic, then that would be perfect, but I looked and am operating under the assumption that it can't be done.

I scanned my Sierra corpus and didn't find any other SCI 1.1 games that predate this interpreter feature with "\0" in a message resource. I don't have them all, but keep in mind that this parsing hex codes has no adverse effect unless you're an SCI 1.1 game whose interprter predates this feature AND you have a message resource that contains the unusual text "\0[A-Z]". That's a small subset and I think you're going to end up with just this hintbook screen.

The wiki lists games which games have this and don't, and it matches what I found by inspecting interpreters. (Love it when you just have to test for a string!)

With those constraints, the options are:

  • Make a (big) list of all SCI 1.1 games without parsing
  • Make a (smaller) list of all SCI 1.1 games with parsing and maintain with fan games
  • A small self-contained workaround for the only known message that's affected

I went with the third, which also has the benefit of being upgradable, and in the spirit of other workarounds in message.cpp. I don't have all the SCI 1.1 games so making a list probably means each of us chipping in our findings. BUT... it's easy to test if a game is affected, just search messages for "\0", and unless you find a game with that then all that work is still going to amount to fixing the hintbook screen.

What do you think?

@sluicebox

This comment has been minimized.

Copy link
Member Author

replied Aug 13, 2019

@Kawa-oneechan That's funny, I also considered doing a message workaround for this, since I'm predisposed to being fond of that table...

That would work in fixing this symptom, but it felt like it indirectly addressed the problem far away from the source. The message itself isn't buggy, and the comment would balloon into explaining how we're doing something wrong somewhere else, always a bad sign.

I should have said, I'm all for expanding this into something more comprehensive, and that's one of the reasons I wrote it in the simplest way I could come up with that accurately identified and fixed the immediate issue without regressions I wouldn't be able to test.

@Kawa-oneechan

This comment has been minimized.

Copy link
Contributor

replied Aug 13, 2019

That would work in fixing this symptom, but it felt like it indirectly addressed the problem far away from the source.

But then on the other hand it is a workaround of a sort, and it'd keep the message system and its collected workarounds nice and separate.

Unlike the Choas thing, which has tenure.

Please sign in to comment.
You can’t perform that action at this time.