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

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

Open
chinedufn opened this issue May 2, 2020 · 3 comments

Comments

@chinedufn
Copy link
Collaborator

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
Copy link
Member

kvark commented May 2, 2020

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
Copy link
Collaborator Author

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
Copy link
Member

kvark commented Jul 20, 2020

I aligned the texture API in 461148e

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

No branches or pull requests

2 participants