[Merged by Bors] - [assets] set LoadState properly and more testing! #2226

NathanSWard · 2021-05-20T18:18:31Z

Sets LoadState properly on all failing cases in AssetServer::load_async
Adds more tests for sad and happy paths of asset loading

Note: this brings in the tempfile crate.

crates/bevy_asset/src/asset_server.rs

alice-i-cecile

Just a nit. Looks good to me though.

mockersf · 2021-05-26T07:41:43Z

From your tests, it seems LoadState::NotLoaded is used for:

handle to an asset not yet loaded (for example, building an handle from a path that was not loaded)
handle to an asset built outside of the asset server
handle to an asset created by loading it through the asset server but without an asset loader for its extension

Having those bundled together doesn't seems right to me.

NathanSWard · 2021-05-26T14:31:51Z

Having those bundled together doesn't seems right to me.

This is fair, however, this isn't a change on my part. This was simply the original behavior of the LoadState.

@mockersf
Based on the list you gave I assume you'd expect the following:

handle to an asset not yet loaded (for example, building an handle from a path that was not loaded)

LoadState::NotLoaded

handle to an asset built outside of the asset server

LoadState::???

handle to an asset created by loading it through the asset server but without an asset loader for its extension

LoadState::Failed
The curious part about this last one is that this is the only unhappy path that doesnt change the LoadState to Failed. However this is because if a loader does not exist for the extension type async_load immediately returns before creating a SourceInfo struct for this.
Should we creat a SourceInfo struct for it?

mockersf · 2021-05-26T14:46:03Z

This is fair, however, this isn't a change on my part. This was simply the original behavior of the LoadState.

Right, your tests made this issue visible 😄 . It can be a followup PR if you prefer.

handle to an asset not yet loaded (for example, building an handle from a path that was not loaded)

LoadState::NotLoaded

👍

handle to an asset built outside of the asset server

LoadState::???

LoadState::WhatAreYouDoing? LoadState::NotSomethingYouLoadedWhyYouDoingThis? LoadState::NotFromAssetServer? LoadState::ManuallyLoaded? LoadState::NotALoadedAssetAndNeverWillBe?

Maybe LoadState::Manual is the one I'd prefer.

handle to an asset created by loading it through the asset server but without an asset loader for its extension

LoadState::Failed

👍

The curious part about this last one is that this is the only unhappy path that doesnt change the LoadState to Failed. However this is because if a loader does not exist for the extension type async_load immediately returns before creating a SourceInfo struct for this.
Should we creat a SourceInfo struct for it?

I think a SourceInfo is used to track the life of an asset's source for things like reloading, ... This is not needed here so I would say no.

mockersf · 2021-05-27T01:59:18Z

handle to an asset created by loading it through the asset server but without an asset loader for its extension

LoadState::Failed
The curious part about this last one is that this is the only unhappy path that doesnt change the LoadState to Failed. However this is because if a loader does not exist for the extension type async_load immediately returns before creating a SourceInfo struct for this.
Should we creat a SourceInfo struct for it?

It was mentioned today that it fails silently for that case. It was in a case of loading a gltf model with jpg textures, which doesn't work without --features jpeg. Could you also add a log in that case? (actually this would probably not help in this case at all as the gltf loader doesn't go back to the asset server to load its texture, still would probably be good to log that the user is doing something that can't work)

... sorry to add changes to your pr, this too can be a followup if you prefer...

NathanSWard · 2021-05-27T20:07:02Z

I think a SourceInfo is used to track the life of an asset's source for things like reloading, ... This is not needed here so I would say no.

Well, the SourceInfo struct is what conatins the LoadState.
So in order to set the load state, you would have to insert a SourceInfo.

NathanSWard · 2021-05-29T18:13:28Z

When this gets merged in I'll submit a follow up PR to address #2261 and that calling get_load_state on a Handle manually constructed (not through .load()) should return a specific LoadState.

NathanSWard · 2021-06-02T21:16:23Z

ping @cart 😉

crates/bevy_asset/src/asset_server.rs

NathanSWard · 2021-06-07T00:29:11Z

pinging here again to hopefully get some traction.
Would like to get this merged in as it unblocks me from adding #2310 which then unblocks 2 other PRs.

cart · 2021-06-08T02:29:27Z

Looks good to me!

cart · 2021-06-08T02:29:32Z

bors r+

1) Sets `LoadState` properly on all failing cases in `AssetServer::load_async` 2) Adds more tests for sad and happy paths of asset loading _Note_: this brings in the `tempfile` crate.

bors · 2021-06-08T03:14:32Z

Pull request successfully merged into main.

Build succeeded:

1) Sets `LoadState` properly on all failing cases in `AssetServer::load_async` 2) Adds more tests for sad and happy paths of asset loading _Note_: this brings in the `tempfile` crate.

NathanSWard requested a review from mockersf May 20, 2021 18:19

NathanSWard added A-Assets Load files from disk to use for things like images, models, and sounds C-Bug An unexpected or incorrect behavior labels May 20, 2021

NathanSWard commented May 20, 2021

View reviewed changes

crates/bevy_asset/src/asset_server.rs Outdated Show resolved Hide resolved

mockersf reviewed May 20, 2021

View reviewed changes

crates/bevy_asset/src/asset_server.rs Outdated Show resolved Hide resolved

NathanSWard commented May 20, 2021

View reviewed changes

crates/bevy_asset/src/asset_server.rs Show resolved Hide resolved

kgv mentioned this pull request May 21, 2021

Asset has LoadState::Loaded but Assets::get return None #2224

Closed

NathanSWard force-pushed the nward/asset-server-fixes branch from ab30709 to 4e8ae77 Compare May 24, 2021 15:42

NathanSWard requested review from mockersf and alice-i-cecile May 24, 2021 15:44

NathanSWard commented May 24, 2021

View reviewed changes

crates/bevy_asset/src/asset_server.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed May 26, 2021

View reviewed changes

crates/bevy_asset/src/asset_server.rs Outdated Show resolved Hide resolved

alice-i-cecile approved these changes May 26, 2021

View reviewed changes

mockersf approved these changes May 27, 2021

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 27, 2021

NathanSWard requested a review from cart May 31, 2021 06:36

cart reviewed Jun 3, 2021

View reviewed changes

crates/bevy_asset/src/asset_server.rs Outdated Show resolved Hide resolved

NathanSWard requested a review from cart June 3, 2021 20:13

NathanSWard added 5 commits June 3, 2021 14:14

[assets] set LoadState properly and more testing!

5d37044

fix asset-lifecycle-test to pass

7e5cb44

introduce LoadState::Unloaded and strengthen test

2566b95

remove tempfile glob import

b1099cf

remove adding loaders in AssetServer tests

bce6eb1

NathanSWard force-pushed the nward/asset-server-fixes branch from 633f33e to bce6eb1 Compare June 3, 2021 20:14

update test to work with new changes to update_asset_storage

a7233d5

NathanSWard mentioned this pull request Jun 6, 2021

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

Closed

NathanSWard added 2 commits June 7, 2021 09:23

simplify tests

a961401

fix missing function type

ce852cf

alice-i-cecile approved these changes Jun 7, 2021

View reviewed changes

bors bot changed the title ~~[assets] set LoadState properly and more testing!~~ [Merged by Bors] - [assets] set LoadState properly and more testing! Jun 8, 2021

bors bot closed this Jun 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - [assets] set LoadState properly and more testing! #2226

[Merged by Bors] - [assets] set LoadState properly and more testing! #2226

NathanSWard commented May 20, 2021 •

edited

Loading

alice-i-cecile left a comment

mockersf commented May 26, 2021

NathanSWard commented May 26, 2021

mockersf commented May 26, 2021 •

edited

Loading

mockersf commented May 27, 2021 •

edited

Loading

NathanSWard commented May 27, 2021

NathanSWard commented May 29, 2021 •

edited

Loading

NathanSWard commented Jun 2, 2021

NathanSWard commented Jun 7, 2021

cart commented Jun 8, 2021

cart commented Jun 8, 2021

bors bot commented Jun 8, 2021

[Merged by Bors] - [assets] set LoadState properly and more testing! #2226

[Merged by Bors] - [assets] set LoadState properly and more testing! #2226

Conversation

NathanSWard commented May 20, 2021 • edited Loading

alice-i-cecile left a comment

Choose a reason for hiding this comment

mockersf commented May 26, 2021

NathanSWard commented May 26, 2021

mockersf commented May 26, 2021 • edited Loading

mockersf commented May 27, 2021 • edited Loading

NathanSWard commented May 27, 2021

NathanSWard commented May 29, 2021 • edited Loading

NathanSWard commented Jun 2, 2021

NathanSWard commented Jun 7, 2021

cart commented Jun 8, 2021

cart commented Jun 8, 2021

bors bot commented Jun 8, 2021

NathanSWard commented May 20, 2021 •

edited

Loading

mockersf commented May 26, 2021 •

edited

Loading

mockersf commented May 27, 2021 •

edited

Loading

NathanSWard commented May 29, 2021 •

edited

Loading