WIP: Format API / Transparency

jaysonmaw commented 2 months ago

This is my answer to #17 and #98

WIP

I have only actually implemented the required changes for windows and mac os, as this was just an experiment to see if the idea I had would be doable. And those are the 2 platforms I have/use. If you are on these platforms feel free to point your cargo.toml at the fork to try it out. (If you do want to try it out, keep in mind you have to disable the compatibility feature in your cargo.toml that is enabled by default)

High level overview of changes:

Added ability to tell backend that you want to use transparency/alpha channel
Created RGBX/RGBA structs that can be used on every platform without needing to think about how the platform stores the u32 as the conversion to each of the platforms happens automatically

Essentially the changes are as follows:

Added softbuffer::Surface::new_with_alpha() function that creates an alpha aware surface
Created RGBX and RGBA structs
- Both structs have a Self::new() and Self::new_unchecked()
- new() returns a result, with an error if any of the values passed in overflow a u8
- new_unchecked()returns Self directly, silently ignoring any overflow, the same as if you had 300u32 as u8
- The two functions take any primitive number type (u8,u16,u32,etc)
- I took a quick look at the machine code, when passing in a u8 or u32 it appears to be just as performant as doing bit shifts yourself (Obviously new has some extra jmp for the overflow checks but new_unchecked is very clean). In the case that you pass u8's into new it has the same machine code as new_unchecked
- The structs are #[Repr(C)] with 4 u8's in them. I then change the ordering of the r,g,b,x/a properties based on the platform, and then inside the bufferImpl I can transmute the [u32] to the appropriate type based on with/without alpha
Depreciated old api where you get access to a raw &mut [u32] buffer
- Added feature compatibility to allow the old deref to still point to &mut [u32] buffer, enabled by default.
- Without compatibility feature enabled, buffer now derefs to a &mut [RGBX] when called on a surface created by calling Surface::new() and &mut [RGBA] when called on a surface created by calling Surface::new_with_alpha()
- Can call buffer.pixels_mut() function to access old style &mut [u32] even with compatibility feature disabled
Changed the backend impl for windows and macos
- Added new_with_alpha method
- Made required changes to tell backend that we are using alpha channel if called with new api, but still run the old way otherwise

Example:

Taking the winit.rs example and changing the following relevant lines shows how this works:

let window = winit_app::make_window(elwt, |w| w.with_transparent(true));

let context = softbuffer::Context::new(window.clone()).unwrap();
let surface = softbuffer::Surface::new_with_alpha(&context, window.clone()).unwrap();

(window, surface)

let mut buffer = surface.buffer_mut().unwrap();
for y in 0..height.get() {
    for x in 0..width.get() {
        let red = x % 255;
        let green = y % 255;
        let blue = (x * y) % 255;
        let alpha = x % 255;
        let index = y as usize * width.get() as usize + x as usize;
        // buffer[index] = blue | (green << 8) | (red << 16) | (alpha << 24); // <--- previous method
        buffer[index] = softbuffer::RGBA::new_unchecked(red, green, blue, alpha);

        //Using the RGBX type would not work, and would fail to compile as the rgb type is based on the surface constructor you called
        //buffer[index] = softbuffer::RGBX::new_unchecked(red, green, blue); // <-- Would fail to compile
    }
}

Final Thoughts:

This was just some testing I wanted to do for my own project, if this is not the direction this project wants to go that is absolutely fine, I just thought I would share my experiments on the subject. I only spent about a day on it so no hard feelings if this never gets used.

The rest of the backends I did not update to even compile with the changes I made, so as is even if you just want to use the old api, this will not work on this branch currently.

If you did go this direction, you would probably want some extra conversion functions on the new RGBX/A types to be able to set them directly with a u32. (At least I think? Maybe not?) Of course that has the same issue of before of having conversion/format issues, but you could definitely implement it in such a way that there is either no conversion and it is platform dependent, or there is auto conversions for platforms that don't match the norm. Or a mix of both. You would also want to expose a buffer.pixels_rgb_mut() function just as a shim so that you can access the new methods while having the compatibility feature enabled, just to help the transition.

Side Note: Please excuse the horror I created in the backend_dispatch.rs file. That could probably be cleaned up to be a bit nicer, I just wanted to get it working though.

Thanks for coming to my Ted talk ❤️

notgull commented 2 months ago

WIP

If this PR isn't ready to be merged yet please hit "convert to draft" to convert this to a draft PR.

jaysonmaw commented 2 months ago

WIP

If this PR isn't ready to be merged yet please hit "convert to draft" to convert this to a draft PR.

Sorry about that. I don't use github much. It is a draft PR now. 👍

ids1024 commented 2 months ago

If you did go this direction, you would probably want some extra conversion functions on the new RGBX/A types to be able to set them directly with a u32. (At least I think? Maybe not?) Of course that has the same issue of before of having conversion/format issues, but you could definitely implement it in such a way that there is either no conversion and it is platform dependent, or there is auto conversions for platforms that don't match the norm. Or a mix of both.

For using something like tiny_skia (https://docs.rs/tiny-skia/latest/tiny_skia/struct.PixmapMut.html), it's desirable to have a way to get a &[u8] of the image buffer (with the &[u32] interface, bytemuck at least can safely convert it). Though of course that's dependent on the underlying format used, and conversion may be needed in some cases.

I guess this API is generally not ideal for interoperability with other crates that do rendering (whether or not they use SIMD trickery), since they won't make use of the same types. It does seem like an interesting solution for some use cases though.

jaysonmaw commented 2 months ago

I guess this API is generally not ideal for interoperability with other crates that do rendering

Good feedback, I was only using the crate from the perspective of just wanting an easy/simple api for drawing some pixeles to the screen so this API was very much based around that. I'll take a look at the tiny_skia and imgref crates. I think I have some ideas that will work well and fit with the u32 conversion functions I mentioned. I think I can get something going that is no cost on the platforms that are argb.

From a quick look this also looks overly complicated IMO, I especially dislike the duplicate_item, can't we use normal generics?

Yeah I do agree with you there, I don't love the duplicate_item macro either. I originally had tried to write it using strictly generic's but in the end couldn't find a solution that didn't involve just throwing everything out. The problem is if its generic over A, then the A needs to actually be aware of the platform dependent changes needed to turn on alpha channels, and I couldn't figure out a way to get that to work, especially if different platforms required changes in different places. So instead I just created the two marker types of WithoutAlpha and WithAlpha that way each of the backends can easily implement for those 2 concrete types and change whatever they need to with their impl. The 'duplicate_item' just saves me from having to duplicate impl's where nothing actually changes, and if I change just those impl's to be generic it doesn't compile because it would require types further down the chain to also be generic which brings us back to problem 1.

In reality what was done here was all types were split into something like ______WithAlpha and ______WithoutAlpha, the current generic just hides that so we can treat the two types the same where alphaness does not matter.

Maybe I'm missing something there though, if anyone else can get it working with generics that would be obviously better. I'll also probably take another look, as I really just wanted to get something working with the API that I imagined, a lot of the code can be improved for sure from the concept but just wanted to see if there was any interest before I put more work into it.

Thanks! 👍

jaysonmaw commented 2 months ago

Yeah I do agree with you there, I don't love the duplicate_item macro either. I originally had tried to write it using strictly generic's but in the end couldn't find a solution that didn't involve just throwing everything out. The problem is if its generic over A, then the A needs to actually be aware of the platform dependent changes needed to turn on alpha channels, and I couldn't figure out a way to get that to work, especially if different platforms required changes in different places. So instead I just created the two marker types of WithoutAlpha and WithAlpha that way each of the backends can easily implement for those 2 concrete types and change whatever they need to with their impl. The 'duplicate_item' just saves me from having to duplicate impl's where nothing actually changes, and if I change just those impl's to be generic it doesn't compile because it would require types further down the chain to also be generic which brings us back to problem 1.

In reality what was done here was all types were split into something like ______WithAlpha and ______WithoutAlpha, the current generic just hides that so we can treat the two types the same where alphaness does not matter.

Maybe I'm missing something there though, if anyone else can get it working with generics that would be obviously better. I'll also probably take another look, as I really just wanted to get something working with the API that I imagined, a lot of the code can be improved for sure from the concept but just wanted to see if there was any interest before I put more work into it.

Completely ignore me. I ended up staring at this a bit more and was able to completely remove the duplicate_item macro, in favor of using pure generics. Apparently I just needed to sleep on that. I'll keep looking at the other items we discussed.

jaysonmaw commented 2 months ago

Alright, removed the ugly #[duplicate_item] macro, replaced with generics.

Started work on a format system, currently just implemented 2 formats, 1 of them specifically with tiny_skia in mind, so a u8 rgba format.

I added an example called winit_tiny_skia.rs if you want to try it out.

I also messed around in GodBolt to check/ensure that the conversion functions are essentially no ops if the format you ask for matches the base platform format. And the formats that do require conversion I leave up to the compiler to figure out the best way of doing that instead of trying to hand roll something and that appears to be working fine.

The format API looks something like this: (Copied from the winit_tiny_skia.rs example)

buffer.pixel_u8_slice_rgba(|u8_buffer_rgba: &mut [u8]| {
    let mut pixmap =
        PixmapMut::from_bytes(u8_buffer_rgba, width.get(), height.get())
            .unwrap();
    let mut paint = Paint::default();
    paint.set_color_rgba8(255, 0, 255, 0); 
    paint.anti_alias = true;

    let path = {
        let mut pb = PathBuilder::new();
        let RADIUS: f32 = (width.get().min(height.get()) / 2) as f32;
        let CENTER: f32 = (width.get().min(height.get()) / 2) as f32;
        pb.move_to(CENTER + RADIUS, CENTER);
        for i in 1..8 {
            let a = 2.6927937 * i as f32;
            pb.line_to(CENTER + RADIUS * a.cos(), CENTER + RADIUS * a.sin());
        }
        pb.finish().unwrap()
    };

    let mut stroke = Stroke::default();
    stroke.width = 24.0;
    stroke.line_cap = LineCap::Round;
    stroke.dash = StrokeDash::new(vec![20.0, 40.0], 0.0);

    pixmap.stroke_path(&path, &paint, &stroke, Transform::identity(), None);
});

The format is wrapped in a closure incase it does need to do conversion, it has to convert back before actually presenting to the window so that it matches the platform's expected format, so this ensures that while also not doing any allocation, and in the case there is no conversion it's just free.

I did also briefly look at imgref, from what I can tell there shouldn't be any reason this wouldn't work with that, but I didn't get to test anything with that yet.

Here's an image made with tiny_skia, based somewhat off that example: This shows off transparency working, both on the softbuffer side and tiny_skia, the background drawn with softbuffer and the star using tiny_skia with Blendmode::Clear to cut out a star.

All in all it seems to work pretty well, interested to hear any thoughts or questions.

x4exr commented 1 month ago

Is there anything left that should be done other than the code-tidy?

x4exr commented 1 month ago

I cloned your commits, the buffer type doesnt seem to be correct, RGBX, is that to be done soon? maybe I could try to finish this up although im not competent with this

jaysonmaw commented 1 month ago

Is there anything left that should be done other than the code-tidy?

Yes, I still have to research what is the base format for each of the platforms, and then encode that into the platform dependent RGBX/RGBA structs. Currently only Windows/Mac are confirmed correct. If anyone knows where that is already documented that I may have missed that would be helpful. I also then have to do research on how to enable transparency on each of the platforms, again currently its only implemented for Windows/Mac, then impl the backends, etc.

I've been a bit busy, I've also been meaning to try to reach out to the maintainers for feedback (I think I saw they have a zulip chat or something like that, just haven't had the time) , just so I could get an idea on if they are even interested in going this direction so I don't waste time finishing this up if there is no interest.

I personally have been using the fork in my own project just to dogfood it a bit, and still think it's a decent idea.

I know I should have some time late november to look at this and try to wrap it up, I will probably end up doing that if I don't find the time before then.

I cloned your commits, the buffer type doesn't seem to be correct, RGBX, is that to be done soon?

The buffer type is essentially broken on the other platforms until they at the very least add the appropriate generics, and then ideally also get a WithAlpha impl. Same thing for RGBX/RGBA, will be potentially broken on other platforms. I'm assuming you are on a different platform than the ones mentioned, if not though let me know.

maybe I could try to finish this up although im not competent with this

If you are interested you are definitely welcome to, there is quite a bit to be done still, but it's not very advanced or anything, just a good amount of stuff. Otherwise like I said, probably late november is when I will have a chance to look at this again.

x4exr commented 1 month ago

I tried getting in touch too on their element group but it's dead

I tried your version on windows, I will continue to try to study the internal api and see if I can fix the buffer array for windows

rust-windowing / softbuffer