Opened 16 years ago
Closed 13 days ago
#9013 closed patch (fixed)
Enforce aspect ratio in fullscreen SDL
| Reported by: | tramboi | Owned by: | sev- |
|---|---|---|---|
| Priority: | low | Component: | Ports |
| Version: | 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)
Change History (17)
comment:1 by , 16 years ago
| Owner: | set to |
|---|
comment:2 by , 16 years ago
comment:3 by , 16 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 , 16 years ago
Don't worry about it, it's probably the least urgent patch ever written!
comment:7 by , 15 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 , 15 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 , 15 years ago
| Attachment: | 0001-Option-desired_screen_aspect_ratio-for-fullscreen.patch added |
|---|
git svn rebased against rev 43126
comment:9 by , 15 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 , 15 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 , 15 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 , 15 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:14 by , 6 years ago
| Owner: | removed |
|---|
comment:15 by , 6 years ago
| Component: | → Ports |
|---|
comment:16 by , 13 days ago
| Owner: | set to |
|---|---|
| Resolution: | → fixed |
| Status: | new → closed |
We have this now implemented.
Looks like a nice idea to me. Max?