bevy icon indicating copy to clipboard operation
bevy copied to clipboard

RenderAssetPersistencePolicy → RenderAssetUsage

Open brianreavis opened this issue 2 years ago • 4 comments

Objective

Right now, all assets in the main world get extracted and prepared in the render world (if the asset's using the RenderAssetPlugin). This is unfortunate for two cases:

  1. TextureAtlas / FontAtlas: This one's huge. The individual Image assets that make up the atlas are cloned and prepared individually when there's no reason for them to be. The atlas textures are built on the CPU in the main world. There can be hundreds of images that get prepared for rendering only not to be used.
  2. If one loads an Image and needs to transform it in a system before rendering it, kind of like the decompression example, there's a price paid for extracting & preparing the asset that's not intended to be rendered yet.

  • References #10520
  • References #1782

Solution

This changes the RenderAssetPersistencePolicy enum to bitflags. I felt that the objective with the parameter is so similar in nature to wgpu's TextureUsages and BufferUsages, that it may as well be just like that.

// This asset only needs to be in the main world. Don't extract and prepare it.
RenderAssetUsages::MAIN_WORLD

// Keep this asset in the main world and  
RenderAssetUsages::MAIN_WORLD | RenderAssetUsages::RENDER_WORLD

// This asset is only needed in the render world. Remove it from the asset server once extracted.
RenderAssetUsages::RENDER_WORLD

Alternate Solution

I considered introducing a third field to RenderAssetPersistencePolicy enum:

enum RenderAssetPersistencePolicy {
    /// Keep the asset in the main world after extracting to the render world.
    Keep,
    /// Remove the asset from the main world after extracting to the render world.
    Unload,
    /// This doesn't need to be in the render world at all.
    NoExtract, // <-----
}

Functional, but this seemed like shoehorning. Another option is renaming the enum to something like:

enum RenderAssetExtractionPolicy {
    /// Extract the asset and keep it in the main world.
    Extract,
    /// Remove the asset from the main world after extracting to the render world.
    ExtractAndUnload,
    /// This doesn't need to be in the render world at all.
    NoExtract,
}

I think this last one could be a good option if the bitflags are too clunky.

Migration Guide

  • RenderAssetPersistencePolicy::KeepRenderAssetUsage::MAIN_WORLD | RenderAssetUsage::RENDER_WORLD (or RenderAssetUsage::default())
  • RenderAssetPersistencePolicy::UnloadRenderAssetUsage::RENDER_WORLD
  • For types implementing the RenderAsset trait, change fn persistence_policy(&self) -> RenderAssetPersistencePolicy to fn asset_usage(&self) -> RenderAssetUsages.
  • Change any references to cpu_persistent_access (RenderAssetPersistencePolicy) to asset_usage (RenderAssetUsage). This applies to Image, Mesh, and a few other types.

brianreavis avatar Jan 18 '24 07:01 brianreavis

I'm not sure how I feel about changing the name to Main/Render worlds. You could have the asset data only on the CPU, and still want to access it in the render world for instance, so it's not entirely accurate. It's not a bad approximation though.

@JMS55 We could add a third bitfield GPU that controls whether or not the asset gets processed in prepare_assets so that there is that ultimate control. What do you think?

brianreavis avatar Jan 18 '24 17:01 brianreavis

That sounds a lot more complicated. I think for that case, people shouldn't be using RenderAsset at all.

I'm fine with the current two options, just I feel like there's maybe a better name for them. They're not too bad as-is though.

JMS55 avatar Jan 19 '24 02:01 JMS55

I don't think we need to land this in 0.13 given that unloading render assets was disabled by default, but it's a large breaking change so better we don't push it to 0.14 and break things twice.

JMS55 avatar Jan 24 '24 18:01 JMS55

Agreed. There's also the added performance perk for atlases. I just submitted the change from usage to asset_usage, so I think this is good to go, barring any additional feedback.

brianreavis avatar Jan 24 '24 19:01 brianreavis

Once merge conflicts are resolved ping me and I'll merge this.

alice-i-cecile avatar Jan 29 '24 16:01 alice-i-cecile

@alice-i-cecile All set

brianreavis avatar Jan 30 '24 06:01 brianreavis