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

[Merged by Bors] - Useful error message when two assets have the save UUID #3739

Conversation

ShadowCurse
Copy link
Contributor

Objective

Fixes #2610 and #3731

Solution

Added check for TYPE_UUID duplication in register_asset_type with an error message

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jan 21, 2022
@DJMcNab
Copy link
Member

DJMcNab commented Jan 21, 2022

I think we should check whether the two assets actually have different types, as registration would ideally be idempotent.

@mockersf
Copy link
Member

I think we should check whether the two assets actually have different types, as registration would ideally be idempotent.

This will be handled by #3560

@alice-i-cecile alice-i-cecile added A-Assets Load files from disk to use for things like images, models, and sounds C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jan 21, 2022
@james7132 james7132 removed the S-Needs-Triage This issue needs to be labelled label Jan 21, 2022
Comment on lines 102 to 103
panic!("Error while registering new asset type. Asset with UUID: {:?} is already registered. Can not register another type with the same UUID",
T::TYPE_UUID);
Copy link
Member

Choose a reason for hiding this comment

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

could you also log std::any::type_name::<T>() in the error?

Copy link
Member

Choose a reason for hiding this comment

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

Ideally it would be possible to include the type name of the first type too, although I don't know if we store that information

Copy link
Contributor Author

@ShadowCurse ShadowCurse Feb 6, 2022

Choose a reason for hiding this comment

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

Added type name, but i don't think there is a way to get the original type. We only have trait object, so we can't get the type from it.

@mockersf mockersf 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 Feb 6, 2022
@@ -93,6 +93,15 @@ impl AssetServer {
}

pub(crate) fn register_asset_type<T: Asset>(&self) -> Assets<T> {
if self
Copy link
Member

Choose a reason for hiding this comment

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

rather than grab a read lock and then a write lock immediately after, I think we should just grab a single write lock and reuse it across both cases (especially given that the "read only" case is the "unexpected" / "uncommon" case).

Comment on lines 96 to 104
self.server.asset_lifecycles.write().insert(
let mut asset_lifecycles = self.server.asset_lifecycles.write();
if asset_lifecycles.contains_key(&T::TYPE_UUID) {
panic!("Error while registering new asset type: {:?} with UUID: {:?}. Another type with the same UUID is already registered. Can not register new asset type with the same UUID",
std::any::type_name::<T>(), T::TYPE_UUID);
}
asset_lifecycles.insert(
T::TYPE_UUID,
Box::new(AssetLifecycleChannel::<T>::default()),
);
Copy link
Member

Choose a reason for hiding this comment

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

insert would return Some(_) if the value already exists, so this could be just

        if self
            .server
            .asset_lifecycles
            .write()
            .insert(
                T::TYPE_UUID,
                Box::new(AssetLifecycleChannel::<T>::default()),
            )
            .is_some()
        {
            panic!("Error while registering new asset type: {:?} with UUID: {:?}. Another type with the same UUID is already registered. Can not register new asset type with the same UUID.",
            std::any::type_name::<T>(), T::TYPE_UUID);
        }

@mockersf
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Feb 12, 2022
# Objective
Fixes #2610 and #3731

## Solution

Added check for TYPE_UUID duplication in  `register_asset_type` with an error message
@bors bors bot changed the title Useful error message when two assets have the save UUID [Merged by Bors] - Useful error message when two assets have the save UUID Feb 12, 2022
@bors bors bot closed this Feb 12, 2022
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-Usability A targeted quality-of-life change that makes Bevy easier to use 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.

unwrap() fails in an AssetServer::update_asset_storage() if two assets are registered with the same uuid
7 participants