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

Swap material and mesh bind groups #10485

Merged
merged 4 commits into from
Nov 28, 2023
Merged

Conversation

JMS55
Copy link
Contributor

@JMS55 JMS55 commented Nov 9, 2023

Objective


Changelog

  • For 2d and 3d mesh/material setups (but not UI materials, or other rendering setups such as gizmos, sprites, or text), mesh data is now in bind group 1, and material data is now in bind group 2, which is swapped from how they were before.

Migration Guide

  • Custom 2d and 3d mesh/material shaders should now use bind group 2 @group(2) @binding(x) for their bound resources, instead of bind group 1.
  • Many internal pieces of rendering code have changed so that mesh data is now in bind group 1, and material data is now in bind group 2. Semi-custom rendering setups (that don't use the Material or Material2d APIs) should adapt to these changes.

@JMS55 JMS55 added A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Nov 9, 2023
@JMS55 JMS55 added this to the 0.13 milestone Nov 9, 2023
@JMS55 JMS55 requested review from nicopap, superdump and robtfm and removed request for nicopap, superdump and robtfm November 9, 2023 23:48
@JMS55
Copy link
Contributor Author

JMS55 commented Nov 9, 2023

As a followup PR, we'll probably want to sort draws by pipeline id.

@cart
Copy link
Member

cart commented Nov 10, 2023

Hmm pretty sure someone flipped these for some reason awhile back (with some reasonable justification for going against the "mesh before material" bind frequency advice) . Iirc it was some sort of technical details thing (ex: architectural limitation, platform support), not a performance thing. Worth trying to find that context.

@cart
Copy link
Member

cart commented Nov 10, 2023

Because yeah this is the right approach. And I'm pretty sure I did it this way in the past.

@robtfm
Copy link
Contributor

robtfm commented Nov 10, 2023

cart from the past says:

Pretty sure for performance reasons we need to sort by bind frequency / assume that "later" bind groups will be rebound if earlier ones change. Given that we should be sorting by material and then drawing many meshes with the same material, I think the current order is correct

the current order used to be (potentially) better for performance before we batched meshes. now it makes more sense to put mesh first as the batch of meshes won't need frequent rebinding.

@JMS55
Copy link
Contributor Author

JMS55 commented Nov 10, 2023

Right so after moving mesh data (not mesh vertex data, just the MeshUniforms) to GpuArrayBuffer, the bind group should almost never need to be rebound (for normal meshes).

What does need rebinding is:

  • When the type of mesh data changes (regular model only, skinned mesh, mesh with morph targets)
  • I think an extra dynamic index for each skinned/morph target mesh, but I'm not 100% sure having never really touched that code

(Note: We probably want to sort draws so regular/skinned/morph meshes are grouped together, after material sorting)

So the only way this PR loses out currently is for e.g. skinned meshes, where you may have two different skinned meshes with the same material. This PR would cause you to have to rebind the material for each unique skinned mesh, which is expensive. But this PR is better for regular meshes...

@JMS55
Copy link
Contributor Author

JMS55 commented Nov 10, 2023

For CPU-driven rendering, I guess maybe it makes more sense to keep this as-is to avoid expensive material (pipeline) rebinds. For GPU-driven, where the pipeline is the only rebind, I can just put materials in bind group 2 anyways. I'm thinking we should close this.

@cart
Copy link
Member

cart commented Nov 10, 2023

Cool yeah that all makes sense!

@JMS55 JMS55 closed this Nov 10, 2023
@superdump
Copy link
Contributor

@JMS55 I'm not so sure. I think this is useful for CPU-driven as well. Considering the two bind groups:

  • The mesh bind group has to be bound once for all non-skinned/non-morphed mesh entities, and rebound for each individual skinned / morphed mesh entity currently.
  • The material bind group has to be rebound for every single material instance (as in, each material asset regardless of its type) currently.

The usual case for mesh assets is lots of non-skinned + non-morphed mesh entities and few skinned / morphed mesh entities. So prioritising the defaults for non-skinned / non-morphed entities makes sense. And those require one binding as all the data they need is in one buffer.

The usual case for material assets with the current solution is a bit less clear:

  • One material asset per mesh entity - common for non-instanced models with textures and no texture atlas
  • One material asset per many mesh entities - instanced models, or simpler usage with flat colour materials. I don't really feel like we should optimise for the latter as it's simple enough anyway.

Either way, rebinding for a material asset is at best once per group of mesh entities, and at worst once per mesh entity. The only situation I can think of where it would be 'worse' would be for entities using different skinned/morphed mesh assets which have the same material. Unless I'm missing something, that seems very niche and unlikely?

I think at this point it does make sense to swap the material and mesh bind groups. I'd suggest reopening this and merging it, as well as in a separate PR, changing the sorting of the opaque and alpha mask passes. If we do swap the binding order, then we would want to sort by pipeline id, mesh bind group id + dynamic offset, material bind group id, z front to back. Though we'd want to test the impact on sorting performance for non-trivial configurations to see whether we could cut down on those sort key fields.

@JMS55 JMS55 reopened this Nov 11, 2023
@JMS55
Copy link
Contributor Author

JMS55 commented Nov 11, 2023

Would it be possible to move the skinning and morph data into storage buffers? More perf, and we'd avoid this whole issue to begin with.

Copy link
Contributor

@robtfm robtfm left a comment

Choose a reason for hiding this comment

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

lgtm.

one lingering "group 1" in a comment in bind_group.rs:

/// ... in Bevy material bind groups
/// are generally bound to group 1.

i haven't checked performance - i don't expect any regressions but maybe we ought to benchmark?

@JMS55
Copy link
Contributor Author

JMS55 commented Nov 20, 2023

i haven't checked performance - i don't expect any regressions but maybe we ought to benchmark?

Shouldn't be any regressions atm I don't think, as we don't sort primarily by pipeline/bind group yet. It'll limit the max perf we can have for skinned meshes/morph targets in the future though (as they use a separate vertex buffer per mesh), but that's a tradeoff we'll have to accept, unless we want to add a toggle to the shader code to switch the group orderings (which we could also easily do).

EDIT: I personally can't benchmark anything well for the next 2 weeks, as I only have access to an old laptop atm.

@superdump
Copy link
Contributor

Would it be possible to move the skinning and morph data into storage buffers? More perf, and we'd avoid this whole issue to begin with.

I have a branch locally that does this for skinning. Morph uses textures so it's a different beast. And we still can't avoid it because the current approach is the best we can do for uniform buffers and not everyone has access to storage buffers.

@JMS55 JMS55 requested a review from IceSentry November 28, 2023 16:04
@@ -2,20 +2,8 @@

#import bevy_pbr::mesh_types::Mesh

#ifdef MESH_BINDGROUP_1
Copy link
Contributor

Choose a reason for hiding this comment

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

🚀

@superdump superdump added this pull request to the merge queue Nov 28, 2023
Merged via the queue into bevyengine:main with commit 4bf20e7 Nov 28, 2023
22 checks passed
james7132 pushed a commit to james7132/bevy that referenced this pull request Dec 1, 2023
# Objective
- Materials should be a more frequent rebind then meshes (due to being
able to use a single vertex buffer, such as in bevyengine#10164) and therefore
should be in a higher bind group.

---

## Changelog
- For 2d and 3d mesh/material setups (but not UI materials, or other
rendering setups such as gizmos, sprites, or text), mesh data is now in
bind group 1, and material data is now in bind group 2, which is swapped
from how they were before.

## Migration Guide
- Custom 2d and 3d mesh/material shaders should now use bind group 2
`@group(2) @binding(x)` for their bound resources, instead of bind group
1.
- Many internal pieces of rendering code have changed so that mesh data
is now in bind group 1, and material data is now in bind group 2.
Semi-custom rendering setups (that don't use the Material or Material2d
APIs) should adapt to these changes.
github-merge-queue bot pushed a commit that referenced this pull request Dec 2, 2023
# Objective

Fix #10840 

## Solution

The material's binding group number was changed in #10485
alphastrata added a commit to alphastrata/shadplay that referenced this pull request Feb 25, 2024
alphastrata added a commit to alphastrata/shadplay that referenced this pull request Feb 28, 2024
* wip: took more than the 30 mins I have available -- will have to finish this later

* wip: keycodes, digits... misc renaming

* wip: 2d working -- panic on 3d checking that out now...

* chore: they've all gotta be on grout 2 bevyengine/bevy#10485

* chore: bulk replace all group(1)s to group(2)s for bevy 0.13..

* fix: make the might example work

* chore: make the cube much, much bigger!
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-Performance A change motivated by improving speed, memory usage or compile times M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants