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

Allow UiMaterial to provide a custom draw function #10816

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Davier
Copy link
Contributor

@Davier Davier commented Nov 30, 2023

Objective

The UiMaterial abstraction can currently be used to have custom shaders and one custom bind group. By allowing the material to select its own draw function, it could freely define all bind groups and vertex buffers, while reusing all the parts of the pipeline that it wants. (Edit: adding more bind groups will require more than this PR)

Solution

Add a method to the UiMaterial trait that returns the draw function's ID, with a default implementation.

Make the members of UiMaterialMeta public so that they can be reused.


Changelog

Allow UiMaterials to provide a custom draw function

@alice-i-cecile alice-i-cecile added C-Enhancement A new feature A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets labels Nov 30, 2023
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.

Pretty straightforward :) Should we make similar changes to other material code?

@Davier
Copy link
Contributor Author

Davier commented Nov 30, 2023

Should we make similar changes to other material code?

I honestly don't know, which is why I didn't do it. I would need to try it to see how it interacts with meshes, which parts of the pipelines need to be made public, and if it is usable in the end. I think they deserve their own PRs.

Also, while this PR is sufficient to add vertex buffers, I was too optimistic in the description. It seems that more will be needed to allow for additional bind groups. I'm not sure if this should block this PR or not, since it's already useful on its own.

@Davier
Copy link
Contributor Author

Davier commented Dec 1, 2023

I tried to gather my thoughts.

Vertex buffers

The Material and Material2d pipelines use the Mesh and Mesh2d pipelines, which make it possible to configure the mesh vertex buffer. The UiMaterial pipeline hard-codes its vertex buffer. Being able to add arbitrary vertex buffers makes sense for all of them, but it's much more critical for UiMaterials.

Bind groups

The 3d materials have ExtendedMaterial that can add bindings to the material bind group. It cannot add extra bind groups. I believe its main reason to exist is to make it easier to extend the PBR material (StandardMaterial). I don't think that we should add ExtendedMaterial2d and ExtendedUiMaterial for now, since ColorMaterial is so simple, and there are no implementations of UiMaterial (except the example).
Like for vertex buffers, being able to add arbitrary bind groups could be useful for all three traits. What this PR does is definitely a pre-requisite, but it falls short of enabling it. I'm searching for an ergonomic way to do it, but I'd prefer it to be its own PR since it will be less straightforward.

Other thoughts

Draw functions were always meant to be a building block, it's unfortunate that the materials currently hard-code them.

It would be great in the future to de-duplicate the three material traits (and I wouldn't mind doing the groundwork myself if there is consensus that we want it now).

TL;DR

I'm fine with the current PR 😄

@hymm hymm added this to the 0.14 milestone Feb 5, 2024
@IceSentry
Copy link
Contributor

@Davier Sorry, never saw that PR when it was made. Wouldn't it be possible to have a SpecializedUiPipeline? It would be more in-line with the other rendering pipelines inside bevy and I think it would give you what you need? It would definitely be way more code though and the current approach is kinda nice since it makes it really flexible.

@alice-i-cecile alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through 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 labels May 16, 2024
@alice-i-cecile alice-i-cecile removed this from the 0.14 milestone May 16, 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 A-UI Graphical user interfaces, styles, layouts, and widgets C-Enhancement A new feature 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

4 participants