Skip to content

Make StandardMaterial bindless.#16644

Merged
alice-i-cecile merged 7 commits intobevyengine:mainfrom
pcwalton:bindless-standard-material-2
Dec 10, 2024
Merged

Make StandardMaterial bindless.#16644
alice-i-cecile merged 7 commits intobevyengine:mainfrom
pcwalton:bindless-standard-material-2

Conversation

@pcwalton
Copy link
Contributor

@pcwalton pcwalton commented Dec 4, 2024

This commit makes StandardMaterial use bindless textures, as implemented in PR #16368. Non-bindless mode, as used for example in Metal and WebGL 2, remains fully supported via a plethora of #ifdef BINDLESS preprocessor definitions.

Unfortunately, this PR introduces quite a bit of unsightliness into the PBR shaders. This is a result of the fact that WGSL supports neither passing binding arrays to functions nor passing individual elements of binding arrays to functions, except directly to texture sample functions. Thus we're unable to use the sample_texture abstraction that helped abstract over the meshlet and non-meshlet paths. I don't think there's anything we can do to help this other than to suggest improvements to upstream Naga.

This commit makes `StandardMaterial` use bindless textures, as
implemented in PR bevyengine#16368. Non-bindless mode, as used for example in
Metal and WebGL 2, remains fully supported via a plethora of `#ifdef
BINDLESS` preprocessor definitions.

Unfortunately, this PR introduces quite a bit of unsightliness into the
PBR shaders. This is a result of the fact that WGSL supports neither
passing binding arrays to functions nor passing individual *elements* of
binding arrays to functions, except directly to texture sample
functions. Thus we're unable to use the `sample_texture` abstraction
that helped abstract over the meshlet and non-meshlet paths. I don't
think there's anything we can do to help this other than to suggest
improvements to upstream Naga.
@pcwalton pcwalton requested review from JMS55 and kristoff3r December 4, 2024 07:06
@pcwalton pcwalton added C-Performance A change motivated by improving speed, memory usage or compile times S-Needs-Review Needs reviewer attention (from anyone!) to move forward A-Rendering Drawing game state to the screen labels Dec 4, 2024
@pcwalton pcwalton added this to the 0.16 milestone Dec 4, 2024
Copy link
Contributor

@JMS55 JMS55 left a comment

Choose a reason for hiding this comment

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

Oh boy, absolutely terrible code 😅. We are unlikely to get any improvements to this in naga anytime soon afaik. I think we might want to look into writing a simple macro in naga_oil or bevy for texture sampling.

@pcwalton pcwalton requested a review from Elabajaba December 5, 2024 19:51
@pcwalton
Copy link
Contributor Author

pcwalton commented Dec 5, 2024

Yeah, I'd love a macro in naga_oil. We could do a lot with macros, not just this. For example, we could abstract over bindless/non-bindless declarations.

@pcwalton pcwalton requested a review from IceSentry December 6, 2024 05:09
Copy link
Contributor

@kristoff3r kristoff3r left a comment

Choose a reason for hiding this comment

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

My eyes might have glazed over at some point, but looks correct and tested pbr example on linux+webgl2.

I agree we need macros or similar very soon, the pbr shaders weren't pretty before but they're getting borderline unreadable except for mechanical changes like this.

@pcwalton pcwalton added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 7, 2024
@alice-i-cecile
Copy link
Member

Testing this via an example run then merging. I'm not pleased with the reduced clarity of the shaders, but I agree that this is not the place to solve it (nor should this be blocked on that).

@pcwalton
Copy link
Contributor Author

Example run is green.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 10, 2024
github-merge-queue bot pushed a commit that referenced this pull request Dec 10, 2024
This commit makes `StandardMaterial` use bindless textures, as
implemented in PR #16368. Non-bindless mode, as used for example in
Metal and WebGL 2, remains fully supported via a plethora of `#ifdef
BINDLESS` preprocessor definitions.

Unfortunately, this PR introduces quite a bit of unsightliness into the
PBR shaders. This is a result of the fact that WGSL supports neither
passing binding arrays to functions nor passing individual *elements* of
binding arrays to functions, except directly to texture sample
functions. Thus we're unable to use the `sample_texture` abstraction
that helped abstract over the meshlet and non-meshlet paths. I don't
think there's anything we can do to help this other than to suggest
improvements to upstream Naga.
Merged via the queue into bevyengine:main with commit 7ed1f32 Dec 10, 2024
@rparrett
Copy link
Contributor

rparrett commented Dec 10, 2024

The example run completed successfully, but shows differences in fog_volumes and volumetric_fog on macOS and meshlet on linux which seem to be breakage.

https://pixel-eagle.com/project/B25A040A-A980-4602-B90C-D480AB84076D?filter=PR-16644

The volumetric fog stuff looks like it's just the branch being slightly stale + #16677

I don't know about meshlet. Can't test.

BD103 pushed a commit to BD103/bevy that referenced this pull request Dec 10, 2024
This commit makes `StandardMaterial` use bindless textures, as
implemented in PR bevyengine#16368. Non-bindless mode, as used for example in
Metal and WebGL 2, remains fully supported via a plethora of `#ifdef
BINDLESS` preprocessor definitions.

Unfortunately, this PR introduces quite a bit of unsightliness into the
PBR shaders. This is a result of the fact that WGSL supports neither
passing binding arrays to functions nor passing individual *elements* of
binding arrays to functions, except directly to texture sample
functions. Thus we're unable to use the `sample_texture` abstraction
that helped abstract over the meshlet and non-meshlet paths. I don't
think there's anything we can do to help this other than to suggest
improvements to upstream Naga.
ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Jan 6, 2025
This commit makes `StandardMaterial` use bindless textures, as
implemented in PR bevyengine#16368. Non-bindless mode, as used for example in
Metal and WebGL 2, remains fully supported via a plethora of `#ifdef
BINDLESS` preprocessor definitions.

Unfortunately, this PR introduces quite a bit of unsightliness into the
PBR shaders. This is a result of the fact that WGSL supports neither
passing binding arrays to functions nor passing individual *elements* of
binding arrays to functions, except directly to texture sample
functions. Thus we're unable to use the `sample_texture` abstraction
that helped abstract over the meshlet and non-meshlet paths. I don't
think there's anything we can do to help this other than to suggest
improvements to upstream Naga.
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 S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants