Ensure consistency between Un/Typed AssetId and Handle (#10628)

# Objective

- Fixes #10629
- `UntypedAssetId` and `AssetId` (along with `UntypedHandle` and
`Handle`) do not hash to the same values when pointing to the same
`Asset`. Additionally, comparison and conversion between these types
does not follow idiomatic Rust standards.

## Solution

- Added new unit tests to validate/document expected behaviour
- Added trait implementations to make working with Un/Typed values more
ergonomic
- Ensured hashing and comparison between Un/Typed values is consistent
- Removed `From` trait implementations that panic, and replaced them
with `TryFrom`

---

## Changelog

- Ensured `Handle::<A>::hash` and `UntypedHandle::hash` will produce the
same value for the same `Asset`
- Added non-panicing `Handle::<A>::try_typed`
- Added `PartialOrd` to `UntypedHandle` to match `Handle<A>` (this will
return `None` for `UntypedHandles` for differing `Asset` types)
- Added `TryFrom<UntypedHandle>` for `Handle<A>`
- Added `From<Handle<A>>` for `UntypedHandle`
- Removed panicing `From<Untyped...>` implementations. These are
currently unused within the Bevy codebase, and shouldn't be used
externally, hence removal.
- Added cross-implementations of `PartialEq` and `PartialOrd` for
`UntypedHandle` and `Handle<A>` allowing direct comparison when
`TypeId`'s match.
- Near-identical changes to `AssetId` and `UntypedAssetId`

## Migration Guide

If you relied on any of the panicing `From<Untyped...>` implementations,
simply call the existing `typed` methods instead. Alternatively, use the
new `TryFrom` implementation instead to directly expose possible
mistakes.

## Notes

I've made these changes since `Handle` is such a fundamental type to the
entire `Asset` ecosystem within Bevy, and yet it had pretty unclear
behaviour with no direct testing. The fact that hashing untyped vs typed
versions of the same handle would produce different values is something
I expect to cause a very subtle bug for someone else one day.

I haven't included it in this PR to avoid any controversy, but I also
believe the `typed_unchecked` methods should be removed from these
types, or marked as `unsafe`. The `texture_atlas` example currently uses
it, and I believe it is a bad choice. The performance gained by not
type-checking before conversion would be entirely dwarfed by the act of
actually loading an asset and creating its handle anyway. If an end user
is in a tight loop repeatedly calling `typed_unchecked` on an
`UntypedHandle` for the entire runtime of their application, I think the
small performance drop caused by that safety check is ~~a form of cosmic
justice~~ reasonable.
This commit is contained in:
Zachary Harrold 2023-11-21 12:13:36 +11:00 committed by GitHub
parent 1da0afa3aa
commit 57a175f546
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 461 additions and 84 deletions

View File

@ -11,6 +11,7 @@ use std::{
hash::{Hash, Hasher},
sync::Arc,
};
use thiserror::Error;
/// Provides [`Handle`] and [`UntypedHandle`] _for a specific asset type_.
/// This should _only_ be used for one specific asset type.
@ -191,10 +192,7 @@ impl<A: Asset> Handle<A> {
/// [`Handle::Weak`].
#[inline]
pub fn untyped(self) -> UntypedHandle {
match self {
Handle::Strong(handle) => UntypedHandle::Strong(handle),
Handle::Weak(id) => UntypedHandle::Weak(id.untyped()),
}
self.into()
}
}
@ -224,13 +222,14 @@ impl<A: Asset> std::fmt::Debug for Handle<A> {
impl<A: Asset> Hash for Handle<A> {
#[inline]
fn hash<H: Hasher>(&self, state: &mut H) {
Hash::hash(&self.id(), state);
self.id().hash(state);
TypeId::of::<A>().hash(state);
}
}
impl<A: Asset> PartialOrd for Handle<A> {
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
Some(self.id().cmp(&other.id()))
Some(self.cmp(other))
}
}
@ -249,6 +248,34 @@ impl<A: Asset> PartialEq for Handle<A> {
impl<A: Asset> Eq for Handle<A> {}
impl<A: Asset> From<Handle<A>> for AssetId<A> {
#[inline]
fn from(value: Handle<A>) -> Self {
value.id()
}
}
impl<A: Asset> From<&Handle<A>> for AssetId<A> {
#[inline]
fn from(value: &Handle<A>) -> Self {
value.id()
}
}
impl<A: Asset> From<Handle<A>> for UntypedAssetId {
#[inline]
fn from(value: Handle<A>) -> Self {
value.id().into()
}
}
impl<A: Asset> From<&Handle<A>> for UntypedAssetId {
#[inline]
fn from(value: &Handle<A>) -> Self {
value.id().into()
}
}
/// An untyped variant of [`Handle`], which internally stores the [`Asset`] type information at runtime
/// as a [`TypeId`] instead of encoding it in the compile-time type. This allows handles across [`Asset`] types
/// to be stored together and compared.
@ -326,12 +353,20 @@ impl UntypedHandle {
/// Converts to a typed Handle. This will panic if the internal [`TypeId`] does not match the given asset type `A`
#[inline]
pub fn typed<A: Asset>(self) -> Handle<A> {
assert_eq!(
self.type_id(),
TypeId::of::<A>(),
"The target Handle<A>'s TypeId does not match the TypeId of this UntypedHandle"
);
self.typed_unchecked()
let Ok(handle) = self.try_typed() else {
panic!(
"The target Handle<{}>'s TypeId does not match the TypeId of this UntypedHandle",
std::any::type_name::<A>()
)
};
handle
}
/// Converts to a typed Handle. This will panic if the internal [`TypeId`] does not match the given asset type `A`
#[inline]
pub fn try_typed<A: Asset>(self) -> Result<Handle<A>, UntypedAssetConversionError> {
Handle::try_from(self)
}
/// The "meta transform" for the strong handle. This will only be [`Some`] if the handle is strong and there is a meta transform
@ -383,3 +418,212 @@ impl std::fmt::Debug for UntypedHandle {
}
}
}
impl PartialOrd for UntypedHandle {
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
if self.type_id() == other.type_id() {
self.id().partial_cmp(&other.id())
} else {
None
}
}
}
impl From<UntypedHandle> for UntypedAssetId {
#[inline]
fn from(value: UntypedHandle) -> Self {
value.id()
}
}
impl From<&UntypedHandle> for UntypedAssetId {
#[inline]
fn from(value: &UntypedHandle) -> Self {
value.id()
}
}
// Cross Operations
impl<A: Asset> PartialEq<UntypedHandle> for Handle<A> {
#[inline]
fn eq(&self, other: &UntypedHandle) -> bool {
TypeId::of::<A>() == other.type_id() && self.id() == other.id()
}
}
impl<A: Asset> PartialEq<Handle<A>> for UntypedHandle {
#[inline]
fn eq(&self, other: &Handle<A>) -> bool {
other.eq(self)
}
}
impl<A: Asset> PartialOrd<UntypedHandle> for Handle<A> {
#[inline]
fn partial_cmp(&self, other: &UntypedHandle) -> Option<std::cmp::Ordering> {
if TypeId::of::<A>() != other.type_id() {
None
} else {
self.id().partial_cmp(&other.id())
}
}
}
impl<A: Asset> PartialOrd<Handle<A>> for UntypedHandle {
#[inline]
fn partial_cmp(&self, other: &Handle<A>) -> Option<std::cmp::Ordering> {
Some(other.partial_cmp(self)?.reverse())
}
}
impl<A: Asset> From<Handle<A>> for UntypedHandle {
fn from(value: Handle<A>) -> Self {
match value {
Handle::Strong(handle) => UntypedHandle::Strong(handle),
Handle::Weak(id) => UntypedHandle::Weak(id.into()),
}
}
}
impl<A: Asset> TryFrom<UntypedHandle> for Handle<A> {
type Error = UntypedAssetConversionError;
fn try_from(value: UntypedHandle) -> Result<Self, Self::Error> {
let found = value.type_id();
let expected = TypeId::of::<A>();
if found != expected {
return Err(UntypedAssetConversionError::TypeIdMismatch { expected, found });
}
match value {
UntypedHandle::Strong(handle) => Ok(Handle::Strong(handle)),
UntypedHandle::Weak(id) => {
let Ok(id) = id.try_into() else {
return Err(UntypedAssetConversionError::TypeIdMismatch { expected, found });
};
Ok(Handle::Weak(id))
}
}
}
}
/// Errors preventing the conversion of to/from an [`UntypedHandle`] and an [`Handle`].
#[derive(Error, Debug, PartialEq, Clone)]
#[non_exhaustive]
pub enum UntypedAssetConversionError {
/// Caused when trying to convert an [`UntypedHandle`] into an [`Handle`] of the wrong type.
#[error(
"This UntypedHandle is for {found:?} and cannot be converted into an Handle<{expected:?}>"
)]
TypeIdMismatch { expected: TypeId, found: TypeId },
}
#[cfg(test)]
mod tests {
use super::*;
type TestAsset = ();
const UUID_1: Uuid = Uuid::from_u128(123);
const UUID_2: Uuid = Uuid::from_u128(456);
/// Simple utility to directly hash a value using a fixed hasher
fn hash<T: Hash>(data: &T) -> u64 {
let mut hasher = bevy_utils::AHasher::default();
data.hash(&mut hasher);
hasher.finish()
}
/// Typed and Untyped `Handles` should be equivalent to each other and themselves
#[test]
fn equality() {
let typed = AssetId::<TestAsset>::Uuid { uuid: UUID_1 };
let untyped = UntypedAssetId::Uuid {
type_id: TypeId::of::<TestAsset>(),
uuid: UUID_1,
};
let typed = Handle::Weak(typed);
let untyped = UntypedHandle::Weak(untyped);
assert_eq!(
Ok(typed.clone()),
Handle::<TestAsset>::try_from(untyped.clone())
);
assert_eq!(UntypedHandle::from(typed.clone()), untyped);
assert_eq!(typed, untyped);
}
/// Typed and Untyped `Handles` should be orderable amongst each other and themselves
#[allow(clippy::cmp_owned)]
#[test]
fn ordering() {
assert!(UUID_1 < UUID_2);
let typed_1 = AssetId::<TestAsset>::Uuid { uuid: UUID_1 };
let typed_2 = AssetId::<TestAsset>::Uuid { uuid: UUID_2 };
let untyped_1 = UntypedAssetId::Uuid {
type_id: TypeId::of::<TestAsset>(),
uuid: UUID_1,
};
let untyped_2 = UntypedAssetId::Uuid {
type_id: TypeId::of::<TestAsset>(),
uuid: UUID_2,
};
let typed_1 = Handle::Weak(typed_1);
let typed_2 = Handle::Weak(typed_2);
let untyped_1 = UntypedHandle::Weak(untyped_1);
let untyped_2 = UntypedHandle::Weak(untyped_2);
assert!(typed_1 < typed_2);
assert!(untyped_1 < untyped_2);
assert!(UntypedHandle::from(typed_1.clone()) < untyped_2);
assert!(untyped_1 < UntypedHandle::from(typed_2.clone()));
assert!(Handle::<TestAsset>::try_from(untyped_1.clone()).unwrap() < typed_2);
assert!(typed_1 < Handle::<TestAsset>::try_from(untyped_2.clone()).unwrap());
assert!(typed_1 < untyped_2);
assert!(untyped_1 < typed_2);
}
/// Typed and Untyped `Handles` should be equivalently hashable to each other and themselves
#[test]
fn hashing() {
let typed = AssetId::<TestAsset>::Uuid { uuid: UUID_1 };
let untyped = UntypedAssetId::Uuid {
type_id: TypeId::of::<TestAsset>(),
uuid: UUID_1,
};
let typed = Handle::Weak(typed);
let untyped = UntypedHandle::Weak(untyped);
assert_eq!(
hash(&typed),
hash(&Handle::<TestAsset>::try_from(untyped.clone()).unwrap())
);
assert_eq!(hash(&UntypedHandle::from(typed.clone())), hash(&untyped));
assert_eq!(hash(&typed), hash(&untyped));
}
/// Typed and Untyped `Handles` should be interchangeable
#[test]
fn conversion() {
let typed = AssetId::<TestAsset>::Uuid { uuid: UUID_1 };
let untyped = UntypedAssetId::Uuid {
type_id: TypeId::of::<TestAsset>(),
uuid: UUID_1,
};
let typed = Handle::Weak(typed);
let untyped = UntypedHandle::Weak(untyped);
assert_eq!(typed, Handle::try_from(untyped.clone()).unwrap());
assert_eq!(UntypedHandle::from(typed.clone()), untyped);
}
}

View File

@ -1,4 +1,4 @@
use crate::{Asset, AssetIndex, Handle, UntypedHandle};
use crate::{Asset, AssetIndex};
use bevy_reflect::{Reflect, Uuid};
use std::{
any::TypeId,
@ -6,11 +6,12 @@ use std::{
hash::Hash,
marker::PhantomData,
};
use thiserror::Error;
/// A unique runtime-only identifier for an [`Asset`]. This is cheap to [`Copy`]/[`Clone`] and is not directly tied to the
/// lifetime of the Asset. This means it _can_ point to an [`Asset`] that no longer exists.
///
/// For an identifier tied to the lifetime of an asset, see [`Handle`].
/// For an identifier tied to the lifetime of an asset, see [`Handle`](`crate::Handle`).
///
/// For an "untyped" / "generic-less" id, see [`UntypedAssetId`].
#[derive(Reflect)]
@ -53,16 +54,7 @@ impl<A: Asset> AssetId<A> {
/// _inside_ the [`UntypedAssetId`].
#[inline]
pub fn untyped(self) -> UntypedAssetId {
match self {
AssetId::Index { index, .. } => UntypedAssetId::Index {
index,
type_id: TypeId::of::<A>(),
},
AssetId::Uuid { uuid } => UntypedAssetId::Uuid {
uuid,
type_id: TypeId::of::<A>(),
},
}
self.into()
}
#[inline]
@ -95,6 +87,7 @@ impl<A: Asset> Display for AssetId<A> {
Debug::fmt(self, f)
}
}
impl<A: Asset> Debug for AssetId<A> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
@ -123,6 +116,7 @@ impl<A: Asset> Hash for AssetId<A> {
#[inline]
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
self.internal().hash(state);
TypeId::of::<A>().hash(state);
}
}
@ -164,52 +158,10 @@ impl<A: Asset> From<Uuid> for AssetId<A> {
}
}
impl<A: Asset> From<Handle<A>> for AssetId<A> {
#[inline]
fn from(value: Handle<A>) -> Self {
value.id()
}
}
impl<A: Asset> From<&Handle<A>> for AssetId<A> {
#[inline]
fn from(value: &Handle<A>) -> Self {
value.id()
}
}
impl<A: Asset> From<UntypedHandle> for AssetId<A> {
#[inline]
fn from(value: UntypedHandle) -> Self {
value.id().typed()
}
}
impl<A: Asset> From<&UntypedHandle> for AssetId<A> {
#[inline]
fn from(value: &UntypedHandle) -> Self {
value.id().typed()
}
}
impl<A: Asset> From<UntypedAssetId> for AssetId<A> {
#[inline]
fn from(value: UntypedAssetId) -> Self {
value.typed()
}
}
impl<A: Asset> From<&UntypedAssetId> for AssetId<A> {
#[inline]
fn from(value: &UntypedAssetId) -> Self {
value.typed()
}
}
/// An "untyped" / "generic-less" [`Asset`] identifier that behaves much like [`AssetId`], but stores the [`Asset`] type
/// information at runtime instead of compile-time. This increases the size of the type, but it enables storing asset ids
/// across asset types together and enables comparisons between them.
#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)]
#[derive(Debug, Copy, Clone)]
pub enum UntypedAssetId {
/// A small / efficient runtime identifier that can be used to efficiently look up an asset stored in [`Assets`]. This is
/// the "default" identifier used for assets. The alternative(s) (ex: [`UntypedAssetId::Uuid`]) will only be used if assets are
@ -263,13 +215,20 @@ impl UntypedAssetId {
/// Panics if the [`TypeId`] of `A` does not match the stored type id.
#[inline]
pub fn typed<A: Asset>(self) -> AssetId<A> {
assert_eq!(
self.type_id(),
TypeId::of::<A>(),
"The target AssetId<{}>'s TypeId does not match the TypeId of this UntypedAssetId",
std::any::type_name::<A>()
);
self.typed_unchecked()
let Ok(id) = self.try_typed() else {
panic!(
"The target AssetId<{}>'s TypeId does not match the TypeId of this UntypedAssetId",
std::any::type_name::<A>()
)
};
id
}
/// Try to convert this to a "typed" [`AssetId`].
#[inline]
pub fn try_typed<A: Asset>(self) -> Result<AssetId<A>, UntypedAssetIdConversionError> {
AssetId::try_from(self)
}
/// Returns the stored [`TypeId`] of the referenced [`Asset`].
@ -309,24 +268,30 @@ impl Display for UntypedAssetId {
}
}
impl<A: Asset> From<AssetId<A>> for UntypedAssetId {
impl PartialEq for UntypedAssetId {
#[inline]
fn from(value: AssetId<A>) -> Self {
value.untyped()
fn eq(&self, other: &Self) -> bool {
self.internal().eq(&other.internal())
}
}
impl<A: Asset> From<Handle<A>> for UntypedAssetId {
impl Eq for UntypedAssetId {}
impl Hash for UntypedAssetId {
#[inline]
fn from(value: Handle<A>) -> Self {
value.id().untyped()
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
self.internal().hash(state);
self.type_id().hash(state);
}
}
impl<A: Asset> From<&Handle<A>> for UntypedAssetId {
#[inline]
fn from(value: &Handle<A>) -> Self {
value.id().untyped()
impl PartialOrd for UntypedAssetId {
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
if self.type_id() != other.type_id() {
None
} else {
Some(self.internal().cmp(&other.internal()))
}
}
}
@ -375,3 +340,171 @@ impl From<Uuid> for InternalAssetId {
Self::Uuid(value)
}
}
// Cross Operations
impl<A: Asset> PartialEq<UntypedAssetId> for AssetId<A> {
#[inline]
fn eq(&self, other: &UntypedAssetId) -> bool {
TypeId::of::<A>() == other.type_id() && self.internal().eq(&other.internal())
}
}
impl<A: Asset> PartialEq<AssetId<A>> for UntypedAssetId {
#[inline]
fn eq(&self, other: &AssetId<A>) -> bool {
other.eq(self)
}
}
impl<A: Asset> PartialOrd<UntypedAssetId> for AssetId<A> {
#[inline]
fn partial_cmp(&self, other: &UntypedAssetId) -> Option<std::cmp::Ordering> {
if TypeId::of::<A>() != other.type_id() {
None
} else {
Some(self.internal().cmp(&other.internal()))
}
}
}
impl<A: Asset> PartialOrd<AssetId<A>> for UntypedAssetId {
#[inline]
fn partial_cmp(&self, other: &AssetId<A>) -> Option<std::cmp::Ordering> {
Some(other.partial_cmp(self)?.reverse())
}
}
impl<A: Asset> From<AssetId<A>> for UntypedAssetId {
#[inline]
fn from(value: AssetId<A>) -> Self {
let type_id = TypeId::of::<A>();
match value {
AssetId::Index { index, .. } => UntypedAssetId::Index { type_id, index },
AssetId::Uuid { uuid } => UntypedAssetId::Uuid { type_id, uuid },
}
}
}
impl<A: Asset> TryFrom<UntypedAssetId> for AssetId<A> {
type Error = UntypedAssetIdConversionError;
#[inline]
fn try_from(value: UntypedAssetId) -> Result<Self, Self::Error> {
let found = value.type_id();
let expected = TypeId::of::<A>();
match value {
UntypedAssetId::Index { index, type_id } if type_id == expected => Ok(AssetId::Index {
index,
marker: PhantomData,
}),
UntypedAssetId::Uuid { uuid, type_id } if type_id == expected => {
Ok(AssetId::Uuid { uuid })
}
_ => Err(UntypedAssetIdConversionError::TypeIdMismatch { expected, found }),
}
}
}
/// Errors preventing the conversion of to/from an [`UntypedAssetId`] and an [`AssetId`].
#[derive(Error, Debug, PartialEq, Clone)]
#[non_exhaustive]
pub enum UntypedAssetIdConversionError {
/// Caused when trying to convert an [`UntypedAssetId`] into an [`AssetId`] of the wrong type.
#[error("This UntypedAssetId is for {found:?} and cannot be converted into an AssetId<{expected:?}>")]
TypeIdMismatch { expected: TypeId, found: TypeId },
}
#[cfg(test)]
mod tests {
use super::*;
type TestAsset = ();
const UUID_1: Uuid = Uuid::from_u128(123);
const UUID_2: Uuid = Uuid::from_u128(456);
/// Simple utility to directly hash a value using a fixed hasher
fn hash<T: Hash>(data: &T) -> u64 {
use std::hash::Hasher;
let mut hasher = bevy_utils::AHasher::default();
data.hash(&mut hasher);
hasher.finish()
}
/// Typed and Untyped `AssetIds` should be equivalent to each other and themselves
#[test]
fn equality() {
let typed = AssetId::<TestAsset>::Uuid { uuid: UUID_1 };
let untyped = UntypedAssetId::Uuid {
type_id: TypeId::of::<TestAsset>(),
uuid: UUID_1,
};
assert_eq!(Ok(typed), AssetId::try_from(untyped));
assert_eq!(UntypedAssetId::from(typed), untyped);
assert_eq!(typed, untyped);
}
/// Typed and Untyped `AssetIds` should be orderable amongst each other and themselves
#[test]
fn ordering() {
assert!(UUID_1 < UUID_2);
let typed_1 = AssetId::<TestAsset>::Uuid { uuid: UUID_1 };
let typed_2 = AssetId::<TestAsset>::Uuid { uuid: UUID_2 };
let untyped_1 = UntypedAssetId::Uuid {
type_id: TypeId::of::<TestAsset>(),
uuid: UUID_1,
};
let untyped_2 = UntypedAssetId::Uuid {
type_id: TypeId::of::<TestAsset>(),
uuid: UUID_2,
};
assert!(typed_1 < typed_2);
assert!(untyped_1 < untyped_2);
assert!(UntypedAssetId::from(typed_1) < untyped_2);
assert!(untyped_1 < UntypedAssetId::from(typed_2));
assert!(AssetId::try_from(untyped_1).unwrap() < typed_2);
assert!(typed_1 < AssetId::try_from(untyped_2).unwrap());
assert!(typed_1 < untyped_2);
assert!(untyped_1 < typed_2);
}
/// Typed and Untyped `AssetIds` should be equivalently hashable to each other and themselves
#[test]
fn hashing() {
let typed = AssetId::<TestAsset>::Uuid { uuid: UUID_1 };
let untyped = UntypedAssetId::Uuid {
type_id: TypeId::of::<TestAsset>(),
uuid: UUID_1,
};
assert_eq!(
hash(&typed),
hash(&AssetId::<TestAsset>::try_from(untyped).unwrap())
);
assert_eq!(hash(&UntypedAssetId::from(typed)), hash(&untyped));
assert_eq!(hash(&typed), hash(&untyped));
}
/// Typed and Untyped `AssetIds` should be interchangeable
#[test]
fn conversion() {
let typed = AssetId::<TestAsset>::Uuid { uuid: UUID_1 };
let untyped = UntypedAssetId::Uuid {
type_id: TypeId::of::<TestAsset>(),
uuid: UUID_1,
};
assert_eq!(Ok(typed), AssetId::try_from(untyped));
assert_eq!(UntypedAssetId::from(typed), untyped);
}
}