Skip to content

Add Invalid variant for AssetId, InternalAssetId and UntypedAssetId #18800

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Add Invalid variant for AssetId, InternalAssetId and UntypedAssetId #18800

wants to merge 3 commits into from

Conversation

hukasu
Copy link
Contributor

@hukasu hukasu commented Apr 10, 2025

Objective

With the restructuring of the rendering crates, some types now live in different crates, and this causes some tests for an invalid asset id to be impossible due to the missing type.

e.g. bevy_render_3d relies on bevy_pbr::StandardMaterial, but bevy_pbr relies on bevy_render_3d as a whole.

That way it is impossible to create an invalid AssetId.

Solution

Create Invalid variant for AssetId, InternalAssetId, and UntypedAssetId, the UntypedAssetId does not store an TypeId.

Testing

cargo run -p ci and some of the examples

Alternative

Make locations that store an AssetId take an Option instead. Same for UntypedAssetId.

TODO

  • Garantee that none of the unreachables are reacheable

mockersf
mockersf reviewed Apr 10, 2025
View reviewed changes
crates/bevy_asset/src/handle.rs
UntypedHandle::Weak(id) => id.type_id(),
UntypedHandle::Strong(handle) => {
let Some(type_id) = handle.id.type_id() else {
unreachable!("This should never be called with an Invalid id.");
Copy link
Member

@mockersf mockersf Apr 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

is this unreachable? I think everything to reach it is public
ha it's a todo 😶

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should i have it as an Option<TypeId> here?

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Assets Load files from disk to use for things like images, models, and sounds X-Contentious There are nontrivial implications that should be thought through S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Apr 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mockersf mockersf mockersf left review comments

Assignees
No one assigned
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Feature A new feature, making something new possible S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hukasu @mockersf @alice-i-cecile