[Merged by Bors] - [assets] Fix AssetServer::get_handle_path

[NathanSWard]

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)

[NathanSWard]

A few notes:

This PR unblocks
#2306
#2268

I would like to write some tests for this, however, I would like that base that off of #2226

[cart]

So doing this in load_async creates timing issues that could affect the PRs mentioned above (as discussed in #1290). If we're going to rely on this for things like serialization / deserialization, I don't think we can have nondeterministic behavior based on whether or not a load_async task has been run yet. In some cases, "behavior that cannot be relied upon" is worse than "consistently not working correctly".

[NathanSWard]

So doing this in load_async creates timing issues that could affect the PRs mentioned above (as discussed in #1290). If we're going to rely on this for things like serialization / deserialization, I don't think we can have nondeterministic behavior based on whether or not a load_async task has been run yet. In some cases, "behavior that cannot be relied upon" is worse than "consistently not working correctly".

This is all fair.
However, it worries me that we have to grab the RwLock via write() inside the load_untracked function directly.
Also not only that, but if someone calls

asset_server.load("mypath");

multiple times, the value keeps getting overwritten.

I guess we could do something like:

if self.handle_to_path.read().contains(&handle_id) {
    self.handle_to_path.write().insert(handle_id, asset_path.clone());
}

Actually now thinking about it, this isn't the worst solution.

[NathanSWard]

Just updated the PR to add the path in the load_untracked function to avoid the nondeterministic behavior.

[cart]

Actually now thinking about it, this isn't the worst solution.

Honestly its probably not great. For the "happy path" where asset_server.load("my_path") is called exactly once (which is likely going to be very common), we've added an additional read() lock and hashmap lookup.

Just doing unconditional write().insert() is much better for that case.

[NathanSWard]

Honestly its probably not great. For the "happy path" where asset_server.load("my_path") is called exactly once (which is likely going to be very common), we've added an additional read() lock and hashmap lookup.

I'm not the biggest fan on banking on the called exactly once argument, however, since it does make sense that this is the happy and common path, I'm ok with removing the read call.

[cart]

I'm not the biggest fan on banking on the called exactly once argument

I'd argue that your impl is banking on the "called more than once argument" 😄
This is just a choice about what call pattern we intend to encourage. And I think "reusing handles" is the pattern we should encourage.

[NathanSWard]

I'd argue that your impl is banking on the "called more than once argument" 😄

That's fair 😛

This is just a choice about what call pattern we intend to encourage. And I think "reusing handles" is the pattern we should encourage.

Yep! I complete agree with the argument that we should we should encourage handle reuse!

And the PR is currently updated with the write only call.
So it should be good to merge.
Ideally I'd like to add testing but the tests I'd like to base off #2226 but I'm happy to have this merged first and then I'll follow up with a PR adding tests.

[cart]

Cool I think this is good to go, but I just merged #2226 and I'm happy to wait for tests 😄

[NathanSWard]

Cool I think this is good to go, but I just merged #2226 and I'm happy to wait for tests 😄

thanks! just added the tests!

[NathanSWard]

This PR should be ready to go now with the additional testing :)

[cart]

bors r+

[bors]

Pull request successfully merged into main.

Build succeeded:

• build-android
• build (nightly, ubuntu-latest)
• build (stable, macos-latest)
• build (stable, ubuntu-latest)
• build (stable, windows-latest)
• build-wasm (nightly, ubuntu-latest)
• build-wasm (stable, ubuntu-latest)
• check-doc
• check-markdown-links
• check-missing-examples-in-docs
• markdownlint
• run-examples