Opened 10 years ago

Last modified 12 months ago

#9013 new patch

Enforce aspect ratio in fullscreen SDL

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

Description

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 by sev-, 10 years ago

Owner: set to fingolfin

comment:2 by sev-, 10 years ago

Looks like a nice idea to me. Max?

comment:3 by fingolfin, 10 years ago

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 by tramboi, 10 years ago

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

comment:5 by sev-, 10 years ago

What is the status of this item?

comment:6 by tramboi, 10 years ago

I rebase it from time to time till it get reviewed

comment:7 by fingolfin, 10 years ago

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 by tramboi, 10 years ago

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.

by tramboi, 10 years ago

git svn rebased against rev 43126

comment:9 by tramboi, 10 years ago

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 by fingolfin, 10 years ago

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 = desiredAspectRatio.kw();
+ int ky = desiredAspectRatio.kh();
-> 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 by tramboi, 10 years ago

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 by fingolfin, 10 years ago

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 by lordhoto, 9 years ago

What is the status of this item?

comment:14 by digitall, 12 months ago

Owner: fingolfin removed

comment:15 by digitall, 12 months ago

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