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

Fix get_handle_path() #1290

Closed

Conversation

TheRawMeatball
Copy link
Member

@TheRawMeatball TheRawMeatball changed the title Fixes get_handle_path() Fix get_handle_path() Jan 23, 2021
@cart
Copy link
Member

cart commented Jan 23, 2021

This does add another atomic operation (handle_to_path.write()) to a function that will get called a lot. Probably not terrible but worth calling out.

We could move it into load_async() which would ensure the op happens on a separate thread, but that has the downside of making things like this not work reliably:

let handle = server.load("path"); 
server.get_handle_path(handle);

@TheRawMeatball
Copy link
Member Author

Hmm, my first instinct is to get dashmap as a dependency - it would probably solve the performance without such a concurrency issue. It only depends on num_cpus and cfg-if, both of which already in the build tree, so I'd say its worth considering.

@cart
Copy link
Member

cart commented Jan 25, 2021

Hmm I'm not familiar enough with dashmap's impl to have an immediate response. Pulling in a new dependency will always require a bit more scrutiny than normal, so it would be good to evaluate a couple of things before rolling with that approach:

  1. Is it really a performance improvement and if so, by how much? Imo contention should be pretty uncommon, so a simple benchmark doing a bunch of loads in a loop would resolve most of my concerns.
  2. Does it affect compile times? A couple of clean compiles with/without asset server using dashmap would resolve that question.

@alice-i-cecile alice-i-cecile added the A-Assets Load files from disk to use for things like images, models, and sounds label Feb 17, 2021
Base automatically changed from master to main February 19, 2021 20:44
@NathanSWard
Copy link
Contributor

We could move it into load_async() which would ensure the op happens on a separate thread, but that has the downside of making things like this not work reliably

I actually don't mind this solution of moving it into load_async. As I could easily see a scenario where multiple parallel systems are trying to load some assets and get blocked by .write() call on the internal hash map.

It's definitely a bummer that the get_handle_path won't work immediately, however it's a tradeoff I'm happy to take.

@NathanSWard
Copy link
Contributor

I was pointed to this PR via #2306 and this is a pretty glaringly missing feature.
We definitely need to support get_handle_path as it has use cases, and not to mention that the AssetServer's internal handle_to_path HashMap is currently just wasting memory.

Comment on lines +361 to +364
let owned_path = asset_path.to_owned();
let id: HandleId = asset_path.into();
self.server.handle_to_path.write().insert(id, owned_path);
id
Copy link
Contributor

Choose a reason for hiding this comment

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

Just as an addition to this, could we add a test to validate this behavior?

@NathanSWard NathanSWard added the C-Bug An unexpected or incorrect behavior label Jun 6, 2021
@NathanSWard NathanSWard mentioned this pull request Jun 6, 2021
2 tasks
@NathanSWard
Copy link
Contributor

#2268 and #2306 are both blocked by this PR.
@TheRawMeatball let me know if you want to help finalize this PR (with the last few changes) or I'm also more than happy to open another PR like this which addresses the issues haha ☺️☺️

@TheRawMeatball
Copy link
Member Author

Help is always appreciated :) If you want to push this PR to completion, you can open a new PR using this as a base if you want and I can close this one. I've been fairly short on time recently, so it'd probably be better.

@NathanSWard
Copy link
Contributor

Help is always appreciated :) If you want to push this PR to completion, you can open a new PR using this as a base if you want and I can close this one. I've been fairly short on time recently, so it'd probably be better.

No worries! I'll probably end up making a new PR with this as a base :)

@NathanSWard
Copy link
Contributor

Closed in favor of #2310

@NathanSWard NathanSWard closed this Jun 6, 2021
bors bot pushed a commit that referenced this pull request Jun 9, 2021
# Objective

- Currently `AssetServer::get_handle_path` always returns `None` since the inner hash map is never written to.

## Solution

- Inside the `load_untracked` function, insert the asset path into the map.

This is similar to #1290 (thanks @TheRawMeatball)
ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
# Objective

- Currently `AssetServer::get_handle_path` always returns `None` since the inner hash map is never written to.

## Solution

- Inside the `load_untracked` function, insert the asset path into the map.

This is similar to bevyengine#1290 (thanks @TheRawMeatball)
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-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants