Make ObjectId structure and invariants idiomatic (#3347)

CHANGELOG.md

@@ -122,6 +122,7 @@ Additionally `Surface::get_default_config` now returns an Option and returns Non
 - Dereferencing a buffer view is now marked inline. By @Wumpf in [#3307](https://github.com/gfx-rs/wgpu/pull/3307)
 - The `strict_assert` family of macros was moved to `wgpu-types`. By @i509VCB in [#3051](https://github.com/gfx-rs/wgpu/pull/3051)
 - Add missing `DEPTH_BIAS_CLAMP` and `FULL_DRAW_INDEX_UINT32` downlevel flags. By @teoxoy in [#3316](https://github.com/gfx-rs/wgpu/pull/3316)
+- Make `ObjectId` structure and invariants idiomatic. By @teoxoy in [#3347](https://github.com/gfx-rs/wgpu/pull/3347)
 
 #### WebGPU
 

wgpu-core/src/id.rs

@@ -172,18 +172,13 @@ pub(crate) struct Valid<I>(pub I);
 /// need to construct `Id` values directly, or access their components, like the
 /// WGPU recording player, may use this trait to do so.
 pub trait TypedId: Copy {
- fn as_raw(&self) -> NonZeroId;
  fn zip(index: Index, epoch: Epoch, backend: Backend) -> Self;
  fn unzip(self) -> (Index, Epoch, Backend);
  fn into_raw(self) -> NonZeroId;
 }
 
 #[allow(trivial_numeric_casts)]
 impl<T> TypedId for Id<T> {
- fn as_raw(&self) -> NonZeroId {
- self.0
- }
-
  fn zip(index: Index, epoch: Epoch, backend: Backend) -> Self {
  assert_eq!(0, epoch >> EPOCH_BITS);
  assert_eq!(0, (index as IdType) >> INDEX_BITS);

wgpu/Cargo.toml

@@ -80,7 +80,7 @@ name = "stencil-triangles"
 test = true
 
 [features]
-default = ["wgsl", "expose-ids"]
+default = ["wgsl"]
 # Apply run-time checks, even in release builds. These are in addition
 # to the validation carried out at public APIs in all builds.
 strict_asserts = ["wgc?/strict_asserts", "wgt/strict_asserts"]

wgpu/src/backend/direct.rs

@@ -2885,22 +2885,21 @@ impl crate::Context for Context {
 }
 
 impl<T> From<ObjectId> for wgc::id::Id<T> {
- // If the id32 feature is enabled, this conversion is not useless.
- #[allow(clippy::useless_conversion)]
  fn from(id: ObjectId) -> Self {
- let raw = std::num::NonZeroU128::from(id);
- // If the id32 feature is enabled, this will truncate the id to a NonZeroU32.
- let id = raw.try_into().expect("Id exceeded 32-bits");
- // FIXME: This is not safe
+ // If the id32 feature is enabled in wgpu-core, this will make sure that the id fits in a NonZeroU32.
+ #[allow(clippy::useless_conversion)]
+ let id = id.id().try_into().expect("Id exceeded 32-bits");
+ // SAFETY: The id was created via the impl below
  unsafe { Self::from_raw(id) }
  }
 }
 
 impl<T> From<wgc::id::Id<T>> for ObjectId {
- // If the id32 feature is enabled, this conversion is not useless.
- #[allow(clippy::useless_conversion)]
  fn from(id: wgc::id::Id<T>) -> Self {
- ObjectId::from(std::num::NonZeroU128::from(id.as_raw()))
+ // If the id32 feature is enabled in wgpu-core, the conversion is not useless
+ #[allow(clippy::useless_conversion)]
+ let id = id.into_raw().into();
+ Self::from_global_id(id)
  }
 }
 

wgpu/src/backend/web.rs

@@ -6,7 +6,6 @@ use std::{
  cell::RefCell,
  fmt,
  future::Future,
- num::NonZeroU128,
  ops::Range,
  pin::Pin,
  rc::Rc,
@@ -26,10 +25,11 @@ use crate::{
 fn create_identified<T>(value: T) -> Identified<T> {
  cfg_if::cfg_if! {
  if #[cfg(feature = "expose-ids")] {
- static NEXT_ID: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0);
- Identified(value, NEXT_ID.fetch_add(1, std::sync::atomic::Ordering::Relaxed))
+ static NEXT_ID: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(1);
+ let id = NEXT_ID.fetch_add(1, std::sync::atomic::Ordering::Relaxed);
+ Identified(value, crate::Id(core::num::NonZeroU64::new(id).unwrap()))
  } else {
- Identified(value, 0)
+ Identified(value)
  }
  }
 }
@@ -44,25 +44,30 @@ fn create_identified<T>(value: T) -> Identified<T> {
 
 impl<T: FromWasmAbi<Abi = u32> + JsCast> From<ObjectId> for Identified<T> {
  fn from(id: ObjectId) -> Self {
- let raw = std::num::NonZeroU128::from(id);
- let global_id = (raw.get() >> 64) as u64;
-
+ let id = id.id().get() as u32;
  // SAFETY: wasm_bindgen says an ABI representation may only be cast to a wrapper type if it was created
  // using into_abi.
  //
  // This assumption we sadly have to assume to prevent littering the code with unsafe blocks.
- let wasm = unsafe { JsValue::from_abi(raw.get() as u32) };
+ let wasm = unsafe { JsValue::from_abi(id) };
  wgt::strict_assert!(wasm.is_instance_of::<T>());
  // SAFETY: The ABI of the type must be a u32, and strict asserts ensure the right type is used.
- Self(wasm.unchecked_into(), global_id)
+ Self(
+ wasm.unchecked_into(),
+ #[cfg(feature = "expose-ids")]
+ id.global_id(),
+ )
  }
 }
 
 impl<T: IntoWasmAbi<Abi = u32>> From<Identified<T>> for ObjectId {
  fn from(id: Identified<T>) -> Self {
- let abi = id.0.into_abi();
- let id = abi as u128 | ((id.1 as u128) << 64);
- Self::from(NonZeroU128::new(id).unwrap())
+ let id = core::num::NonZeroU64::new(id.0.into_abi() as u64).unwrap();
+ Self::new(
+ id,
+ #[cfg(feature = "expose-ids")]
+ id.1,
+ )
  }
 }
 
@@ -72,7 +77,7 @@ unsafe impl<T> Send for Sendable<T> {}
 unsafe impl<T> Sync for Sendable<T> {}
 
 #[derive(Clone, Debug)]
-pub(crate) struct Identified<T>(T, #[cfg(feature = "expose-ids")] u64);
+pub(crate) struct Identified<T>(T, #[cfg(feature = "expose-ids")] crate::Id);
 unsafe impl<T> Send for Identified<T> {}
 unsafe impl<T> Sync for Identified<T> {}
 

wgpu/src/context.rs

@@ -1,6 +1,4 @@
-use std::{
- any::Any, fmt::Debug, future::Future, num::NonZeroU128, ops::Range, pin::Pin, sync::Arc,
-};
+use std::{any::Any, fmt::Debug, future::Future, num::NonZeroU64, ops::Range, pin::Pin, sync::Arc};
 
 use wgt::{
  strict_assert, strict_assert_eq, AdapterInfo, BufferAddress, BufferSize, Color,
@@ -985,27 +983,47 @@ pub trait Context: Debug + Send + Sized + Sync {
 }
 
 /// Object id.
-///
-/// An ObjectId is a 128-bit number internally, where the first 64-bits represent a backend internal id and
-/// the last 64-bits are a global id.
-#[derive(Debug, Clone, Copy)]
-pub struct ObjectId(NonZeroU128);
+#[derive(Debug, Copy, Clone, PartialEq, Eq)]
+pub struct ObjectId {
+ /// ID that is unique at any given time
+ id: Option<NonZeroU64>,
+ #[cfg(feature = "expose-ids")]
+ /// ID that is unique at all times
+ global_id: Option<crate::Id>,
+}
 
 impl ObjectId {
- pub fn global_id(&self) -> u64 {
- (self.0.get() >> 64) as u64
+ const UNUSED: Self = ObjectId {
+ id: None,
+ #[cfg(feature = "expose-ids")]
+ global_id: None,
+ };
+
+ pub fn new(id: NonZeroU64, #[cfg(feature = "expose-ids")] global_id: crate::Id) -> Self {
+ Self {
+ id: Some(id),
+ #[cfg(feature = "expose-ids")]
+ global_id: Some(global_id),
+ }
  }
-}
 
-impl From<NonZeroU128> for ObjectId {
- fn from(raw: NonZeroU128) -> Self {
- Self(raw)
+ #[allow(dead_code)]
+ pub fn from_global_id(global_id: NonZeroU64) -> Self {
+ Self {
+ id: Some(global_id),
+ #[cfg(feature = "expose-ids")]
+ global_id: Some(crate::Id(global_id)),
+ }
  }
-}
 
-impl From<ObjectId> for NonZeroU128 {
- fn from(id: ObjectId) -> Self {
- id.0
+ pub fn id(&self) -> NonZeroU64 {
+ self.id.unwrap()
+ }
+
+ #[cfg(feature = "expose-ids")]
+ #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))]
+ pub fn global_id(&self) -> crate::Id {
+ self.global_id.unwrap()
  }
 }
 
@@ -1029,18 +1047,16 @@ fn downcast_mut<T: Debug + Send + Sync + 'static>(data: &mut crate::Data) -> &mu
 #[derive(Debug, Clone, Copy)]
 pub struct Unused;
 
-const UNUSED_SENTINEL: Option<NonZeroU128> = NonZeroU128::new(u128::MAX);
-
 impl From<ObjectId> for Unused {
  fn from(id: ObjectId) -> Self {
- strict_assert_eq!(Some(NonZeroU128::from(id)), UNUSED_SENTINEL);
+ strict_assert_eq!(id, ObjectId::UNUSED);
  Self
  }
 }
 
 impl From<Unused> for ObjectId {
  fn from(_: Unused) -> Self {
- ObjectId::from(UNUSED_SENTINEL.expect("This should never panic"))
+ ObjectId::UNUSED
  }
 }
 

wgpu/src/lib.rs

@@ -4027,7 +4027,7 @@ impl Surface {
 #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))]
 #[repr(transparent)]
 #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
-pub struct Id(u64);
+pub struct Id(core::num::NonZeroU64);
 
 #[cfg(feature = "expose-ids")]
 impl Adapter {
@@ -4037,7 +4037,7 @@ impl Adapter {
  /// The returned value is guaranteed to be different for all resources created from the same `Instance`.
  #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))]
  pub fn global_id(&self) -> Id {
- Id(self.id.global_id())
+ self.id.global_id()
  }
 }
 
@@ -4049,7 +4049,7 @@ impl Device {
  /// The returned value is guaranteed to be different for all resources created from the same `Instance`.
  #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))]
  pub fn global_id(&self) -> Id {
- Id(self.id.global_id())
+ self.id.global_id()
  }
 }
 
@@ -4061,7 +4061,7 @@ impl Queue {
  /// The returned value is guaranteed to be different for all resources created from the same `Instance`.
  #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))]
  pub fn global_id(&self) -> Id {
- Id(self.id.global_id())
+ self.id.global_id()
  }
 }
 
@@ -4073,7 +4073,7 @@ impl ShaderModule {
  /// The returned value is guaranteed to be different for all resources created from the same `Instance`.
  #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))]
  pub fn global_id(&self) -> Id {
- Id(self.id.global_id())
+ self.id.global_id()
  }
 }
 
@@ -4085,7 +4085,7 @@ impl BindGroupLayout {
  /// The returned value is guaranteed to be different for all resources created from the same `Instance`.
  #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))]
  pub fn global_id(&self) -> Id {
- Id(self.id.global_id())
+ self.id.global_id()
  }
 }
 
@@ -4097,7 +4097,7 @@ impl BindGroup {
  /// The returned value is guaranteed to be different for all resources created from the same `Instance`.
  #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))]
  pub fn global_id(&self) -> Id {
- Id(self.id.global_id())
+ self.id.global_id()
  }
 }
 
@@ -4109,7 +4109,7 @@ impl TextureView {
  /// The returned value is guaranteed to be different for all resources created from the same `Instance`.
  #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))]
  pub fn global_id(&self) -> Id {
- Id(self.id.global_id())
+ self.id.global_id()
  }
 }
 
@@ -4121,7 +4121,7 @@ impl Sampler {
  /// The returned value is guaranteed to be different for all resources created from the same `Instance`.
  #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))]
  pub fn global_id(&self) -> Id {
- Id(self.id.global_id())
+ self.id.global_id()
  }
 }
 
@@ -4133,7 +4133,7 @@ impl Buffer {
  /// The returned value is guaranteed to be different for all resources created from the same `Instance`.
  #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))]
  pub fn global_id(&self) -> Id {
- Id(self.id.global_id())
+ self.id.global_id()
  }
 }
 
@@ -4145,7 +4145,7 @@ impl Texture {
  /// The returned value is guaranteed to be different for all resources created from the same `Instance`.
  #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))]
  pub fn global_id(&self) -> Id {
- Id(self.id.global_id())
+ self.id.global_id()
  }
 }
 
@@ -4157,7 +4157,7 @@ impl QuerySet {
  /// The returned value is guaranteed to be different for all resources created from the same `Instance`.
  #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))]
  pub fn global_id(&self) -> Id {
- Id(self.id.global_id())
+ self.id.global_id()
  }
 }
 
@@ -4169,7 +4169,7 @@ impl PipelineLayout {
  /// The returned value is guaranteed to be different for all resources created from the same `Instance`.
  #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))]
  pub fn global_id(&self) -> Id {
- Id(self.id.global_id())
+ self.id.global_id()
  }
 }
 
@@ -4181,7 +4181,7 @@ impl RenderPipeline {
  /// The returned value is guaranteed to be different for all resources created from the same `Instance`.
  #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))]
  pub fn global_id(&self) -> Id {
- Id(self.id.global_id())
+ self.id.global_id()
  }
 }
 
@@ -4193,7 +4193,7 @@ impl ComputePipeline {
  /// The returned value is guaranteed to be different for all resources created from the same `Instance`.
  #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))]
  pub fn global_id(&self) -> Id {
- Id(self.id.global_id())
+ self.id.global_id()
  }
 }
 
@@ -4205,7 +4205,7 @@ impl RenderBundle {
  /// The returned value is guaranteed to be different for all resources created from the same `Instance`.
  #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))]
  pub fn global_id(&self) -> Id {
- Id(self.id.global_id())
+ self.id.global_id()
  }
 }
 
@@ -4217,7 +4217,7 @@ impl Surface {
  /// The returned value is guaranteed to be different for all resources created from the same `Instance`.
  #[cfg_attr(docsrs, doc(cfg(feature = "expose-ids")))]
  pub fn global_id(&self) -> Id {
- Id(self.id.global_id())
+ self.id.global_id()
  }
 }