Skip to content
This repository has been archived by the owner on Apr 18, 2022. It is now read-only.

Asset hot reloading #266

Closed
torkleyy opened this issue Jul 28, 2017 · 38 comments
Closed

Asset hot reloading #266

torkleyy opened this issue Jul 28, 2017 · 38 comments
Assignees
Labels
pri: normal An issue that is causing a noticeable impact, but can be worked around team: assets type: feature A request for a new feature.
Projects

Comments

@torkleyy
Copy link
Member

I've figured most parts out already, however I'm stuck at the last one. After a file has changed, it's been read, parsed and turned into an asset, how should we replace the old assets?

Most likely assets live as components in the world, but still, how should we track them? Or is limiting it to the World a good idea at all?

I'm not sure whether or not we need this feature before 0.5 release, probably that needs discussion, too.

@torkleyy torkleyy added diff: normal Achievable by an reasonable experienced developer. If new to Amethyst, may need some guidance. pri: normal An issue that is causing a noticeable impact, but can be worked around type: RFC labels Jul 28, 2017
@torkleyy torkleyy self-assigned this Jul 28, 2017
@ebkalderon ebkalderon added this to New in Engine Jul 28, 2017
@Xaeroxe
Copy link
Member

Xaeroxe commented Jul 28, 2017

In order to prevent memory corruption issues we'll have to pause execution while the replacement occurs. My first reaction is to put assets in an Arc<RwLock> so we can track them and still hand out references to them. We could keep some kind of Boolean "dirty" bit (Per application, not per asset) representing that assets are ready to be replaced, and check that every frame. If they are ready to be replaced then pop the new assets from a queue and replace each one until the queue is empty.

EDIT: This also gives us an easy answer to "what happens if an asset is unloaded that's still attached to an entity?" Well the asset won't unload until the entity in question is removed from the game. In my opinion that's the best answer we could give to that question as it means the code won't spontaneously have assets disappear until it is done with them.

@zakarumych
Copy link
Member

zakarumych commented Jul 28, 2017

Global pauses are unnecessary. There are data structures which permit update of value while it is read.
We should use one that works for one writer and many readers.

@Xaeroxe
Copy link
Member

Xaeroxe commented Jul 28, 2017

@omni-viral I don't know of any such data structures. Can you provide one for me to read about?

@zakarumych
Copy link
Member

zakarumych commented Jul 28, 2017

@Xaeroxe I'll find link to the paper to post it here.
Our case has quiescent points so we can search in that direction.

@zakarumych
Copy link
Member

Maybe there is no need of fancy data structures either if we can update assets between systems run

@torkleyy
Copy link
Member Author

torkleyy commented Jul 28, 2017 via email

@Xaeroxe
Copy link
Member

Xaeroxe commented Jul 28, 2017

We would need some kind of central AssetManager structure that keeps a HashMap<String, Arc<RwLock<T>>> so it can be referred to when the time to replace the asset comes.

@Xaeroxe
Copy link
Member

Xaeroxe commented Jul 28, 2017

(Said structure would also require a way to remove references so that the memory the assets consume can be freed)

@zakarumych
Copy link
Member

Put all updates in a queue. Replace the queue atomically in maintenance phase. Flush updates into the world.

@zakarumych
Copy link
Member

@Xaeroxe no locks please)

@Xaeroxe
Copy link
Member

Xaeroxe commented Jul 28, 2017

Then the update will have to occur in unsafe code, which is fine but we'll have to be much more careful about memory corruption.

@zakarumych
Copy link
Member

Well. We should wrap unsafe code into nice and safe type and test it heavily

@torkleyy
Copy link
Member Author

torkleyy commented Jul 28, 2017 via email

@Xaeroxe
Copy link
Member

Xaeroxe commented Jul 28, 2017

Ok here's a nice approach that gives us minimal pausing: Arc<Cell<Box<T>>>

Rather than locking the value while it is replaced you just atomically call "set" on the cell and replace the Box value. No unsafe code, no locking.

@Xaeroxe
Copy link
Member

Xaeroxe commented Jul 28, 2017

Except that doesn't work because Box doesn't implement Copy. Yeah we'll have to use unsafe code, maybe TrustCell?

@torkleyy
Copy link
Member Author

torkleyy commented Jul 28, 2017 via email

@zakarumych
Copy link
Member

zakarumych commented Jul 28, 2017

@torkleyy I can't find any docs on TrustCell
upd: Found it in source

@Xaeroxe
Copy link
Member

Xaeroxe commented Jul 28, 2017

@omni-viral It's not part of the official API, look here

@zakarumych
Copy link
Member

zakarumych commented Jul 28, 2017

@torkleyy TrustCell would work find as asset holder inside World for async read acces

@zakarumych
Copy link
Member

One more question. How many asset producers will be there? Single or multiple?

@Xaeroxe
Copy link
Member

Xaeroxe commented Jul 28, 2017

@omni-viral Current code permits multiple, which is nice for asynchronous loading from multiple systems but we may require some kind of unification to implement hot reloading.

@zakarumych
Copy link
Member

zakarumych commented Jul 28, 2017

I'm suggesting following algorithm.
Have Arc<Mutex<Vec<AssetUpdate>>> inside Engine
Store all updates in this queue. Blocking of updating threads is OK I think.
At maintenance phase (between systems run) swap queue with empty one. And flush all updates into TrustCells

AssetUpdate = Box<FnOnce(&mut World) + Send> is a closure with new asset and id to find TrustCell in world

@zakarumych
Copy link
Member

Mutex in Arc<Mutex<Vec<AssetUpdate>>> could be replaced with something that will allow not to block main thread.

@Xaeroxe
Copy link
Member

Xaeroxe commented Jul 28, 2017

I disagree, that's not performance critical code and in my opinion it doesn't warrant the risk of unsafety to replace Mutex in that type with a non-locking unsafe variant.

@Xaeroxe
Copy link
Member

Xaeroxe commented Jul 28, 2017

Alternatively if blocking is really that undesirable, we could use a channel to send the AssetUpdates

@zakarumych
Copy link
Member

@Xaeroxe mpsc channel is the data structure with internal unsafe you were opposed to one comment earlier 😄

@Xaeroxe
Copy link
Member

Xaeroxe commented Jul 28, 2017

Yeah I just thought you meant some kind of TrustCell for the queue, which seemed excessive to me. Channel works much better though :)

@torkleyy
Copy link
Member Author

torkleyy commented Jul 28, 2017 via email

@Xaeroxe
Copy link
Member

Xaeroxe commented Jul 28, 2017

That is a good point. In order to make the feature work we need some kind of tracking and continuous updating though. This seems to be the easiest way to accomplish that. Perhaps we could use #[cfg(build = "debug")] to prevent hot reloading code from being in the final game.

@zakarumych
Copy link
Member

#[cfg(build = "debug")] isn't best here. You probably want to run your game with release build during development cause it may run too slow in debug.
Instead there should be distinct feature.

@torkleyy
Copy link
Member Author

torkleyy commented Jul 28, 2017 via email

@Rhuagh
Copy link
Member

Rhuagh commented Jul 29, 2017

If hot reloading is a pure development feature, some risk might be worth it, for a simpler solution?

@Xaeroxe
Copy link
Member

Xaeroxe commented Jul 29, 2017

Ok, what are you proposing? Mpsc channel seems to be a best of all worlds approach. It's a non-locking std library type with internal unsafe and no need for us to write unsafe sections of code.

@Rhuagh
Copy link
Member

Rhuagh commented Jul 29, 2017

Oh, I don't know what would be best, my suggestion is just to do the simplest solution that fits the bill if it's only gonna be used for dev, and not for released product :)

@Xaeroxe Xaeroxe added team: assets type: feature A request for a new feature. and removed diff: normal Achievable by an reasonable experienced developer. If new to Amethyst, may need some guidance. team: rendering type: RFC labels Aug 7, 2017
@torkleyy
Copy link
Member Author

So essentially that problem we discussed about (updating the asset) disappeared. This was part of my second assets rework, which introduced asset storages and handles, allowing us to control reads and writes in a central place.

bors bot added a commit that referenced this issue Oct 24, 2017
445: Add asset hot reloading r=Rhuagh a=torkleyy

* Adds a new resource to the world, `Arc<rayon::ThreadPool>` (or short: `core::ThreadPool`)
* Adds `WeakHandle`s
* 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
@bors bors bot closed this as completed in #445 Oct 24, 2017
@erlend-sh
Copy link
Member

Fair amount of discussion about this in the Rust community recently thanks to @phaazon's warmy.

Seems @icefoxen is already using it successfully in ggez.

@torkleyy
Copy link
Member Author

@erlend-sh I mostly like what we have currently, it fits nicely into ECS and works in parallel. warmy has dependencies, which is something that's still missing.

But at a first glance we couldn't even use warmy because it's not thread-safe.

@hadronized
Copy link

warmy is not thread-safe yet. :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pri: normal An issue that is causing a noticeable impact, but can be worked around team: assets type: feature A request for a new feature.
Projects
No open projects
Engine
  
New
Development

No branches or pull requests

6 participants