Opened 10 years ago

Last modified 4 months ago

#9013 new patch

Enforce aspect ratio in fullscreen SDL

Reported by: tramboi Owned by:
Priority: low Component: Ports
Keywords: Cc:


This patch adds a new "desired_aspect_ratio" option, which can be set to "auto"(default), "4/3", "16/9" and "16/10".
When not "auto", a switch to fullscreen in the SDL backend chooses the tightest resolution available that respects the ratio, if possible.
The rendering takes place in the upper left corner, without stretching (but the software scaling is taken into account).
A next version will render the surface centered, but it was "riskier", so I chose to submit this intermediate version!

Ticket imported from: #2784961. Ticket imported from: patches/1118.

Attachments (1)

0001-Option-desired_screen_aspect_ratio-for-fullscreen.patch (6.2 KB) - added by tramboi 10 years ago.
git svn rebased against rev 43126

Download all attachments as: .zip

Change History (16)

comment:1 Changed 10 years ago by sev-

Owner: set to fingolfin

comment:2 Changed 10 years ago by sev-

Looks like a nice idea to me. Max?

comment:3 Changed 10 years ago by fingolfin

I'll get to it, but there are half a dozen patches or so right now I want to review. I'll try to find time for this tonight or tomorrow.

comment:4 Changed 10 years ago by tramboi

Don't worry about it, it's probably the least urgent patch ever written!

comment:5 Changed 10 years ago by sev-

What is the status of this item?

comment:6 Changed 10 years ago by tramboi

I rebase it from time to time till it get reviewed

comment:7 Changed 10 years ago by fingolfin

Overall the patch looks OK to me. There are some issues that should be easy to address:

* The config keyys "desired_aspect_ratio" and "aspect_ratio" are easy to confuse and difficult to distinguish, from their name only. I guess "aspect_ratio" is badly named to start with (we should have called it "aspect_ratio_correction").
But we are more or less stuck with it... Still, I think something like "desired_screen_aspect_ratio" might make it slightly clearer what the new key is about

* Replace the AspectRatio enum by a struct:
struct AspectRatio { byte x; byte y; };
(well, maybe rename x to hori and y to vert ? or something else whatever). Replace "auto" by "x*y==0" or so.
Then, adapt fixupResolutionForAspectRatio accordingly (it'll just become a lot shorter).

Finally adapt retrieveDesiredAspectRatioFromConfig() by either parsing arbitrary strings of the form "N/M"; or if that seems like too much trouble, just let it match the four strings we support right now, and worry about generalizations later on.

Other than that, I see nothing that prevents this from being added. Sorry for taking so long to review this. Sadly, I have little time left for ScummVM these days.

comment:8 Changed 10 years ago by tramboi

I renamed "desired_aspect_ratio" into "desired_screen_aspect_ratio" as asked but I didn't convert the AspectRatio structure from an enum to a generic pair yet...
Just rebased the patch and fixed the conflicts so that it doesn't rot, until I can test on a widescreen again.

Changed 10 years ago by tramboi

git svn rebased against rev 43126

comment:9 Changed 10 years ago by tramboi

Here it goes... the main recommendations are done and it is functional.
(I left "auto" because it makes more sense from a user POV)

comment:10 Changed 10 years ago by fingolfin

Looks good. If somebody else can give it some testing, and if the following remarks are taken acount, I'll be happy to see this in trunk:

* I'd replace "int *width, int *height" by "int &width, int &height" (and get rid of the related asserts ;).

* I am also puzzled by
+ int kx =;
+ int ky =;
-> why kw and kh resp kx and ky ; and why is kx set to kw (i.e. why aren't both named the same?). I am confused :). I'd use a consistent name -- so either kx, or kw, or something more meaningful.

* retrieveDesiredAspectRatioFromConfig -> getDesiredAspectRatio (for consistency with other code)

* add a TODO comment to retrieveDesiredAspectRatioFromConfig stating that we could add support for arbitrary rations in the future?

* Finally, we should decide whether we treat this as a secret undocumented feature (and keep it as is), or whether we add it to the README. In the latter case, somebody needs to write down the specs.

comment:11 Changed 10 years ago by tramboi

I did the few fixes and commited it to the trunk.
Next step toward elegance would be centering the picture in black borders (with paying attention to screenshakes) :)
Concerning the documentation, if you think this shouldn't be a ultrasecret option, just tell me all the places I should document the option and I'll gladly do it :) (didn't find this info on the wiki)

comment:12 Changed 10 years ago by fingolfin

The README and the usage string in base/commandline.cpp (i.e. what is output by "./scummvm --help") are the two primary places to update. Then there is the Wiki manual, I guess.

There is no wiki page that describes this. Primarily, I guess, since so far the page would have consisted of that single sentence, and so its easier to just say that than to give out an equally long Wiki URL... Also, the README and usage strings are obvious, so... :)

comment:13 Changed 9 years ago by lordhoto

What is the status of this item?

comment:14 Changed 4 months ago by digitall

Owner: fingolfin deleted

comment:15 Changed 4 months ago by digitall

Component: Ports
Note: See TracTickets for help on using tickets.