Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add stretch modes #1189

Merged
merged 4 commits into from Jul 8, 2018

Conversation

@criezy
Copy link
Member

commented May 12, 2018

This PR is a proof of concept to discuss and experiment with the possibility to scale the display to the window in various ways for backends that supports scaling (SDL 2 Surface or SDL 1.2 and 2 OpenGL).

This is different from the scaling of the graphics mode for the SDL Surface backend. This graphics mode causes the display to be scaled to a surface with a scaler of 1x, 2x or 3x. But then this surface is further scaled to fit the window (or screen in fullscreen mode). The OpenGL backend similarly scales the texture to fit the window or screen.

In this PR I have implemented four different modes:

No scaling: no additional scaling is performed after the graphics mode scaling was applied
scummvm_scale_none

Integral scaling: scale by an integral amount as much as possible but not bigger than the window.
scummvm_scale_integer

Fit to window: scale to fit the window while respecting the aspect ratio. This is the current behaviour, and still the default behaviour after my changes.
scummvm_scale_fit

Fit and stretch to window: in this mode the aspecy ratio is not respected and there is no black bars.
scummvm_scale_fit_stretch

There is also a second commit that adds the Ctrl+Alt+s hotkey to cycle through the scale modes.

At this point there are two main pending questions.

  1. Is there interest in this idea? Or is this a bad idea?
    There seems to be since the question was asked on the forum a few time (see for example here or here). In my case I was mostly interested in the integral scaling, but while implementing this I decided to push the idea a bit further.

  2. Assuming this is a good idea, should this be exposed in the GUI?
    I think it probably should. The hotkeys are sufficient to test the idea, but most people will not be aware of it unless it appears in the Options dialog. On the other hand if we think few people will be interested in these options, then there is no point adding this to the Options dialog.
    This should probably be added to the command line in any case though.

  3. Do you see a potential issues with those changes? Or ways to make them better?

If there is interest I will spend some time to cleanup the code and finalise this pull request.

@Kroc

This comment has been minimized.

Copy link

commented May 12, 2018

This may be confusing when there exists the 1x/2x/3x scaling beforehand -- perhaps this feature would be better described as "fit to screen", rather than "scaling"

@tsoliman

This comment has been minimized.

Copy link
Member

commented May 13, 2018

I really like this as a feature from the backend point of view, especially the integral scaling option. It really helps with odd-resolution monitors that aren't HiDPI (HiDPI monitors don't seem to suffer from the non-integral scaling artifacts as much)

I am not sure how to make it less confusing for the user if it is exposed as a GUI option. I like Kroc's "fit to screen" or maybe something like "further scaling"?
Other ideas: "scaling" vs "fit"

The current "Graphics Mode" seems to be a catch-all for two existing concepts: the existing Surface/OpenGL/etc in addition to the scaling. (OpenGL's submodes are handled by a "secret" keystroke). Maybe that itself should be broken down into "Graphics Mode" (surface/openGL/etc) and "Scaling/Filtering" and then the 3rd "Screen Fit" can be a 3rd dropdown?

Maybe this is outside the scope of the PR. Just my 2 cents.

@lotharsm

This comment has been minimized.

Copy link
Member

commented May 15, 2018

I really like the idea of this PR, especially the integer scaling for a pixel-perfect, crisp image. Not sure if we should expose this to the GUI since it might lead to confusion... If we decide to expose it to the GUI, I vote for a dropdown menu which allows selecting the different stretching modes.

@bonki

This comment has been minimized.

Copy link
Member

commented May 16, 2018

I like this and we should definitely expose it to the GUI.

The current "Graphics Mode" seems to be a catch-all for two existing concepts: the existing Surface/OpenGL/etc in addition to the scaling. (OpenGL's submodes are handled by a "secret" keystroke). Maybe that itself should be broken down into "Graphics Mode" (surface/openGL/etc) and "Scaling/Filtering" and then the 3rd "Screen Fit" can be a 3rd dropdown?

+1

@sev-

This comment has been minimized.

Copy link
Member

commented May 17, 2018

I like this too, and I think we have to expose it in GUI. Maybe not just call it stretching (vs scaling), but maybe rename scaling to 'discrete scaling' or 'integer scaling'.

width = fracToInt(height * inputAspect);
void populateDisplayAreaDrawRect(const frac_t displayAspect, int originalWidth, Common::Rect &drawRect) const {
int mode = ConfMan.getInt("scaling_mode");
// Mode 0 = use original size, or divide byan integral amount if window is smaller than game surface

This comment has been minimized.

Copy link
@lotharsm

lotharsm May 19, 2018

Member

Small typo ("byan")

@lotharsm

This comment has been minimized.

Copy link
Member

commented Jun 29, 2018

What's the current state of this PR? Any chance to see this merged in the near future?

@criezy

This comment has been minimized.

Copy link
Member Author

commented Jun 29, 2018

This is a timely question ;-)
My plan is to work on this this week-end since the weather forecast is that it will get even warmer and the room with my computer is in the coolest one (oriented to the north) and the smallest one (there is not much else to do that use the computer).

Somewhat related to the comment from @tsoliman I don't really like the way we currently handle graphics modes in the GUI, and the ticket #7719 in our bug tracker is related to this. But I consider this is outside the scope of this PR.

@criezy criezy force-pushed the criezy:scaling-mode branch 2 times, most recently from 89197c2 to ac957d3 Jul 1, 2018

@criezy

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2018

I have now cleaned the code, added the option to the Options dialog, and added a command line argument for it.

I think this PR is now in a state where it can be reviewed for a possible merge.

@criezy criezy changed the title Add scaling modes Add stretch modes Jul 2, 2018

@criezy

This comment has been minimized.

Copy link
Member Author

commented Jul 2, 2018

Here is what the options currently look like. Any suggestion on a better wording is welcome.

scummvm_stretch_options

@Kroc

This comment has been minimized.

Copy link

commented Jul 2, 2018

Wording suggestions:

s/Center/Center-in-window
s/Integral scaling/Stretch, but keep pixels whole
s/Fit to Window/Stretch, but keep aspect ratio
s/Stretch to Window/Stretch to fill whole window

@lotharsm

This comment has been minimized.

Copy link
Member

commented Jul 3, 2018

I tested this new feature and so far, it looks and works great for me!

The only suggestion I have is to reword the Integral scaling feature to something more descriptive like "Pixel-perfect scaling", but this is just my personal opinion.

Great job, looking forward to see this in master!

@bonki

This comment has been minimized.

Copy link
Member

commented Jul 3, 2018

I have tested this as well and it works as intended with minor annoyances when switching from fullscreen back to windowed mode but I assume these to not be related to this PR (I haven't thoroughly checked yet and I don't usually toggle between modes). I'll compare against a current vanilla build.

Other than that I agree with Lothar. My vote also goes to keeping Center, Fit to window and Stretch to window and only give Integral scaling another thought, even though it makes total sense once you realize what it does. I have no strong opinion against it, though.

As an aside, why is Window capitalized? I also notice that Device in Audio -> Preferred Device and Speech in Audio -> Text and Speech are capitalized while others are not. To me this looks wrong, is there some thought or system behind this?

As another aside, the cycling hotkey doesn't work on my Windows 10 test box because it seems to be a system default and isn't captured by ScummVM.

@bonki bonki closed this Jul 3, 2018

@bonki

This comment has been minimized.

Copy link
Member

commented Jul 3, 2018

Sorry, hit the wrong button.

@bonki bonki reopened this Jul 3, 2018

@lotharsm

This comment has been minimized.

Copy link
Member

commented Jul 3, 2018

Since cycling through the different modes using the CTRL+ALT+S hotkey works for me on Windows 10 using the default SDL2 renderer (have not tested OpenGL yet), this might need some more investigation though.

@criezy

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2018

Some information on my choices:

  • Ctrl+Alt+s seemed to be a logical choice for a shortcut since Ctrl+Alt+a is used for aspect ratio correctiopn, Ctrl+Alt+f is use to toggle filtering and Ctrl+Alt+plus/minus is used to increase or decrease the scale factor.
  • The Center, Fit and Stretch names come from what I have in Windows 7 for the background image options. It also has a Fill that cuts part of the image and I decided not to implement in ScummVM (missing part of the display doesn't seems like a god idea to me for a game). Using the same nomenclature as Windows seemed a good idea to me as I would hope Microsoft put some thoughs in those, and these would also be somewhat familiar to the user.
  • Microsoft does not specify Window though (so only Fit and not Fit to Window for example). To me this looked a bit confusing, which is why I have added window, but maybe that actually makes it more confusing since the option also impact fullscreen display, where this would be better named as Fit to screen.
  • And the main question mark for me is indeed on the remaining option (Integral scaling). Pixel-perfect scaling could indeed be an option.

And I agree with @bonki that we might want to make some changes so that we are consistent in the capitalization (probably only capitalize the first word since this seems to be what we do in most places).

@bonki

This comment has been minimized.

Copy link
Member

commented Jul 3, 2018

I don't know why it didn't work yesterday but I can confirm that the hotkey does work, so forget I ever said anything.

Ad Fit to screen: I'm not sure if this is better (or worse), one could also misinterpret that as the window being fit to the whole screen if one wants. I guess both can be interpreted in the wrong way so I'm totally fine with either.

Ad Integral vs. Pixel-perfect scaling: My stance is that both make sense but neither is self-explanatory so a user would need to consult a possible tooltip or the manual anyway, and then it doesn't really matter. So personally, I'm fine with either.

Ad capitalization: In my book only the first word should be capitalized except where it makes sense not to. I suggest lowercasing window (or screen, for that matter) here and I can do a PR for the rest of the GUI.

@criezy

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2018

I have sent an email to -devel about capitalisation and I will wait for the discussion there to reach a conclusion before updating this PR.

@criezy criezy force-pushed the criezy:scaling-mode branch from 6cc8560 to f29035e Jul 8, 2018

criezy added some commits Jul 1, 2018

SDL: Implement stretch mode API
Four modes are supported:
 - Use original size with no scaling
 - Scale by an integral amount as much as possible but not bigger
   than the window.
 - Scale to fit the window while respecting the aspect ratio. There
   may be black bars on the left and right, or on the top and bottom,
   but not both. This is the default, and the old behaviour.
 - Scale and stretch to fit the window. In this mode the aspecy ratio
   is not respected and there is no black bars.
The mode is controled by the "scaling_mode" value (between 0 and 3) in
the config file.

Also add Crtl-Alt-s hotkey to cycle through scaling modes

@criezy criezy force-pushed the criezy:scaling-mode branch from f29035e to e7f6408 Jul 8, 2018

@criezy criezy merged commit 89f1b1c into scummvm:master Jul 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.