devkitPro / wut

Let's try to make a Wii U Toolchain / SDK for creating rpx/rpl.
zlib License
236 stars 52 forks source link

libwhb: Rename render-related functions to be less ambiguous #176

Closed aboood40091 closed 2 years ago

aboood40091 commented 2 years ago

I renamed the following functions so that it's more obvious what they do:

Since these existed for a long time, I deprecated them instead of removing them to retain compatibility with existing homebrew.

I also added the following functions for convenience:

exjam commented 2 years ago

I do not think I agree with this change.

Yes you renamed the functions to describe what GX2 functions they call, but this isn't necessarily an improvement.

The WHBGfx stuff is supposed to be a simple library to help get something on the screen. We do not need to bleed implementation details into the function names, like WHBGfxBeginRender and WHBGfxEndRender are just functions you call when you start rendering and end rendering, and internally they will do whatever they need to do to make this happen.

I think a user who wants to know WHBGfxBeginRender is actually waiting for a swap or WHBGfxBeginRenderDRC is just setting the current context to the one correctly initialised for DRC rendering is better off just using more GX2 APIs directly over WHBGfx rather than renaming all the WHBGfx APIs to describe their internal implementation details.

fincs commented 2 years ago

I agree with @exjam.

Another thing to point out is that there are certain codebases (such as SDL) which currently depend on libwhb and which would be needlessly broken by this set of changes.

On top of that, I consider libwhb to be quite limited, containing several things that are flawed by design, or that don't belong in wut. For this reason I would not recommend its usage regardless. This leads me to prefer deprecating/replacing libwhb altogether with new wrapper APIs that are more generally useful and flexible.

GaryOderNichts commented 2 years ago

I agree with fincs, a more advanced wrapper API would probably be the best option to fix WhbGfx's flaws.

Just my thoughts about those changes, and why I had approved them: Libraries like GLFW and EGL also work with buffer swap calls and contexts, which in my opinion would make it easier for someone to understand coming from that side. glfwMakeContextCurrent -> WHBGfxMake{DRC/TV}ContextCurrent glfwSwapBuffers -> WHBGfxSwapScanBuffers

But especially WHBGfxBeginRenderTV / WHBGfxBeginRenderDRC seem kind of confusing to me. Someone might as well do something like this outside of the rendering loop:

    // Enable depth testing
    WHBGfxBeginRenderTV();
    GX2SetDepthOnlyControl(TRUE, TRUE, GX2_COMPARE_FUNC_LESS);
    WHBGfxBeginRenderDRC();
    GX2SetDepthOnlyControl(TRUE, TRUE, GX2_COMPARE_FUNC_LESS);

There is now nothing going on related to begin rendering a frame, but the function names suggest that.

Regarding breaking codebases: This PR seems to be compatible with older codebases, by only deprecating the older functions.

fincs commented 2 years ago

Libraries like GLFW and EGL also work with buffer swap calls and contexts, which in my opinion would make it easier for someone to understand coming from that side.

I had in mind something inspired by the fencing/presentation model used in APIs such as Vulkan, which seems to suit GX2's workflow better. I.e. a WaitForPresent function that waits for a flip, and a Present function that copies the specified color buffers to the TV/DRC's scan buffers + applies a scan buffer swap. Context management would be entirely handled user-side. Either way your point is taken.

Regarding breaking codebases: This PR seems to be compatible with older codebases, by only deprecating the older functions.

While the code in this PR attempted to preserve source-level compatibility, it introduced ABI changes that render existing compiled code (i.e. pacman packages) incompatible.