Add asset hot reloading #445

torkleyy · Oct 22, 2017

Adds a new resource to the world, Arc<rayon::ThreadPool> (or short: core::ThreadPool)
Adds WeakHandles
Automatically reloads assets (checked every second)

Fixes #266

Problems

~~If a file has been written only partially and the asset storage tries to load it, it fails :( I'm not sure if there's a check we can do~~ Fixed by using fallback

So as @Rhuagh pointed out, we need some sort of notification system, but I think it's better to do that in a follow-up.

torkleyy · Oct 22, 2017

So as @Rhuagh pointed out, we need some sort of notification system, but I think it's better to do that in a follow-up.

Rhuagh · Oct 22, 2017

Rhuagh reviewed Oct 22, 2017

View changes

Rhuagh · Oct 22, 2017

amethyst_assets/examples/hl.rs

 struct Ron;
 
-impl Format<MeshAsset> for Ron {
+impl SimpleFormat<MeshAsset> for Ron {


Good idea to simplify the simple formats :)

Rhuagh · Oct 22, 2017

amethyst_assets/src/loader.rs

@@ -25,6 +26,7 @@ impl Loader {
     {
         Loader {
             directory: Arc::new(Directory::new(directory)),
+            hot_reload: true, // TODO: allow disabling


Should we really default to true here? Even for the first iteration?

No, I have to add configs to this; the param is just true to try it out.

omni-viral · Oct 23, 2017

omni-viral requested changes Oct 23, 2017

View changes

I like the idea.
But I have concerns about overhead when you don't use the feature.
And I see it's hard to make all this feature-gated.

omni-viral · Oct 23, 2017

amethyst_renderer/src/formats/texture.rs

-    ) -> Result<TextureData, BoxedErr> {
-        imagefmt::bmp::read(&mut Cursor::new(source.load(&name)?), ColFmt::RGBA)
+    fn import(&self, bytes: Vec<u8>, options: TextureMetadata) -> Result<TextureData, BoxedErr> {
+        imagefmt::bmp::read(&mut Cursor::new(bytes), ColFmt::RGBA)


It's better to read BMP file directly into gpu visible memory.
But this can be addressed later. Just put a note, please.

omni-viral · Oct 23, 2017

amethyst_assets/src/asset.rs

     fn import(
         &self,
         name: String,
         source: Arc<Source>,
         options: Self::Options,
-    ) -> Result<A::Data, BoxedErr>;
+        create_reload: bool,
+    ) -> Result<(A::Data, Option<Box<Reload<A>>>), BoxedErr>;


This result type looks sophisticated.
Maybe it's worth to make a structure.

omni-viral · Oct 23, 2017

amethyst_assets/src/reload.rs

+            source,
+            options,
+            ..
+        } = this;


Can't you just write *self here?

It seems to be an unsoundness w.r.t. the Box<Self> stuff.

You mean that if you write *self here it will not compile?

omni-viral · Oct 23, 2017

amethyst_assets/src/asset.rs

+    /// The reload structure has metadata which allows the asset management
+    /// to reload assets if necessary (for hot reloading).
+    /// You should only create this if `create_reload` is `true`.
+    /// Also, the parameter is just a request, which means you can also return `None`.
     fn import(
         &self,
         name: String,


name and options should be passed by reference to avoid unnecessary cloning.

And in SimpleFormat as well. Otherwise cloning will be necessary 😄

I'm aware of the fact that an allocation is not desirable, but we can't point to the stack in a multi-threaded system.

I don't follow. If you use those only locally in function (or nested function calls). It's perfectly ok to pass them as references. And if you need to save them for future-use (In returned Reload) you clone value and store it.

Please point me to the exact location where I could just pass a reference.

name and options in Format::import and options in SimpleFormat.
And then in impl Reload for SingleFile you can reuse SingleFile value instead of creating new one

As I've explained, you can't do that.

omni-viral · Oct 23, 2017

amethyst_assets/src/reload.rs

+            ..
+        } = this;
+
+        format.import(path, source, options, true)


Can't it ask not to create new Reload object and return self?

No, imagine a GLTF file which points to other files; in case the GLTF file gets reloaded, new files might have been added which we also want to track.

I makes sense for multi file assets. But this is SingleFile.

So how would you do that?

omni-viral · Oct 23, 2017

amethyst_assets/src/reload.rs

+    /// Returns the asset name.
+    fn name(&self) -> String;
+    /// Returns the format name.
+    fn format(&self) -> String;


All formats has it's name as associated &'static str. So you can contain and return &'static str.

I had this as &'static str first, but changed it back for an unknown reason. I'll try to change this later.

omni-viral · Oct 23, 2017

amethyst_assets/src/storage.rs

@@ -30,9 +33,11 @@ impl Allocator {
 pub struct AssetStorage<A: Asset> {
     assets: VecStorage<A>,
     bitset: BitSet,
-    handles: Vec<Handle<A>>,
+    handles: Vec<WeakHandle<A>>,


No need for using Weak inside AssetStorage

Upgrading Weak has more overhead than cloning Arc. There is possibly infinite loop with CAS.

You get nothing from Weak as you store simple u32 and you don't have to drop it asap to free some resource. No memory get freed until you drop last Weak.

And then you have to allocate new Arc after you finally drop Weak.

Using WeakHandle outside of AssetStorage may make some sense though.

The biggest reason I used WeakHandle here is for consistency, since the AssetStorage does not in fact own a handle (logically). And for reloads it is indeed necessary.

omni-viral · Oct 23, 2017

omni-viral requested changes Oct 23, 2017

View changes

omni-viral · Oct 23, 2017

amethyst_assets/src/storage.rs

-        while let Some(i) = self.handles.iter().position(Handle::is_unused) {
-            let old = self.handles.swap_remove(i);
-            let id = i as u32;
+        while let Some(i) = self.handles.iter().position(Handle::is_unique) {


You shouldn't search from start each time.
Try this.

let mut iter = self.handles.iter(); let mut i = 0; while let Some(next) = iter.by_ref().position(Handle::is_unique) { i += next;

~~Sorry, I was being stupid during refactoring.~~

I'm not sure your solution works, but I'll change this. Anyway, I plan on changing that cleanup code as to make it more efficient.

omni-viral · Oct 23, 2017

amethyst_assets/src/storage.rs

-            self.unused_handles.push(old);
+
+            // Can't reuse old handle here, because otherwise weak handles would still be valid.
+            // TODO: maybe just store u32?


Can reuse again

Sorry, I don't understand?

You can reuse Handle. As it's not WeakHandle again.

I think what you're saying is that because I changed it back to Handle, I can reuse the handles again?

I don't think we'd want that, because:

Handle A gets allocated

Handle A has a downgraded version in a cache

Handle A gets dropped

Handle A gets added to the unused_handles queue

Another allocation is required; unused_handles gives us the "old" handle A

The cached value we stored before (using WeakHandle) is now valid again

That's why I added that comment and recreated the handle. Did a make a mistake in my reasoning?

Makes sense.

omni-viral · Oct 23, 2017

amethyst_assets/src/storage.rs

+        }
+
+        // Reload every seconds
+        // TODO: more configuration


You can add more details to TODO.
Configure timeout.
Configure automatic or only manual with special method in AssetStorage.

I will add such a structure in this PR.

omni-viral · Oct 23, 2017

amethyst_assets/src/storage.rs

-    pub format: String,
-    pub handle: Handle<A>,
-    pub name: String,
+pub enum Processed<A: Asset> {


It pub necessary here?

Enums can't have private members.

I think I misunderstood your question. Yes, Processed needs to be pub, because Loader creates such a structure. It's not exported at crate root, though.

Can you make it pub(crate) then?

There's no need for that and besides, I think that syntax is horrible. I try not to use it where possible.

Why? It's very straightforward and obvious. You don't need to look at other files to figure out it's not actually pub, and pub(crate) permits easy re-use of internal structures.

So as @Rhuagh pointed out, we need some sort of notification system, but I think it's better to do that in a follow-up.

Events??? :P

jojolepro · Oct 23, 2017

So as @Rhuagh pointed out, we need some sort of notification system, but I think it's better to do that in a follow-up.

Events??? :P

@jojolepro Not really, more like watching a directory.

torkleyy · Oct 23, 2017

@jojolepro Not really, more like watching a directory.

omni-viral · Oct 23, 2017

omni-viral requested changes Oct 23, 2017

View changes

omni-viral · Oct 23, 2017

amethyst_assets/src/reload.rs

+    /// The frame after calling this, all changed assets will be reloaded.
+    pub fn trigger(&mut self) {
+        let counter = match self.inner {
+            HotReloadStrategyInner::Trigger { counter } => counter.checked_add(1).unwrap_or(0),


Just wrapping_add instead of checked_add and unwrap_or

That's what I've been searching for, thanks!

omni-viral · Oct 23, 2017

amethyst_assets/src/reload.rs

+    pub fn trigger(&mut self) {
+        let counter = match self.inner {
+            HotReloadStrategyInner::Trigger { counter } => counter.checked_add(1).unwrap_or(0),
+            _ => 0,


It shouldn't switch do another strategy.

It probably shouldn't switch from Every.

omni-viral · Oct 23, 2017

amethyst_assets/src/reload.rs

+            source,
+            options,
+            ..
+        } = this;


omni-viral · Oct 23, 2017

amethyst_assets/src/asset.rs

+    /// The reload structure has metadata which allows the asset management
+    /// to reload assets if necessary (for hot reloading).
+    /// You should only create this if `create_reload` is `true`.
+    /// Also, the parameter is just a request, which means you can also return `None`.
     fn import(
         &self,
         name: String,


name and options in Format::import and options in SimpleFormat.
And then in impl Reload for SingleFile you can reuse SingleFile value instead of creating new one

omni-viral · Oct 23, 2017

amethyst_assets/src/storage.rs

-            self.unused_handles.push(old);
+
+            // Can't reuse old handle here, because otherwise weak handles would still be valid.
+            // TODO: maybe just store u32?


Makes sense.

omni-viral · Oct 23, 2017

amethyst_assets/src/storage.rs

+    fn hot_reload(&mut self, pool: &ThreadPool) {
+        let elapsed = self.last_reload.elapsed().as_secs();
+
+        if elapsed >= 1 {


You've already checked that reload is required with strategy.

Oops, forgot to remove the old code.

Xaeroxe · Oct 23, 2017

Xaeroxe reviewed Oct 23, 2017

View changes

Looks pretty good, although I'd be overall much happier if we provided a hot reload strategy that uses file system events to trigger hot reloading. Would probably help a lot for games with lots of file assets.

Xaeroxe · Oct 23, 2017

amethyst_assets/src/asset.rs

+/// This is a simplified version of `Format`, which doesn't give you as much as freedom,
+/// but in return is simpler to implement.
+/// All `SimpleFormat` types automatically implement `Format`.
+/// This format assumes that the asset name is the full path and also the only file.


Minor grammar nit: Maybe this should end with "and the asset is only contained within one file."

Makes sense, thanks.

Xaeroxe · Oct 23, 2017

amethyst_assets/src/storage.rs

-    pub format: String,
-    pub handle: Handle<A>,
-    pub name: String,
+pub enum Processed<A: Asset> {


Why? It's very straightforward and obvious. You don't need to look at other files to figure out it's not actually pub, and pub(crate) permits easy re-use of internal structures.

Okay, all rebased, fixed, addressed and ready for a final review ;)

torkleyy · Oct 23, 2017

Okay, all rebased, fixed, addressed and ready for a final review ;)

I'd be overall much happier if we provided a hot reload strategy that uses file system events to trigger hot reloading.

Me too, but I don't want to implement everything in one PR.

torkleyy · Oct 23, 2017

I'd be overall much happier if we provided a hot reload strategy that uses file system events to trigger hot reloading.

Me too, but I don't want to implement everything in one PR.

Rhuagh · Oct 23, 2017

Rhuagh reviewed Oct 23, 2017

View changes

Rhuagh · Oct 23, 2017

amethyst_assets/src/asset.rs

+    }
+}
+
+/// This is a simplified version of `Format`, which doesn't give you as much as freedom,


"as much as freedom" should lose an "as".

Oh ;P It's funny to see how crappy the docs I wrote are.

Rhuagh · Oct 23, 2017

amethyst_assets/src/loader.rs

+    /// If set to `true`, this `Loader` will ask formats to
+    /// generate "reload instructions" which *allow* reloading.
+    /// Calling `set_hot_reload(true)` does not actually enable
+    /// hot reloading; this is controlled by the `HotReloadStrategy`


Ah, that's better :)

Rhuagh · Oct 23, 2017

amethyst_assets/src/reload.rs

+    }
+
+    /// No periodical hot-reloading is performed.
+    /// Instead, you can `trigger` it to check for changed assets


This documentation seems to conflict with the documentation on trigger below.

It does, and it's outdated.

Rhuagh · Oct 23, 2017

amethyst_core/src/lib.rs

-#[cfg(test)]
-#[cfg_attr(test, macro_use)]
-extern crate quickcheck;
+//#[cfg(test)]


Why is this commented out ?

There's not a single usage in the whole crate, I'm not sure what @Aceeri did here.

Ah, seems he removed all the tests using it. Remove it altogether then I'd say.

Xaeroxe · Oct 23, 2017

Xaeroxe approved these changes Oct 23, 2017

View changes

LGTM, thanks!

Rhuagh · Oct 23, 2017

Rhuagh requested changes Oct 23, 2017

View changes

Rhuagh · Oct 23, 2017

amethyst_assets/src/reload.rs

     Never,
 }
 
+/// System for updating `HotReloadStrategy`.
+/// **NOTE:** You have to add this after all asset processing systems.


The RenderSystem which process Mesh and Texture will mess this up.

omni-viral · Oct 24, 2017

omni-viral approved these changes Oct 24, 2017

View changes

Looks great!

Xaeroxe · Oct 24, 2017

Xaeroxe approved these changes Oct 24, 2017 • edited

View changes

Looks great! I'm glad the frame numbers were helpful.

Pro tip: There is no frame 0, as the number is incremented at the beginning of the frame rather than the end, so if you need a special value in the future 0 works just as well as MAX.

Assigning @Rhuagh to merge in case of an approval.

torkleyy · Oct 24, 2017

Assigning @Rhuagh to merge in case of an approval.

Rhuagh · Oct 24, 2017

Rhuagh approved these changes Oct 24, 2017

View changes

bors r+

Rhuagh · Oct 24, 2017

bors r+

Build succeeded

bors · Oct 24, 2017

Build succeeded

Yay! No more rebasing! :P

Xaeroxe · Oct 24, 2017

Yay! No more rebasing! :P

torkleyy added project: assets type: feature labels Oct 22, 2017

torkleyy added the status: ready label Oct 22, 2017

torkleyy requested review from Xaeroxe and Rhuagh Oct 22, 2017

torkleyy requested a review from Rhuagh Oct 23, 2017

Xaeroxe referenced this pull request Oct 23, 2017
Merged
Add frame_number to Time resource #446

torkleyy requested review from Rhuagh and omni-viral Oct 24, 2017

torkleyy assigned Rhuagh Oct 24, 2017

bors bot merged commit 51ae37f into amethyst:develop Oct 24, 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

torkleyy deleted the torkleyy:hot branch Oct 24, 2017

amethyst/amethyst

Join GitHub today

Add asset hot reloading #445

Conversation

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

Problems

torkleyy added project: assets type: feature labels Oct 22, 2017

This comment has been minimized.

torkleyy commented Oct 22, 2017

torkleyy added the status: ready label Oct 22, 2017

torkleyy requested review from Xaeroxe and Rhuagh Oct 22, 2017

Rhuagh reviewed Oct 22, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

omni-viral requested changes Oct 23, 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.

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 Oct 23, 2017 • edited Edited 1 time torkleyy edited Oct 23, 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.

omni-viral Oct 23, 2017 • edited Edited 1 time omni-viral edited Oct 23, 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.

omni-viral requested changes Oct 23, 2017 View changes

This comment has been minimized.

This comment has been minimized.

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

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

omni-viral Oct 23, 2017 • edited Edited 1 time omni-viral edited Oct 23, 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.

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

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jojolepro commented Oct 23, 2017

This comment has been minimized.

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

torkleyy requested a review from Rhuagh Oct 23, 2017

omni-viral requested changes Oct 23, 2017 View changes

This comment has been minimized.

torkleyy commented Oct 22, 2017 •

edited

Edited 1 time

torkleyy edited Oct 22, 2017 (most recent)

Rhuagh reviewed Oct 22, 2017

View changes

omni-viral requested changes Oct 23, 2017

View changes

torkleyy Oct 23, 2017 •

edited

Edited 1 time

torkleyy edited Oct 23, 2017 (most recent)

omni-viral Oct 23, 2017 •

edited

Edited 1 time

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

omni-viral requested changes Oct 23, 2017

View changes

torkleyy Oct 23, 2017 •

edited

Edited 1 time

torkleyy edited Oct 23, 2017 (most recent)

omni-viral Oct 23, 2017 •

edited

Edited 1 time

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

torkleyy Oct 23, 2017 •

edited

Edited 1 time

torkleyy edited Oct 23, 2017 (most recent)

torkleyy commented Oct 23, 2017 •

edited

Edited 1 time

torkleyy edited Oct 23, 2017 (most recent)

omni-viral requested changes Oct 23, 2017

View changes