Broken Sword 1 & 2 pretends to be 640x480 but it is never initialized / updated as such

[mikrosk]

The core bug is that the game is *not* 480 rows tall, it is always 400. But the engine for some reason initializes it to 480, since its first commit (oversight?)

Then however, it is never initialized like that. So the first copyRectToScreen() I see is: 0, 40, 640, 400, i.e. it correctly updates the active part (good) but it can't be bothered to at least clear the top and bottom bars.

Another annoyance is that on an 8-bit palette-based screen the top/bottom bars mimic the black colour so if there is a fading happening (like in the beginning of the game), the bars change color as well.

So an easy fix would be to at least fix the bars init, the proper fix would be to change all the hardcoded constants to respect the game's original height and that is 400 rows.

[GermanTribun]

Ehm, what you are saying there is complete nonsense. Both games ARE 640x480. The black bars at the top and bottom are for the inventory and the dialogue options (and in part two for the system icons), which fade in when used. The game designers simply decided for whatever reason to even keep these areas black during menues and cutscenes. Perhaps for the sake of consistency?

[mikrosk]

Fair enough. But still the top and bottom bars should be initialized, don't you think? Now I can run, say, The Curse of Monkey Island, go back to the launcher and start BS1/BS2 and see the remains of the CMI's graphics.

Sure, the SDL backend doesn't suffer from this bug but if there is only one shared screen surface which is not reinitialized every time, the bars are left as they are... I hacked it in my code (to zero the maximum possible shared screen area) but so far I have to do this only for BS1/BS2.

[GermanTribun]

The weird thing is, I don't have that problem, the games run as they should on ScummVM. Is there anything special with your setup that could cause that problem?

[mikrosk]

As I said, it's not visible on the main backed (which is SDL I presume). It is easily observable if you alloc just one screen surface, say, 640x480 (which can be shared for all 320x200, 320x240, 640x400 and 640x480 games in 8bpp).

CMI will happily blit 640x480 images there.

Then switch back to the overlay (different screen surface, so the game surface is not touched).

Run BS1. Screen surface is blit to {0, 40, 640, 400} and the leftovers from CMI are visible. So now I have to implement a workaround in my code so that the screen surface must be blit/zeroed even if there is no dirty rectangle to process.

[GermanTribun]

That sounds like having to do something really specific in order to get that problem, which about 99% of the players never do, since they most likely will never use anything else than SDL Surface (the default). That perhaps is also why the team didn't find it, as the path to find it sounds really obscure.

[mikrosk]

Up to you of course, I can live with my workaround.

The idea is to contribute my backend into the main tree, though. So it will be a valid bug for that specific backend. :)

[AndywinXp]

Hi mikrosk! Thanks for your ticket, could you tell us something more about your specific setup (and in particular which backend is affected?)

Please GermanTribun, don't speak on behalf of the team. There are a moltitude of backends, and we need to be sure that the games performs correctly on each of them.

We really appreciate contributions, so we look forward to it!

[GermanTribun]

I actually never intended to speak for the team. Sorry that it happened by accident (did't even notice).

[mikrosk]

The backed is a new one, it aims to replace (or at least to supplement) the Atari/FreeMiNT package (which is too big, too slow and AFAIK broken) with something more low-level (and faster).

So one of the goals is to avoid unnecessary redraws, buffers in-between etc. This is how I noticed this bug, I presume SDL always initializes its screen surfaces but this is not the case of my backend (which just reuses the same shared memory buffer).

As I understand it, the "contract" is that what is not updated via copyRectTosScreen() doesn't need a redraw. And since BS1/BS2 doesn't do a full (640x480) redraw in the beginning, there are the aforementioned glitches present.

It's possible some other backend is also affected but it must be one of the more 'obscure' ones, which took a similar path as I did.

[lephilousophe]

I would say that, even if it's not documented, initSize call clears the screen.
So the only thing you have to do is to zero out your buffer at the end of the initSize call in your backend.

[mikrosk]

That's exactly what I'm doing. :) (but I hadn't before)

Btw there's another interesting situation when it comes to initSize() (and friends, like setGraphicsMode)...

ScummVM starts by default in 320x200.
Then the overlay is shown (presumably in the same resolution but not necessarily).
Then one starts Broken Sword in 640x480.

What if the transaction fails? The overlay is activated, shows the error message but there is no further attempt to correct / rollback the situation, i.e. the backed is, in the best case, in its original resolution 320x200 but the game starts anyway.

I haven't found any recommendation how to handle this - and it does create errors, for instance I got copyRectToScreen() with some ridiculous X, Y. It seemed like some overflow happened in BS1 (when called getWidth() / getHeight() and subtracted 640 / 480 from this number and casted it as unsigned).

So my (yet another) workaround was to disable copyRectToScreen() if previous transaction end failed.

[lephilousophe]

If size fails, initGraphics checks the result of endGFXTransaction which reports failure.
Then, it checks the result and if the flag kTransactionSizeChangeFailed is set, it errors out with the message "Could not switch to resolution '%dx%d'.".

error() must abort the program and never return (call to exit(1) at the end).

What have been described so far is not a bug in sword1 so I am closing the ticket.
If you need help to develop your backend please come on Discord or IRC to ask your questions. Bugs are not meant for support.

[mikrosk]

Oh, that's interesting. Perhaps I'm missing something in my backend then, thank you for clarification.

And indeed, this is out of scope of BS1. I'll use Discord in the future, thanks again.

EDIT: just for the sake of completion: I was returning kTransactionModeSwitchFailed as the primary error code but the engine checks only for kTransactionSizeChangeFailed otherwise it continues the way I described earlier.