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

Improve render asset #8149

Closed
wants to merge 6 commits into from
Closed

Conversation

vallrand
Copy link
Contributor

Objective

  • Remove duplicated implementation of asset extraction/preparation in material/material2d
  • Allow to have multiple GPU representations for the same "main world" asset.

Solution

Remove one to one correlation between "main world" asset and GPU asset, by swapping base type that implements RenderAsset to be the GPU asset type. That would make RenderAssetPlugin more flexible, being able to prepare different data based on same source asset (example: effect plugin could preprocess mesh buffer data for specialized rendering, without affecting main render flow).
That also makes RenderAssets<GPUType> more explicit.

Changelog

Changed

  • RenderAsset trait definition
  • Material and Material2d now use RenderAssetPlugin
  • RenderMaterials and RenderMaterials2d were replaced with more generic RenderAssets

Migration Guide

  • RenderMaterials<M> can now be accesed as RenderAssets<PreparedMaterial<M>>
  • RenderMaterials2d<M> can now be accesed as RenderAssets<PreparedMaterial2d<M>>
  • RenderAssets<Image> is now RenderAssets<GpuImage>
  • RenderAssets<Mesh> is now RenderAssets<GpuMesh>
  • RenderAsset trait should now be implemented on prev RenderAsset::PreparedAsset type, while source asset type is defined as RenderAsset::SourceAsset

Before

impl RenderAsset for Image {
    type ExtractedAsset = Image;
    type PreparedAsset = GpuImage;

    fn extract_asset(&self) -> Self::ExtractedAsset {
        self.clone()
    }
}

After

impl RenderAsset for GpuImage {
    type SourceAsset = Image;
    type ExtractedAsset = Image;

    fn extract_asset(source_asset: &Self::SourceAsset) -> Self::ExtractedAsset {
        source_asset.clone()
    }
}

@github-actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@JMS55
Copy link
Contributor

JMS55 commented Mar 21, 2023

Do you have a specific use case in mind for "Allow to have multiple GPU representations for the same "main world" asset"?.

I'm exploring ideas for reducing copies of render assets in memory by deleting them from RAM once they're on the GPU's VRAM.

@vallrand
Copy link
Contributor Author

@JMS55

That multiple representations was perhpaps a bit misleading.
From the perspective of decoupling generic "main world" types
from specific rendering implementation, as example if one would want to swap to renderer implementation where mesh assets are all kept in one large buffer, but keep using Handle<Mesh>.

One could always just duplicate the original asset, if it needs to be uploaded to the gpu twice.

Wrote some feedback if you'll find it useful 8132

@james7132 james7132 added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen labels Mar 22, 2023
@james7132 james7132 requested a review from superdump March 22, 2023 17:56
@james7132 james7132 added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Mar 22, 2023
@JMS55 JMS55 added this to the 0.14 milestone Feb 5, 2024
@JMS55
Copy link
Contributor

JMS55 commented Feb 5, 2024

After some further experimentation with meshlets and ray tracing, I'm in favor of something like this PR.

@alice-i-cecile alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Feb 22, 2024
@superdump
Copy link
Contributor

@vallrand - could you update this PR? I have long disliked the split between RenderAssets and RenderMaterials and wanted materials to be just another asset.

@superdump
Copy link
Contributor

I've adopted this in #12827 as it was a great idea, ahead of its time. :)

github-merge-queue bot pushed a commit that referenced this pull request Apr 9, 2024
# Objective

- Replace `RenderMaterials` / `RenderMaterials2d` / `RenderUiMaterials`
with `RenderAssets` to enable implementing changes to one thing,
`RenderAssets`, that applies to all use cases rather than duplicating
changes everywhere for multiple things that should be one thing.
- Adopts #8149 

## Solution

- Make RenderAsset generic over the destination type rather than the
source type as in #8149
- Use `RenderAssets<PreparedMaterial<M>>` etc for render materials

---

## Changelog

- Changed:
- The `RenderAsset` trait is now implemented on the destination type.
Its `SourceAsset` associated type refers to the type of the source
asset.
- `RenderMaterials`, `RenderMaterials2d`, and `RenderUiMaterials` have
been replaced by `RenderAssets<PreparedMaterial<M>>` and similar.

## Migration Guide

- `RenderAsset` is now implemented for the destination type rather that
the source asset type. The source asset type is now the `RenderAsset`
trait's `SourceAsset` associated type.
@JMS55 JMS55 closed this Apr 11, 2024
@JMS55 JMS55 removed this from the 0.14 milestone Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide M-Needs-Release-Note Work that should be called out in the blog due to impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants