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.

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

Expand Mega Surface support #148

Open
wants to merge 32 commits into
base: dev
Choose a base branch
from
Open

Conversation

WaywardHeart
Copy link

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.

Bitmap::stretchBlt handles that now
Code mostly taken from the implementation in Bitmap::drawText
…rship of the surface, and provide an option to leave it as a mega surface
Not quite accurate to what VX Ace is doing, but I think that's mostly because of kerning differences.
…ent outlines

Outlines should 1) Not change the position of the text and 2) Be visible even if the text is cut off from reaching the edge of the rect, without leaving the rect itself.
We're currently not drawing full text for the outlines like RGSS does, so we don't perfectly succeed at (2), but if we ever start doing that then the logic is already there.

Text transparency with outlines present still doesn't match Enterbrain's implementation, but I gave up on trying to figure that out.
…too-large dimensions.

I also fixed a small bug in how the wave position was calculated.

I ALSO fixed src_rects in general for waves. (How was that not noticed before now?)
Ruby is not guaranteed to delete bitmaps after any windows, sprites, etc that they are attached to. In the event that the attached object is not deleted before the next call to Graphics.update, this will most likely result in a segfault, as isDisposed() is not guaranteed to return true for a deleted bitmap.

Bitmap::invalid was added in an attempt to guard against this for sprites, but since the bitmap in question is deleted it's not guaranteed that the pointer actually points to null, which made the fix unreliable.
I'm implementing this via "child" bitmaps, which determine which part of the parent will be visible, manually shrink it if necessary, and send back new values for zoom and offsets.

Window contents and Planes are currently fully functional. Window skins still aren't supported, because it seemed unlikely that would be needed.
Sprites support everything except patterns.
The old code only functioned perfectly on squares.
@Splendide-Imaginarius
Copy link

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
Copy link

@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
Copy link
Author

WaywardHeart commented Jan 15, 2024

@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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants