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

Assetv2: Unprocessed asset meta defaults. #13785

Closed
wants to merge 17 commits into from

Conversation

brandon-reinhart
Copy link
Contributor

@brandon-reinhart brandon-reinhart commented Jun 9, 2024

Objective

When processing assets, a meta file is produced per asset. By default, these meta files are redundant. If many assets are intended to be processed the same way, only one meta file should be needed per asset type.

This PR enables the creation of an "extension.defaults" file that will govern the processing behavior of all assets that do not have their own meta file.

An example where f.cool.ron will use cool.ron.defaults for processing because it has no meta file of its own.

-a---            6/9/2024  3:39 PM            174 a.cool.ron
-a---            6/9/2024  3:39 PM            433 a.cool.ron.meta
-a---            6/9/2024  3:39 PM            280 d.cool.ron
-a---            6/9/2024  4:51 PM            432 d.cool.ron.meta
-a---            6/9/2024  4:40 PM            114 f.cool.ron
-a---            6/9/2024  5:55 PM            464 cool.ron.defaults

Solution

  • Provide the ability to define an "extension.defaults" file which is used as a fallback if an unprocessed asset lacks meta.
  • Re-process assets if the meta defaults change (add/remove/modify) while a file watcher is active.

Questions

  • Do we want processed asset folders to have support for defaults also?

Testing

  • The asset_processing example has been modified to show fallback folder level meta being used by the f.cool.ron asset. This asset has no meta and so the folder's default cool.ron.defaults file is are used instead. This default processes the text in f.cool.ron to append "-modified_by_default_meta" to the contained string during asset processing.
  • Hot reloading can be tested by monkeying with the defaults (add / remove / modify) while running the example.

Changelog

  • Added support for .defaults files in the unprocessed asset folder. These files specify default metadata to be used per asset type during processing. For example, the asset alice.png will use png.defaults if alice.png.meta does not exist. Asset processing defaults support nesting, see the "asset_processing" example.

Migration Guide

  • ...

@brandon-reinhart brandon-reinhart changed the title Assetv2: Default meta defined at the folder level. Assetv2: Unprocessed asset meta defaults. Jun 9, 2024
@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 C-Needs-Release-Note Work that should be called out in the blog due to impact labels Jun 9, 2024
@tbillington
Copy link
Contributor

I think it could get noisy having a settings file per extension. Would a map in a single settings file make more sense?

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.

Okay, I've taken a look. Opinions:

  1. I really like the ability to set defaults for these files: I think it's really essential to make working with larger numbers of assets actually ergonomic and usable for teams.
  2. I would prefer to build towards a model of specific-overrides-general nested meta files from the beginning, with a recursive upwards search through the folder structure, stopping when a matching meta-file is hit. This is generally very powerful, and should let us reduce the special-casing for per-file meta files.
  3. I'm fine with extension-specific meta-files for this PR. Being able to bundle multiples types into a dictionary is an interesting direction, but orthogonal to this work.
  4. Obviously, this code needs more work, docs etc. Bother me when you want a nitpicking pass.

@alice-i-cecile alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels Jun 10, 2024
@brandon-reinhart
Copy link
Contributor Author

brandon-reinhart commented Jun 10, 2024

I think it could get noisy having a settings file per extension. Would a map in a single settings file make more sense?

I don't think so.

  1. When we search up the parent hierarchy, we would have to search in all defaults files for any matching defaults block.

  2. It seems easier to just delete/add/remove the single file related to a particular type than have to search through existing files to find the one you want and remove the correct block.

  3. We'd need to add support for a new type, which is the map of types to meta.

I did consider the map in a single file and actually started with that locally, but I think it is a lot more error prone. I'm thinking in terms of artist workflows for the most part, where dropping a "png.defaults" type file in a parent directory is pretty easy to figure out.

- Change meta extension name to "defaults"
- Define consts for meta and defaults extensions.
- Properly support compound extensions. Fixes looking for "ron.defaults" instead of "cool.ron.defaults"
- Update example to test parent directory defaults.
…/modify) anywhere in the assetsource filesystem tree.
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.

Great :) I like this design, the code quality is good, and there's new documentation addressing this. LGTM once CI is green and the typo I pointed out is fixed.

@brandon-reinhart
Copy link
Contributor Author

brandon-reinhart commented Jun 10, 2024

Great :) I like this design, the code quality is good, and there's new documentation addressing this. LGTM once CI is green and the typo I pointed out is fixed.

I need to investigate the other types of AssetReader, which don't yet support this feature.

@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jun 10, 2024
@JMS55 JMS55 added this to the 0.15 milestone Jun 28, 2024
@dead-money dead-money closed this by deleting the head repository Jul 5, 2024
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 C-Needs-Release-Note Work that should be called out in the blog due to impact D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants