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

wgpu re-exports are inconsistent and confusing #3823

Open
aloucks opened this issue Jan 31, 2022 · 7 comments
Open

wgpu re-exports are inconsistent and confusing #3823

aloucks opened this issue Jan 31, 2022 · 7 comments
Labels
A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change

Comments

@aloucks
Copy link
Contributor

aloucks commented Jan 31, 2022

Some re-exports have conflicting names with bevy types. Sometimes these are renamed with a Wgpu prefix and other times they are renamed with a Raw prefix. The current mechanism re-exports each item individually and can easily fall out of date and miss things.

What solution would you like?

It would be a lot nicer to simply re-export wgpu as a module instead of the individual items. In other words, use this:

pub use wgpu;

instead of this:

// TODO: decide where re-exports should go
pub use wgpu::{
util::BufferInitDescriptor, AddressMode, BindGroupDescriptor, BindGroupEntry,
BindGroupLayoutDescriptor, BindGroupLayoutEntry, BindingResource, BindingType, BlendComponent,
BlendFactor, BlendOperation, BlendState, BufferAddress, BufferBinding, BufferBindingType,
BufferDescriptor, BufferSize, BufferUsages, ColorTargetState, ColorWrites, CommandEncoder,
CommandEncoderDescriptor, CompareFunction, ComputePassDescriptor, ComputePipelineDescriptor,
DepthBiasState, DepthStencilState, Extent3d, Face, Features as WgpuFeatures, FilterMode,
FragmentState as RawFragmentState, FrontFace, ImageCopyBuffer, ImageCopyBufferBase,
ImageCopyTexture, ImageCopyTextureBase, ImageDataLayout, ImageSubresourceRange, IndexFormat,
Limits as WgpuLimits, LoadOp, MapMode, MultisampleState, Operations, Origin3d, PipelineLayout,
PipelineLayoutDescriptor, PolygonMode, PrimitiveState, PrimitiveTopology,
RenderPassColorAttachment, RenderPassDepthStencilAttachment, RenderPassDescriptor,
RenderPipelineDescriptor as RawRenderPipelineDescriptor, SamplerBindingType, SamplerDescriptor,
ShaderModule, ShaderModuleDescriptor, ShaderSource, ShaderStages, StencilFaceState,
StencilOperation, StencilState, StorageTextureAccess, TextureAspect, TextureDescriptor,
TextureDimension, TextureFormat, TextureSampleType, TextureUsages, TextureViewDescriptor,
TextureViewDimension, VertexAttribute, VertexBufferLayout as RawVertexBufferLayout,
VertexFormat, VertexState as RawVertexState, VertexStepMode,
};

The wgpu crate name (and when re-exported wholesale as a module) is short enough that it is not tedious to simply always use the fully qualified name (e.g. wgpu::Buffer). This usage is pretty common in the wgpu ecosystem including other areas of bevy.

What problem does this solve or what need does it fill?

Re-exporting the crate as a module makes things much more consistent and reduces the maintenance burden of having to add every wgpu type manually.

What alternative(s) have you considered?

Leave things as they are.

Additional context

I can draft a PR if there's interest in making this change.

@aloucks aloucks added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Jan 31, 2022
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change and removed C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Jan 31, 2022
@alice-i-cecile
Copy link
Member

I am definitely in favor of a more coherent re-export strategy. I'll defer to @superdump on the details though.

@mcobzarenco
Copy link
Contributor

Indeed, I fully sympathize and spent much time myself hunting for the place where a particular wgpu resource is exported, occasionally with a different name. The fact that not all wgpu types are exported is also a limitation -- and given wgpu is the way to extend bevy's rendering, just re-exporting the whole library as pub use wgpu; seems the right solution

@blaind
Copy link
Contributor

blaind commented Feb 8, 2022

Agreed, current one is confusing. Actually, I did not know of the wgpu re-exports and for some reason rust-analyzer auto-import did suggest them from the render_resource path. Ended up adding wgpu dependency to my crate. The re-export would be better solution.

@superdump
Copy link
Contributor

The problem is that we want to not reexport wgpu types that we wrap for reference counting purposes. But maybe exporting wgpu as a module indeed solves this as an IDE or rust-analyzer should suggest multiple imports. People can still make mistakes but they should be able to figure it out. What do you think @cart ?

@cart
Copy link
Member

cart commented Feb 8, 2022

Heres my rationale for not blanket exporting wgpu:

  1. We need wrappers for wgpu resource types (Buffer, Texture, etc) to provide unique resource ids (which wgpu doesnt provide). Exporting wgpu as a whole creates a confusing situation where we have two different Buffer types in our "namespace" (I treat the bevy api surface as a global namespace). There should be no confusion about what Buffer type we are talking about. Rust Analyzer will suggest both if we export both.
  2. Exporting wgpu as a whole means that we are providing "unfiltered" access to wgpu types. This means that wgpu can add a new type transparently to us, which might confusingly conflict with our own types. Again, we treat Bevy as a global (curated) namespace. I want everything we export to be intentionally selected and named.

Note that (1) might ultimately be resolved if wgpu exposes their internal "resource id" api to public consumers. We've asked for that in the past and the wgpu devs seemed open to it.

I'm of the opinion that if we are missing types we think users need, we should re-export them. The biggest pain point so far is "missing re-exports". And thats because nobody has done a "full pass" over wgpu types to export whats needed. Instead we've taken an "add exports as they are needed" approach, which obviously will create friction in the early days.

@aloucks
Copy link
Contributor Author

aloucks commented Sep 24, 2022

Exporting wgpu as a whole creates a confusing situation where we have two different Buffer types in our "namespace" (I treat the bevy api surface as a global namespace)

I wasn't suggesting that the wgpu re-export should be included in the prelude. I was only suggesting a way to enable users to avoid having to add a direct dependency on wgpu when the raw types are needed, including Buffer and Texture.

@Meyermagic
Copy link

This is especially annoying when bevy includes functions that depend on these unexported wgpu types, like how https://docs.rs/bevy/latest/bevy/render/renderer/struct.RenderDevice.html#method.poll depends on https://docs.rs/wgpu/latest/wgpu/type.Maintain.html . At the very least bevy should re-export these types that it requires as arguments, or provide and document a constructor or wrapper type for these cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change
Projects
None yet
Development

No branches or pull requests

7 participants