MovingBlocks / gestalt

A family of libraries providing a variety of core capabilities for use by games and game engines.
Apache License 2.0
27 stars 23 forks source link

chore: update asset scheme #109

Open pollend opened 3 years ago

pollend commented 3 years ago

this changes how assets work with the assetmanager and attempts to address some problems.

  1. for transient assets the current scheme will slowly leak weak pointers since they are not properly removed from the multimap.
  2. there is now just a single map from a resource urn to the start of a chain of assets. (non-transient assets use soft-reference and transient assets use a weakreference but weak references use a strong reference so the non-transient asset so it's not disposed unless all transient assets have been disposed in the chain.
    • the non-transient asset is always placed first and all transient assets use a weak reference
    • each asset is chained from the beginning forward as new assets are added they are placed at the beginning of the chain unless if its a non-transient asset then it skips the first entry

image

pollend commented 3 years ago

Overall, this request needs work. I'm concerned firstly with the scope of the changes. I think it is a bit overdone - in particular I do not believe a node based linked list system needed to be developed, as simpler tweaks around the existing implementation would have enabled the improvements. Also I'm concerned some changes introduce bugs or break encapsulation. Also some style issues.

I do agree with:

* Changing normal assets to be kept via soft references

* Cleaning up null weak references from the instance asset map

* Adding tracking of parent urn to the asset references to enable the above 

One of the benefits with this scheme is it decouples a lot of the book keeping from AssetType. Assets can clone and insert a reference into the chain without depending on or notifying assetType of the change. at most you would just need to introduce a way to lock the urn which isn't too hard to add to the current code. an AssetType would just manage the bookkeeping and clean up references as they go stale. The implementation could be cleaned up quite a bit since there is a large amount of book keeping with the code I added. The PR is pretty rough since the implementation is quite a bit more complicated.

pollend commented 3 years ago

@immortius so I think this is a lot simpler then the original system. I decoupled the ownership of instanced assets from assetTypes. so assetTypes now only track non instanced assets. assets that create instances will now push it to a linked list. reloading just follows the chain of instanced assets and since each instance also has a strong link back to the parent asset then that resource is only held on as long as instanced assets are still valid. if a parent becomes invalid then all instanced assets should become invalid as well :D.

pollend commented 3 years ago

Update Asset Scheme for gestalt