Fix `StrongHandle` `TypeId` mismatch when using labels #10465

Selene-Amanita · 2023-11-09T01:40:41Z

Objective

Note

I didn't test this PR thoroughly, but it does seem to fix the bug at least.
There might be a better way to fix the bug, but I don't see it, don't hesitate to make another PR or adopt this one if there is too much to change
I don't know what the type of a "MorphTarget" is supposed to be
I'm not sure what to do with the CI error about the error type being too big

Solution

Force the implementation of label_type_name and label_type_id methods on the AssetLoader trait
(previously, changed by 52bd527) Modify the asset path string parser to get a "label_type", which is the last word after the last # and / characters and before digits, for example for cup.glb#Mesh0/Primitive0 it would be Primitive, that's what will be passed to the methods above
use all of this to assign the correct TypeId to the underlying StrongHandle when using load or load_untyped

See the conversation here for more information: https://discord.com/channels/691052431525675048/1062754025218510918/1171562681736642622

Migration Guide

If you have a type that implements AssetLoader, and that AssetLoader should support label, you should implement two methods, label_type_name and label_type_id. They take a label string that you can parse to return the corresponding TypeId/TypeName, or None if it's an invalid label.

NiklasEi

Nice, thank you for doing this 🙂
I would prefer a more general API instead of focusing on the last word in the label and expecting it to have a number. Can we have the new methods take the asset path or the complete label? Then the loader can decide how to parse it.

NiklasEi · 2023-11-09T06:22:11Z

crates/bevy_asset/src/loader.rs

@@ -39,6 +39,10 @@ pub trait AssetLoader: Send + Sync + 'static {

    /// Returns a list of extensions supported by this asset loader, without the preceding dot.
    fn extensions(&self) -> &[&str];
+    /// Returns the type name of the sub-asset type with the given `label`.
+    fn label_type_name(&self, label_type: &str) -> Option<&'static str>;


I think the new methods should have default implementations returning name/id of Self::Asset

That should make this change non breaking.

I was on the fence yeah. I kinda wanted to make the implementation of AssetLoader for the user as easy as possible, and parsing a label is kinda annoying, but yeah in doing that I kinda set in stone what a label should look like and we might not want that, also the code of AssetPath becomes kinda messy yeah.

I would like to hear another opinion on this.

NiklasEi · 2023-11-09T06:23:38Z

crates/bevy_asset/src/loader.rs

    fn asset_type_id(&self) -> TypeId {
        TypeId::of::<L::Asset>()
    }

-    fn asset_type_name(&self) -> &'static str {
-        std::any::type_name::<L::Asset>()
+    fn label_type_name(&self, label_type: Option<&str>) -> Option<&'static str> {


In my opinion this logic should be part of the loader. We could directly call the methods on the loader here.

That puts the responsibility of thinking about "if there is no label it's the root Asset type" on the person who implement AssetLoader. I think the only reason we might want that is if we want to allow an AssetLoader to only take paths with labels, but even if we do that it would still load an asset with a handle with a path without a label so it doesn't really make sense I think?

In most cases an asset loader should not care about the new functions at all. If the default implementations of the functions return the type name/id of the main asset, only the ones requiring their own implementations need to handle "no label" => "main asset type" and is is fine imo. So far I am only aware of the Gltf loader that would need implementations of the new functions.

NiklasEi · 2023-11-09T06:24:53Z

crates/bevy_asset/src/path.rs

+    /// This is given as a convenience only when parsing an asset path string,
+    /// but it is not set when a `label` is set directly.
+    /// Should not be used for comparison.
+    pub(crate) label_type: Option<CowArc<'a, str>>,


I think we can remove this and leave the path parsing to the loader.

NiklasEi · 2023-11-09T06:47:44Z

The error about the error type is because the new asset loader error is quite big and included in the Gltf loader error. The new error could just contain the path, which might make it small enough.

Selene-Amanita · 2023-11-09T11:05:53Z

I don't really understand how the error is big, I basically copied MissingLabel into WrongLabel, it's a bunch of String?

Selene-Amanita · 2023-11-09T11:41:51Z

Based on @NiklasEi 's feedback, commit 52bd527 stops preemptively parsing the label and leaves that to the AssetLoader implementation. You can check before and after the commit to see which one you prefer.

NiklasEi · 2023-11-09T11:54:53Z

Maybe we should have the new functions return the type names and IDs without Option. If an asset loader will be used to load a certain path, there should always be a handle type for that path. Then we can also remove the new error variant.

cart · 2023-11-09T21:01:10Z

Hmm I'll need to think on this a bit as something seems off with this approach. AssetLoader::label_type_name(label: &str) feels wrong. I think we shouldn't support "storing type names in label names" as a first class citizen in the asset system, especially given that it won't work in all cases. "Works by convention / if people remember to implement it correctly" isn't safe enough, and assigners of labels should feel free to use whatever convention makes sense for the asset. Assets (in general) are asynchronously resolved. AssetLoaders can run without knowing the type of a label ahead of time. load_untyped should also have no need to know the type name before the AssetLoader has run.

# Objective Fixes #10436 Alternative to #10465 ## Solution `load_untyped_async` / `load_internal` currently has a bug. In `load_untyped_async`, we pass None into `load_internal` for the `UntypedHandle` of the labeled asset path. This results in a call to `get_or_create_path_handle_untyped` with `loader.asset_type_id()` This is a new code path that wasn't hit prior to the newly added `load_untyped` because `load_untyped_async` was a private method only used in the context of the `load_folder` impl (which doesn't have labels) The fix required some refactoring to catch that case and defer handle retrieval. I have also made `load_untyped_async` public as it is now "ready for public use" and unlocks new scenarios.

Selene-Amanita · 2023-11-14T11:42:40Z

Fixed by #10514, closing this.

# Objective Fixes #10436 Alternative to #10465 ## Solution `load_untyped_async` / `load_internal` currently has a bug. In `load_untyped_async`, we pass None into `load_internal` for the `UntypedHandle` of the labeled asset path. This results in a call to `get_or_create_path_handle_untyped` with `loader.asset_type_id()` This is a new code path that wasn't hit prior to the newly added `load_untyped` because `load_untyped_async` was a private method only used in the context of the `load_folder` impl (which doesn't have labels) The fix required some refactoring to catch that case and defer handle retrieval. I have also made `load_untyped_async` public as it is now "ready for public use" and unlocks new scenarios.

# Objective Fixes bevyengine#10436 Alternative to bevyengine#10465 ## Solution `load_untyped_async` / `load_internal` currently has a bug. In `load_untyped_async`, we pass None into `load_internal` for the `UntypedHandle` of the labeled asset path. This results in a call to `get_or_create_path_handle_untyped` with `loader.asset_type_id()` This is a new code path that wasn't hit prior to the newly added `load_untyped` because `load_untyped_async` was a private method only used in the context of the `load_folder` impl (which doesn't have labels) The fix required some refactoring to catch that case and defer handle retrieval. I have also made `load_untyped_async` public as it is now "ready for public use" and unlocks new scenarios.

Fix Handle TypeId mismatch when using labels

4976194

Selene-Amanita added C-Bug An unexpected or incorrect behavior A-Assets Load files from disk to use for things like images, models, and sounds P-Crash A sudden unexpected crash labels Nov 9, 2023

Selene-Amanita added this to the 0.12.1 milestone Nov 9, 2023

Selene-Amanita added 2 commits November 9, 2023 01:58

CI

e6591bb

CI-2

312799c

alice-i-cecile requested review from cart, mockersf and NiklasEi November 9, 2023 02:09

NiklasEi suggested changes Nov 9, 2023

View reviewed changes

Don't preemptively parse the label

52bd527

Selene-Amanita added 2 commits November 9, 2023 11:48

CI

12d1625

Remove unnecessary implementations

1a4026c

Remove unnecessary implementation for CoolText unit test

f7d0999

cart mentioned this pull request Nov 11, 2023

Fix untyped labeled asset loading #10514

Merged

cart removed this from the 0.12.1 milestone Nov 11, 2023

Selene-Amanita closed this Nov 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `StrongHandle` `TypeId` mismatch when using labels #10465

Fix `StrongHandle` `TypeId` mismatch when using labels #10465

Selene-Amanita commented Nov 9, 2023 •

edited

Loading

NiklasEi left a comment •

edited

Loading

NiklasEi Nov 9, 2023 •

edited

Loading

NiklasEi Nov 9, 2023

Selene-Amanita Nov 9, 2023 •

edited

Loading

NiklasEi Nov 9, 2023

Selene-Amanita Nov 9, 2023

NiklasEi Nov 9, 2023 •

edited

Loading

NiklasEi Nov 9, 2023

NiklasEi commented Nov 9, 2023

Selene-Amanita commented Nov 9, 2023

Selene-Amanita commented Nov 9, 2023

NiklasEi commented Nov 9, 2023 •

edited

Loading

cart commented Nov 9, 2023

Selene-Amanita commented Nov 14, 2023

Fix StrongHandle TypeId mismatch when using labels #10465

Fix StrongHandle TypeId mismatch when using labels #10465

Conversation

Selene-Amanita commented Nov 9, 2023 • edited Loading

Objective

Note

Solution

Migration Guide

NiklasEi left a comment • edited Loading

Choose a reason for hiding this comment

NiklasEi Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

NiklasEi Nov 9, 2023

Choose a reason for hiding this comment

Selene-Amanita Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

NiklasEi Nov 9, 2023

Choose a reason for hiding this comment

Selene-Amanita Nov 9, 2023

Choose a reason for hiding this comment

NiklasEi Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

NiklasEi Nov 9, 2023

Choose a reason for hiding this comment

NiklasEi commented Nov 9, 2023

Selene-Amanita commented Nov 9, 2023

Selene-Amanita commented Nov 9, 2023

NiklasEi commented Nov 9, 2023 • edited Loading

cart commented Nov 9, 2023

Selene-Amanita commented Nov 14, 2023

Fix `StrongHandle` `TypeId` mismatch when using labels #10465

Fix `StrongHandle` `TypeId` mismatch when using labels #10465

Selene-Amanita commented Nov 9, 2023 •

edited

Loading

NiklasEi left a comment •

edited

Loading

NiklasEi Nov 9, 2023 •

edited

Loading

Selene-Amanita Nov 9, 2023 •

edited

Loading

NiklasEi Nov 9, 2023 •

edited

Loading

NiklasEi commented Nov 9, 2023 •

edited

Loading