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.

Sign up for GitHub

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

Open
wants to merge 26 commits into
base: ogc-sdl-2.28
Choose a base branch
from
Open

[OGC] Add support for Custom Aspect Ratios #91

Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
8eaebcf
Adds WIP Widescreen Support
Fancy2209 Feb 4, 2025
37151df
Fix Cursor
Fancy2209 Feb 4, 2025
62191d5
Add orthoWidth to draw init log
Fancy2209 Feb 4, 2025
19e2688
Delete accidentally commited file
Fancy2209 Feb 4, 2025
e1f2bfd
Fix orthoWidth in Renderer
Fancy2209 Feb 5, 2025
6570d34
Add missing include
Fancy2209 Feb 5, 2025
4a73bb7
Remove resizing from Renderer, doesn't work properly
Fancy2209 Feb 5, 2025
fc69931
Fix SDL Renderer and OGC_set_viewport
Fancy2209 Feb 5, 2025
ab85398
Remove debug logging
Fancy2209 Feb 5, 2025
d6efcad
Update src/render/ogc/SDL_render_ogc.c
Fancy2209 Feb 5, 2025
4dc34db
Apply reviews
Fancy2209 Feb 5, 2025
7afc872
Merge branch 'Widescreen' of https://github.com/Fancy2209/SDL into Wi…
Fancy2209 Feb 5, 2025
016de5f
Close comment
Fancy2209 Feb 5, 2025
a5d6187
Expand comment
Fancy2209 Feb 5, 2025
7fc3f5c
Fix build
Fancy2209 Feb 5, 2025
052e287
Fix Widescreen Vide Mode Width Calculation
Fancy2209 Feb 5, 2025
027d5d2
Fix mouse Widescreen Viewport
Fancy2209 Feb 5, 2025
5219b8f
Revert ogcmouse changes, simply swap the names of screen_w and viewpo…
Fancy2209 Feb 5, 2025
d1b37b4
Assorted Fixes
Fancy2209 Feb 5, 2025
412eb51
Delete .vscode
Fancy2209 Feb 5, 2025
e254a96
Better region check when setting the video mode center point
Fancy2209 Feb 6, 2025
072708b
Make code much cleaner
Fancy2209 Feb 7, 2025
95c62a7
Merge branch 'Widescreen' of https://github.com/Fancy2209/SDL into Wi…
Fancy2209 Feb 7, 2025
40e5e47
Remove unused header
Fancy2209 Feb 7, 2025
984d25b
Add SDL_OGC_ASPECT_RATIO
Fancy2209 Feb 7, 2025
ad259a7
Make OGC_get_aspect_ratio return a float and scale the mouse only bas…
Fancy2209 Feb 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions src/video/ogc/SDL_ogcgxcommon.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include <ogc/cache.h>
#include <ogc/gx.h>
#include <ogc/video.h>
#include <ogc/conf.h>

static const f32 tex_pos[] __attribute__((aligned(32))) = {
0.0,
Expand All @@ -40,12 +41,32 @@ static const f32 tex_pos[] __attribute__((aligned(32))) = {
1.0,
};

bool OGC_get_aspect_ratio_dimensions(float *w, float *h)
Copy link
Collaborator

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

Suggested change
bool OGC_get_aspect_ratio_dimensions(float *w, float *h)
bool OGC_get_aspect_ratio(float *ratio)

or

Suggested change
bool OGC_get_aspect_ratio_dimensions(float *w, float *h)
bool OGC_get_aspect_ratio_dimensions(int *w, int *h)

Copy link
Collaborator

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.

Copy link
Author

@Fancy2209 Fancy2209 Feb 8, 2025 •
edited
Loading

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

Copy link
Author

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

{
const char *ratioString;
ratioString = SDL_getenv("SDL_OGC_ASPECT_RATIO");

if(ratioString == NULL) {
#ifdef __wii__
if(CONF_GetAspectRatio() == CONF_ASPECT_16_9) {
*w = 16.0f;
*h = 9.0f;
return true;
}
#endif
*w = 4.0f;
*h = 3.0f;
return true;
} else if(SDL_sscanf(ratioString, "%f:%f", w, h) == 2) return true;
return false;
}

void OGC_set_viewport(int x, int y, int w, int h)
{
Mtx44 proj;

GX_SetViewport(x, y, w, h, 0, 1);
GX_SetScissor(x, y, w, h);
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);
Comment on lines +64 to 68
Copy link
Collaborator

@mardy mardy Feb 8, 2025 •
edited
Loading

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).

Copy link
Author

@Fancy2209 Fancy2209 Feb 8, 2025 •
edited
Loading

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

Expand Down
1 change: 1 addition & 0 deletions src/video/ogc/SDL_ogcgxcommon.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#define GX_COLOR_AS_U32(c) *((u32*)&c)

void OGC_draw_init(int w, int h);
bool OGC_get_aspect_ratio_dimensions(float *w, float *h);
void OGC_set_viewport(int x, int y, int w, int h);
void OGC_load_texture(void *texels, int w, int h, u8 gx_format,
SDL_ScaleMode scale_mode);
Expand Down
16 changes: 13 additions & 3 deletions src/video/ogc/SDL_ogcmouse.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include "SDL_ogcgxcommon.h"
#include "SDL_ogcmouse.h"
#include "SDL_ogcpixels.h"
#include "SDL_ogcvideo.h"

#include "../SDL_sysvideo.h"
#include "../../render/SDL_sysrender.h"
Expand Down Expand Up @@ -178,6 +179,10 @@ void OGC_draw_cursor(_THIS)
int screen_w, screen_h;
float angle = 0.0f;

float aspect_w = 4.0f;
float aspect_h = 3.0f;
OGC_get_aspect_ratio_dimensions(&aspect_w, &aspect_h);

if (!mouse || !mouse->cursor_shown ||
!mouse->cur_cursor || !mouse->cur_cursor->driverdata) {
return;
Expand All @@ -195,11 +200,16 @@ void OGC_draw_cursor(_THIS)
screen_h = _this->displays[0].current_mode.h;

curdata = mouse->cur_cursor->driverdata;
OGC_load_texture(curdata->texels, curdata->w, curdata->h, GX_TF_RGBA8,
SDL_ScaleModeNearest);
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);
Copy link
Collaborator

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.

Copy link
Author

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


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);
Copy link
Collaborator

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).

Copy link
Author

@Fancy2209 Fancy2209 Feb 8, 2025 •
edited
Loading

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

Copy link
Author

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


if (angle != 0.0f) {
Mtx rot;
guMtxRotDeg(rot, 'z', angle);
Expand Down
38 changes: 36 additions & 2 deletions src/video/ogc/SDL_ogcvideo.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@

#include <malloc.h>
#include <ogc/color.h>
#include <ogc/conf.h>
#include <ogc/gx.h>
#include <ogc/system.h>
#include <ogc/video.h>
Expand Down Expand Up @@ -87,13 +88,22 @@ static void OGC_VideoQuit(_THIS);

static void init_display_mode(SDL_DisplayMode *mode, const GXRModeObj *vmode)
{
float aspect_w = 4.0f;
float aspect_h = 3.0f;
OGC_get_aspect_ratio_dimensions(&aspect_w, &aspect_h);

u32 format = VI_FORMAT_FROM_MODE(vmode->viTVMode);

/* Use a fake 32-bpp desktop mode */
SDL_zero(*mode);
mode->format = SDL_PIXELFORMAT_ARGB8888;
mode->w = vmode->fbWidth;

mode->h = vmode->efbHeight;
if (aspect_w > 4.0f && aspect_h > 3.0f) {
mode->w = mode->h * aspect_w / aspect_h;
mode->w = (mode->w + 1) & ~1;
} else
mode->w = vmode->fbWidth;
switch (format) {
case VI_DEBUG:
case VI_NTSC:
Expand Down Expand Up @@ -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?
Copy link
Collaborator

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

if(vmode->viWidth == 240 || vmode->viWidth == 640)
vmode->viWidth = vmode->viWidth + (VI_MAX_WIDTH_NTSC - 640);

// set Center point
if (VI_FORMAT_FROM_MODE(vmode->viTVMode) == VI_PAL) {
vmode->viXOrigin = (VI_MAX_WIDTH_PAL - vmode->viWidth) / 2;
vmode->viYOrigin = (VI_MAX_HEIGHT_PAL - vmode->viHeight) / 2;
} else {
vmode->viXOrigin = (VI_MAX_WIDTH_NTSC - vmode->viWidth) / 2;
vmode->viYOrigin = (VI_MAX_HEIGHT_NTSC - vmode->viHeight) / 2;
}


VIDEO_SetBlack(true);
VIDEO_Configure(vmode);

Expand All @@ -170,7 +194,8 @@ static void setup_video_mode(_THIS, GXRModeObj *vmode)
VIDEO_Flush();

VIDEO_WaitVSync();
if (vmode->viTVMode & VI_NON_INTERLACE) VIDEO_WaitVSync();
if (vmode->viTVMode & VI_NON_INTERLACE)
VIDEO_WaitVSync();

/* Setup the EFB -> XFB copy operation */
GX_SetDispCopySrc(0, 0, vmode->fbWidth, vmode->efbHeight);
Expand Down Expand Up @@ -277,6 +302,15 @@ int OGC_VideoInit(_THIS)
VIDEO_Init();

vmode = VIDEO_GetPreferredMode(NULL);
vmode->viWidth = VI_MAX_WIDTH_NTSC; // VI_MAX_WIDTH is the same for all regions
// set Center point
if (VI_FORMAT_FROM_MODE(vmode->viTVMode) == VI_PAL) {
vmode->viXOrigin = (VI_MAX_WIDTH_PAL - vmode->viWidth) / 2;
vmode->viYOrigin = (VI_MAX_HEIGHT_PAL - vmode->viHeight) / 2;
} else {
vmode->viXOrigin = (VI_MAX_WIDTH_NTSC - vmode->viWidth) / 2;
vmode->viYOrigin = (VI_MAX_HEIGHT_NTSC - vmode->viHeight) / 2;
}

videodata->gp_fifo = memalign(32, DEFAULT_FIFO_SIZE);
memset(videodata->gp_fifo, 0, DEFAULT_FIFO_SIZE);
Expand Down