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

Allow games to specify their pixel aspect ratio - DO NOT MERGE #1132

Closed
wants to merge 9 commits into from

Conversation

@criezy
Copy link
Member

@criezy criezy commented Feb 2, 2018

DO NOT MERGE: This pull request is incomplete and is here to ask for feedback.

The way the aspect ratio correction works in ScummVM is currently too restrictive for some games. Backends hardcode a 6:5 correction designed to go from 320x200 to 320x240 or 640x400 to 640x480. When aspect ratio correction is on, the SDL backends (both surface and OpenGL) for example systematically apply this 6:5 correction when the game original resolution is either 320x200 or 640x400 and do no correction otherwise.

Here are some cases where it fails:

  • Dreamweb, despite having a resolution of 320x200, was designed with square pixels in mind (as can be seen with the circle in the introduction, or with the rotating fan in Eden's bedroom).
  • ADL games with an original resolution of 560x384 need to be corrected to 560x420, which means a 35:32 correction (see conversation starting at 07:47 at http://logs.scummvm.org/log.php?log=scummvm.log.21Jun2017).
  • Some mac SCI games have a native resolution of 320x190 and apparently need to be corrected to 320x228 (see http://forums.scummvm.org/viewtopic.php?t=14303)

The approach that I have taken here is to modify Graphics::Mode to contain a pixel aspect ratio in addition to the width and height of the game. I then modified OSystem::initSize(int width, int height, const Graphics::PixelFormat *format = NULL) to OSystem::initSize(const Graphics::Mode &mode, const Graphics::PixelFormat *format = NULL) and adapted the OpenGL and SurfaceSdl graphics backend as well as the dreamweb and ADL engines.

All other backends are broken since I did not report the change in the OSystem API to them yet.

Another limitation of this pull request is that the SurfaceSdl backend only supports 1:1 or 6:5 corrections. So any other correction is ignored. To test those currently you have to use the OpenGL mode.

Note that in theory this approach allows the backend to choose the correction based on the given game pixel aspect ratio and the current display device (i.e screen) pixel aspect ratio. In the changes I made the backends assume the display uses square pixel when doing the correction.

At this points this allows to see that the approach works. Before I continue further I would like some feedback:

  • Does this approach look good to you? Do you have a better suggestion?
  • Currently the aspect ratio is stored as a frac_t, which has some precision issue. See commit 109b772. Should Common::Rational be used instead?
  • At some point while working on this I got confused between OSystem::GraphicsMode and Graphics::Mode. Maybe the later should be renamed?
  • The aspect correction code for the SurfaceSdl backend, which is in graphics/scaler/aspect.cpp assumes a 6:5 correction. Rewriting this code seems like a big task. Maybe we could let SDL2 do the scaling (I think it can do it? And it might even use hardware acceleration?) ? But then what of SDL 1? Can the support be dropped? Can we just leave it with the current code and no correction other than 6:5? Do you have any other suggestion?
criezy added 5 commits Feb 1, 2018
The main idea is to allow engines to pass the pixel aspect ratio
the game was designed for and let the backend use this information
to correct for the game pixel aspect ratio depending on the pixel
aspect ratio of the device or computer on which ScummVM is running.
This replaces the hardcoded 6/5 correction that was used until now.

The OpenGL graphics backend has been updated to handle the new
pixel aspect ratio information. The SurfaceSdl graphics backend
has been partially updated but will currently ignore any pixel
aspect ratio different from 1 or 6/5. Compilation for other
backends is likely to be broken.

In engines initGraphics() has also be updated. By default
engines will use a pixel aspect ratio of 6/5 if the game size
is 320x200 or 640x400 and a ratio of 1 otherwise (as the backend
was doing previously). Bug games now have the possibility to
explicitly specify the game pixel aspect ratio.
…n Graphics::Mode

The reason for this change is to work around precision issue in frac_t.
It uses integer division and this means that the fraction stored can
actually be a bit smaller than its real value. When it was stored as
pixel height/width, to get the correct screen height we were multiplying
the fraction with the screen height and could get a height slightly too
low. For example 640x400 was corrcted to 640x479. Inverting the fraction
still stores a value that can be a bit lower than the real value, but as
it is now used as the denominator in an integer division to get the
corrected screen, dividing by a value too small and rounding down the
result more or less cancel each others.

An alternative here would be to use Common::Rational instead of frac_t
to store and handle the game pixel aspect ratio.
Sources indicate that Apple II game are designed to be displayed
on a 4/3 monitor, which means that for a resolution of 560x384
the pixel aspect ratio is 32/35.
@csnover
Copy link
Member

@csnover csnover commented Feb 4, 2018

Here are some cases where it fails: […]

Do you have a more complete list of cases anywhere? I believe NES Maniac Mansion also belongs on this list since it is supposedly around 8:7 PAR, and @rsn8887 mentioned that at least some of Monkey Island 1’s assets appeared to be designed for square pixels. Maybe this ticket can be repurposed for this AR work if you don’t have one already since it discusses this a bit.

Does this approach look good to you? Do you have a better suggestion?

So far the changes are in line with what I was expecting when Graphics::Mode was introduced, so seems good to me so far.

The dimensions-only constructor is convenient, and after thinking about it, I’m still leaning toward wanting to always have engines give a PAR instead of having a “guess the PAR from the dimensions” magic. Adding some predefined constants for the common PARs would be helpful so developers have an easier time picking the right one. I’ve also been wondering whether a helper function to generate the PAR from a width + height + display aspect ratio might be convenient (or maybe it just ends up being confusing and unnecessary, I’m not sure).

Currently the aspect ratio is stored as a frac_t, which has some precision issue. See commit 109b772. Should Common::Rational be used instead?

Probably so. In absence of a specific performance problem requiring reduced precision, it makes more sense to me to default to Common::Rational. I think I only continued to use frac_t in some of the graphics code because it was there before, but my memory is not great right now so I could be making that up. The reasons against using frac_t are certainly not just with precision, it is also not type safe since it will implicitly convert to int which makes it way too easy to introduce programming errors by applying wrong operations to a frac_t. (If frac_t needs to continue to exist at all it should probably be turned into a proper class so it isn’t possible to misuse in this way, but that’s out of scope of this review.)

At some point while working on this I got confused between OSystem::GraphicsMode and Graphics::Mode. Maybe the later should be renamed?

I don’t have a good sense of how to address this. While theoretically generic, it seems like the OSystem “graphics mode” is used in practice to define different ways of scaling the video output, so maybe it should have a different name using a word like “scaler” or “filter” or “effect” or “shader” instead. I’m not sure how to successfully avoid confusion by changing Graphics::Mode, the only other names I can think of are similarly ambiguous (like VideoMode). I am open to any idea in this regard.

The aspect correction code for the SurfaceSdl backend, which is in graphics/scaler/aspect.cpp assumes a 6:5 correction. Rewriting this code seems like a big task. Maybe we could let SDL2 do the scaling (I think it can do it? And it might even use hardware acceleration?) ?

SDL2’s own scaling does seem preferable for the final pass for the reasons you’ve mentioned. There is also some arbitrary scaling code in TransparentSurface which I’ve wanted moved out for some time now which could also be used if needed. I had some trouble when trying to use the SDL scaling code for 32bpp cursors (IIRC 2.0.4 on Windows would not render anything), though that trouble may have been also from the broken RLE optimisation since it happened around the same time.

But then what of SDL 1? Can the support be dropped? Can we just leave it with the current code and no correction other than 6:5? Do you have any other suggestion?

As much as I would like to eliminate the legacy baggage of SDL1, I think it needs to stick around until we don’t have any more platforms without an SDL2 port. That said, I don’t think those legacy platforms need to receive first-class support at this point, so accurate scaling only with SDL2 & OpenGL seems like a reasonable compromise.

Finally, mentioning this at the end only since you added a docblock on it: the operator< on Mode probably should be comparing AR-corrected dimensions and then initSizeHint would use it for finding the largest dimensions (there is a TODO in there about this). Or it should be removed entirely since it’s a weird dimension-based instead of area-based comparison and isn’t used at all currently.

Loading

criezy added 4 commits Mar 26, 2018
The main reason for this change is to try to avoid confusion with
OSystem::GraphicsMode.
The use of frac_t could result in small errors (usually of 1 pixel)
due to rounding.
@bluegr
Copy link
Member

@bluegr bluegr commented Jun 23, 2021

@criezy Any news on this?

Loading

@sev-
Copy link
Member

@sev- sev- commented Nov 1, 2021

I think, this now could be closed as way too outdated. Though I like the idea and I think, this is a good improvement.

Loading

@criezy
Copy link
Member Author

@criezy criezy commented Nov 1, 2021

Right, I was only keeping this open as a reminder to myself that I should get back to it at some point. But that reminder has not really worked 😞
I will close this now.

Loading

@criezy criezy closed this Nov 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants