Skip to content

Comments

Metal: Reexpose safe wrappers for static MTLDevice getters#638

Closed
MarijnS95 wants to merge 1 commit intomadsmtm:masterfrom
Traverse-Research:mtl-device-static-getters
Closed

Metal: Reexpose safe wrappers for static MTLDevice getters#638
MarijnS95 wants to merge 1 commit intomadsmtm:masterfrom
Traverse-Research:mtl-device-static-getters

Conversation

@MarijnS95
Copy link
Contributor

The safe wrappers for these static functions seem to have been commented out as it is really hard to extend a dynamic trait object with some concrete static functions (that exist regardless of a concrete type implementing the MTLDevice trait). Instead, make them available as free functions prefixed with mtl_device_*(), to allow users to more easily read the "system default" device or get a list of all devices.

Copy link
Owner

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! We should work on fixing this in the generator itself, instead of fixing it locally like this, but for now I'm fine with fixing this locally for Metal.


// impl<P: MTLDevice> MTLDeviceExt for P {}

pub fn mtl_device_system_default() -> Option<Id<ProtocolObject<dyn MTLDevice>>> {
Copy link
Owner

@madsmtm madsmtm Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather have these be something like:

pub extern "C" fn MTLCreateSystemDefaultDevice() -> Option<Retained<ProtocolObject<dyn MTLDevice>>> {
    extern "C" {
        pub fn MTLCreateSystemDefaultDevice() -> *mut ProtocolObject<dyn MTLDevice>;
    }
    unsafe { Retained::from_raw(MTLCreateSystemDefaultDevice()) }
}

That is, change the signature of MTLCreateSystemDefaultDevice to do the Retained::from_raw step internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be lovely! I take it from this that we should keep the name to match Objective-C with CamelCase too?

Let me know when you can set that up in the generator, which would save us the hassle of reimplementing. But if you don't get to that in time, landing this now should make it easier to notice that there's some files that can be removed in the future.

Copy link
Owner

@madsmtm madsmtm Jul 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I've been (and will be) away for various summer holiday activities in recent times, so I'm a bit out of the loop myself. I think it would be fairly easy to implement generally, but I probably won't have time to do it before August.

I'm fine with pulling this change now, but in any case, I'm probably not going to cut a new release before it's implemented generally (opened #639), so you could also just wait for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't absolutely need these functions right now (would be nice to start using them in the example in Traverse-Research/gpu-allocator#225).

Shall I leave this PR open then to not forget to remove this custom file when you've resolved #639? Thanks in advance!

impl<T: ?Sized> Retained<T> {
#[inline]
pub(crate) unsafe fn new_nonnull(ptr: NonNull<T>) -> Self {
pub unsafe fn new_nonnull(ptr: NonNull<T>) -> Self {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to expose this, users should use Retained::from_raw(ptr.as_ptr()).unwrap() instead as the unwrap should be optimized away (and if it isn't, we have Option::unwrap_unchecked for that).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm that seems a bit silly when there is an API primitive available that is nicer and easier to read?

from_raw() uses this under the hood after all, and the only difference I see is the extra + Message bound to get access to it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree that it is a bit silly; but I'm trying to follow std here, and the prime examples of smart pointers, Box and Arc, only expose Box::from_raw and Arc::from_raw, and I don't really want to differ from these here if I can avoid it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't thought about matching std at all, where I don't use these functions very often (they're after all only to for raw pointers that originate from Rust at some point, i.e. temporarily creating a "user pointer" for FFI).

Here in objc2 it seems to make sense given that your bindings already generate a NonNull annotation where applicable.

@MarijnS95 MarijnS95 force-pushed the mtl-device-static-getters branch from 2217830 to 8106263 Compare July 13, 2024 23:05
@madsmtm madsmtm added this to the objc2 v0.6 milestone Jul 14, 2024
@madsmtm
Copy link
Owner

madsmtm commented Dec 22, 2024

I've fixed this and #639 in bae00d9, thanks for the input here!

@madsmtm madsmtm closed this Dec 22, 2024
@MarijnS95 MarijnS95 deleted the mtl-device-static-getters branch December 22, 2024 08:54
@MarijnS95
Copy link
Contributor Author

Thanks @madsmtm! The only thing I seem to be missing is turning the mention of MTLCreateSystemDefaultDevice() into an intradoc link inside objc2-metal's module-level documentation.

@madsmtm
Copy link
Owner

madsmtm commented Dec 22, 2024

Good point, done in 839be2e

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.

2 participants