-
Notifications
You must be signed in to change notification settings - Fork 51
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
WIP: Format API / Transparency #241
base: master
Are you sure you want to change the base?
Conversation
… 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
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. 👍 |
For using something like 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have much opinion on the top-level API, though I will say:
- I think anything we change should eventually be compatible with
imgref
- From a quick look this also looks overly complicated IMO, I especially dislike the
duplicate_item
, can't we use normal generics?
But the CoreGraphics impl looks fine, and I think that transparency is desired somehow.
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.
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 In reality what was done here was all types were split into something like 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! 👍 |
Completely ignore me. I ended up staring at this a bit more and was able to completely remove the |
moved all structs and windows/mac backends to new generics added format conversion system, only 2 formats currently to test out. exposed a few more pixels_ functions added a winit_tiny_skia example script using new format
Alright, removed the ugly 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 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 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 Here's an image made with tiny_skia, based somewhat off that example: All in all it seems to work pretty well, interested to hear any thoughts or questions. |
Added some documentation with examples to better show how different parts work together Added u32/8 conversions for formats and main RGBX/RGBA structs Refactored RGBFormat impl's into macro as it was a bunch of repeated code.
Is there anything left that should be done other than the code-tidy? |
I cloned your commits, the buffer type doesnt seem to be correct, |
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.
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
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. |
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 |
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:
Essentially the changes are as follows:
RGBX
andRGBA
structsSelf::new()
andSelf::new_unchecked()
new()
returns a result, with an error if any of the values passed in overflow a u8new_unchecked()
returnsSelf
directly, silently ignoring any overflow, the same as if you had300u32 as u8
new
has some extra jmp for the overflow checks butnew_unchecked
is very clean). In the case that you pass u8's intonew
it has the same machine code asnew_unchecked
#[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&mut [u32]
buffercompatibility
to allow the old deref to still point to&mut [u32]
buffer, enabled by default.compatibility
feature enabled, buffer now derefs to a&mut [RGBX]
when called on a surface created by callingSurface::new()
and&mut [RGBA]
when called on a surface created by callingSurface::new_with_alpha()
buffer.pixels_mut()
function to access old style &mut [u32] even withcompatibility
feature disabledExample:
Taking the winit.rs example and changing the following relevant lines shows how this works:
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 thecompatibility
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 ❤️