Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for loading sub-assets as if they were separate files. #6504

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yodaldevoid
Copy link

Objective

The current AssetServer system does not support loading assets from inside of container assets that have no way of knowing what they might contain such as zip files or Doom-style WAD files. This aims to add support for this style of asset storage.

Fixes #3319

Solution

This implementation adds a way to defer loading internal assets (a.k.a. sub-assets) from a container asset's (a.k.a. super-asset) AssetLoader. These deferred loads are then loaded using the existing Asset loading system once the container asset's AssetLoader returns.

This system required changes to the existing AssetPath to allow the system to determine both the sub-asset's path for Asset IDs and to determine the right AssetLoader and the super-asset path to determine the file on the filesystem backing the sub-asset.


Changelog

Added

  • Added sub-asset support to AssetPath
  • Added ability to register sub-assets from an AssetLoader

@alice-i-cecile alice-i-cecile added C-Enhancement A new feature A-Assets Load files from disk to use for things like images, models, and sounds X-Controversial There is active debate or serious implications around merging this PR labels Nov 7, 2022
@yodaldevoid yodaldevoid marked this pull request as ready for review November 7, 2022 03:05
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nicely done for a first PR! This is quite complex, but you've kept the scope tight. I'll defer to @cart on the ultimate direction here (he's been chewing on assets for a while), but the overall code quality and design seem excellent.

@yodaldevoid yodaldevoid force-pushed the sub_assets_main branch 3 times, most recently from 5aa9322 to b136f5f Compare November 9, 2022 23:38
@yodaldevoid
Copy link
Author

yodaldevoid commented Nov 10, 2022

The one remaining CI failure is not due to my changes.

In fact it seems to be due to a slow rollout of releases by the smol team.

Edit: See #6538

@yodaldevoid
Copy link
Author

More dependency errors from unrelated dependencies.

@cart
Copy link
Member

cart commented Dec 16, 2022

Given that the asset system is in flux right now / will almost certainly be replaced soon, I think we should put a pause on this until we have more certainty about the direction of the asset system. I'm currently working on an experimental rewrite and I'll try to consider this use case as a part of that. Lets leave this PR open to ensure this scenario is captured somewhere and we can revisit in a few weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Enhancement A new feature X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

amethyst_assets::Source Equivalent
3 participants