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

Send an AssetEvent when modifying using get_id_mut #323

Merged
merged 1 commit into from
Aug 24, 2020

Conversation

DJMcNab
Copy link
Member

@DJMcNab DJMcNab commented Aug 24, 2020

Fixes #321

@ncallaway
Copy link
Contributor

ncallaway commented Aug 24, 2020

This change makes sense to me. I could see also updating get_with_id to call self.get in a similar way for similar reasons. I we ever decided to do more logic in get() it'd be nice to not have to also touch get_with_id().

pub fn get_with_id(&self, id: HandleId) -> Option<&T> {
  // - self.assets.get(&Handle::from_id(id))
  self.get(&Handle::from_id(id))
}

As a tangent from your change, I'm curious if you or anyone else knows why we have divergent names get_with_id and get_id_mut. I think all things being equal it'd be nice if those names were harmonized as get_with_id and get_with_id_mut or get_id and get_id_mut, but I think any changes there should be separate and discussed with @cart first.

@cart
Copy link
Member

cart commented Aug 24, 2020

I think 'get_with_id()` is the better name. Another pr for that would be welcome.

@cart cart merged commit f713150 into bevyengine:master Aug 24, 2020
@karroffel karroffel added the A-Assets Load files from disk to use for things like images, models, and sounds label Aug 24, 2020
BimDav pushed a commit to BimDav/bevy that referenced this pull request Aug 26, 2020
@DJMcNab DJMcNab deleted the assets_get_id_event branch August 30, 2020 19:01
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

I'm not sure why it's different here
4 participants