📀 Make `wgpu` an optional dependency #359

Jasper-Bekkers · 2023-09-11T05:43:27Z

This turns wgpu into an optional dependency that's default enabled.

In our project we're not using wgpu as the renderer so interop-ing with this library is a bit trick, but I think I've find a way that would work for us. We can take the pre-recorded command buffer Recording and convert it to our own abstraction.

As for the shaders, we can use naga and patch files to make them work in our environment.

@raphlinus We disccussed various ways of going about this problem some time ago, but I think this wouldn't be too intrusive on either of our projects. If it is, let me know.

DJMcNab

I really don't like quite how many cfg(feature="wgpu") there are here. I can already foresee a significant number of changes running into "oh, this needs wpgu".

I think the recommended approach for other renderers might be to use the shaders crate? (That might only be for non-Rust languages though, I've not looked into it too much)

DJMcNab · 2023-09-11T06:11:34Z

.github/workflows/ci.yml

@@ -23,6 +23,8 @@ jobs:
      - run: cargo check --workspace
      # Check vello (the default crate) without the features used by `with_winit` for debugging
      - run: cargo check 
+      # Check vello (the default crate) without wgpu
+      - run: cargo check --no-default-features


How long does this check take?

The CI run for that step (on this PR) was ~3 seconds.

Fair enough! Useful to have it documented here though. I've got no objection to adding this check on that metric then.

DJMcNab · 2023-09-11T06:15:47Z

src/engine.rs

-pub struct Id(NonZeroU64);
+pub struct Id(pub NonZeroU64);

 static ID_COUNTER: AtomicU64 = AtomicU64::new(0);

+#[cfg(feature = "wgpu")]
 pub struct Engine {


These sections together are a bit strange

This suggests the engine module is too entangled with wgpu/that these IDs shouldn't be in this module.
If this is the wgpu engine implementation, it should maybe be more separated?

It seems like at least there was an intention behind splitting Engine and Recording up, maybe moving all of the Engine stuff to a wgpu.rs file is better going forward?

Fwiw, anybody implementing another Engine type thing would need access to these internals to be able to translate between Recording and it's own internal representation since the Engine is responsible for handing these out.

DJMcNab · 2023-09-11T06:20:01Z

src/util.rs

@@ -175,6 +175,7 @@ impl std::task::Wake for NullWake {
 /// Block on a future, polling the device as needed.
 ///
 /// This will deadlock if the future is awaiting anything other than GPU progress.
+#[cfg(feature = "wgpu")]


Why is this double cfg(wgpu)ed?

My bad - I later realized this entire file was just wgpu helpers.

raphlinus · 2023-09-11T19:23:47Z

Very interesting to see this!

The Engine abstraction was designed to support multiple GPU execution engines, and we are doing some work with just using the shaders, and a separate crate for encoding (completely reimplementing the dispatching.

The direction I'd like to see is that Engine is factored so that Recording is in its own module that would always be present, but all of Engine and its dependencies would be gated by a single feature gate. An even sharper separation would be to have that as a separate crate, but that might increase friction for making further changes to the engine (see #360, as well as changes needed to support WGPU extensions, including f16, subgroups, and descriptor indexing). But I can see this as a reasonable first step; slightly messy but not a massive impact on the code.

raphlinus

Yes, let's land this, as it's a fairly minimal change so won't have much conflict with other work in flight. As a followup, I think we should move the wgpu backend into a separate rs file, to make the granularity of the cfg coarser. I've done enough experimentation to be confident that will be straightforward.

Thanks!

linebender/vello#359 made `wgpu` an optional but default dependency/feature, but any crate using `velato` will be forced to opt in to `wgpu` due to not disabling the default here.

…feature linebender/vello#359 made `wgpu` an optional but default dependency/feature, but any crate using `velato` will be forced to opt in to `wgpu` due to not disabling the default here. Since `velato` does not need any rendering backend at all, this feature should be disabled by default. As the `vello` crate is reexported from the root, a `vello-wgpu` passthrough feature is provided so that users can turn it back on without explicitly depending on `vello` in their crate dependencies.

linebender/vello#359 made `wgpu` an optional but default dependency/feature, but any crate using `vello_svg` will be forced to opt in to `wgpu` due to not disabling the default here. Since `vello_svg` does not need any rendering backend at all, this feature should be disabled by default. As the `vello` crate is reexported from the root, a `vello-wgpu` passthrough feature is provided so that users can turn it back on without explicitly depending on `vello` in their crate dependencies.

…feature linebender/vello#359 made `wgpu` an optional but default dependency/feature, but any crate using `vello_svg` will be forced to opt in to `wgpu` due to not disabling the default here. Since `vello_svg` does not need any rendering backend at all, this feature should be disabled by default. As the `vello` crate is reexported from the root, a `vello-wgpu` passthrough feature is provided so that users can turn it back on without explicitly depending on `vello` in their crate dependencies.

…ature linebender/vello#359 made `wgpu` an optional but default dependency/feature, but any crate using `velato` will be forced to opt in to `wgpu` due to not disabling the default here. Since `velato` does not need any rendering backend at all, this feature should be disabled by default. As the `vello` crate is reexported from the root, a `wgpu` passthrough feature is provided so that users can turn it back on without explicitly depending on `vello` in their crate dependencies.

…ature linebender/vello#359 made `wgpu` an optional but default dependency/feature, but any crate using `vello_svg` will be forced to opt in to `wgpu` due to not disabling the default here. Since `vello_svg` does not need any rendering backend at all, this feature should be disabled by default. As the `vello` crate is reexported from the root, a `wgpu` passthrough feature is provided so that users can turn it back on without explicitly depending on `vello` in their crate dependencies.

…feature (#10) linebender/vello#359 made `wgpu` an optional but default dependency/feature, but any crate using `vello_svg` will be forced to opt in to `wgpu` due to not disabling the default here. Since `vello_svg` does not need any rendering backend at all, this feature should be disabled by default. As the `vello` crate is reexported from the root, a `vello-wgpu` passthrough feature is provided so that users can turn it back on without explicitly depending on `vello` in their crate dependencies. Same change as linebender/velato#17.

#10) linebender/vello#359 made `wgpu` an optional but default dependency/feature, but any crate using `vello_svg` will be forced to opt in to `wgpu` due to not disabling the default here. Since `vello_svg` does not need any rendering backend at all, this feature should be disabled by default. As the `vello` crate is reexported from the root, a `vello-wgpu` passthrough feature is provided so that users can turn it back on without explicitly depending on `vello` in their crate dependencies. Same change as linebender/velato#17.

Jasper-Bekkers force-pushed the no-wgpu branch from d24aea1 to bb456c3 Compare September 11, 2023 06:01

DJMcNab reviewed Sep 11, 2023

View reviewed changes

Jasper-Bekkers force-pushed the no-wgpu branch from bb456c3 to 9bb675a Compare September 11, 2023 18:39

📀 Make wgpu an optional dependency

a264033

Jasper-Bekkers force-pushed the no-wgpu branch from 9bb675a to a264033 Compare September 11, 2023 19:27

raphlinus approved these changes Sep 15, 2023

View reviewed changes

Jasper-Bekkers merged commit 34d313d into linebender:main Sep 15, 2023
4 checks passed

MarijnS95 mentioned this pull request Apr 29, 2024

Replace vello's default wgpu feature with an optional wgpu feature linebender/velato#17

Merged

MarijnS95 mentioned this pull request Apr 29, 2024

Replace vello's default wgpu feature with an optional wgpu feature linebender/vello_svg#10

Merged

DJMcNab mentioned this pull request Aug 16, 2024

Better API for already GPU-resident images #664

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

📀 Make `wgpu` an optional dependency #359

📀 Make `wgpu` an optional dependency #359

Jasper-Bekkers commented Sep 11, 2023

DJMcNab left a comment

DJMcNab Sep 11, 2023

Jasper-Bekkers Sep 11, 2023

DJMcNab Sep 11, 2023

DJMcNab Sep 11, 2023

Jasper-Bekkers Sep 11, 2023

Jasper-Bekkers Sep 11, 2023

DJMcNab Sep 11, 2023

Jasper-Bekkers Sep 11, 2023

raphlinus commented Sep 11, 2023

raphlinus left a comment

📀 Make wgpu an optional dependency #359

📀 Make wgpu an optional dependency #359

Conversation

Jasper-Bekkers commented Sep 11, 2023

DJMcNab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphlinus commented Sep 11, 2023

raphlinus left a comment

Choose a reason for hiding this comment

📀 Make `wgpu` an optional dependency #359

📀 Make `wgpu` an optional dependency #359