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

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

Closed
TheLeonsver1 opened this issue Aug 6, 2021 · 3 comments
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

Comments

@TheLeonsver1
Copy link

How can Bevy's documentation be improved?

The unwrap here:

let channel = asset_lifecycle
.downcast_ref::<AssetLifecycleChannel<T>>()
.unwrap();

fails when two asset types are registered with the same UUID, the stack trace of the crash isn't assisting a lot in understanding why that might happen. I would like it If it could be documented.

@TheLeonsver1 TheLeonsver1 added C-Docs An addition or correction to our documentation S-Needs-Triage This issue needs to be labelled labels Aug 6, 2021
@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 D-Trivial Nice and easy! A great choice to get started with Bevy and removed C-Docs An addition or correction to our documentation S-Needs-Triage This issue needs to be labelled labels Aug 6, 2021
@alice-i-cecile
Copy link
Member

My first impression is that this could be improved by changing the unwrap to an .expect with a useful error message. It looks like this should fail if the downcast_ref results in the boxed value not actually being of the appropriate type.

In this case, that means a failure in type registration: I suspect you'll get a panic at the same place if you fail to register it at all.

@alice-i-cecile alice-i-cecile removed the D-Trivial Nice and easy! A great choice to get started with Bevy label Aug 6, 2021
@alice-i-cecile
Copy link
Member

@TheRawMeatball points out that this core issue of duplicate type registration can (and should) first be caught at the add_asset call here. Following the chain, it looks like this is actually an issue with register_type, which should fail around here if a duplicate exists. IMO this should be a Result, but I'm not sure that plays nice with our builder pattern API for App...

That said, even improving the panic with a real error message would help folks debug.

@ShadowCurse
Copy link
Contributor

Hi everyone. I have looked into this issue and it seems that problem lies here. Basically when we register a new asset type there is no check if entry with the same UUID exists. If two types have the same UUID, the later type will overwrite the previous one. This is why downcast_ref fails later.
I think a simple check with an error message would be fine for now.
Something like this:

if self.server.asset_lifecycles.read().contains_key(&T::TYPE_UUID) {
    panic!("Asset with UUID: {:?} already registered. Can not register another type with the same UUID", T::TYPE_UUID);
}

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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants