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

key by handle id #17

Merged
merged 4 commits into from
Mar 16, 2022
Merged

key by handle id #17

merged 4 commits into from
Mar 16, 2022

Conversation

B-Reif
Copy link
Contributor

@B-Reif B-Reif commented Mar 13, 2022

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.

let texture: Handle<Image> = commands
.get_handle(self, &self.texture_path)
.expect("Expected Image handle to have been created");
let texture: Handle<Image> = asset_server.get_handle(&self.texture_path);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By changing this line to get the handle from the AssetServer instead of the ProtoCommands, the bench time on my machine decreased to avg 20ms, which is actually 5ms faster than the average programmatic batch!

It might be worth investigating why getting the handle from ProtoData is slower.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine it's because getting the handle in ProtoData involves 3 lookups, whereas I believe AssetServer only needs one.

@MrGVSV MrGVSV linked an issue Mar 14, 2022 that may be closed by this pull request
Copy link
Owner

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! This is a much better way of storing handles.

@B-Reif
Copy link
Contributor Author

B-Reif commented Mar 14, 2022

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
Copy link
Owner

MrGVSV commented Mar 14, 2022

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
Copy link
Contributor Author

B-Reif commented Mar 14, 2022

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
Copy link
Owner

MrGVSV commented Mar 14, 2022

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
Copy link
Owner

MrGVSV commented Mar 14, 2022

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

@MrGVSV
Copy link
Owner

MrGVSV commented Mar 14, 2022

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
Copy link
Contributor Author

B-Reif commented Mar 14, 2022

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.

@MrGVSV MrGVSV merged commit 30c27b6 into MrGVSV:main Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow ProtoData to store handles without paths
2 participants