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

Fix Asset Loading Bug #10698

Merged

Conversation

bushrat011899
Copy link
Contributor

@bushrat011899 bushrat011899 commented Nov 22, 2023

Objective

Solution

Fixed obvious blunder in PartialEq implementation for UntypedAssetId's where the TypeId was not included in the comparison. This was not picked up in the unit tests I added because they only tested over a single asset type.

Caused by improper comparison between `UntypedAssetId`'s
@nicopap nicopap added C-Bug An unexpected or incorrect behavior A-Assets Load files from disk to use for things like images, models, and sounds labels Nov 22, 2023
@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 22, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Nov 22, 2023
Merged via the queue into bevyengine:main with commit f613c45 Nov 22, 2023
26 checks passed
@DasLixou
Copy link
Contributor

@bushrat011899 does that also apply here? https://github.com/bevyengine/bevy/blob/main/crates/bevy_asset/src/id.rs#L356 I don't get which eq is called here but the eq from Asset doesn't check for typeid

@bushrat011899
Copy link
Contributor Author

No, that line calls the PartialEq implementation directly above it. https://github.com/bevyengine/bevy/blob/main/crates/bevy_asset/src/id.rs#L349-L356

rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

- Fixes bevyengine#10695

## Solution

Fixed obvious blunder in `PartialEq` implementation for
`UntypedAssetId`'s where the `TypeId` was not included in the
comparison. This was not picked up in the unit tests I added because
they only tested over a single asset type.
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-Bug An unexpected or incorrect behavior 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.

Bug in asset loading
4 participants