mupen64plus / mupen64plus-core

Core module of the Mupen64Plus project
1.29k stars 257 forks source link

Add Video Extension functions to support Vulkan #1022

Closed Rosalie241 closed 1 year ago

Rosalie241 commented 1 year ago

This introduces the following video extension functions:

VidExt_InitWithRenderMode() Which allows a front-end to initialize based on a specific render mode (Vulkan or OpenGL), I left the original VidExt_Init() alone for backwards compatability, and a front-end can easily just call their own VidExt_Init() when the render mode is OpenGL.

VidExt_VK_GetSurface() Which allows a plugin to retrieve a vulkan surface to render to, note that it's a pointer to void*, that's due to the fact that VkSurfaceKHR is a opaque handle, but since including the vulkan headers seems wasteful, It makes sense to use a void* instead, the plugin and front-end can do the casting themselves.

VidExt_VK_GetInstanceExtensions() Which allows a plugin to retrieve the supported vulkan extensions, note that it's upto the vidext implementation to create a const char* array and pass that back with it's size.

Ping @loganmc10 because these changes are currently untested, but I feel like this is enough for a front-end to support both OpenGL and Vulkan plugins.

loganmc10 commented 1 year ago

Yes, this is very similar to what I currently do in simple64, but I would say this:

loganmc10 commented 1 year ago

The difference between const char** Extensions and const char** Extensions[] is a matter of ownership.

const char** Extensions is "a list of strings", whereas const char** Extensions[] is "a pointer to a list of strings".

const char** Extensions would mean that the video plugin would need to allocate the memory for that list, and the frontend (vidext) would just populate it.

const char** Extensions[] would mean that the frontend (vidext) needs to allocate the memory, and the video plugin is just getting a pointer to the list.

Because the video plugin doesn't know how big the list will be (it doesn't know the number of extensions), it can't initialize the array/list. It makes sense to pass a pointer, and then have the frontend (vidext) create and populate the list, passing back the length of the list in the NumExtensions variable

Rosalie241 commented 1 year ago

Rather than using uint64_t* for the VkSurface, I would just use void* (this is what I do in simple64). Rather than try to approximate the pointer, since it's not the proper type anyway, it's better to just make it void*, it's going to get cast anyway on the frontend side.

The function is used differently than what you have in simple64, i.e in my proposal it'll be used like

VkSurfaceKHR surface;
VkInstance instance;
VidExt_VK_GetSurface((uint64_t*)&surface, (uint64_t)instance);

Instead of the code you have now:

VkInstance instance;
VkSurfaceKHR surface = (VkSurfaceKHR)VidExt_GetVkSurface((void*)instance);

For VidExt_VK_GetInstanceExtensions, I use const char** Extensions [], not const char** Extensions. You can see how I use it with parallel-rdp here and here

That makes sense, I'll change it.

loganmc10 commented 1 year ago

Oh, I see, I mean to me it makes sense that the vksurface is the return value for the function, since it is created by the function, whereas the instance is an input for the function. But either way would work.

But regardless, they should both be void* (I guess void** for the surface if you pass it in), since they are both opaque handles. The only reason they are uint64_t on your system is because pointers on your system are 64 bits. On a 32 bit system they would be 32 bit pointers.

Rosalie241 commented 1 year ago

makes sense, changed it to void** and void*

loganmc10 commented 1 year ago

LGTM

Narann commented 1 year ago

LGTM too. Would wait for @richard42 to give final inputs.

Mastergatto commented 1 year ago

If I may suggest...I would just rename VidExt_Init2() to something like VidExt_InitVk(), so you get a more descriptive name of the function.

loganmc10 commented 1 year ago

I think it makes sense, it's not Vulkan specific, it allows you to choose the rendering mode (currently OpenGL or Vulkan, but it leaves it open for others in the future)

It could maybe be something like VidExt_InitWithRenderMode or VidExt_InitWithMode just like there is currently VidExtFuncSetMode and VidExtFuncSetModeWithRate

Mastergatto commented 1 year ago

Ah right, it should be better this way.

Rosalie241 commented 1 year ago

I've renamed VidExt_Init2 to VidExt_InitWithRenderMode as suggested, and I've implemented the vulkan video extension functions for the default SDL implementation, which I've tested and it works, it requires SDL 2.0.6 at the very least and isn't supported on macos due to mac not supporting vulkan.

loganmc10 commented 1 year ago

You've gone above and beyond @Rosalie241 , good work!