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

Impl FromWrold for Handle<T> #6525

Open
PhaestusFox opened this issue Nov 9, 2022 · 3 comments
Open

Impl FromWrold for Handle<T> #6525

PhaestusFox opened this issue Nov 9, 2022 · 3 comments
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-Complex Quite challenging from either a design or technical perspective. Ask for help!

Comments

@PhaestusFox
Copy link

What problem does this solve or what need does it fill?

currently loading Handles from dynamic scenes loaded from a file will give you a weak handle. it would be nice if handles loaded this way were strong so that you don't have to keep a strong handle somewhere else to prevent assets from unloading that are in the scene. this would be necessary if we wanted to store asset dependency paths in scenes.

What solution would you like?

  1. Remove the Default implementation for Handle; use Handle::weak instead for this functionality
  2. impl FromWrold for Handle; Access Asset so handle can be strong
  3. Add set_id method that decrements the current id and increments the new id

What alternative(s) have you considered?

This could also be done with the ReflectComponent implementation but would take significantly more work since that is (as far as I could tell) all auto-implemented with generics.

Additional Info

it would also allow for a nice expansion of dynamic scenes when serialised we could add to the API

fn save_world(
    world: &World,
    asset_server: Res<AssetServer>,
    type_registry: Res<TypeRegistry>,
) {
    let scene = DynamicScene::from_world(world, &type_registry);
    let ron = scene.serialize_ron_with_dependencys(&type_registry, &asset_server);
    std::fs::write("./assets/scene.scn.ron", ron).unwrap();
}

this would require the asset server and extract any paths that the handles came from

@PhaestusFox PhaestusFox added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Nov 9, 2022
@jakobhellermann
Copy link
Contributor

I think this would need more changes to the asset system than just a FromWorld impl.
The asset id is not stable, so when we serialize and deserialize it, we have no idea what it referred to previously.

The real solution would probably be to serialize an asset as its path, and while deserializing provide some context as thread local state that allows you to defer loading an asset and get back a future asset id for a handle. (or instead of thread local state, specifically handle assets in the scene deserialization logic).

In the past there has been talk about migrating to distill (#708), or reworking bevy_asset to get some of its features. (hot reloading on web/mobile, statically embedding assets in final build, config parameters for asset import, etc.).

I don't think there has been a decision on whether we want to continue improving bevy_asset or contribute missing features to distill and switch to using that.

@mockersf
Copy link
Member

mockersf commented Nov 9, 2022

The asset id is not stable

They should be, the hasher is instantiated with fixed keys

@alice-i-cecile alice-i-cecile 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 D-Complex Quite challenging from either a design or technical perspective. Ask for help! and removed C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Nov 9, 2022
@oddfacade
Copy link
Contributor

The fundamental problem is that neither FromWorld nor Default have access to the handle id. So implementing FromWorld doesn't fix the problem because even if you get a strong handle the id will be wrong. The only point in the entire scene spawning process where you have all the needed information to properly construct a strong handle (asset ID and world/asset server access) is in ReflectComponent::apply. If you got a dummy strong handle in FromWorld you could fix it up here, but at that point you also have world access so you might as well skip the FromWorld impl and just get a strong handle there. But there is a problem: ReflectComponent doesn't have a bespoke FromType<Handle<T>> implementation. It's using the blanket implementation defined over all FromWorld components.

So, if we wanted to take this approach, we have to give Handle<T> a specialized ReflectComponent. There are two ways I can think to do this:

  1. Use autoref specialization in the reflect macro. I have an outdated PR that lays the groundwork for that here: FromType specialization #6055. I am willing to update it if this is the approach we want to take.
  2. Have the asset registration method overwrite the type data entry at runtime.

Besides specializing ReflectComponent here are other options I have considered:

  • Replace Handle<T>'s Default with a FromWorld that gets a strong handle with a dummy ID, then write a Reflect::apply implementation for the handle that sends the proper signals to the asset server when the ID is changed to the real one. I like this option, but it requires figuring out how to make Handle<T> not Default. This is a bit tricky because it's used as part of a few bundles.
  • Instead of having deserialization produce a handle directly, have it produce some other kind of object that can be later resolved into a handle by calling some type data method with world access. DynamicScene::write_to_world would then check for this ReflectComponentFuture type data before checking for ReflectComponent. I made a draft implementation of this ages ago, but abandoned it because I didn't have a good way to deal with inline assets. Now that we have asset serialization this option is a bit more attractive, but I still think the other ones are better.

Relates to: #6055

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-Complex Quite challenging from either a design or technical perspective. Ask for help!
Projects
Status: Concrete and Controversial
Development

No branches or pull requests

5 participants