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

Ensure consistency between Un/Typed AssetId and Handle #10628

Merged

Conversation

bushrat011899
Copy link
Contributor

@bushrat011899 bushrat011899 commented Nov 18, 2023

Objective

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.

…dle`/`Handle`

- 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
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Assets Load files from disk to use for things like images, models, and sounds C-Code-Quality A section of code that is hard to understand or change labels Nov 18, 2023
@ItsDoot ItsDoot added C-Enhancement A new feature C-Bug An unexpected or incorrect behavior and removed C-Bug An unexpected or incorrect behavior C-Enhancement A new feature labels Nov 18, 2023
Copy link
Contributor

@KirmesBude KirmesBude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can not comment on the implementation (looks fine though), but I ran into this mismatch of (untyped) TypeIds and these changes fix the problem. Thanks :)

Is it possible to separate the underlying change from the breaking changes for potential inclusion in 0.12.1? (Whether that is something actually worth pursuing, I will leave up to someone else, but I would appreciate it :D)

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that cleaning up the trait impls using the Borrow trait is nice, but we can do that in a different PR if you prefer.

On board with fixing this, and the code and tests are good.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Nov 18, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Nov 19, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 19, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Nov 21, 2023
Merged via the queue into bevyengine:main with commit 57a175f Nov 21, 2023
28 checks passed
@nicopap nicopap added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label Nov 22, 2023
@nicopap nicopap added this to the 0.13 milestone Nov 22, 2023
inodentry pushed a commit to TheSeekerGame/bevy that referenced this pull request Jan 4, 2024
…e#10628)

# Objective

- Fixes bevyengine#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.
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
…e#10628)

# Objective

- Fixes bevyengine#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Bug An unexpected or incorrect behavior C-Code-Quality A section of code that is hard to understand or change S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UntypedAssetId and AssetId<A> have different hashes for the same Asset
5 participants