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

Consolidate Render(Ui)Materials(2d) into RenderAssets #12827

Merged
merged 14 commits into from Apr 9, 2024

Conversation

superdump
Copy link
Contributor

@superdump superdump commented Apr 1, 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 Improve render asset #8149

Solution

  • Make RenderAsset generic over the destination type rather than the source type as in Improve render asset #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.

This will enable removal of RenderMaterials, and it allows implementing
creation of GPU types from any type.

Co-authored-by: Wilhelm Vallrand <>
@superdump superdump mentioned this pull request Apr 1, 2024
@superdump
Copy link
Contributor Author

superdump commented Apr 1, 2024

I would love to find a way to not require people to implement AssetUsages for custom materials, and instead just use a default. But I haven't had any good ideas yet. I can't implement for AssetUsages for T at least. And not every Asset should be a RenderAsset.

@NthTensor NthTensor added C-Enhancement A new feature A-Rendering Drawing game state to the screen A-Assets Load files from disk to use for things like images, models, and sounds C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide labels Apr 1, 2024
@superdump superdump force-pushed the render-asset-generic-over-target branch from 8f133a8 to 7abd50e Compare April 2, 2024 01:13
@infmagic2047
Copy link
Contributor

I would love to find a way to not require people to implement AssetUsages for custom materials, and instead just use a default. But I haven't had any good ideas yet. I can't implement for AssetUsages for T at least. And not every Asset should be a RenderAsset.

My ideas: Keep the asset_usage() method on RenderAsset instead of using a separate trait (it should take &Self::SourceAsset param now). Add methods to Material traits that allow overriding the usage, and use them in the RenderAsset implementation of PreparedMaterial. The required migration would be minimal in this way.

@superdump
Copy link
Contributor Author

@infmagic2047 great idea! So simple! :) I love it.

@IceSentry IceSentry self-requested a review April 5, 2024 21:20
Copy link
Contributor

@pcwalton pcwalton left a comment

Choose a reason for hiding this comment

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

This looks fine. I'll trust you that it was necessary to move RenderAsset over to be implemented on the render world type.

@IceSentry
Copy link
Contributor

@pcwalton here's the justification from the original PR:

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 more explicit.

@IceSentry IceSentry added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Apr 6, 2024
@superdump
Copy link
Contributor Author

Please don’t merge this yet. I want to think about a couple of things with it so I am convinced implementing RenderAsset on the destination type is better.

@superdump superdump removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Apr 7, 2024
@superdump superdump added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Apr 9, 2024
@superdump superdump requested a review from robtfm April 9, 2024 13:18
@superdump superdump enabled auto-merge April 9, 2024 13:24
@superdump superdump added this pull request to the merge queue Apr 9, 2024
@superdump
Copy link
Contributor Author

I turned out that robtfm had a branch that did basically the same thing last year after I commented that implementing RenderAsset on types that implement Materials was problematic in various ways:

Merged via the queue into bevyengine:main with commit ab7cbfa Apr 9, 2024
28 checks passed
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 A-Rendering Drawing game state to the screen C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Enhancement A new feature S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants