search

Traverse-Research / gpu-allocator

🦀 GPU memory allocator for Vulkan, DirectX 12 and Metal. Written in pure Rust
https://traverse.nl/
Apache License 2.0
374 stars 49 forks source link

cargo: Replace `metal-rs` patch with `git` dependency #228

Closed MarijnS95 closed 3 months ago

MarijnS95 commented 4 months ago

gpu-allocator is a library crate, meaning that [patch]es don't get applied when users depend on our crate, and falsely end up with a broken build while our CI said everything was hunky dory ( https://github.com/Traverse-Research/gpu-allocator/issues/224). Even though we cannot normally have a direct git dependency as that blocks us from publishing to crates.io, the current gpu-allocator release (though only for Metal) is broken anyway. It's relying on functionality that has just now been merged upstream, but still has to make it into a (followup patch) release.

What's worse, the harcoded hash that this was pointing to no longer has a live reference on our fork (maybe due to a force-push), and the CI is now failing to to fetch that commit by hash while build-testing gpu-allocator.

Let's bump to a git dependency for now, and replace that as soon as we have a workable solution, however that pans out.

MarijnS95 commented 3 months ago

The CI is probably not going to like this change either, because the patch is needed to build metal. The upstream PR (https://github.com/gfx-rs/metal-rs/pull/316) isn't merged yet and as explained a [patch] or git relesae doesn't work anyway, so we might just switch this dep to our custom registry and postpone crates.io releases until metal-rs gets all outstanding PRs merged, together with a minor/patch version bump?

Alternatively, could we make the metal code compatible with the latest release (i.e. drop the heap type and acceleration structure allocations) to preempt all this mess?

MarijnS95 commented 3 months ago

Alternatively, could we make the metal code compatible with the latest release (i.e. drop the heap type and acceleration structure allocations) to preempt all this mess?

I think we should bite the bullet and do this. Neither d3d12 nor Vulkan provide helpers to turn an Allocation into a buffer/texture/accelerationstructure, but give direct access to the underlying heap and offset+size of the region that's provided for this allocation. Besides, it doesn't even publicly expose those basic fields like the other APIs.

Unfortunately once we fix that we're still left with a missing heap_acceleration_structure_size_and_align_with_size() to create an AllocationCreateDesc.

MarijnS95 commented 3 months ago

We could also do a quick hack and push a tag to make https://github.com/Traverse-Research/metal-rs/commit/a354c33 valid again, but that doesn't make any of the other issues outlined here go away.