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

Update objc2 to v0.6, and use new objc2-core-graphics crate #254

Merged
merged 2 commits into from
Feb 5, 2025

Conversation

madsmtm
Copy link
Member

@madsmtm madsmtm commented Jan 23, 2025

objc2 now provides an interface to CoreGraphics, which has automatic memory management, and interfaces better with the other objc2-* crates. Note that this doesn't have the CGDataProvider helper we were using before, but I'd argue that's actually a benefit, since it forces us to implement the data provider ourselves (and makes it very visible that that's a place where there's room for improvement).

Another notable change is the introduction of Retained::downcast, which we use to ensure that our types are correct in the key-value observer (instead of blindly casting, and hoping that they are).

This requires an MSRV bump to Rust 1.71 to get access to extern "C-unwind" functions.

@madsmtm madsmtm added CoreGraphics macOS/iOS/tvOS/watchOS/visionOS backend dependencies Pull requests that update a dependency file labels Jan 23, 2025
@madsmtm madsmtm force-pushed the madsmtm/objc2-v6 branch 2 times, most recently from 5e2d34a to 497ecb9 Compare January 23, 2025 02:11
@ids1024
Copy link
Member

ids1024 commented Jan 23, 2025

Looks like the released winit only requires 1.70.0 (master requires 1.73), but the bump here is still well within the rust-windowing MSRV policy, so it seems perfectly reasonable.

Note that this doesn't have, but I'd argue that's actually a benefit, since it forces us to implement the data provider ourselves

I think there's a word or two missing in this sentence after "doesn't have"? CGImage maybe?

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@madsmtm
Copy link
Member Author

madsmtm commented Jan 23, 2025

Note that this doesn't have, but I'd argue that's actually a benefit, since it forces us to implement the data provider ourselves

I think there's a word or two missing in this sentence after "doesn't have"? CGImage maybe?

Added "the CGDataProvider helper we were using before", thanks.

(I'd have linked to the helper, but it's marked #[doc(hidden)], which makes it a bit difficult).

@ids1024
Copy link
Member

ids1024 commented Feb 5, 2025

It was a while ago, but I was wondering if we could do something custom with the CGDataProvider to swap between buffers instead of allocating each present (#83 (comment)). So yeah, not having that helper in objc2 isn't necessarily a bad thing.

Testing on an M1 Mac Mini, I see an index out of bounds error in the winit example trying to resize the window, but that seems to happen on master too... not something I see on Linux/Wayland, anyway. Perhaps the same issue as #253.

src/backends/cg.rs Outdated Show resolved Hide resolved
@ids1024
Copy link
Member

ids1024 commented Feb 5, 2025

I can't claim expertise with the CoreGraphics APIs, but most of this seems like a straightforward change from the old API, and the into_raw/from_raw part seems correct.

@madsmtm
Copy link
Member Author

madsmtm commented Feb 5, 2025

Thanks for the reviews!

Testing on an M1 Mac Mini, I see an index out of bounds error in the winit example trying to resize the window, but that seems to happen on master too... not something I see on Linux/Wayland, anyway. Perhaps the same issue as #253.

I think that has been fixed with newer Winit, try cargo update in this repo?

@madsmtm madsmtm merged commit 438a8cc into master Feb 5, 2025
38 checks passed
@madsmtm madsmtm deleted the madsmtm/objc2-v6 branch February 5, 2025 07:23
@ids1024
Copy link
Member

ids1024 commented Feb 5, 2025

Ah right. Forgot to try that, and of course I have an old Cargo.lock. Seems to work fine after a cargo update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CoreGraphics macOS/iOS/tvOS/watchOS/visionOS backend dependencies Pull requests that update a dependency file
Development

Successfully merging this pull request may close these issues.

2 participants