mkxp-z / mkxp-z

Open-source cross-platform player for (some) RPG Maker XP / VX / VX Ace games. A very heavily modified fork of mkxp. RGSS on steroids with a stupid name.
https://github.com/mkxp-z/mkxp-z/wiki
GNU General Public License v2.0
140 stars 39 forks source link

Expand Mega Surface support #148

Open WaywardHeart opened 5 months ago

WaywardHeart commented 5 months ago

Here we are. The big PR. Sorry for disappearing for a while, I burned myself out obsessing over trying to figure out a couple quirks in text rendering, and when I got back to it decided I really wanted to get this done.

Almost all RGSS features now support mega surfaces. The only exceptions are window_skins, which I figured wasn't needed, and radial_blur, which I took a look at recently and think I know how to handle it, but I want another break from working on this so that'll have to happen later. I did fix radial blurring on rectangles for normal bitmaps, though. I can split that into a separate PR if you want.

This PR includes #82, #98, #91, #145, #146, and #147.

As far as mkxp specific features go, I should be able to do animations without much issue, but my concern here was RGSS features so it was a little out of scope for me. I haven't even looked at sprite patterns but could probably handle it if there's demand for it. I think it should handle HiRes bitmaps reasonably well, although someone should probably decide if hires bitmaps should ever have the LoRes version drawn to because I think we have a few inconsistencies there.

The 32-bit pixman regions probably aren't really necessary, but I've seen warnings in the console about it before (from long text strings in the debug menu, which shouldn't happen anymore anyway) and it was an easy change.

I also fixed a small bug in hue_change, and disallowed negative zooms in sprites and planes. Partly because it matches RGSS behavior (sprites don't render at that point, and planes cause the game to hang. Probably because they get capped to 0, too, and then try to draw an infinite number of 0-pixel images).

And while I fully understand your desire for a test suite for this one, I really do want a break from this and making something to show off all of the potential edge cases I addressed isn't my idea of fun so expect it to be at least a few months before I get around to it.

Splendide-Imaginarius commented 5 months ago

Hi, thank you for the impressive barrage of PR's in the past day! I'll try to review them ASAP, though it may take a little while to work through them. Really appreciate the work.

Splendide-Imaginarius commented 5 months ago

@WaywardHeart I'm trying to understand how this PR affects multithreading. In current dev branch, Bitmap operations can only be done on the main thread, because they produce OpenGL API calls, and OpenGL is not thread-safe. In contrast, SDL software surfaces can be safely accessed from any thread, as long as only one thread is accessing a given surface at once.

I have some WIP patches (not yet submitted here) that add a Ruby binding for a Surface class, which is a very basic wrapper around SDL software surfaces, along with a Ruby binding that transforms a Surface into a Bitmap; the idea is that you can set up Surfaces on a background thread (where CPU power is cheap) and then only transform them into Bitmaps when you actually need them on the main thread. This yields a quite massive performance boost when dealing with large images.

What I'm trying to figure out is, can I drop usage of my Surface class (and associated Ruby bindings) in favor of your modified Bitmap class with Mega Surfaces? I looked at the initFromSurface implementation in this PR and, as best I can tell, there are no OpenGL calls issued when creating a Bitmap from an SDL surface in Mega Surface mode. Have I failed to notice something that would preclude safely accessing Bitmaps that use Mega Surfaces from a background thread?

If this is indeed safe to do, then perhaps a follow-up PR would be useful that modifies the GFX_GUARD_EXC wrapper on the Ruby bindings for Bitmap, so that the thread guarding only happens when the Bitmap is not a Mega Surface.

(I'm not specifically asking you to create such a PR, I'm fine with doing that work myself, I'm just looking for a sanity check on whether you think this is safe and/or advisable.)

WaywardHeart commented 5 months ago

@Splendide-Imaginarius I'm still using OpenGL calls for hueChange and blur. hueChange because my first attempt didn't preserve luminosity and I didn't feel like trying to fix it, and blur because most of the work for having OpenGL do it piecemeal was already done and I figured it would be more performant that way anyway. I think hueChange ended up a little more performant, too. stretchBlt would also need locked when pulling from a normal Bitmap unless it was already cached.

Beyond that, I'd be a little concerned about multiple threads accessing the same surface when one is using direct pixel access. getPixel and setPixel would probably both be fine, stretchBlt only needs it when it needs to mirror one of the dimensions, and radialBlur will definitely need it when I get around to implementing it. I guess I should look into spawning multiple threads to speed that last one up, because it looks expensive. Edit: Oh, I looked back up and see you mentioned the "only one thread" assumption. Never mind that bit, then.

If you feel that none of those things are a concern, then changing how it's guarded should be fine, although not in the exact way you proposed. The three cases that use OpenGL all feel fairly niche, at least. Speaking of guards, I should probably also take another look at 705b319. I just blindly slapped down GFX_GUARD_EXC on everything that looked like it even might need a guard when GUARD_EXC would probably be more appropriate for some of them.

Splendide-Imaginarius commented 5 months ago

@WaywardHeart OK, that all seems reasonable. In that case I'll probably shelve my existing threaded Surface implementation until we can get this PR merged, and then I'll see if I can rewrite my implementation to use Mega Surfaces instead, and test the performance of both.

and I figured it would be more performant that way anyway. I think hueChange ended up a little more performant, too.

I suspect that using OpenGL will be less performant if using LLVMpipe, which is an interesting use case for expanding Mega Surfaces. But we can worry about optimizing for LLVMpipe in a follow-up PR. Agreed that piecemeal is probably more performant when using a real GPU.