SCI: (KQ6WinCD) - fix hires drawing glitches by athrxx · Pull Request #6319 · scummvm/scummvm

athrxx · 2024-12-16T16:12:40Z

This should fix issues 1- 3 from bug ticket no. 15594.

I have added redrawing functionality for the hires gfx from the original interpreter. I guess the original only needed that code to handle WM_PAINT messages (= full window redraw). It also does actually call a redraw in kDisposeWindow, the same way I implemented it here, to refresh the inventory. But that is only needed for the speech+text mode which isn't supported in the original. Maybe they ditched the mode only after they had already implemened that, who knows...

I have also improved the workaround for the out-of-bounds portraits to catch the case from the bug ticket.

(items 1- 3 from bug ticket no. 15594) I have added redrawing functionality for the hires gfx from the original interpreter. I guess the original only needed that code to handle WM_PAINT messages (= full window redraw). It also does actually call a redraw in kDisposeWindow, the same way I implemented it here, to refresh the inventory. But that is really only needed for the speech+text mode which isn't supported in the original. Maybe they ditched the mode only after they had already implemened that, who knows... I have also improved the workaround for the out-of-bounds portraits to catch the case from the bug ticket.

sluicebox · 2024-12-16T22:50:07Z

engines/sci/graphics/paint16.cpp

+	// window background when recieving WM_PAINT messages (which is irrelevant for us, since that happens in the backend) and the other is
+	// redrawing the inventory after displaying a text window over it. This only happens in mixed speech+text mode which does not even exist
+	// in the original. We do have that mode as a ScummVM feature, though. That's why we have that code, to be able to refresh the inventory.
+	// We also check if the portrait is drawn outside the viewport boundaries (happens in the inofficial mixed speech+text mode) and set


"unofficial"

sluicebox · 2024-12-16T22:51:44Z

engines/sci/graphics/paint16.cpp

-// This is used as replacement for drawCelAndShow() when hires-cels are drawn to
-// screen. Hires-cels are available only SCI 1.1+.
-void GfxPaint16::drawHiresCelAndShow(GuiResourceId viewId, int16 loopNo, int16 celNo, uint16 leftPos, uint16 topPos, byte priority, uint16 paletteNo, reg_t upscaledHiresHandle, uint16 scaleX, uint16 scaleY) {
+// This is used as replacement for drawCelAndShow() when hires-cels are drawn to screen. Hires-cels are available only SCI 1.1+.


"SCI 1.1+" => "SCI 1.1", since this is SCI16-only code. (I know it's an existing comment)

sluicebox · 2024-12-16T23:02:54Z

LGTM; I haven't tested this but it's well commented and, as always, I'm convinced you know what you're doing =)

Thanks for reverse engineering these edge cases! I'll let @bluegr take a look and merge it, so that I can blame him if it crashes later.

engines/sci/graphics/paint16.cpp

engines/sci/graphics/paint16.h

bluegr

LGTM! Nice work :)
Shouldn't we add a game ID check in all that hires SCI11 code? If no other games are affected, we can skip the check

athrxx · 2024-12-17T00:44:30Z

LGTM! Nice work :) Shouldn't we add a game ID check in all that hires SCI11 code? If no other games are affected, we can skip the check

Thanks! KQ6Win is the only game to reach the relevant code paths. No other game will call drawHiresCelAndShow(). And if that method is not called, the draw chain (_hiresDrawObjs) will remain a nullptr. Which means, the rest of the code will not be executed either. Unless I made a mistake somwhere...

Co-authored-by: Filippos Karapetis <bluegr@gmail.com>

sluicebox · 2024-12-17T03:27:41Z

LGTM! Nice work :) Shouldn't we add a game ID check in all that hires SCI11 code? If no other games are affected, we can skip the check

Thanks! KQ6Win is the only game to reach the relevant code paths. No other game will call drawHiresCelAndShow(). And if that method is not called, the draw chain (_hiresDrawObjs) will remain a nullptr. Which means, the rest of the code will not be executed either. Unless I made a mistake somwhere...

When I was reviewing I was going to something similar to @bluegr but then I looked close and saw that the linked list implementation meant a nullptr wasn't going to crash it. So while I don't think it needs to change, that's two people who this raised a question for. Although for me it was on the kDisposeWindow change because that gets called by so many other games. @athrxx it's up to you, but maybe a game/version check there for more the sake of clarity and documentation? I don't feel strongly about this, but when two of us do the same double take, maybe it could be clearer..

I added a check in kDisposeWindow. It isn't strictly necessary, but it it makes the code easier to understand. I also renamed a var that I had previously renamed in a way that makes no sense. I had renamed it from `upscaledHiresRect' to 'portraitRect' since it is neither upscaled nor hires (it is just a lowres rect). But it isn't a portrait rect either. The portraits get drawn elsewhere. It's more likely to be a picture frame, a gui control or an inventory item.

athrxx · 2024-12-17T13:45:03Z

I have added two sanity checks at locations which might not be so obvious (including kDisposeWindow ).

And I also renamed a var that I had renamed previously, because the name made no sense. But I managed to rename it to something that also made no sense 😅 .

bluegr · 2024-12-17T14:41:24Z

Looks good now, thanks! Squashing

sluicebox reviewed Dec 16, 2024

View reviewed changes