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

Add macOS support using Core Graphics #1

Merged
merged 2 commits into from
Jan 19, 2022

Conversation

sanxiyn
Copy link
Contributor

@sanxiyn sanxiyn commented Jan 19, 2022

It draws something, but I am not confident it is correct. I am especially worried about coordinate system.

What should I expect to see for winit example? I think it would help to include screenshots in README (plural, to show resizing), and for example to be recognizably different when horizontally or vertically flipped. I propose a 45-60-75 triangle.

@john01dav
Copy link
Collaborator

I've added a new fruit example to show a proper image that I got off Wikimedia Commons. That should test a bit more thoroughly than a simple triangle since it has lots of colors and is otherwise more complicated so more can go wrongly. I'll probably add something to the README later, but I wanted to get this to you quickly.

Here are some screenshots (2 on Fedora Linux and two on Windows 10, note that when the window is larger than the displayed image it doesn't matter what is displayed and is different between Linux and Windows):

Screenshot from 2022-01-18 22-05-25
Screenshot from 2022-01-18 22-05-53
Screenshot from 2022-01-18 22-06-26
Screenshot from 2022-01-18 22-06-38

@john01dav
Copy link
Collaborator

Here is the winit example on both Linux and Windows. Note which side of the squares each color shows up on:
Screenshot from 2022-01-18 22-12-21
Screenshot from 2022-01-18 22-14-24

@sanxiyn
Copy link
Contributor Author

sanxiyn commented Jan 19, 2022

Thanks! That was helpful. I am now confident this works correctly.

@john01dav john01dav merged commit 55f5c79 into rust-windowing:master Jan 19, 2022
@john01dav
Copy link
Collaborator

Thanks for the PR!

@sanxiyn sanxiyn deleted the core-graphics branch January 19, 2022 06:02
@john01dav
Copy link
Collaborator

@sanxiyn I'm updating the README to reflect the new AppKit support. Do you have a preference for what name you are credited with?

@sanxiyn
Copy link
Contributor Author

sanxiyn commented Jan 19, 2022

Please use my Git user.name, that is Seo Sanghyeon.

@john01dav
Copy link
Collaborator

@sanxiyn I recently got access to a Mac computer of my own (albeit an old one, it supports nothing after High Sierra) and I was testing some related code on it and I found some cases in your implementation that don't work correctly. I was able to solve one of them (drawing an empty image causes an assertion to fail, so it's easy enough to fix by simply not drawing the image if width and height are zero), but I was unable to fix the other.

It seems that once an image is drawn to the window, it is impossible to draw to it again with a new image until after the window resizes. I tried to fix this, but my unfamiliarity with the core graphics API made this not work out. I'll keep trying, but you might have a better chance to figure it out.

I've added a new animation example that reliably reproduces the bug on Mac OS. Here's a video of what it is supposed to look like. I had to put it in a zip so Github would accept the upload. This example also lags pretty hard on Mac OS (but not on Windows or Linux, except when resizing as it blocks the main thread to re-render the animation, but even if that is an issue it's a winit one or an issue with the example and not a softbuffer one -- it lags on Mac OS when set_buffer alone is spammed).

@kpreid
Copy link
Contributor

kpreid commented Feb 7, 2022

It seems that once an image is drawn to the window, it is impossible to draw to it again with a new image until after the window resizes.

I'm seeing the same behavior you describe on macOS Monterey 12.2 (with cargo run --example animation --release, or with my own code).

Unfortunately I also don't have any ideas about how to fix it, or at least not ones that worked. I was wondering if it was missing some sort of “flush” operation, but msg_send![graphics_context, flushGraphics] and context.flush(); changed nothing. Accessing/creating the graphics context anew for each set_buffer didn't work entirely. Another idea I had was that maybe the graphics context wasn't available as intended (perhaps set to clip everything) except when resizing, but if that's so, I don't see what to change to fix that, short of implementing a “proper” NSView subclass. (I used to work with AppKit years ago, but never Core Graphics.)

@john01dav
Copy link
Collaborator

john01dav commented Feb 7, 2022 via email

@kpreid
Copy link
Contributor

kpreid commented Feb 7, 2022

I'm not quite following. You say it doesn't work here, but when I follow the link I see something that says it would work if winit was changed to call setNeedsDisplay? (Which sounds like a fine idea to me — except that it might be an extra cost to applications that don't need it, and it means that softbuffer would need to document that drawing must be done from RedrawRequested, rather than that it can be done there.)

Another likely correctness issue here is that the current set_buffer() code uses the thread current graphics context rather than one obtained from the window and remembered or re-fetched; this would surely draw in the wrong place if there are multiple windows, and I suspect the current behavior has to do with the state of that context. However, I tried to fix that and both of the approaches I tried resulted in no drawing at all rather than the current sometimes-drawing.

Maybe the GPU dependent drawing is the better answer all around, since it means you get a layer of your own independent of what the AppKit window drawing is doing.

@john01dav
Copy link
Collaborator

It turned out that I had missed something obvious in my testing of setNeedsDisplay. It was actually working, but because I had slowed the update rate to 1Hz and because the animation is one second long it was showing the same frame each time and thus appearing to not work. Once I increased the animation speed back to where it should be, everything works. I have now made a PR to winit about setNeedsDisplay.

@tinaun
Copy link

tinaun commented Feb 10, 2022

I do want to try an alternative pr using a custom layer backed NSView at some point

@john01dav
Copy link
Collaborator

@tinaun I always want the code to be as correct as possible, but I am unsure what the advantage is of doing it that way. Is it just a general sense of correctness (and there's nothing wrong with this), or is there some more practical advantage? Please don't take this to be discouraging in any way. I'm just trying to understand your motivation and any potential shortcomings of the current solution.

@tinaun
Copy link

tinaun commented Feb 10, 2022

  • this will (hopefully) support scaling in retina modes better, as the current examples always draw the images in logical pixels not physical pixels
  • your pr for using setNeedsDisplay in winit breaks opengl/metal
  • that was the approach I was going to try before knowing this repo existed :v

@kpreid
Copy link
Contributor

kpreid commented Feb 10, 2022

as the current examples always draw the images in logical pixels not physical pixels

In some applications this might be a desirable option, as the application might be performing an expensive per-pixel operation (e.g. raytracing, fractals), and retina/hidipi displays can be a surprising additional cost, especially if the developer hasn't tested on one themselves. Of course, programmable scale / resampling is even better, but that probably gets into GPU-drawing territory.

But, in any case, softbuffer needs a consistent interpretation of sizes — the documentation of GraphicsContext::set_buffer says “Use your windowing library to find the size of the window.” and there must be a correct answer to which size which does not depend on platform. The top-level doc example uses winit's Window::inner_size, which is a PhysicalSize, which argues for indeed using the physical pixels.

@john01dav
Copy link
Collaborator

@tinaun Those are great reasons, thank you for clarifying. If you make the PR I'll definitely merge it.

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

Successfully merging this pull request may close these issues.

None yet

4 participants