Skip to content

Conversation

madsmtm
Copy link
Contributor

@madsmtm madsmtm commented Apr 29, 2025

Connections
Allows #5641 to not contain breaking changes.

Description
wgpu-hal's dependency on the metal and objc crates is currently indirectly exposed via. the parameter types on hal::metal::Surface::from_view, hal::metal::Surface::from_layer and hal::metal::Instance::create_surface_from_layer.

This is unfortunate, since it ties wgpu-hal to specific dependencies, and specific versions of said dependencies. Instead, this PR changes these methods to use NonNull<c_void>, and prefers to instead document which type the user is expected to pass here. This mirrors the top-level API already exposed in Instance::create_surface_metal.

Additionally, this PR removes metal::Instance::create_surface_from_layer, since it was only a simple wrapper, and splits metal::Surface::from_view into from_ns_view and from_ui_view to make it clearer/easier to document what the requirements are for the given type. This matches raw_window_metal::Layer's methods.

Testing
Tested using:

# macOS
cargo run --bin wgpu-examples -- cube

# iOS on Mac Catalyst using Cargo Bundle
echo "[package.metadata.bundle]" >> examples/features/Cargo.toml
cargo bundle --format=ios --target=aarch64-apple-ios-macabi --bin wgpu-examples
./target/aarch64-apple-ios-macabi/debug/bundle/ios/wgpu-examples.app/wgpu-examples cube

I believe this should suffice, as this mostly modifies the entry point to surface creation.

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy --tests. If applicable, add:
    • --target wasm32-unknown-unknown
  • Run cargo xtask test to run tests.
  • If this contains user-facing changes, add a CHANGELOG.md entry.

@madsmtm madsmtm force-pushed the metal-hal-no-expose-deps branch from 1ffe038 to 3b5ebf9 Compare April 29, 2025 14:06
@madsmtm madsmtm mentioned this pull request Apr 29, 2025
4 tasks
@madsmtm madsmtm marked this pull request as ready for review April 29, 2025 14:08
@madsmtm madsmtm requested a review from a team as a code owner April 29, 2025 14:08
@cwfitzgerald cwfitzgerald self-assigned this May 28, 2025
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Sorry for the big delay on this.

I'm a bit conflicted on this. The code itself is fine. I'm just not sure if we need this change. When we switch to objc2, I think it's fine to also require all of our users who are using these advanced features to use objc2 as well (if they need to use metal/objc1, they can convert at the call site) as a breaking change.

@madsmtm
Copy link
Contributor Author

madsmtm commented Jun 6, 2025

Hmm, I think my main argument against that is that the objc2 crates are a fair ways away from v1.0, and I think it'll be annoying that updating it will be a breaking change. By using NonNull<c_void>, we sidestep such issues (at the cost of the API being unsafe).

@cwfitzgerald
Copy link
Member

cwfitzgerald commented Jun 11, 2025

I think for the time being we can tolerate breaking changes in our backing libraries. I think losing the typing and type-clarity is worse than tying ourselves to a specific version. If we want to talk about more long term stability in our APIs, we can start investigate ways to abstract over this without losing typing. On this basis, I'm going to close this.

I'm neither here nor there on the re-organization, so if you want to re-organize these methods in some way in a new PR, that would be fine.

We also spontaneously discussed this during triage at the maintainers meeting today.

Discuss #7649

  • Probably don’t need to accept this.
  • Conversion is possible for a user.
  • Reduces typing (type-safety, not keyboard typing!).

@madsmtm madsmtm deleted the metal-hal-no-expose-deps branch August 23, 2025 19:59
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.

3 participants