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

Subassets of subassets do not work. #15417

Open
andriyDev opened this issue Sep 25, 2024 · 1 comment · May be fixed by #15481
Open

Subassets of subassets do not work. #15417

andriyDev opened this issue Sep 25, 2024 · 1 comment · May be fixed by #15481
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 D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Design This issue requires design work to think about how it would best be accomplished X-Controversial There is active debate or serious implications around merging this PR

Comments

@andriyDev
Copy link
Contributor

andriyDev commented Sep 25, 2024

Problem

Today, ErasedLoadedAsset holds the asset as Box<dyn AssetContainer>, and holds subassets as HashMap<String, ErasedLoadedAsset> (not really, but for simplicity, this is true). This means that subassets can themselves hold subassets, and so on, and so on. This makes it very confusing when you find out that these "nested subassets" get "flattened" when they get inserted by the AssetServer. For example, consider the following loader:

async fn load(load_context: &mut LoadContext) -> Result<Self::Asset, Self::Error> {
    load_context.add_labeled_asset("A".into(), Self::Asset(1));
    load_context.labeled_asset_scope("B".into(), |load_context| {
        // This asset replaces the "root" subasset labeled "A"
        load_context.add_labeled_asset("A".into(), Self::Asset(2));
        Self::Asset(3)
    });
    Ok(Self::Asset(4))
}

The ErasedLoadedAsset for this looks like:

{
    value: SomeAsset(4),
    labeled_assets: {
        "A": ErasedLoadedAsset {
            value: SomeAsset(1),
        },
        "B": ErasedLoadedAsset {
            value: SomeAsset(3),
            labeled_assets: {
                "A": ErasedLoadedAsset {
                    value: SomeAsset(2)
                }
            }
        }
    }
}

The resulting Assets would be:

{
    "my_asset.blah": SomeAsset(4),
    "my_asset.blah#A": SomeAsset(2),
    "my_asset.blah#B": SomeAsset(3),
}

What solution would you like?

  1. Remove labeled_assets from ErasedLoadedAsset/LoadedAsset and propagate it separately. The hashmap of labeled assets should continue to just use ErasedLoadedAsset.

  2. Make the LoadContext::labeled_asset_scope (and related) merge the current and the new set of subassets.

    1. We could consider disallowing subassets to themselves load subassets (though this is likely not desirable)
    2. Overwriting a subasset should probably be a warning or maybe even a panic.
  3. Make AssetServer::send_loaded_asset no longer recursive.

What alternative(s) have you considered?

Do nothing. Keep the confusing behavior. This is only an issue if, in an asset loader, in a LoadContext::labeled_asset_scope (or equivalent), a subasset shares a name with a previously added subasset.

Another alternative: Make them actually work! We could create some notation to support nested subassets in AssetPath (for example, "my_asset.blah#A#B#C" or maybe "my_asset.blah#A/B/C"). It's not clear that this is something anyone wants or needs, so the complexity seems unwarranted.

@andriyDev andriyDev added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Sep 25, 2024
@andriyDev
Copy link
Contributor Author

I suspect also the meta field also doesn't need to exist for subassets. This might point to splitting the LoadContext into a SubassetLoadContext or something.

@BenjaminBrienen BenjaminBrienen 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 M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Design This issue requires design work to think about how it would best be accomplished X-Controversial There is active debate or serious implications around merging this PR D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes and removed S-Needs-Triage This issue needs to be labelled labels Sep 25, 2024
@ItsDoot ItsDoot removed the C-Feature A new feature, making something new possible label Sep 26, 2024
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 D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Design This issue requires design work to think about how it would best be accomplished X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants