Remove processed variants of fields from AssetSourceBuilder.

[andriyDev]

Objective

• Previously, asset sources were kind of annoying to create. A big reason for this is the fact that you need to define both the reader and the processed reader. Otherwise, your asset source would not be usable if AssetPlugin::mode was set to Processed.
  • In other words, asset sources were not very composable, since you may want to handle the processed and unprocessed assets differently.
• In addition, the whole asset processing system was "entangled" with the rest of the asset system - everywhere we needed to read assets we'd need to match on the AssetPlugin::mode, and pick the appropriate reader. In all these cases though, we just wanted to read the final assets.

Solution

• AssetSources now only include the final sources (the sources that your game will read).
• When registering sources, there are now two methods register_asset_source (for sources that won't be processed), or register_processed_asset_source (for sources that should be processed). Users need to pick which one they want.
  • register_processed_asset_source stores the asset source in a separate UnprocessedAssetSourceBuilders resource. Later in AssetPlugin we create the processed source for each unprocessed source.

This significantly decouples asset processing from the rest of the asset system. There are 2 main ways that they remain coupled now:

1. We can't build the asset sources until the AssetPlugin is added. This means we don't have things like the ProcessingState to gate the processed sources on. This means the AssetPlugin (which builds the sources) needs to know about processing to create the sources.
   • If we had runtime asset sources, we could initialize the ProcessingState in a ProcessingPlugin, and then immediately create the asset sources when calling register_processed_asset_source without needing to wait or deal with this in the AssetPlugin.
2. We don't have a good way to make the default asset source processed. Currently this is controlled by AssetPlugin::mode.
   • Solving this would also be nice for runtime asset sources.

Solving this coupling would also make it easier to turn asset processing on and off: currently, it's a feature on bevy_asset. This could instead be a plugin the is conditionally added in DefaultPlugins - meaning we have to recompile much less.

Testing

• Ran the asset_processing example. It works with and without the asset_processor enabled (i.e., in dev and in prod).

[ChristopherBiscardi]

Suggested change

	// We don't intend to write to the unprocessed sources, so we can avoid create the root
	// We don't intend to write to the unprocessed sources, so we can avoid creating the root

[ChristopherBiscardi]

Chonky, but looks good to me. Asset processing example still runs as expected too.

Getting to "third party implementable asset processing" would be pretty cool and enable some nice ecosystem experimentation.

[greeble-dev]

The overall direction sounds good to me, but I found some parts a bit confusing.

[greeble-dev]

I found this confusing. The function's called register_processed_asset_source, but it registers an un-processed source?

register_processed_asset_source_with_final_source below is also tricky - seems like "processed source" = the unprocessed_source parameter, and "final source" = processed_source parameter?

I think I'd prefer one function like this:

fn register_processing_asset_sources(
    &mut self,
    id: impl Into<AssetSourceId<'static>>,
    unprocessed_source: Option<AssetSourceBuilder>,
    processed_source: Option<AssetSourceBuilder>,
) -> &mut Self;

If a source is None then the default source is used.

[andriyDev]

I found this confusing. The function's called register_processed_asset_source, but it registers an un-processed source?

Yes this is definitely confusing. The way I want users to interpret this function is "register an asset source that should be processed", in which case providing the unprocessed source is correct. Maybe "unprocessed" is just a bad word and we should call it "pre-processed"? Even that is wrong. I've reworded the doc comment to say what I said above much more clearly (rereading the previous doc comment I realized it was entirely unhelpful lol).

I think I'd prefer one function like this:

I intentionally don't want this. In the future, we might entirely remove register_processed_asset_source_with_final_source if/when our dev-time, "in-progress" processed asset source starts differing from our "published" processed asset source. The intent here is to make it so users don't need to care about where their processed assets end up - they should just say "process this folder" and it's done. The same way that today they say mode = processed and their default asset source is now really imported_assets/Default.

Providing an extra argument here means it's something users should consider and they'll ask "why do I need to set this to None every time?"

[greeble-dev]

Might have to agree to disagree on this one. I do agree that "processed source" is a bit ambiguous (either "the source to be processed" or "the source that has been processed"). That's why I went for "processing source(s)` as it's less specific - so it's read as sources related to processing.

[greeble-dev]

The naming of this struct kept confusing me. In the non-processing case I think of the asset sources as "un-processed", but they don't go in UnprocessedAssetSourceBuilders.

What if it was like this?

struct ProcessingAssetSourceBuilders {
    processed: AssetSourceBuilders,
    unprocessed: AssetSourceBuilders,
}

So register_asset_source writes to the global AssetSourceBuilders as before. And register_processed_asset_source (and the final variant) only writes to ProcessingAssetSourceBuilders. I think that's clearer as the processing and non-processing cases are clearly separated.

[andriyDev]

I've renamed this to be AssetSourceBuildersToProcess! I've also made similar renamings like register_processed_asset_source now takes a source_to_process.

So register_asset_source writes to the global AssetSourceBuilders as before. And register_processed_asset_source (and the final variant) only writes to ProcessingAssetSourceBuilders.

We can't do this. We only AssetServer::load from the sources we built from AssetSourceBuilders. Splitting them up means we can't load the final processed assets at all!

The main idea behind this PR is that the only thing that matters for a user is what source ends up in the final AssetSourceBuilders - everything else is just a fancy way of creating that asset source. So register_processed_asset_source is just a fancy way of creating the final processed source. The fact that we read from the "source to process" and write to the final processed source is completely irrelevant, so we keep that stuff in a separate structure.

[greeble-dev]

We can't do this. We only AssetServer::load from the sources we built from AssetSourceBuilders. Splitting them up means we can't load the final processed assets at all!

I don't understand this - can't AssetPlugin::build put things in the right place depending on AssetMode? As far as I can tell the AssetSourceBuilders and AssetSourceBuildersToProcess resources are only used temporarily during initialisation. So during AssetPlugin::build all the source builders get cloned into AssetServer and optionally AssetProcesser. Nothing inside the asset system uses the resources after that, so they could be removed?

Unfortunately AssetSourceBuilders is public so this is an API change. But I'm not sure why it needs to be public? What's the use case for something outside the asset system using these builders?

[greeble-dev]

I found the occasional use of the "final sources" name confusing. Wouldn't processed_sources be more consistent?

[andriyDev]

final_sources includes all sources, both processed and unprocessed (not to be confused with "sources to process", which is not included). I settled on the name "final" since it's ambiguous, but hopefully makes it clear these are the sources that we actually load out of.

[greeble-dev]

I'm not clear what "final sources" adds though. At least within the asset processor, sticking to processed_sources and unprocessed_sources seems pretty clear. I realise that outside the asset processer there's the ambiguity with the non-processing case (i.e. the source from a plain register_asset_source).

If we're sticking with the current names (final_sources and sources_to_process) then I think there's some inconsistencies within the processor that should be reviewed, e.g. from AssetProcessor::new():

let unprocessed_sources = sources_to_process.build_as_sources_to_process();

[greeble-dev]

I'm clicking approve as I agree with the fundamental change of making an asset source either processed or unprocessed. I did find some naming and structural choices rather confusing (see earlier comments), but I'm not sure if that should be a blocker.

Personally I'd make these changes:

• Change AssetSourceBuilders to not be a resource.
  • Instead it's a regular struct shared between two other resources.
  • Also change to pub(crate) since I don't see a reason to expose it.
• New RegularAssetSourceBuilders resource to replace the single AssetSourceBuilders resource.
  • Contains one AssetSourceBuilders.
  • "Regular" is debatable, I just want to distinguish it from the processing builders.
• Change the AssetSourceBuildersToProcess resource to ProcessingAssetSourceBuilders.
  • Also change it from containing a single AssetSourceBuilders to having one for the unprocessed sources and one for the processed sources.
  • So RegularAssetSourceBuilders and ProcessingAssetSourceBuilders are mutually exclusive - regular is for AssetMode::Unprocessed, processing for AssetMode::Processed.
• Rename registration. Option 1:
  • register_processed_asset_source -> register_processing_asset_source
  • register_processed_asset_source_with_final_source -> register_processing_asset_sources
  • Not great since the it's subtly plural, but keeps the existing structure.
• Or option 2:
  • register_processed_asset_source -> register_unprocessed_asset_source
  • register_processed_asset_source_with_final_source -> register_processed_asset_source, and change it to only take a single source.
  • So instead of calling register_processed_asset_source_with_final_source, the user calls both register_unprocessed_asset_source and register_processed_asset_source.