Opened 7 years ago

Closed 6 years ago

#10196 closed feature request (fixed)

SCI: LSL7: Better cel scaling

Reported by: DanielSWolf Owned by: criezy
Priority: normal Component: Engine: SCI
Version: Keywords: sci32
Cc: csnover Game: Leisure Suit Larry 7

Description

This issue suggests an enhancement that I'd like to implement myself.

Leisure Suit Larry 7 has great graphics. Just about the only thing that has always bugged me about it is that some sprites (especially Larry) get upscaled when they appear too close in the foreground. See the attached screenshot for an example.

I believe it should be possible to enhance SCALER_Scale in celobj32.cpp to give better-looking results than the current nearest-neighbor implementation, while still staying in 8-bit color mode. I'm thinking of implementing something similar to AdvMAME2x/3x, but with non-integer scaling factors.

As a start, I'd like to implement this only for LSL7. All cels in this game use flat colors without anti-aliasing. This should give good results with this kind of scaler.

Before I blindly start coding, I have some questions:

  • Are there any reasons not to do this? E.g. fundamental concerns, similar ongoing initiatives, etc.
  • It is my understanding that cel scaling in SCI32 games has to happen in 8-bit color mode. However, I've seen some code comments by @csnover concerning USE_RGB_COLOR that sound as if that might change in the future. So I'd like to make sure I won't put effort into a special palette-based scaler that will soon be superfluous due to a move to true color.

Attachments (1)

scaling.png (294.2 KB ) - added by DanielSWolf 7 years ago.

Download all attachments as: .zip

Change History (19)

by DanielSWolf, 7 years ago

Attachment: scaling.png added

comment:1 by csnover, 7 years ago

There’s not currently any timetable for a true-colour mode beyond what already exists to enable higher-quality video interpolation, and I’m not aware of any other work in this area, so I don’t think your work would be superseded by a new RGB mode or by anyone else any time soon.

There are fewer caveats for the scalers in SCI3 games than in earlier SCI32 games. The games that used an internal script resolution of 320x200 instead of 640x480 (all SCI2 & 2.1early games, and some 2.1mid games) had specific requirements for how scaling needed to work in order for picture cels to line up correctly (see the comment in SCALER_Scale for details). It is possible that there are screen items in LSL7 or other SCI3 games that also require pixel-perfect alignment to avoid rendering errors, though I don’t know of any examples offhand where you are likely to see such a resource being scaled. Other than that I can’t think of any fundamental problems with using a scaler that works better than NN and doesn’t interpolate the transparency keycolor.

The only super important thing I can say is to make sure that the feature can be toggled off in the game options like the various other ScummVM enhancements, so we can offer faithful rendering for historical preservation reasons and so we can compare against screenshots from the original interpreter to verify renderer correctness. I am happy to help you learn these parts of ScummVM to implement such an option (it’s not hard at all, just a bit spread out).

It would also be preferable that this cel scaler doesn’t break any of the games when it’s enabled, including those earlier SCI2/2.1 games with special scaling, even if we decide not to allow it for some of them because it just makes them look horrible. But it’s not mandatory to make this work right away.

Looking forward to seeing what you come up with!

Last edited 7 years ago by csnover (previous) (diff)

comment:2 by DanielSWolf, 7 years ago

Thanks for your detailed answer -- that all makes perfect sense! I'll update this issue as soon as I have something to show (or run into trouble).

comment:3 by csnover, 7 years ago

Keywords: sci32 added

comment:4 by DanielSWolf, 7 years ago

Update: I'm working on a scaling algorithm I'm naming "LarryScale". I've written a proof-of-concept program, and downscaling is working very nicely already. See the following image (which is zoomed to 200%): Downscaling with Nearest Neighbor (left) leads to lots of interrupted lines, while LarryScale (right) manages to preserve the details much better without adding more colors.

http://dannad.de/xchg/permanent/misc/demo.png

I'm now working on the upscaling part. This is proving to be more challenging. I'll update this ticket as I make progress.

Last edited 7 years ago by DanielSWolf (previous) (diff)

comment:5 by csnover, 7 years ago

The new downscaling looks fantastic. Great work so far. Looking forward to your future updates!

comment:6 by DanielSWolf, 7 years ago

Thanks! :-) It will probably be some time until the next update, because getting the upscaling to look right is pretty challenging and I have rather limited time.

comment:7 by DanielSWolf, 7 years ago

As expected, upscaling proved to be much harder than downscaling. The tricky bit is not to write a good 2x scaler (there are a many good 2x scalers), but to write a scaler that gives consistently good results at any scaling factor between 100% and 200%.

It took some tweaking, but I've finally arrived at a version I like:

http://www.dannad.de/xchg/permanent/misc/larry-upscaling.png

So now, I have working POC code for up- and downscaling. Next, I'll need to clean up the code and optimize it for speed.

comment:8 by csnover, 7 years ago

These looks really fantastic! Excellent work on these scalers. Looking forward to the PR! Please let me know if you need any advice on integration.

Last edited 7 years ago by csnover (previous) (diff)

comment:9 by DanielSWolf, 7 years ago

Thanks -- I'm glad you like the results! :-)

I managed to considerably reduce average downscaling and upscaling times per sprite. On my (rather old) desktop machine, they are now at 0.1 ms and 0.25 ms, respectively, and I'm very happy with these times.

I also managed to integrate my scaler with the SCI engine. Due to the nature of a smoothing scaler, the results don't align with the low-res raster of older SCI games, but I don't think there's a way to fix that.

The last thing I'm struggling with is adding an option to enable/disable LarryScale. I modified common/gui_options.h, common/gui_options.cpp, engines/sci/sci.h, engines/sci/detection.cpp, and engines/sci/detection_tables.h, but the new option still won't show up. Now I understand what you meant by "a bit spread out." ;-)

I'd appreciate any pointers!

comment:10 by csnover, 7 years ago

It sounds to me like you hit all the right files. For some reason that I never bothered to figure out, the game-specific options list in the GUI won’t change until you start and quit the game, or until you remove and re-add the game. If you do either of those things, does the option show up?

If you are still having trouble after this and have a fork with the changes or a patch file you can upload here, let me know and I can look specifically for anything that’s missing.

comment:11 by DanielSWolf, 7 years ago

What do you know -- it works! :-) Thanks a lot!

I'll have a PR ready soon.

comment:12 by bonki, 7 years ago

Just wanted to chime in and also congratulate you on these results, they look fantastic! Looking forward to seeing your PR. :-)

comment:13 by DanielSWolf, 7 years ago

Thanks, @bonki! :-)

Here's the promised PR: https://github.com/scummvm/scummvm/pull/1141

comment:14 by DanielSWolf, 7 years ago

It's been four weeks since I created the pull request, and I'm eager to get a code review and have my code merged. Is there anything I can do to speed up the process?

comment:15 by csnover, 7 years ago

I am no longer working on ScummVM so I cannot do anything in this regard. It is now up to the remaining people to decide to do code reviews. Opening the PR as you have is the signal for someone to do this. Your work looks amazing, so I hope someone pays attention.

comment:16 by DanielSWolf, 7 years ago

I'm sad to hear you've left ScummVM. Thank you very much for your encouragement and your help with this issue!

comment:17 by DanielSWolf, 6 years ago

I believe this issue can be closed now. The PR was merged a month ago: https://github.com/scummvm/scummvm/pull/1141

comment:18 by criezy, 6 years ago

Owner: set to criezy
Resolution: fixed
Status: newclosed

Indeed. I forgot there was this ticket in our tracker.

Note: See TracTickets for help on using tickets.