MrGVSV / bevy_proto

Create config files for entities in Bevy
Other
239 stars 25 forks source link

key by handle id #17

Closed B-Reif closed 2 years ago

B-Reif commented 2 years ago

This PR changes ProtoData to key by HandleId instead of HandlePath, per #16.

NB: On my machine, this PR causes a regression in the batch time when running the benchmark example. Currently for me, main branch takes an avg 25ms to insert a batch. Running on this branch increases that to an avg 31ms per batch. We may want to address this before merging.

B-Reif commented 2 years ago

While exploring this, I encountered another issue: I'm not sure how to get the correct HandleId inside of insert_self. Typically with a system I would pull in other resources to get a handle, but I don't have access to the World here 🤔

MrGVSV commented 2 years ago

While exploring this, I encountered another issue: I'm not sure how to get the correct HandleId inside of insert_self. Typically with a system I would pull in other resources to get a handle, but I don't have access to the World here 🤔

Can you not just convert the HandlePath? Or are you referring to the other handles that might be generated (like your aseprite example in #16)?

B-Reif commented 2 years ago

I'm referring to the programmatically generated HandleIds. To insert a programmatically generated asset, the AssetServer just comes up with a random uuid:

/// bevy code: impl<T> Assets<T>
/// Adds an asset to the collection, returning a Strong handle to that asset.
///
/// # Events
/// * [`AssetEvent::Created`]
pub fn add(&mut self, asset: T) -> Handle<T> {
    let id = HandleId::random::<T>();
    self.assets.insert(id, asset);
    self.events.send(AssetEvent::Created {
        handle: Handle::weak(id),
    });
    self.get_handle(id)
}

In order to refer to this handle id again, I would normally store it on a resource type. In the aseprite example I have nested maps that correspond to the file path and the frame number, so getting a Handle<Image> looks like:

fn log_handle(asset_map: Res<AseFileMap>) {
  // internally indexes a map into files, then a map into frame number -> Image
  let handle: Handle<Image> = asset_map.get_texture("my_sprite.aseprite", 0);
  dbg!(handle.id); // Something randomly generated
}
MrGVSV commented 2 years ago

I'm referring to the programmatically generated HandleIds.

Oh okay I understand now. The way I solved this in the reflection rework was to make a custom Command that allows direct access to the EntityMut (and subsequently the World). This might be possible to do now, although it is probably a sizable change. It would involve editing ProtoCommands to take &mut Commands instead of EntityCommands and creating a custom command that passes the EntityMut to the ProtoComponent.

I wonder if there's a simpler way, though? 🤔

MrGVSV commented 2 years ago

For now, though, I think we can consider that a limitation and take care of it in another PR if we need to.

MrGVSV commented 2 years ago

NB: On my machine, this PR causes a regression in the batch time when running the benchmark example. Currently for me, main branch takes an avg 25ms to insert a batch. Running on this branch increases that to an avg 31ms per batch. We may want to address this before merging.

I actually haven't experienced much of a difference. I'm not sure why this might have an average 6ms increase? Could this be machine-dependent?

B-Reif commented 2 years ago

NB: On my machine, this PR causes a regression in the batch time when running the benchmark example. Currently for me, main branch takes an avg 25ms to insert a batch. Running on this branch increases that to an avg 31ms per batch. We may want to address this before merging.

I actually haven't experienced much of a difference. I'm not sure why this might have an average 6ms increase? Could this be machine-dependent?

I eliminated this problem when I changed the bench to access the handle from the asset server instead of the proto data. I suppose an accurate bench should probably still be accessing the ProtoData though.