Opened 13 years ago

Closed 17 months ago

#7559 closed enhancement (fixed)

BASS: Don't limit the screen height to 192 pixels

Reported by: jvprat Owned by: bonki
Priority: normal Component: Engine: Sky
Keywords: has-pull-request, original Cc:
Game: Beneath a Steel Sky

Description

It seems the whole game shows just 192 pixels of height, leaving a 8 pixels black bar at the bottom.

eriktorbjorn says it would be relatively easy to fix the intro, where there are some cut images (it's easy to notice in the images when joey is shot, the pink border is cut at the bottom).

It would also be nice to know if the game data, for example the backgrounds, can fill the 200 pixels and it can be enhanced or not.

Sometimes, when there are exits in the bottom of the screen, the mouse cursor can get to the border of the screen (over the black bar), but the text that follows the cursor is just cut at 192 pixels. It gives a strange feeling.

Thanks for your work!

Ticket imported from: #1689523. Ticket imported from: feature-requests/375.

Attachments (11)

sky-hack.diff (7.5 KB ) - added by eriktorbjorn 13 years ago.
Patch against current SVN
sky-hack2.diff (1.3 KB ) - added by jvprat 13 years ago.
A different approach
sky-hack3.diff (7.1 KB ) - added by eriktorbjorn 13 years ago.
Slightly simplified version of original approach
sky-hack3b.diff (8.0 KB ) - added by jvprat 13 years ago.
Small update over the 3rd patch
sky-hack4.diff (8.6 KB ) - added by jvprat 13 years ago.
Trying to show 2 more fullscreen images
sky-hack3-updated.diff (8.7 KB ) - added by eriktorbjorn 13 years ago.
Updated version of sky-hack3.diff
sky-hack3b-updated.diff (9.7 KB ) - added by jvprat 13 years ago.
Updated version of sky-hack3b.diff
sky-hack4-updated.diff (10.7 KB ) - added by jvprat 13 years ago.
Updated version of sky-hack4.diff
sky-hack4b.diff (10.1 KB ) - added by eriktorbjorn 13 years ago.
Updated version of sky-hack4-updated.diff
scummvm00009.png (220.2 KB ) - added by dafioram 2 years ago.
This issue is about there being a small black rectangle at the bottom of the screen. Attached is a picture showing this.
sky-hack4c.diff (10.7 KB ) - added by eriktorbjorn 18 months ago.
Updated patch

Download all attachments as: .zip

Change History (36)

comment:1 by eriktorbjorn, 13 years ago

To clarify slightly, this glitch appears to be present in the original as well.

It's very easy to get the intro to draw the bottom rows of the images that are displayed by Screen::showScreen(uint16 fileNum) (the SHOWSCREEN directive in the sequence data). I don't know if all the images displayed by that function are 200 pixels tall, but that can easily be checked by looking at the size of the data that has just been loaded.

The trick seems to be to get it to erase the bottom rows when they are no longer desired. The fix I had in mind when I said it was relatively easy leaves garbage during some of the "dissolve" effects. (Are these the ones started by the BGFLIRT directive?)

comment:2 by eriktorbjorn, 13 years ago

Owner: set to lavosspawn

by eriktorbjorn, 13 years ago

Attachment: sky-hack.diff added

Patch against current SVN

comment:3 by eriktorbjorn, 13 years ago

Not as simple as I had hoped, but I'm attaching an experimental patch that shows the "fullscreen" images in the cases where it doesn't cause animation glitches.
File Added: sky-hack.diff

by jvprat, 13 years ago

Attachment: sky-hack2.diff added

A different approach

comment:4 by jvprat, 13 years ago

After further examining (and also thanks to the patch) I've found that the problem comes from Screen::processSequence().

If I set GAME_SCREEN_HEIGHT to 200 and limit processSequence to work with just 192 rows, it works and shows the images at full screen. The attached patch shows this.

This leaves some dirty areas (which were manually cleared in eriktorbjorn's patch), but they're there only when the screen is updated by processSequence. The images that appear with some kind of animation, do it through this function, and it's currently limited (by my patch) to 192 rows, which means these images are still cut, and they can't cover the bottom of the screen.

I think the way to go is trying to do processSequence work with 200 rows. I've had no time to watch into this. Maybe this weekend.

Setting GAME_SCREEN_HEIGHT to 200 makes the game unplayable (it crashes when changing some scenes), so it will need more work to get a fully functional patch.
File Added: sky-hack2.diff

comment:5 by eriktorbjorn, 13 years ago

Yes, that's a simpler approach, of course. The problem I had when making all the intro backgrounds use the entire screen was that some of them interfered with the animation to some extent: The cars at the opening of the floppy intro, the shadow at the end of the helicopter crash, garbage left after some scene transitions, etc. (I marked such images with a comment in my patch, as you probably saw.) And as I recall it, there was one where the bottom part was just solid red.

That's why I opted for displaying the full image on a case-by-case basis.

comment:6 by lavosspawn, 13 years ago

Those sequences _are_ 320x192 pixels in size and there is no way to make them work with 200 lines, unless you draw the remaining images manually and patch them into the datafiles.
The games was simply made to run at that resolution and it does that in all areas of the game. Trying to make a few images show 8 more lines and thereby even causing graphical glitches feels rather pointless to me.

comment:7 by jvprat, 13 years ago

After carefully reading the code I've also realized that the sequences depend on the game data. I thought they might contain transition commands like "scroll down" on static images (in which case it could have been enhanced), instead of some kind of video. It's a pity we can't enjoy the extra 8 pixels of this game, even if some parts are there ;) In this case, the first patch is all we can hope.

I'll try to see if we can enhance at least some backgrounds in-game, or if it's really pointless.

Thanks for your comments.

comment:8 by eriktorbjorn, 13 years ago

> Trying to make a few images show 8 more lines and
> thereby even causing graphical glitches feels rather
> pointless to me.

That's why I limited my patch to the intro images that, as far as I could tell after repeated viewings of both intros, could be shown without causing any glitches. I deemed even minor glitches to be unacceptable.

Though the CLEARGFX directive turned out to be a bit of an overkill since I use it only to clear the same area of the screen, and only right between FADEDOWN and SHOWSCREEN. I'll submit an updated patch later tonight, probably.

by eriktorbjorn, 13 years ago

Attachment: sky-hack3.diff added

Slightly simplified version of original approach

comment:9 by eriktorbjorn, 13 years ago

It was possible to simplify it slightly, as I thought/hoped. New patch attached.
File Added: sky-hack3.diff

by jvprat, 13 years ago

Attachment: sky-hack3b.diff added

Small update over the 3rd patch

comment:10 by jvprat, 13 years ago

I'm attaching a slightly modified version of the 3rd patch with two changes:
- In some cases, CLEARSCREEN + SHOWSCREEN could be changed to SHOWSCREEN | FULLSCREEN (no need for an extra screen update).
- The cockpit scene can also be shown in fullscreen. The spanish subtitles in the floppy intro get lower than row 192 and it shows a strange effect if the image isn't shown in fullscreen.
File Added: sky-hack3b.diff

by jvprat, 13 years ago

Attachment: sky-hack4.diff added

Trying to show 2 more fullscreen images

comment:11 by jvprat, 13 years ago

Here comes another change.

I've managed to show 2 more fullscreen images (when the old man talks to Reich and Foster watching the security symbol) by mimicking the sequences by cleaning the bottom of the screen when needed, making the graphic glitches almost unnoticeable. Feel free to ignore this patch if you feel the result isn't acceptable.
File Added: sky-hack4.diff

comment:12 by eriktorbjorn, 13 years ago

> - In some cases, CLEARSCREEN + SHOWSCREEN could be changed
> to SHOWSCREEN | FULLSCREEN (no need for an extra screen update).

I thought of doing it that way, but it seemed counter-intuitive to flag background images as 'fullscreen' unless they actually used the bottom part of the screen.

> The spanish subtitles in the floppy intro get lower than row
> 192 and it shows a strange effect if the image isn't shown in
> fullscreen.

Interesting... Incidentally, the original draws spanish subtitles below the 192nd pixel too, but it's less noticeable there.

by eriktorbjorn, 13 years ago

Attachment: sky-hack3-updated.diff added

Updated version of sky-hack3.diff

comment:13 by eriktorbjorn, 13 years ago

Lavosspawn fixed the subtitle problem, accidentally breaking the intro patches in the process. I've updated my own latest patch, though. There's one slight change in that I now return true in the CLEARSCREEN case, instead of using break. That's just to be consistent with the surrounding code, and shouldn't make any difference.

File Added: sky-hack3-updated.diff

by jvprat, 13 years ago

Attachment: sky-hack3b-updated.diff added

Updated version of sky-hack3b.diff

comment:14 by jvprat, 13 years ago

Here come my updated patches

File Added: sky-hack3b-updated.diff

by jvprat, 13 years ago

Attachment: sky-hack4-updated.diff added

Updated version of sky-hack4.diff

comment:15 by jvprat, 13 years ago

File Added: sky-hack4-updated.diff

comment:16 by eriktorbjorn, 13 years ago

I don't think the helicopter cockpit has to be fullscreen any more, with lavosspawn's subtitle fix.

by eriktorbjorn, 13 years ago

Attachment: sky-hack4b.diff added

Updated version of sky-hack4-updated.diff

comment:17 by eriktorbjorn, 13 years ago

This is just an updated version of sky-hack4-updated.diff, with the "fullscreen" flag removed from the helicopter interior. As far as I can tell, it's not needed.

I've watched both the floppy and CD intros, and as far as I can tell the patch adds no new glitches to either of them. The way it's written, it shouldn't affect the rest of the game at all.

However, I don't want to make any decisions about it since I'm not the engine maintainer.
File Added: sky-hack4b.diff

comment:18 by fingolfin, 12 years ago

So... what shall we do with these patches, I wonder? Hmm...

comment:19 by eriktorbjorn, 12 years ago

The last thing I heard was that lavosspawn didn't seem very enthusiastic about them, though I don't remember why.

comment:20 by csnover, 2 years ago

Component: Engine: Sky
Game: Beneath a Steel Sky

by dafioram, 2 years ago

Attachment: scummvm00009.png added

This issue is about there being a small black rectangle at the bottom of the screen. Attached is a picture showing this.

comment:21 by bonki, 18 months ago

Eleven years later, how do you feel about this? Should we drop or merge this @eriktorbjorn?

by eriktorbjorn, 18 months ago

Attachment: sky-hack4c.diff added

Updated patch

comment:22 by eriktorbjorn, 18 months ago

I still don't remember what the objections were, but I don't really mind either way. The patch only affects the game intro (mainly the CD intro), where some images are somewhat noticeably cut off. It shouldn't affect the rest of the game at all.

The patch doesn't apply cleanly any more, so I'm attaching an updated version. I've given it a quick test, and it seems to work.

comment:23 by bonki, 18 months ago

Thanks for your quick reply. In its latest form this looks rather harmless to me. I cannot imagine that not displaying the images in full resolution was intentional so I vote for merging this.

I opened PR-1232 in your name and will merge this unless someone objects during the next couple of days.

comment:24 by bonki, 18 months ago

Keywords: has-pull-request original added

comment:25 by bonki, 17 months ago

Owner: changed from lavosspawn to bonki
Resolution: fixed
Status: newclosed

Merged in commit 0ffb057b66db7cb8319bec1c5e54067e6865c9d6.

Note: See TracTickets for help on using tickets.