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 loader registration warning #11870

Conversation

thepackett
Copy link
Contributor

Objective

When registering and preregistering asset loaders, there would be a warn! if multiple asset loaders use a given extension, and an info! if multiple asset loaders load the same asset type. Since both of these situations are individually fine, it was decided that these messages should be removed.

Solution

Replace both of these messages with a new warn! that notes that if multiple asset loaders share the same asset type and share extensions, that the loader must be specified in the .meta file for those assets in order to solve the ambiguity. This is a more useful message, since it notes when a user must take special action / consideration.

@AxiomaticSemantics AxiomaticSemantics added the A-Diagnostics Logging, crash handling, error reporting and performance analysis label Feb 15, 2024
Copy link
Contributor

@AxiomaticSemantics AxiomaticSemantics left a comment

Choose a reason for hiding this comment

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

I still think this should be hidden behind a feature, but there are many things that should be

crates/bevy_asset/src/server/loaders.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/server/loaders.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/server/loaders.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/server/loaders.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

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

Simple change that resolves a problem I was facing earlier. Nice work!

Copy link
Contributor

@doonv doonv left a comment

Choose a reason for hiding this comment

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

This is great! This also fixes #11881.

@mockersf mockersf added this to the 0.13 milestone Feb 16, 2024
@mockersf mockersf added the A-Assets Load files from disk to use for things like images, models, and sounds label Feb 16, 2024
@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 Feb 16, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 16, 2024
Merged via the queue into bevyengine:main with commit 16d710c Feb 16, 2024
25 checks passed
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 A-Diagnostics Logging, crash handling, error reporting and performance analysis 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.

None yet

6 participants