-
-
Notifications
You must be signed in to change notification settings - Fork 15
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
[OGC] Add support for Custom Aspect Ratios #91
base: ogc-sdl-2.28
Are you sure you want to change the base?
Conversation
I believe this is now ready for review |
Stuff was setting the viewport and scissor to 854 Renderer now only adjusts projection if it's trying to set the viewport to 640*480 (this feels like not the intended way but I haven't found a better one)
Co-authored-by: Alberto Mardegan <[email protected]>
Fix 4:3 Make mouse actually unstreched on 16:9 Fix SDL_DisplayMode 16:9 width (for real this time)
Btw there's something wrong with the code for centering the window, on widescreen VVVVVVV is setting the window x pos to 207, seems related to SDL_WINDOWPOS_CENTERED_DISPLAY (the x was smaller when I sent the picture above but it changed and I haven't found out why ) |
src/render/ogc/SDL_render_ogc.c
Outdated
if (CONF_GetAspectRatio() == CONF_ASPECT_16_9) | ||
OGC_set_viewport(viewport->x, viewport->y, viewport->w, viewport->h, | ||
viewport->w==640 ? (viewport->h*16.0f/9.0f)+1 : viewport->w); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to think a bit more about it, but it feels like this if
should be inside OGC_set_viewport() itself...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried, and it won't work
VVVVVV is the example for this
it has 3 Viewports
326*326 for the Cursor
320*240 for the Renderer
640*480 for the Window (width is 854 if strech is on)
If we removed the width adjusted for widescreen and just always did this correction, stuff would break because the Renderer would be aspect corrected, and then the surface it renders too would too, making the output squished
And while yes I could keep the check for viewport->w==640, I'm not sure if it's the right approach, I fear if the renderer and window have the same width this check will not be doing it's job
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After more research, seems like UpdateLogicalSize should work properly without this workaround but it gets the width as 640 instead 854, and this makes it's letterboxing not work?
Co-authored-by: Alberto Mardegan <[email protected]>
I just realized this will break all OpenGX apps when they update as they'll be setting the viewport (and maybe clipping plane) to the wrong resolution (projections would be right but if they set the viewport and clipping plane wrong output gets garbled) Ill submit a PR that changes glViewport and glScissor so they have a check for this, that way widescreen should be fully abstracted away |
Fix to prevent apps from dying when/if devkitPro/SDL#91 gets merged, as it reports the screen width as 854, and the viewport and scissor can't be over 640, just making sure developers don't have their app break without knowing why
I don't think it would be correct: the SDL documentation says (though not very clearly) that the aspect ratio can be deduced by looking at the screen size, and that applications should get the actual pixel size from But I've thought of another issue with this PR: we should not add a new mode for the 16:9 aspect ratio; what we should do, is check if Maybe the code in retroarch can help, because it shows how to build a video mode manually (it's not what we have to do here, but it can help in clarifying how things work): https://github.com/libretro/RetroArch/blob/master/gfx/drivers/gx_gfx.c Another question is where you got the number 704 for the viWidth on the 16:9 aspect ratio; shouldn't we try to use the maximum available width (VI_MAX_WIDTH_PAL == VI_MAX_WIDTH_NTSC == 720) instead? |
I believe setting viWidth and fbWidth to 720 is normally only done when abusing undefined behaviour like emgba and Sonic Mania do which allows for a much better picture clarity, but I'm not sure we want to implement this? Even if we do I'll do it on a separate PR though |
After seeing your PR on opengx I understood why you made, and it makes sense. I've added some comments there, but the idea is good.
I've now tried the various options on both my TV, and on a computer monitor (both connected to the Wii through HDMI, with a cheap adaptor), using wiiscreen -- the app is a pain to use, but it's useful to play with VI settings. My TV is 16:10, I guess, and it behaves horribly: when I set it to 16:9, it decides to use the full image horizontally, but then it cuts a few lines off the top and the bottom of the screen; I noticed it long ago when porting VVVVVV, but then understood that it's just a dumb TV and there's little I can to to fix this. And unless I force the TV to stay on 4:3, the image gets cropped if I increase the viWidth above 680; so even 704 is out of reach there. But I suggest ignoring my TV, at least for now :-) My computer monitor, instead, behaves very well: if I configure it to respect the input aspect ratio, I can set viWidth to 720 and I get a nicely stretched image (it does not fill the whole of the horizontal space, but it gets much closer than the Wii's system menu does). It would be nice if we could get a few people to try it on different TVs/monitors, but the value seems to be safe. |
We could technically support 16:10 or any aspect ratio for that matter |
So I had an idea
|
I think that playing with environment variables is an excellent idea, because it allows to "fix" applications with just a couple of lines, without having to go deep into their code. And this can also be much easier to implement :-) I don't see much value in const char *ratio = "4:3";
#ifdef __wii__
if (CONF_GetAspectRatio() == CONF_ASPECT_16_9) ratio = "16:9";
#endif
setenv("SDL_OGC_ASPECT_RATIO", ratio, 1); The important variable is that which allows specifying the aspect ratio. We should also decide if it ought to be an environment variable or a proper hint -- I would vote for the former, I don't see any benefit of adding the hint machinery here. |
SDL_WII_RESPECT_ASPECT_RATIO_CONFIG was just to allow apps to go into widescreen by default on wii but I can just make it what happens if SDL_OGC_ASPECT_RATIO isn't set |
Did a lot of cleanup, it wasn't worth to add the adjusted widescreen width |
Makes sense! |
By the way just curious, why did you use the std lib directly for OGC_JoystickInit? I thougth that SDL was meant to use the SDL stdlib (as in getenv and strcmp instead of SDL_getenv and SDL_strcmp like the rest of the lib) |
Because who doesn't want to make a Wii App that outputs at 21:9?
By the way, something that is really bothering me is that when I set the window to widescreen the mouse starts having pixels jitter, like depending where it is some pixels get cut off in one of the sides |
SDL is trying to be as portable as possible, so it redefines these functions to allow platforms to reimplement them, if needed. But for us it just introduces a slow-down, since while the stdlib functions can be optimized away from the compiler, the SDL equivalent can not (that's my guess, at least, since it does not know them). I would suggest avoiding them, unless you are writing code in the SDL common modules and you plan to submit it upstream. |
src/video/ogc/SDL_ogcgxcommon.c
Outdated
@@ -40,12 +41,32 @@ static const f32 tex_pos[] __attribute__((aligned(32))) = { | |||
1.0, | |||
}; | |||
|
|||
bool OGC_get_aspect_ratio_dimensions(float *w, float *h) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not really needed to use two floats here. Either do
bool OGC_get_aspect_ratio_dimensions(float *w, float *h) | |
bool OGC_get_aspect_ratio(float *ratio) |
or
bool OGC_get_aspect_ratio_dimensions(float *w, float *h) | |
bool OGC_get_aspect_ratio_dimensions(int *w, int *h) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it's better to cache the result of this function, since we don't expect the variable to change during run-time. If you decide to go with a single float, caching could be achieved like this:
static bool OGC_get_aspect_ratio_real(float *ratio) {
...implementation...
}
bool OGC_get_aspect_ratio(float *ratio) {
static float cached_ratio = -1.0f;
if (cached_ratio < 0) {
bool has_ratio = OGC_get_aspect_ratio_real(&cached_ratio);
if (!has_ratio) cached_ratio = 0.0;
}
*ratio = cached_ratio;
return ratio != 0.0;
}
You could even remove the boolean value, and make the function return the float directly; then, if it's 0.0, it means that no ratio has been set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I did it this way was just for accuracy, I feared rounding errors if I didn't use 2 floats
If I just return the ratio that'll be enough though,
Also caching the value would be annoying because I was gonna make a SDL test app that allows you to recreate the window with a different aspect ratio to test the output... But if it's worth it I'll do it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made it a float, will cache it when I get the chance
src/video/ogc/SDL_ogcmouse.c
Outdated
if (aspect_w > 4.0f && aspect_h > 3.0f) | ||
OGC_load_texture(curdata->texels, curdata->w, curdata->h, GX_TF_RGBA8, | ||
SDL_ScaleModeLinear); | ||
else | ||
OGC_load_texture(curdata->texels, curdata->w, curdata->h, GX_TF_RGBA8, | ||
SDL_ScaleModeNearest); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what this does, and I'm not sure that the if
is correct (what if the ratio is 3:2? -- by the way, for this type of checks it appears that using a single float for the aspect ratio instead of two ints would be better).
Note that the scaling is done either by the TV or by the VI interface, so I don't think we should be changing how the texture is being filtered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cursor looks very jittery on other aspect ratios, so I changed it to Linear since it wouldn't be as noticeable, but it's still noticeable and I have no idea why it's cutting pixels from the image depending on where it is
src/video/ogc/SDL_ogcmouse.c
Outdated
|
||
guMtxIdentity(mv); | ||
guMtxScaleApply(mv, mv, screen_w / 640.0f, screen_h / 480.0f, 1.0f); | ||
guMtxScaleApply(mv, mv, screen_w / (480.0f * aspect_w / aspect_h), screen_h / 480.0f, 1.0f); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You cannot do this on the modelview matrix, because then you are actually changing the cursor width, and then if the cursor then gets rotated it will appear wrong. I think that all this code should stay unchanged, and we should only operate on how the OGX_set_viewport function gets called.
It sounds a bit silly, since it was me who introduces this scaling, but looking at it now, it appears wrong: we already have the viewport to do the scaling; can you try to just remove this line?
And in the call to OGC_set_viewport() below, we should probably pass the EFB dimensions, not the screen dimensions adjusted for the aspect ratio (at least if you follow my suggestion about retrieving the aspect ratio from OGC_set_viewport).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would I retrieve the aspect ratio from the viewport function tough? I stopped doing a different width for the matrix because it's more correct, an app will look right if it's widescreen
If an app doesn't try to use widescreen, doing work arounds so maybe it'll look right felt wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i made the resize be sceen height dependant
src/video/ogc/SDL_ogcvideo.c
Outdated
@@ -157,6 +167,20 @@ static void setup_video_mode(_THIS, GXRModeObj *vmode) | |||
{ | |||
SDL_VideoData *videodata = (SDL_VideoData *)_this->driverdata; | |||
|
|||
// TODO: Should I make this only happen if the aspect ratio isn't 4:3? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To simplify things I suggest forgetting about the VI changes for now (have them in a separate PR later), because handing of the CONF_GetAspectRatio() is a bit orthogonal to setting the best video mode. It will make testing and reviewing easier, if we tackle one issue at a time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
GX_SetViewport(x, y, (w > 640) ? 640 : w, h, 0, 1); | ||
GX_SetScissor(x, y, (w > 640) ? 640 : w, h); | ||
|
||
// matrix, t, b, l, r, n, f | ||
guOrtho(proj, 0, h, 0, w, 0, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that GX_SetViewport and GX_SetScissor can be set to the EFB size (that is, we can consider w
and h
to be in pixels), and if we want to play with the aspect ratio we can play with the width parameter passed to guOrtho.
In other words, I would set leave the calls to GX_SetViewport and GX_SetScissor exactly as they were, then call OGC_get_aspect_ratio_dimensions right from within this function and call
const float ratio4_3 = 4.0f / 3.0f;
guOrtho(proj, 0, h, 0, w * ratio / ratio4_3, 0, 1);
Of course, this needs to be checked. :-) The thing is, if you pass the same values to both functions (GX_SetViewport and guOrtho) you get a 1:1 mapping, which is not what we want for widescreen cases (or for any case where the ratio is not 4/3).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave up aspect correcting apps that aren't using 16:9
Fixing this here would cause issues, as I've showed before it makes VVVVVVV less wide than it should.
The whole guOrtho thing being a new var really felt like a hack, and I think leaving it as is is the best approach, especially since some apps resize the viewport twice every frame and it will distort the image more than it should if we do it
I just tested Wii screen on my TV |
Description
Adds a video mode for widescreen
Existing Issue(s)
Fixes #73