Core asset management #244

torkleyy · Jun 13, 2017

Adds a crate "amethyst-assets"
Examples and README.md

This PR contains the core functionality for asset management. This means that e.g. formats or special syntax are to be done.

It also does not contain any tests yet. I'm creating this PR to get some feedback early on.

Replaces #195

ebkalderon · Jun 13, 2017

ebkalderon requested changes Jun 13, 2017 • edited

View changes

@torkleyy This is a fantastic start to the new asset management system! I really like the API overall, though there are some nitpicks and questions to resolve.

Also, would you mind explaining how this PR would interact with the recently-merged project module? It already specifies a project directory and parses files, so merging this PR as-is would result in a lot of duplicate code. Wouldn't a text configuration file also be considered an asset? I would think so, since the values might change on disk and be hot reloaded at runtime. I guess amethyst_assets would end up replacing the project module eventually?

Speaking of, do you have any plans to include hot reloading of modified assets eventually? Futures would be perfect for this use case!

Phew, sorry for the wall of text. Great work so far! 👍

ebkalderon · Jun 13, 2017

assets/src/store.rs

+    type Location;
+
+    /// Loads the bytes given a category, id and extension of the asset.
+    fn load(&self, category: &str, id: &str, ext: &str) -> Result<Vec<u8>, Self::Error>;


I'd recommend using AsRef here for ergonomics' sake, like this:

fn load<S: AsRef<str>>(&self, category: S, id: S, ext: S) -> Result<Vec<u8>, Self::Error>;

In this case, every S (category, id and ext) must be the same type. I'm not sure it's okay

Hold on, there's no need to use a type parameter here, it's never called by the user.

ebkalderon · Jun 13, 2017

assets/src/loader.rs

+impl Loader {
+    /// Creates a new asset loader, initializing the directory store with the
+    /// given path.
+    pub fn new(alloc: &Allocator, directory: PathBuf, pool: Arc<ThreadPool>) -> Self {


It might be more ergonomic to use Into<PathBuf> here, like this:

pub fn new<P: Into<PathBuf>>(alloc: &Allocator, dir: P, pool: Arc<ThreadPool>) -> Self {

That's true; I didn't clean up yet after refactoring.

ebkalderon · Jun 13, 2017

assets/src/asset.rs

+    pub fn new(name: String, ext: &'static str, store: StoreId) -> Self {
+        AssetSpec {
+            ext,
+            name: name,


Why was the field name specified explicitly here but not for the other two?

ebkalderon · Jun 13, 2017

assets/examples/cache.rs

+    let pool = Arc::new(ThreadPool::new(cfg).expect("Invalid config"));
+
+    let alloc = Allocator::new();
+    let mut loader = Loader::new(&alloc, "examples/assets".into(), pool);


If Into<PathBuf> is used for Loader::new(), this .into() will no longer be needed.

ebkalderon · Jun 13, 2017

assets/Cargo.toml

@@ -0,0 +1,15 @@
+[package]
+name = "amethyst-assets"


Please use an underscore instead of a hyphen to match the convention of the other crates.

ebkalderon · Jun 13, 2017

assets/src/store.rs

+use std::path::PathBuf;
+use std::sync::atomic::{AtomicUsize, Ordering};
+
+/// An `Allocator`, holding a counter for producing unique ideas for the stores.


Did you mean "IDs" instead of "ideas" here? 😄

😄 😄 😄 Err, this somehow happens to me in the last time - I speak what I'm about to write in my head but write words that sound similar instead.

ebkalderon · Jun 13, 2017

assets/Cargo.toml

+futures = "0.1.14"
+parking_lot = "0.4.4"
+rayon = { version = "0.7", features = ["unstable"] }
+


Don't forget to specify documentation, homepage, repository, categories, keywords, travis-ci, and appveyor. Also, the trailing newline isn't needed, I think.

ebkalderon · Jun 13, 2017

assets/Cargo.toml

+[dependencies]
+fnv = "1"
+futures = "0.1.14"
+parking_lot = "0.4.4"


Do we need to specify the patch numbers here according to semver?

Id personally prefer it if we did since there is no guarantee a dependency couldnt accidentally break something. Moreso it is just an honor code right now.

I basically agree with what @Aceeri said: I prefer to just specify the latest patch version, since I don't know whether or not I'm using features of a patch, so it could work for me and on CI, but the dependency version isn't correct.

ebkalderon · Jun 13, 2017

assets/LICENSE-APACHE

@@ -0,0 +1,201 @@
+                            Apache License


If amethyst_assets contains its own README.md and license files, should the other crates contain their own copies as well? Not against it; just wanted to know what you think we should do going forward.

I'm not sure; AFAIK licenses should be distributed with the source code. Now, a cargo package contains the source code, but not the top-level license. That's the reason I put it in there, just to not cause any trouble. But I personally don't care whether or not there's a license in these crates.

Looking at gfx and Servo as an example, the subcrates do not each contain licenses, presumably because the license files found at the repository root apply to all source code in the tree. But then again, IANAL.

Let's remove them then.

ebkalderon · Jun 13, 2017

CHANGELOG.md

@@ -9,6 +9,11 @@ The format is based on [Keep a Changelog][kc], and this project adheres to
 
 ## [Unreleased]
 


I don't think this trailing newline should be here, strictly speaking, according to KeepAChangelog.

Wow, thanks for the detailed review! I'm actually still resolving some questions myself. And sorry for the weird API, I did a lot of refactoring so not everything has been optimized for usage yet.

torkleyy · Jun 14, 2017

Wow, thanks for the detailed review! I'm actually still resolving some questions myself. And sorry for the weird API, I did a lot of refactoring so not everything has been optimized for usage yet.

I guess amethyst_assets would end up replacing the project module eventually?

Yes, it could be made an asset then, but the config! macro would remain.

torkleyy · Jun 14, 2017

I guess amethyst_assets would end up replacing the project module eventually?

Yes, it could be made an asset then, but the config! macro would remain.

Speaking of, do you have any plans to include hot reloading of modified assets eventually?

That would be a great feature! But there is so much else to figure out. This is much harder than I thought from a design point of view.

torkleyy · Jun 14, 2017

Speaking of, do you have any plans to include hot reloading of modified assets eventually?

That would be a great feature! But there is so much else to figure out. This is much harder than I thought from a design point of view.

omni-viral · Jun 14, 2017

omni-viral reviewed Jun 14, 2017

View changes

omni-viral · Jun 14, 2017

assets/src/loader.rs

+    let name = name.into();
+
+    let store_id = storage.store_id();
+    let spec = AssetSpec::new(name.clone(), F::extension(), store_id);


I'd like to avoid allocations (String::clone) in case of getting cached asset

I don't see how that is possible.

omni-viral · Jun 14, 2017

assets/src/store/mod.rs

+/// methods for loading
+pub trait Store {
+    type Error: Error + 'static;
+    type Location;


I didn't find how it is used. Can you explain? )

Refactoring... sorry

Thanks for so much feedback so far! I'll continue development soon; no need to review in that time, I may have already fixed it. I'll tell you when it's ready!

torkleyy · Jun 14, 2017

Thanks for so much feedback so far! I'll continue development soon; no need to review in that time, I may have already fixed it. I'll tell you when it's ready!

ebkalderon · Jun 15, 2017

ebkalderon reviewed Jun 15, 2017

View changes

Keep up the good work, @torkleyy. Just added a few more comments and questions.

ebkalderon · Jun 15, 2017

assets/src/error.rs

+use AssetSpec;
+
+/// Error type returned when loading an asset.
+/// Includes the `AssetSpec` and the error (`LoadErrorPure`).


Thanks for addressing my concerns, @torkleyy. Don't forget to correct this doc comment! 😉

ebkalderon · Jun 15, 2017

assets/src/loader.rs

+        .map_err(LoadError::StorageError)?;
+    let data = format.import(bytes).map_err(LoadError::FormatError)?;
+    let a = Asset::from_data(data, context)
+        .map_err(LoadError::AssetError)?;


To avoid these .map_err()s everywhere, I would recommend implementing From<Whatever> for LoadError.

That's not possible; I didn't define the error type I'm getting an instance of here, it's an associated type implementing Error.

ebkalderon · Jun 15, 2017

assets/src/error.rs

+/// Used as a placeholder for associated error types if
+/// something cannot fail.
+#[derive(Debug)]
+pub enum NoError {}


I don't have access to a computer right now, but could the usage of this placeholder error be viably replaced with ()?

A phantom type has better semantics than ()

In addition to what @kvark said, () does not implement Error.

It's not really a phantom type. It's a void or 'bottom' type - ie. an enum with no variants. The neat thing about it is that it is impossible to create at run time, unlike () which can be created exactly one time - ie. using (). If you used () you would have to sprinkle around unreacable!()s in a bunch of places, where as with an empty enum the compiler will allow you to return from an impossible-to-call function by doing match impossible {}.

ebkalderon · Jun 15, 2017

assets/src/asset.rs

+
+    /// Notifies about an asset load. This is can be used to cache the asset.
+    /// To return a cached asset, see the `cached` function.
+    fn asset_loaded(_context: &Self::Context, _spec: AssetSpec, _asset: &Self) {}


I don't quite understand from the doc comment what you mean by "notifies about an asset load." Notifies whom? What do you mean by an asset load (from disk, from Store, from a cache)?

Also, if the point of this method is to cache an asset, perhaps you could rename this method to something like cache() and then rename the method below to retrieve()?

ebkalderon · Jun 15, 2017

assets/LICENSE-APACHE

@@ -0,0 +1,201 @@
+                            Apache License


Looking at gfx and Servo as an example, the subcrates do not each contain licenses, presumably because the license files found at the repository root apply to all source code in the tree. But then again, IANAL.

ebkalderon · Jun 15, 2017

assets/src/asset.rs

+
+impl AssetSpec {
+    /// Creates a new asset specifier from the given parameters.
+    pub fn new(name: String, ext: &'static str, store: StoreId) -> Self {


Would you be able to make this an Into<String> as well? I think I missed this on the last review.

It's only used in the code of this crate, so I don't think it's necessary.

Cool, fair enough.

ebkalderon · Jun 15, 2017

assets/src/asset.rs

+{
+    /// Creates a new `Cache` and initializes it with the default values.
+    pub fn new() -> Self {
+        Default::default()


Shouldn't you #[derive(Default)] and then replace this line with Self::default()? Or would that complicate things with T if T doesn't implement Default?

Self::default and Default::default are the same here

derive doesn't work here because of exactly that reason

Cool! Never mind my comment here, then.

So it looks like some of my work might end up having a merge conflict with this, I'm specifically working on key rebinding. I'd like to make the key rebinding PR sometime between 2017-06-17 & 2017-06-18. I'm thinking that the "default" keybinding should probably be an asset, with the active player rebinding file being a config file, similar to our display config files.

This PR kind of looks like it's in the thick of iteration, so I don't think it'll be merged by the time the key rebinding functionality is ready. Should I just integrate that default file with the existing asset system? I could also make a PR to the branch that is hosting this PR to give it the same functionality I'd be giving the old asset management system.

Xaeroxe · Jun 16, 2017

So it looks like some of my work might end up having a merge conflict with this, I'm specifically working on key rebinding. I'd like to make the key rebinding PR sometime between 2017-06-17 & 2017-06-18. I'm thinking that the "default" keybinding should probably be an asset, with the active player rebinding file being a config file, similar to our display config files.

This PR kind of looks like it's in the thick of iteration, so I don't think it'll be merged by the time the key rebinding functionality is ready. Should I just integrate that default file with the existing asset system? I could also make a PR to the branch that is hosting this PR to give it the same functionality I'd be giving the old asset management system.

@Xaeroxe I'm not sure because I don't know how much this PR will change. I don't expect it to change too much though, I guess the main part I'll change is the Loader, so I think you should be able to base your key bindings on the Format and Asset traits.

torkleyy · Jun 17, 2017

@Xaeroxe I'm not sure because I don't know how much this PR will change. I don't expect it to change too much though, I guess the main part I'll change is the Loader, so I think you should be able to base your key bindings on the Format and Asset traits.

Xaeroxe · Jun 21, 2017

Xaeroxe reviewed Jun 21, 2017

View changes

Looks great! Very robust and re-usable code here. I had a few nits and chimed in on some existing conversation. Thanks for your work!

Xaeroxe · Jun 21, 2017

assets/src/asset.rs

+    }
+
+    /// Inserts an asset, locking the internal `RwLock` to get write access to the hash map.
+    pub fn insert(&self, spec: AssetSpec, asset: T) {


If a value exists for the given key in a HashMap the insert operation will return the value it replaced. It might be helpful to have this function do the same, so that users of this function can identify when they did a redundant insert, and modify their code to not do those redundant inserts. It's certainly not necessary, it's just helpful for debugging.

Well, why would somebody check the value? The insert will only be called if there hasn't been a cached version.

I guess the "somebody" would be us, in the event we ever have to modify how this function is called.

Xaeroxe · Jun 21, 2017

assets/src/asset.rs

+
+    /// Retrieves an asset, locking the internal `RwLock` to get read access to the hash map.
+    /// In case this asset has been inserted previously, it will be cloned and returned.
+    /// Otherwise, you'll `None`.


Doc comment is slightly bad english, should be "Otherwise, you'll receive None.

Oops, I think I the verb here 😂

Xaeroxe · Jun 21, 2017

assets/src/asset.rs

+    /// * `"png"`
+    /// * `"obj"`
+    /// * `"wav"`
+    fn extension() -> &'static str;


Personally I think we shouldn't support files with no extension. Not all formats/files have header data and extensions are a nice way to make some sane assumptions about the underlying data.

Xaeroxe · Jun 21, 2017

assets/src/asset.rs

+    fn extension() -> &'static str;
+
+    /// Imports the given bytes and produces asset data.
+    fn import(&self, bytes: Vec<u8>) -> Result<Self::Data, Self::Error>;


Some in-memory asset data structures (I might even argue most) won't need to store the source data contained on disk. So it'd be nice to pass in a slice here and let the implementation clone the data if it needs to. Having to clone the vec when one was already allocated is an unfortunate downside to this but I personally think it's the correct approach.

Xaeroxe · Jun 21, 2017

assets/src/error.rs

+
+    fn description(&self) -> &str {
+        match *self {
+            LoadError::AssetError(_) => "Failed to load asset",


Failed to do thing type error messages have been very frustrating for me to debug in the past, due to the lack of additional information. Perhaps we could also append the description() for the inner value? That would help this a bit.

The error description method is extremely unhelpful. Normal use cases would just use a Debug implementation because here you need a reference to a string, which makes it impossible to create a new string other than one stored inside the structure or is a static string.

Yeah you're right. These thoughts started occurring to me after I made my comment of course :P

Xaeroxe · Jun 21, 2017

assets/src/loader.rs

+    {
+        let context = self.contexts
+            .get(&TypeId::of::<A>())
+            .expect("Assets need to be registered");


I feel like panic is a good thing to do here, mainly because if a problem causes code to be fundamentally broken it's important to have high visibility on it in debugging.

No idea why, but I can't comment on:

I feel like panic is a good thing to do here, mainly because if a problem causes code to be fundamentally broken it's important to have high visibility on it in debugging.

Anyways, expect does panic, it is basically a match of:

match result {
    Ok(ok) => ok,
    Err(_) => panic!(msg),
}

Aceeri · Jun 21, 2017

No idea why, but I can't comment on:

I feel like panic is a good thing to do here, mainly because if a problem causes code to be fundamentally broken it's important to have high visibility on it in debugging.

Anyways, expect does panic, it is basically a match of:

match result {
    Ok(ok) => ok,
    Err(_) => panic!(msg),
}

@Aceeri yeah that comment is out of context thanks to GitHub. I was responding to @ebkalderon.

Xaeroxe · Jun 21, 2017

@Aceeri yeah that comment is out of context thanks to GitHub. I was responding to @ebkalderon.

Blocked on rayon-rs/rayon#344

torkleyy · Jul 6, 2017

Blocked on rayon-rs/rayon#344

lschmierer · Jul 12, 2017

lschmierer reviewed Jul 12, 2017

View changes

lschmierer · Jul 12, 2017

assets/src/loader.rs

+
+    let closure = move || load_asset::<A, F, _, S>(&*context, &format, name, &*storage);
+
+    AssetFuture(thread_pool.spawn_future_async(lazy(closure)))


Mayber return a Stream for hot-reloading support later?

For noe we can simply use
futures::stream::futures_ordered
http://alexcrichton.com/futures-rs/futures/stream/fn.futures_ordered.html

Interesting idea..I have to think about it. Just keep in mind that hot-reloading is more a feature for development, so it's not necessary for the final game.

torkleyy · Jul 12, 2017

torkleyy reviewed Jul 12, 2017

View changes

torkleyy · Jul 12, 2017

assets/src/loader.rs

+/// Represents a future value of an asset. This future may be
+/// added to the ECS world, where the responsible system can poll it and merge
+/// it into the persistent storage once it is `Ready`.
+pub struct AssetFuture<A, E> {


~~So there is something wrong about AssetFuture. Calling Future::wait on it just blocks forever...~~

Fixed now.

Okay, as discussed with @Xaeroxe, this PR should be merged after the core functionality is implemented, which it is. Stuff like hot reloading will be implemented later.

So this PR is ready for review now.

torkleyy · Jul 12, 2017

Okay, as discussed with @Xaeroxe, this PR should be merged after the core functionality is implemented, which it is. Stuff like hot reloading will be implemented later.

So this PR is ready for review now.

Xaeroxe · Jul 12, 2017

Xaeroxe reviewed Jul 12, 2017

View changes

Looks good to me for the most part, I had a few nits on the internals but I like the API. Thanks!

Xaeroxe · Jul 12, 2017

assets/src/error.rs

+
+impl Display for NoError {
+    fn fmt(&self, _: &mut Formatter) -> FmtResult {
+        match *self {}


This is a bizarre syntactic construct. I'm kind of surprised it even compiles. This appears to return () in a function which should be returning FmtResult. According to rust-lang/rust#24590 this appears to be undefined behavior. Is there any way we can improve upon this? Return some kind of default useless value maybe? This might be a pain point in the future.

I could not find the issue saying it's undefined behavior. I'd actually see it's very clearly defined and makes sense - NoError can never be instantiated, and this empty match perfectly reflects that.

The undefined behavior is still instantiating NoError, not the empty match.

I still think we should change it primarily because this could in future Rust versions lead to E0308. This statement appears to return (), not FmtResult. I feel something like Ok(()) is much less likely to break in the future.

Basically I'm pretty sure the only reason this compiles is because the compiler optimizes out this implementation in the final binary. In my opinion that's a bug in rustc and we shouldn't rely on bugs in our compiler.

The reason it compiles is because NoError has no variants, and thus can never be created. This means that it is impossible for fmt to ever be called! You do have to figure out a way to return a value from the function though. To do this you use the funny looking match on no cases. In the future we will have the never type, ! (under the #[feature(never_type)] flag). This basically gives us:

enum ! {}

I think it will impl a bunch of core traits for cases like this. Using a NoError type in the interim seems like a reasonable thing to do though though.

See https://github.com/rust-lang/rfcs/blob/master/text/1216-bang-type.md

Putting unreachable!() is completely unnecessary, because the code can never be compiled. This is what a great type system allows you to do! 😄

Think about it. Say if I have:

enum Impossible {} fn foo(impossible Impossible) -> Vec<String> { // ... }

How do I ever call foo?

let vec = foo(Impossible::/*ugghhhhh*/);

Nothing you ever write, will ever satisfy the compiler. Therefore the compiler will allow you to do crazy things like summoning a vector from nothing, because it knows the function can never be called:

fn foo(impossible Impossible) -> Vec<String> { match impossible {} }

This is a super useful tool when using other folk's API's that take type parameters, or associated types, like the futures crate. It allows you to tell the compiler at the type level that, 'this future will always succeed'. Eg.

fn no_unwrapping_wait<F: Future<Error = Impossible>>(f: F) -> F::Item { match f.wait() { Ok(item) => item, } }

Note that there is no Err case, because that is impossible to get to, and is hence ruled out by the compiler.

Yeah that was more or less the conclusion I came to below. I'm content with this solution.

Yeah, sorry, missed your update! 😅 - was fun thinking about how to explain it though!

Was thinking for some reason that the enum could be instantiated similar to a zero-sized struct.

Xaeroxe · Jul 12, 2017

assets/src/error.rs

+
+impl Error for NoError {
+    fn description(&self) -> &str {
+        match *self {}


Same as above.

Xaeroxe · Jul 12, 2017

assets/src/loader.rs

+    }
+}
+
+struct AssetFutureInner<A, E> {


AssetFutureCell might be a better name for this struct.

Xaeroxe · Jul 12, 2017

Xaeroxe requested changes Jul 12, 2017 • edited

View changes

I'm leaving my review status as "Request changes" until the empty match statements I commented on earlier are replaced. I am otherwise very eager to merge this.

Xaeroxe · Jul 13, 2017

Xaeroxe approved these changes Jul 13, 2017

View changes

So @torkleyy wrote this: https://www.reddit.com/r/rust/comments/6mwkqk/empty_matches/ and the comments on here actually make a pretty convincing case so I'm going to drop my complaint from before.

Requesting review from @ebkalderon and @nchashch. I'd like to prioritize getting this merged as it's blocking rather a lot of work.

Xaeroxe · Jul 13, 2017

Requesting review from @ebkalderon and @nchashch. I'd like to prioritize getting this merged as it's blocking rather a lot of work.

omni-viral · Jul 13, 2017

omni-viral reviewed Jul 13, 2017

View changes

omni-viral · Jul 13, 2017

assets/src/asset.rs

+    fn from_data(data: Self::Data, context: &Self::Context) -> Result<Self, Self::Error>;
+
+    /// Notifies about an asset load. This is can be used to cache the asset.
+    /// To return a cached asset, see the `cached` function.


where is this cached function?

As the crate name is amethyst_assets, the folder should probably called amethyst_assets, too.

Like amethyst_renderer on @ebkalderon's renderer PR.

lschmierer · Jul 13, 2017

As the crate name is amethyst_assets, the folder should probably called amethyst_assets, too.

Like amethyst_renderer on @ebkalderon's renderer PR.

At least some tetsts would be nice.

lschmierer · Jul 13, 2017

At least some tetsts would be nice.

@lschmierer I know that, but I thought that at this stage we would like to proceed a bit faster because changes are rather big atm. But I agree - I'll definitely write some before we release a new version!

torkleyy · Jul 13, 2017

@lschmierer I know that, but I thought that at this stage we would like to proceed a bit faster because changes are rather big atm. But I agree - I'll definitely write some before we release a new version!

As the crate name is amethyst_assets, the folder should probably called amethyst_assets, too.

But why prefix everything with amethyst_? I mean that's already clear from the repository the folder is in, isn't it?

OTOTH, consistency would be nice. So maybe @ebkalderon could rename the folder to just renderer?

torkleyy · Jul 13, 2017

As the crate name is amethyst_assets, the folder should probably called amethyst_assets, too.

But why prefix everything with amethyst_? I mean that's already clear from the repository the folder is in, isn't it?

OTOTH, consistency would be nice. So maybe @ebkalderon could rename the folder to just renderer?

nchashch · Jul 14, 2017

nchashch approved these changes Jul 14, 2017

View changes

I skimmed through the code, great work 👍.

Aceeri · Jul 14, 2017

Aceeri requested changes Jul 14, 2017

View changes

Looks pretty nice to me, just a couple nitpicks about documentation and a couple of questions.

Aceeri · Jul 14, 2017

assets/examples/cache.rs

+    let pool = Arc::new(ThreadPool::new(cfg).expect("Invalid config"));
+
+    let alloc = Allocator::new();
+    let mut loader = Loader::new(&alloc, "examples/assets", pool);


Might be better to do

format!("{}/examples/assets", env!("CARGO_MANIFEST_DIR"));

here so running the example doesn't require building from the crate root.

Aceeri · Jul 14, 2017

assets/src/asset.rs

+/// A specifier for an asset, uniquely identifying it by
+///
+/// * the extension (the format it was provided in)
+/// * it's name


"it's" -> "its"

Aceeri · Jul 14, 2017

assets/src/asset.rs

+    pub ext: &'static str,
+    /// The name of this asset.
+    pub name: String,
+    /// The storage id, uniquely identifying the storage it's been loaded from.


same as above

Are you sure? I thought I could use it as a shorthand for "it has"?

I suppose, it just sounds odd to me.

Aceeri · Jul 14, 2017

assets/src/asset.rs

+    fn extension() -> &'static str;
+
+    /// Reads the given bytes and produces asset data.
+    fn parse(&self, bytes: Vec<u8>) -> Result<Self::Data, Self::Error>;


I feel like there was a discussion about this earlier, but I can't find it. Would it be better for this to take in a &'a [u8]? I'd assume that this wouldn't be modifying the bytes passed in at least.

There was a discussion on this but Github marked it outdated due to other changes.

No it would not be better to take a slice, primarily because the bytes would be dropped after this function is called and for some very simple asset types the bytes themselves might be part of the asset, so passing in a Vec here allows us to save an allocation in some instances, and is harmless in others.

The best would probably be a stream.

Do you plan to make this and the other similar methods accept a stream instead of a Vec before merging?

@ebkalderon I think that was going to be a follow-up PR.

Aceeri · Jul 14, 2017

assets/src/loader.rs

+        }
+    }
+
+    pub fn add_store<I, S>(&mut self, name: I, store: S)


Should this have docs or do we think it's self-explanatory enough?

I feel it's pretty self-explanatory.

Oh yeah, probably, but I'd like every item in the crate to have docs. Should enable that lint.

Aceeri · Jul 14, 2017

assets/src/loader.rs

+            .insert(TypeId::of::<A>(), Box::new(Arc::new(context)));
+    }
+
+    /// Like `load_from`, but doesn't ask the cache for a


Incomplete documentation.

Aceeri · Jul 14, 2017

assets/src/loader.rs

+    ///
+    /// # Panics
+    ///
+    /// Panics if the asset wasn't registered.


Is this ideal? Are assets statically registered similar to components/resources in specs or can they be unregistered and such?

Assets can't be unregistered and with this system are intended to be statically registered, so I think this is fine.

Yeah it's fine, not ideal, however. The problem is that registering can only work mutably, but loading an asset should be immutable. So they cannot be auto-registered or smth like that.

Is it just an unregistered asset format? Or do you also need to register each asset themselves for it to not panic?

You must register asset formats before loading an asset of that format. You are not required to register the individual assets only the formats which they take.

You register every type that implements Asset (in case you want to load it at some point).

Aceeri · Jul 14, 2017

assets/src/store/mod.rs

+    }
+}
+
+pub trait AnyStore: Send + Sync + 'static {


Should probably be docs here as well or a forward to the Store docs.

Can add a short note, but it's not exposed to the user.

Aceeri · Jul 14, 2017

assets/src/loader.rs

+
+/// Like `load_asset`, but loads the asset on a worker thread and returns
+/// an `AssetFuture` immediately.
+pub fn reload_asset_future<A, F, N, S>


Should these reload methods specify the difference between what they do and just the regular load methods?

Okay, so I see there are still things left (like streams for the storages, hot reloading, implementation of asset formats, ...), but I think we can still merge this for now. That way @Xaeroxe can already build audio stuff, and @ebkalderon rendering assets. If there are critical issues to fix before merging, feel free to push to my branch, because I won't have access to my computer for the next few days.

torkleyy · Jul 16, 2017

Okay, so I see there are still things left (like streams for the storages, hot reloading, implementation of asset formats, ...), but I think we can still merge this for now. That way @Xaeroxe can already build audio stuff, and @ebkalderon rendering assets. If there are critical issues to fix before merging, feel free to push to my branch, because I won't have access to my computer for the next few days.

Aceeri · Jul 16, 2017

Aceeri approved these changes Jul 16, 2017

View changes

ebkalderon · Jul 19, 2017

ebkalderon requested changes Jul 19, 2017

View changes

Terrific job @torkleyy! Thank you for addressing pretty much all the concerns raised. Almost ready to merge, I think, though there are a few final nitpicks left to address.

ebkalderon · Jul 19, 2017

assets/src/asset.rs

+    pub ext: &'static str,
+    /// The name of this asset.
+    pub name: String,
+    /// The storage id, uniquely identifying the storage it's been loaded from.


This comment sounds a tad awkward. Perhaps it could read instead:

/// Unique identifier indicating the Storage from which the asset was loaded.

ebkalderon · Jul 19, 2017

assets/src/asset.rs

+    fn extension() -> &'static str;
+
+    /// Reads the given bytes and produces asset data.
+    fn parse(&self, bytes: Vec<u8>) -> Result<Self::Data, Self::Error>;


Do you plan to make this and the other similar methods accept a stream instead of a Vec before merging?

ebkalderon · Jul 19, 2017

assets/Cargo.toml

+description = """
+Asynchronous asset management for games.
+"""
+


To keep amethyst_assets consistent with other crates, please don't forget to add:

license = "MIT/Apache-2.0" keywords = ["game", "asset", "resource", "management", "amethyst"] categories = ["filesystem"] documentation = "https://www.amethyst.rs/doc/master/amethyst" homepage = "https://www.amethyst.rs/" repository = "https://github.com/amethyst/amethyst" [badges] appveyor = { repository = "amethyst/amethyst", branch = "develop" } travis-ci = { repository = "amethyst/amethyst" }

ebkalderon · Jul 19, 2017

Cargo.toml

@@ -75,4 +75,4 @@ name = "assets"
 path = "examples/05_assets/main.rs"
 
 [workspace]
-members = ["src/renderer"]
+members = ["assets", "src/renderer"]


I would prefer to name this folder amethyst_assets simply to differentiate subcrates from the book and src directories. It may look a little verbose, but at least users won't be misled into thinking there are real game assets in that folder, as @lschmierer pointed out in the Gitter chat a while ago.

Thanks to everyone who participated and contributed! This is a pretty big win for us. 🎉

Special thanks to @torkleyy for his hard work, and also to @Xaeroxe for finalizing and merging this pull request on such short notice.

ebkalderon · Jul 19, 2017

Thanks to everyone who participated and contributed! This is a pretty big win for us. 🎉

Special thanks to @torkleyy for his hard work, and also to @Xaeroxe for finalizing and merging this pull request on such short notice.

torkleyy referenced this pull request Jun 13, 2017
Closed
Restructuring of the asset management #195
14 of 14 tasks complete

nchashch referenced this pull request Jun 13, 2017
Closed
Release 0.5.0 #241
13 of 13 tasks complete

Xaeroxe referenced this pull request Jun 17, 2017
Merged
Input rebind patch #247

nchashch self-requested a review Jun 19, 2017

ebkalderon referenced this pull request Jun 30, 2017
Merged
Convert to specs v0.9 #250

torkleyy referenced this pull request in slide-rs/specs Jul 1, 2017
Merged
Integrate futures #209

bors bot added a commit to slide-rs/specs that referenced this pull request Jul 2, 2017

Merge #209
209: Integrate futures r=torkleyy cc @ebkalderon @Xaeroxe Will be used by amethyst/amethyst#244

Loading status checks…

56e3b69

torkleyy added this to the 0.5.0 milestone Jul 4, 2017

torkleyy added some commits Jul 12, 2017

Nearly there

Loading status checks…

bfca4f6

Remove license files

Loading status checks…

f64fc70

Use amethyst_assets as crate name

Loading status checks…

4430d88

torkleyy added some commits Jul 12, 2017

Reexport specs BoxedErr

3db07f5

Fix Future::wait

Loading status checks…

20bd059

torkleyy requested review from ebkalderon and Xaeroxe Jul 12, 2017

Rename functions of Asset
This commit renames asset_loaded to cache and cached to retrieve.

Loading status checks…

f56beee

Rename AssetFutureInner to AssetFutureCell

Loading status checks…

c4c3146

Fix doc comment

Loading status checks…

aeb1358

torkleyy added some commits Jul 16, 2017

Make assets path relative to CARGO_MANIFEST_DIR

791018d

Improve documentation

Loading status checks…

d4dde98

Xaeroxe added some commits Jul 19, 2017

Address Eyal Kalderon's comments

5bd9b3c

Merge branch 'develop' into assets

Loading status checks…

438e6fe

Xaeroxe merged commit 1b897a1 into amethyst:develop Jul 19, 2017
0 of 2 checks passed

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

amethyst/amethyst

Join GitHub today

Core asset management #244

Conversation

torkleyy commented Jun 13, 2017

torkleyy referenced this pull request Jun 13, 2017

nchashch referenced this pull request Jun 13, 2017

ebkalderon requested changes Jun 13, 2017 • edited 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.

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.

This comment has been minimized.

ebkalderon Jun 13, 2017 • edited Edited 1 time ebkalderon edited Jun 13, 2017 (most recent)

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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

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

This comment has been minimized.

torkleyy commented Jun 14, 2017

This comment has been minimized.

torkleyy commented Jun 14, 2017

torkleyy added some commits Jun 13, 2017

omni-viral reviewed Jun 14, 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 commented Jun 14, 2017

ebkalderon reviewed Jun 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.

This comment has been minimized.

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

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.

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.

Xaeroxe commented Jun 16, 2017 • edited Edited 1 time Xaeroxe edited Jun 16, 2017 (most recent)

This comment has been minimized.

torkleyy commented Jun 17, 2017

Xaeroxe referenced this pull request Jun 17, 2017

nchashch self-requested a review Jun 19, 2017

Xaeroxe reviewed Jun 21, 2017 View changes

This comment has been minimized.

ebkalderon requested changes Jun 13, 2017 • edited

View changes

ebkalderon Jun 13, 2017 •

edited

Edited 1 time

ebkalderon edited Jun 13, 2017 (most recent)

torkleyy commented Jun 14, 2017 •

edited

Edited 1 time

torkleyy edited Jun 14, 2017 (most recent)

omni-viral reviewed Jun 14, 2017

View changes

ebkalderon reviewed Jun 15, 2017

View changes

torkleyy Jun 16, 2017 •

edited

Edited 1 time

torkleyy edited Jun 16, 2017 (most recent)

Xaeroxe commented Jun 16, 2017 •

edited

Edited 1 time

Xaeroxe edited Jun 16, 2017 (most recent)

Xaeroxe reviewed Jun 21, 2017

View changes