Skip to content

Use ShaderDefVal to avoid redundant code#9408

Closed
766974616c79 wants to merge 1 commit into
bevyengine:mainfrom
766974616c79:use-ShaderDefVal
Closed

Use ShaderDefVal to avoid redundant code#9408
766974616c79 wants to merge 1 commit into
bevyengine:mainfrom
766974616c79:use-ShaderDefVal

Conversation

@766974616c79

@766974616c79 766974616c79 commented Aug 10, 2023

Copy link
Copy Markdown

Objective

The purpose of this pull request is to complete this issue (closes #9256)

Migration guide

The MESH_BINDGROUP_1 shader def has been replaced by the MESH_BINDGROUP ShaderDefVal.

If in your shader you previously used:

#ifdef MESH_BINDGROUP_1
@group(1) @binding(0)
var<storage> mesh: array<Mesh>;
#else // MESH_BINDGROUP_1
@group(2) @binding(0)
var<storage> mesh: array<Mesh>;
#endif // MESH_BINDGROUP_1

You can now write it:

@group(#{MESH_BINDGROUP}) @binding(0)
var<storage> mesh: array<Mesh>;

Other important change: You should now add the MESH_BINDGROUP define even if the binding group is 0.
Add this line to your shader specialization pipeline:

shader_defs.push(ShaderDefVal::UInt("MESH_BINDGROUP".into(), 2));

@github-actions

Copy link
Copy Markdown
Contributor

Welcome, new contributor!

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

@nicopap nicopap added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Aug 10, 2023
@github-actions

Copy link
Copy Markdown
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@nicopap

nicopap commented Aug 10, 2023

Copy link
Copy Markdown
Contributor

You need to update the CPU-side code as well.

Replace all instance of:

shader_defs.push("MESH_BINDGROUP_1".into());

with

shader_defs.push(ShaderDefVal::UInt("MESH_BINDGROUP".into(), 1));

You'll also need to add

shader_defs.push(ShaderDefVal::UInt("MESH_BINDGROUP".into(), 2));

to existing pipelines that do not add the MESH_BINDGROUP_1 binding.

Also, could you add a "closes #9256" to your PR description? So that it automatically closes the linked issue when this gets merged.

A summary on writing the migration guide lines:

---------

### Migration guide

The `MESH_BINDGROUP_1` shader def has been replaced by the `MESH_BINDGROUP` `ShaderDefVal`.

If in your shader you previously used:
``\`
#ifdef MESH_BINDGROUP_1
@group(1) @binding(0)
var<storage> mesh: array<Mesh>;
#else // MESH_BINDGROUP_1
@group(2) @binding(0)
var<storage> mesh: array<Mesh>;
#endif // MESH_BINDGROUP_1
``\`

You can now write it:

``\`
@group(#{MESH_BINDGROUP}) @binding(0)
var<storage> mesh: array<Mesh>;
``\`

**Other important change**: You should now add the `MESH_BINDGROUP` define even if the binding group is 0.
Add this line to your shader specialization pipeline:

``\`
shader_defs.push(ShaderDefVal::UInt("MESH_BINDGROUP".into(), 2));
``\`

@766974616c79

Copy link
Copy Markdown
Author

Thank you very much for these details. I have a quick question about the pipelines. Should I add the code for the SpecializedMeshPipeline implementations or for literally all pipelines?

@nicopap

nicopap commented Aug 10, 2023

Copy link
Copy Markdown
Contributor

I'm not 100%, try only SpecializedMeshPipeline.

Run shader_prepass and swap between prepass mod to see if it panics or not.

@robtfm

robtfm commented Aug 10, 2023

Copy link
Copy Markdown
Contributor

Can we please have a default of 2 for when the def is not specified. It used to be necessary to specify for every pipeline and it was very annoying, I would prefer not to go back to that situation again.

@nicopap

nicopap commented Aug 10, 2023

Copy link
Copy Markdown
Contributor

Ooops my bad, my instructions mentioned a default of 0, I updated them to 2.

@nicopap

nicopap commented Aug 10, 2023

Copy link
Copy Markdown
Contributor

So how would we setup a default? Maybe passing the shader_defs as an argument to the specialize method on SpecializedMeshPipeline?

@766974616c79

Copy link
Copy Markdown
Author

I'm not 100%, try only SpecializedMeshPipeline.

Run shader_prepass and swap between prepass mod to see if it panics or not.

Only on SpecializedMeshPipeline works perfectly

@robtfm

robtfm commented Aug 10, 2023

Copy link
Copy Markdown
Contributor

So how would we setup a default? Maybe passing the shader_defs as an argument to the specialize method on SpecializedMeshPipeline?

I was thinking the shader could branch on #ifdef as it does currently (I didn’t look at the code yet though)

@james7132
james7132 requested a review from superdump October 2, 2023 01:16
@JMS55

JMS55 commented Feb 5, 2024

Copy link
Copy Markdown
Contributor

MESH_BIND_GROUP_1 was removed #10485

@JMS55 JMS55 closed this Feb 5, 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-Code-Quality A section of code that is hard to understand or change M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace MESH_BINDGROUP_1 with a ShaderDefVal

4 participants