gfx-rs / metal-rs

Rust bindings for Metal
Apache License 2.0
591 stars 112 forks source link

Make function parameter names and order consistent with Apple's documentation #144

Open chinedufn opened 4 years ago

chinedufn commented 4 years ago

In Apple's documentation, the order of function parameters might look like this:

func replace(region: MTLRegion, 
 mipmapLevel level: Int, 
   withBytes pixelBytes: UnsafeRawPointer, 
 bytesPerRow: Int)

whereas in metal-rs it might look like this

    pub fn replace_region(
        &self,
        region: MTLRegion,
        mipmap_level: NSUInteger,
        stride: NSUInteger,
        bytes: *const std::ffi::c_void,
    ) {
        // ...
    }

I found that the order differences (last two parameters swapped) and name differences (stride vs. bytes_per_row) made it difficult to cross reference Apple's Metal docs when working with metal-rs.

My understanding is that these were all written by hand over time (I'm just basing this assumption on the first few commits from 2016 https://github.com/gfx-rs/metal-rs/commits/master?after=09433e64682ffe4498988118c062e608a4971eb6+220)

Given that - it's probably quite a bit of manual work and breaking changes to get consistent (or at least to the degree possible) with the official metal APIs.

How does metal-rs typically think about breaking changes? Is this a no-go? Or could this fit in at some point in the future?

I could use some insight here.

Cheers!

kvark commented 4 years ago

We are totally fine with breaking changes like that, but we adhere to semver like any other crate. Therefore, breaking changes will go into the breaking revision.

I agree in this case that we should rename the arg to bytes_per_row. There are other cases, which can be more complex, i.e. setBuffer in Metal accepts the slot in a non-obvious position. We can discuss all of them here.

chinedufn commented 4 years ago

Sounds good thanks for explaining.

Sweet. I'll bring them up here on a case by case basis whenever I'm working with metal-rs and run into one.

kvark commented 4 years ago

I aligned the texture API in 461148ed39a14e319992a2c1478d9a978cc32ffd