-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Allow using short type names for asset processors. #21339
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
base: main
Are you sure you want to change the base?
Conversation
bc052c7
to
fc361d5
Compare
It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note. Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes. |
It looks like your PR is a breaking change, but you didn't provide a migration guide. Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code lgtm!
Please add a migration guide for the TypePath changes, and a release note entry showcasing this work.
Also, while I won't block on it, it would be great to have a unit test for ambiguous processor names. I'm worried that that is easy to break in future code changes.
Also check the CI jobs. Looks like you need to add the TypePath derive to some more stuff. |
05c0cc2
to
e266eed
Compare
0062acb
to
0fd8d69
Compare
Fixed up CI, added the migration guide + release notes, and also added tests for getting the processor by name. We definitely need more testing around asset processing itself (I'm only testing that getting the processor by name all works), but that should be a different PR. |
I think this is the first Bevy feature that will allow previously written assets (I guess .meta files in this case) to break when installing a new plugin. (ex: if that plugin also includes an ImageLoader, or CoolTextLoader in the example here). So the general guidance will probably become "namespace your loaders" like This may also be introducing a difference in behavior between other types and loaders, ex: the scenes example asset currently requires full type paths for components, Resources, etc I don't have a strong feeling either way (slight preference for adding new plugins not being able to break current applications), but I do see "why can I wrote loaders with shortnames but not components" coming up and we should have an answer for it. |
Deriving |
Only if you use long type names. If you use short type names, then there's a risk that something like I still think this is a win. I don't think it's desirable for users to have to specify the module path since that can include info that doesn't matter to them, like |
ah, so the comment was about namespacing it on the |
The comment was that rather than calling their loaders |
Meanwhile, in my code |
Sorry, not going to have time to review this for a bit. Hopefully other community members can help with reviews. |
… the short path first.
…f fully qualifying.
…ts in the correct processing overall.
0fd8d69
to
229ed49
Compare
Rebased and added a test in lib.rs showing that short type names work! |
Objective
core::any::type_name
is generally undesirable since it is only intended for debugging.Solution
TypePath
impl onAssetLoader
,AssetTransformer
,AssetSaver
, andAssetProcessor
.TypePath::type_path
. This makes the path format stable.TypePath::short_type_path
, and ensure there are no other processors with the sameshort_type_path
. If a user tries to use an ambiguous short type path, we list out all full type paths.Note: I left doing this with asset loaders as a future PR. There's more complexity there (since we have to deal with pre-registering asset loaders), and this seems like the bigger "bang for our buck".
Testing
asset_processing::sneaky::CoolTextTransformer
and a new processor that looked identical to the regular one (but using the sneaky transformer). It printed an error, listing out the full paths of both processors!