Implement Extensions Interface #703
Conversation
![monocodus[bot]](https://avatars.githubusercontent.com/in/28526?s=60&v=4){kind=small}
cwfitzgerald
force-pushed
the
extensions-interface
branch
from
b932426
to
08d857b
Compare
cwfitzgerald
force-pushed
the
extensions-interface
branch
from
08d857b
to
0db2c2c
Compare
wgpu-core/src/instance.rs
Outdated
| @@ -552,6 +555,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> { | |||
|
|
|||
| wgt::Limits { | |||
| max_bind_groups: (limits.max_bound_descriptor_sets as u32).min(MAX_BIND_GROUPS as u32), | |||
| _non_exhaustive: wgt::NonExhaustive, | |||
There was a problem hiding this comment.
Let's be consistent on whether we include stuff from wgt or not. My preference is to just use wgt:: everywhere.
wgpu-core/src/instance.rs
Outdated
| trace_path: Option<&std::path::Path>, | ||
| id_in: Input<G, DeviceId>, | ||
| ) -> DeviceId { | ||
| if desc.extensions.contains(Extensions::UNSAFE_EXTENSIONS) { |
There was a problem hiding this comment.
I can never keep those straight for the life of me...
There was a problem hiding this comment.
I've hit that problem many times as well.. I wonder if it would be better called contains_some/contains_all instead of intersects/contains, but that train is off gone by now.
wgpu-types/src/lib.rs
Outdated
| #[derive(Debug, Copy, Clone, Default, Eq, PartialEq, Hash)] | ||
| #[cfg_attr(feature = "trace", derive(Serialize))] | ||
| #[cfg_attr(feature = "replay", derive(Deserialize))] | ||
| pub struct NonExhaustive; |
There was a problem hiding this comment.
Can we make it so the type is opaque and the only way to create it is using an unsafe constructor?
This way, users have to at least go through unsafe() if they decide that they want to use the type
wgpu-types/src/lib.rs
Outdated
| const ANISOTROPIC_FILTERING = 0x01; | ||
| const ANISOTROPIC_FILTERING = 0x0000_0000_0001_0000; | ||
| /// Extensions which are part of the upstream webgpu standard | ||
| const WEBGPU_EXTENSIONS = 0x0000_0000_0000_FFFF; |
There was a problem hiding this comment.
I wonder if we can rename this to avoid repeating ourself (in "extensions"). How do you feel about ALL_WGPU, ALL_UNSAFE, and ALL_NATIVE?
wgpu-core/src/instance.rs
Outdated
| @@ -579,9 +583,18 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> { | |||
| &self, | |||
| adapter_id: AdapterId, | |||
| desc: &DeviceDescriptor, | |||
| unsafe_extensions: UnsafeExtensions, | |||
There was a problem hiding this comment.
Actually, let's lift this up to request_adapter.
First, because it already has one native-specific parameter backends, so adding another one doesn't make it less consistent.
Secondly, because the device has to be created with extensions exposed by an adapter anyway. So, if we lift this to adapter request, and the user doesn't give the permission, then the adapter simply doesn't expose any unsafe extensions.
There was a problem hiding this comment.
I think that's reasonable, though we would also need to have it as a parameter on the various list/choose adapter functions and would likely need to be stored as part of the adapter struct itself.
This also makes me realize we never actually validated that the extensions that are requested are a subset of what the adapter exposed
cwfitzgerald
force-pushed
the
extensions-interface
branch
from
0db2c2c
to
fc66e38
Compare
cwfitzgerald
force-pushed
the
extensions-interface
branch
from
fc66e38
to
f32cb10
Compare
|
Alright, everything should be all cleaned up, this time with the right contains/intersects code... hopefully |
| @@ -603,6 +622,20 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> { | |||
| ); | |||
| } | |||
|
|
|||
| // Verify all extensions were exposed by the adapter | |||
| if !adapter.unsafe_extensions.allowed() { | |||
There was a problem hiding this comment.
this isn't needed here, I think. The user can't request extensions that adapter doesn't provide, anyway
There was a problem hiding this comment.
I know it I isn't strictly necessary, but I did it to try to provide a more helpful error messages.
35: update bindings for wgpu #675 and #703 r=kvark,cwfitzgerald a=DevOrc Updates the bindings for gfx-rs/wgpu#703 and gfx-rs/wgpu#675 Co-authored-by: Noah Charlton <ncharlton002@gmail.com>
350: Implement Extensions Interface r=kvark a=cwfitzgerald This implements gfx-rs/wgpu#703 in wgpu-rs. Notable changes include the removal of the anisotropic field from the examples, in favor of the now mandatory `..Default::default()` syntax. Co-authored-by: Connor Fitzgerald <connorwadefitzgerald@gmail.com>
350: Implement Extensions Interface r=kvark a=cwfitzgerald This implements gfx-rs#703 in wgpu-rs. Notable changes include the removal of the anisotropic field from the examples, in favor of the now mandatory `..Default::default()` syntax. Co-authored-by: Connor Fitzgerald <connorwadefitzgerald@gmail.com>
Description
This is the first step of implementing #691. This implements the described changes to all the core structs and to
SamplerDescriptor. I intend this PR to be an example of what the plan looks like. If we think this works, we can roll out the rest of the changes either in bulk or as we need them.The rolling out of this is also tied to the rolling out of #689. Hopefully the macro work done in gfx-rs/wgpu-native#34 will make this easier.
Questions
One outstanding question I had is what the default values for lod_min/max should be. As of right now I just derive default, which puts them at zero, which is probably a reasonable default, though most of the examples use -100, 100 which is normally what you use when doing mipmapping.
TODO: