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)
Change History (19)
by , 7 years ago
Attachment: | scaling.png added |
---|
comment:2 by , 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 , 7 years ago
Keywords: | sci32 added |
---|
comment:4 by , 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.
I'm now working on the upscaling part. This is proving to be more challenging. I'll update this ticket as I make progress.
comment:5 by , 7 years ago
The new downscaling looks fantastic. Great work so far. Looking forward to your future updates!
comment:6 by , 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 , 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:
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 , 7 years ago
These looks really fantastic! Excellent work on these scalers. Looking forward to a PR with this scaler! Let me know if you need any advice on integration.
comment:9 by , 6 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 , 6 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 , 6 years ago
What do you know -- it works! :-) Thanks a lot!
I'll have a PR ready soon.
comment:12 by , 6 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 , 6 years ago
Thanks, @bonki! :-)
Here's the promised PR: https://github.com/scummvm/scummvm/pull/1141
comment:14 by , 6 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 , 6 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 , 6 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 , 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 , 6 years ago
Owner: | set to |
---|---|
Resolution: | → fixed |
Status: | new → closed |
Indeed. I forgot there was this ticket in our tracker.
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!