Rework assets crate #416

torkleyy · Oct 14, 2017

Changes

Access sources from Format
Let the user handle future assets and loaded assets the same (Handle)
Add Progress struct to allow loading screens and similar stuff
Add asset storages, which will greatly simplify hot reloading
Improve examples
Rename Store to Source

TODO

Fix docs, especially outdated descriptions
Rewrite other crates to use new API
Cleanup

This file pretty much shows the new design: https://github.com/amethyst/amethyst/pull/416/files#diff-1d28361fa1117c22df23aaee9e82da0a

torkleyy · Oct 14, 2017

This file pretty much shows the new design: https://github.com/amethyst/amethyst/pull/416/files#diff-1d28361fa1117c22df23aaee9e82da0a

Rhuagh · Oct 14, 2017

Rhuagh reviewed Oct 14, 2017

View changes

FWIW I like it! Gonna hold off until the PR is done to do a full review, but do far it looks good.

Rhuagh · Oct 15, 2017

Should return mat also

Yeah, did locally already ;)

omni-viral · Oct 15, 2017

omni-viral reviewed Oct 15, 2017

View changes

omni-viral · Oct 15, 2017

amethyst_assets/src/asset.rs

-    /// Request for clearing the whole cache.
-    fn clear_all(&self) {}
-}
-
 /// A format, providing a conversion from bytes to asset data, which is then
 /// in turn accepted by `Asset::from_data`. Examples for formats are


No such method.
How to convert Asset::Data into Asset now?

Check RenderSystem

The advantage is that now systems can do that. So the problem you complained about (meshes taking two frames to be displayed) is fixed now.

omni-viral · Oct 15, 2017

amethyst_renderer/src/pass/flat.rs

     camera: Option<Fetch<'a, Camera>>,
-    mesh: ReadStorage<'a, M>,
-    material: ReadStorage<'a, N>,
+    mesh_storage: Fetch<'a, AssetStorage<Mesh>>,


You need AssetStorage<Texture> as well

I know, it's WIP

Nearly done the passes though

@omni-viral Now passes should be fixed. Code is not very organized, but logically it should be all there now.

torkleyy · Oct 15, 2017

@omni-viral Now passes should be fixed. Code is not very organized, but logically it should be all there now.

@torkleyy so new system doesn't require any notification mechanism?

omni-viral · Oct 15, 2017

@torkleyy so new system doesn't require any notification mechanism?

It's pull based, the asset loading systems will call process and be handed the data that has finished loading, and process them, turning them into assets

Rhuagh · Oct 15, 2017

It's pull based, the asset loading systems will call process and be handed the data that has finished loading, and process them, turning them into assets

There's a simple implementation of a loading system that can be used for simple assets, check Processor

Rhuagh · Oct 15, 2017

There's a simple implementation of a loading system that can be used for simple assets, check Processor

A benchmark would be great. ;)

jojolepro · Oct 15, 2017

A benchmark would be great. ;)

@jojolepro I'm pretty sure it's faster than before, but yes, we could really use some benchmarks. That said, I don't think I'll include them in this PR. It's already big enough.

torkleyy · Oct 15, 2017

@jojolepro I'm pretty sure it's faster than before, but yes, we could really use some benchmarks. That said, I don't think I'll include them in this PR. It's already big enough.

jojolepro · Oct 15, 2017

jojolepro reviewed Oct 15, 2017

View changes

jojolepro · Oct 15, 2017

src/ecs/audio/bundle.rs

@@ -21,14 +21,15 @@ use shred::ResourceId;
 ///
 /// Panics during `DjSystem` registration if the bundle is applied twice.
 ///
-pub struct DjBundle<'a> {
+pub struct DjBundle<'a, F> {


Since Dj is gone, maybe rename to AudioBundle?

But there's still DjSystem.

Nah, okay, let's rename it.

Right now things are still verbose, but it's getting better. I really hope that I can increase the expressibility in future PRs, not limited to the assets crate.

torkleyy · Oct 15, 2017

Right now things are still verbose, but it's getting better. I really hope that I can increase the expressibility in future PRs, not limited to the assets crate.

Yeah, we could have all asset resources in examples/resources

Rhuagh · Oct 16, 2017

Yeah, we could have all asset resources in examples/resources

omni-viral · Oct 16, 2017

omni-viral approved these changes Oct 16, 2017

View changes

I can't wait to see this in action.

I'll hold on reviewing, as I don't have anywhere enough time to check that much code. Following the discussions on gitter, conceptually this PR is good. 👍

jojolepro · Oct 16, 2017

I'll hold on reviewing, as I don't have anywhere enough time to check that much code. Following the discussions on gitter, conceptually this PR is good. 👍

Thanks @jojolepro . I'm not sure I understood you correctly, are you self-requesting a review but saying you can't do the review now, or are you saying you can't do a full review because you don't have enough time?

torkleyy · Oct 16, 2017

Thanks @jojolepro . I'm not sure I understood you correctly, are you self-requesting a review but saying you can't do the review now, or are you saying you can't do a full review because you don't have enough time?

Can't fully review.

jojolepro · Oct 16, 2017

Can't fully review.

omni-viral · Oct 16, 2017

omni-viral reviewed Oct 16, 2017

View changes

omni-viral · Oct 16, 2017

amethyst_assets/Cargo.toml

 log = "0.3.8"
 parking_lot = "0.4.4"
 rayon = "0.8"
 specs = { version = "0.10", features = ["common"] }
+
+[dev-dependencies]
+ron = "0.1.4"


Why don't you specify less strict versionron = "0.1"

That's because 0.1.3 had a a bug which has been fixed in 0.1.4, so I thought it would be important to require that version.

Then use ^0.1.4
Otherwise it will be locked on 0.1.4

No, 0.1.4 implies ^0.1.4.

=0.1.4 would lock it.

Indeed ¯\_(ツ)_/¯

omni-viral · Oct 16, 2017

omni-viral reviewed Oct 16, 2017

View changes

I have a little concern in mind.
If an error occur in Source::load, Format::import and ends Errors passed in AssetStorage::process how user would know what Handle will never give him loaded asset?

omni-viral · Oct 16, 2017

amethyst_assets/src/storage.rs

+            errors.execute::<AssetError, _>(|| {
+                println!("Got asset with name {}", &name);
+
+                let asset = data.and_then(|d| f(d))


Maybe and_then(f) ?

how user would know what Handle will never give him loaded asset

This actually is a problem, I know. You'd basically check the errors. So it's not the most ergonomic thing, but before the error was also just added to Errors.

torkleyy · Oct 16, 2017

how user would know what Handle will never give him loaded asset

This actually is a problem, I know. You'd basically check the errors. So it's not the most ergonomic thing, but before the error was also just added to Errors.

omni-viral · Oct 16, 2017

omni-viral reviewed Oct 16, 2017

View changes

omni-viral · Oct 16, 2017

amethyst_assets/src/storage.rs

+            let bitset = &mut self.bitset;
+            let handles = &mut self.handles;
+            errors.execute::<AssetError, _>(|| {
+                let asset = data.and_then(|d| f(d))


Maybe and_then(f)?

Tried, doesn't work since it would move the closure.

Ah. It's in loop.
and_then(&mut f) in this case.
Shows borrowing more explicitly.

Is FnMut implemented for &mut T where T: FnMut? I think not, will try later.

You'd basically check the errors.

How you can find connection between error in Errors (BoxedErr) and Handles?

omni-viral · Oct 16, 2017

You'd basically check the errors.

How you can find connection between error in Errors (BoxedErr) and Handles?

As I said, this is not fully resolved yet. To be honest, I don't know a very good solution right now; as I said, this problem existed before, so this is neither making the problem worse nor solves it.

torkleyy · Oct 16, 2017

As I said, this is not fully resolved yet. To be honest, I don't know a very good solution right now; as I said, this problem existed before, so this is neither making the problem worse nor solves it.

Couldn't we use Progress for this? Push Handle<..> and the Error and have a separate function for it ?

Rhuagh · Oct 16, 2017

Couldn't we use Progress for this? Push Handle<..> and the Error and have a separate function for it ?

@Rhuagh This is a very, very, very good idea!

torkleyy · Oct 16, 2017

@Rhuagh This is a very, very, very good idea!

I actually thought about adding such a method, but I didn't realize it would solve this problem 😄

torkleyy · Oct 16, 2017

I actually thought about adding such a method, but I didn't realize it would solve this problem 😄

Again, this is great! 👍 I'll implement it!

torkleyy · Oct 16, 2017

Again, this is great! 👍 I'll implement it!

The only question that is left is should

the progress tracker get the full BoxedErr or should we
give it to Errors and give Progress only a description of the error?

torkleyy · Oct 16, 2017

The only question that is left is should

the progress tracker get the full BoxedErr or should we
give it to Errors and give Progress only a description of the error?

I'm not sure. Whichever way is easiest for now, we can improve on it later.

Rhuagh · Oct 16, 2017

I'm not sure. Whichever way is easiest for now, we can improve on it later.

I've addressed all review comments, so please give it a final review. @Aceeri self-requested, so we need to wait for their review before merging.

torkleyy · Oct 16, 2017

I've addressed all review comments, so please give it a final review. @Aceeri self-requested, so we need to wait for their review before merging.

torkleyy · Oct 16, 2017

torkleyy reviewed Oct 16, 2017

View changes

torkleyy · Oct 16, 2017

amethyst_assets/src/progress.rs

+    fn create_tracker(self) -> Self::Tracker;
+}
+
+impl Progress for () {


So basically now you can also pass () if you don't care and just want the assets to pop in once they're there.

Error handling needs improvements, but I think one can see that this design at least allows them, thus we should do that in a future PR.

Rhuagh · Oct 16, 2017

Rhuagh approved these changes Oct 16, 2017

View changes

Like you said, error handling needs improvement but let's do that in a separate PR.

Xaeroxe · Oct 16, 2017

Xaeroxe requested changes Oct 16, 2017

View changes

I would prefer that we fix music in pong prior to merging, otherwise looks great!

Rhuagh · Oct 16, 2017

Rhuagh approved these changes Oct 16, 2017

View changes

Aceeri · Oct 16, 2017

Aceeri approved these changes Oct 16, 2017

View changes

Looks like a great improvement to me.

Aceeri · Oct 16, 2017

amethyst_assets/examples/hl.rs

+}
+
+impl State {
+    /// Returns `true` if the app should quit.


Its an example, but this should probably be "Returns Some(_) ..."

Aceeri · Oct 16, 2017

amethyst_assets/src/storage.rs

+            self.bitset.remove(id);
+            self.unused_handles.push(old);
+
+            println!("Removed value!");


Debug print? or logging?

Oops, will remove. I figure I used it for debugging.

Xaeroxe · Oct 16, 2017

Xaeroxe approved these changes Oct 16, 2017

View changes

LGTM! Thanks!

So I assigned @Rhuagh for merging this when it's ready, and I assigned myself for 💤

torkleyy · Oct 16, 2017

So I assigned @Rhuagh for merging this when it's ready, and I assigned myself for 💤

bors r+

Rhuagh · Oct 16, 2017

bors r+

Build succeeded

bors · Oct 16, 2017

Build succeeded

torkleyy added project: assets status: working type: RFC labels Oct 14, 2017

Finally fix passes

Loading status checks…

6b112dc

torkleyy added some commits Oct 15, 2017

renderable example compiles, but doesn't render

Loading status checks…

e9767db

Fix renderable example

Loading status checks…

c211241

More work, add Format::Options

Loading status checks…

0934836

Start rewriting pong

4fb1370

Merge ECSBundle PR into assets branch

Loading status checks…

d90e5d2

Do audio refactoring

Loading status checks…

b542507

torkleyy added some commits Oct 15, 2017

Rename bundle, do formatting

Loading status checks…

538db21

Fix renderable example

4059485

Fix pong example except music

908310c

Improve docs, cleanup, fix style issues

36c9e4e

Merge Rhuagh's changes into assets branch

Loading status checks…

b7992d2

Fix doc example

7688803

Aceeri self-requested a review Oct 16, 2017

torkleyy added some commits Oct 16, 2017

Add progress module, improve comments, fix docs

Loading status checks…

6f01dc8

Pass asset processing closure by mutable reference

Loading status checks…

1a00483

torkleyy requested a review from Rhuagh Oct 16, 2017

torkleyy added some commits Oct 16, 2017

Fix music by letting picker fetch resource

Loading status checks…

42ed7f0

Fix example comment and remove debugging println

Loading status checks…

f95b6ae

torkleyy assigned Rhuagh Oct 16, 2017

Fixed pong build error

Loading status checks…

f8355db

bors bot merged commit f8355db into amethyst:develop Oct 16, 2017
3 checks passed

3 checks passed

bors Build succeeded

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

amethyst/amethyst

Join GitHub today

Rework assets crate #416

Conversation

torkleyy commented Oct 14, 2017 • edited Edited 1 time torkleyy edited Oct 14, 2017 (most recent)

Changes

TODO

torkleyy added project: assets status: working type: RFC labels Oct 14, 2017

This comment has been minimized.

torkleyy commented Oct 14, 2017

Rhuagh reviewed Oct 14, 2017 View changes

torkleyy and others added some commits Oct 14, 2017

This comment has been minimized.

Rhuagh commented on examples/02_sphere/main.rs in c246f45 Oct 15, 2017

This comment has been minimized.

torkleyy replied Oct 15, 2017

omni-viral reviewed Oct 15, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

torkleyy Oct 15, 2017 • edited Edited 1 time torkleyy edited Oct 15, 2017 (most recent)

This comment has been minimized.

This comment has been minimized.

torkleyy commented Oct 15, 2017 • edited Edited 1 time torkleyy edited Oct 15, 2017 (most recent)

This comment has been minimized.

omni-viral commented Oct 15, 2017 • edited Edited 1 time omni-viral edited Oct 15, 2017 (most recent)

This comment has been minimized.

Rhuagh commented Oct 15, 2017

This comment has been minimized.

Rhuagh commented Oct 15, 2017

torkleyy added some commits Oct 15, 2017

This comment has been minimized.

jojolepro commented Oct 15, 2017

This comment has been minimized.

torkleyy commented Oct 15, 2017 • edited Edited 1 time torkleyy edited Oct 15, 2017 (most recent)

jojolepro reviewed Oct 15, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

torkleyy added some commits Oct 15, 2017

This comment has been minimized.

torkleyy commented Oct 15, 2017

This comment has been minimized.

Rhuagh commented Oct 16, 2017

omni-viral approved these changes Oct 16, 2017 View changes

This comment has been minimized.

jojolepro commented Oct 16, 2017

This comment has been minimized.

torkleyy commented Oct 16, 2017

This comment has been minimized.

jojolepro commented Oct 16, 2017

omni-viral reviewed Oct 16, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

omni-viral Oct 16, 2017 • edited Edited 1 time omni-viral edited Oct 16, 2017 (most recent)

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

omni-viral Oct 16, 2017 • edited Edited 1 time omni-viral edited Oct 16, 2017 (most recent)

omni-viral reviewed Oct 16, 2017 View changes

This comment has been minimized.

This comment has been minimized.

torkleyy commented Oct 16, 2017 • edited Edited 1 time torkleyy edited Oct 16, 2017 (most recent)

omni-viral reviewed Oct 16, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

omni-viral Oct 16, 2017 • edited Edited 1 time omni-viral edited Oct 16, 2017 (most recent)

This comment has been minimized.

torkleyy Oct 16, 2017 • edited Edited 1 time torkleyy edited Oct 16, 2017 (most recent)

This comment has been minimized.

omni-viral commented Oct 16, 2017

This comment has been minimized.

torkleyy commented Oct 16, 2017

This comment has been minimized.

Rhuagh commented Oct 16, 2017

This comment has been minimized.

torkleyy commented Oct 14, 2017 •

edited

Edited 1 time

torkleyy edited Oct 14, 2017 (most recent)

Rhuagh reviewed Oct 14, 2017

View changes

Rhuagh commented on `examples/02_sphere/main.rs` in `c246f45` Oct 15, 2017

omni-viral reviewed Oct 15, 2017

View changes

torkleyy Oct 15, 2017 •

edited

Edited 1 time

torkleyy edited Oct 15, 2017 (most recent)

torkleyy commented Oct 15, 2017 •

edited

Edited 1 time

torkleyy edited Oct 15, 2017 (most recent)

omni-viral commented Oct 15, 2017 •

edited

Edited 1 time

omni-viral edited Oct 15, 2017 (most recent)

torkleyy commented Oct 15, 2017 •

edited

Edited 1 time

torkleyy edited Oct 15, 2017 (most recent)

jojolepro reviewed Oct 15, 2017

View changes

omni-viral approved these changes Oct 16, 2017

View changes

omni-viral reviewed Oct 16, 2017

View changes

omni-viral Oct 16, 2017 •

edited

Edited 1 time

omni-viral edited Oct 16, 2017 (most recent)

omni-viral Oct 16, 2017 •

edited

Edited 1 time

omni-viral edited Oct 16, 2017 (most recent)

omni-viral reviewed Oct 16, 2017

View changes

torkleyy commented Oct 16, 2017 •

edited

Edited 1 time

torkleyy edited Oct 16, 2017 (most recent)

omni-viral reviewed Oct 16, 2017

View changes

omni-viral Oct 16, 2017 •

edited

Edited 1 time

omni-viral edited Oct 16, 2017 (most recent)

torkleyy Oct 16, 2017 •

edited

Edited 1 time

torkleyy edited Oct 16, 2017 (most recent)