[Merged by Bors] - StandardMaterial: expose a cull_mode option #3982
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rparrett
approved these changes
Feb 18, 2022
rib
force-pushed
the
wip/rib/standard-material-cull-mode
branch
from
4e3a833
to
2b2a186
Compare
|
just fixed the formatting that was flagged by |
superdump
added
A-Rendering
This makes it possible for materials to configure front or back face culling, or disable culling. Initially I looked at specializing the Mesh which currently controls this state but conceptually it seems more appropriate to control this at the material level, not the mesh level. _Just for reference this also seems to be consistent with Unity where materials/shaders can configure the culling mode between front/back/off - as opposed to configuring any culling state when importing a mesh._ After some archaeology, trying to understand how this might relate to the existing 'double_sided' option, it was determined that double_sided is a more high level lighting option originally from Filament that will cause the normals for back faces to be flipped. For sake of avoiding complexity, but keeping control this currently keeps the options orthogonal, and adds some clarifying documentation for `double_sided`. This won't affect any existing apps since there hasn't been a way to disable backface culling up until now, so the option was essentially redundant. double_sided support could potentially be updated to imply disabling of backface culling.
rib
force-pushed
the
wip/rib/standard-material-cull-mode
branch
from
2b2a186
to
7ac30d3
Compare
|
err, actually I apparently just force pushed redundantly without including the cargo fmt changes - this time hopefully actually includes the CI fix. |
superdump
approved these changes
Mar 2, 2022
IceSentry
approved these changes
Mar 2, 2022
mockersf
approved these changes
Mar 2, 2022
mockersf
added
the
S-Ready-For-Final-Review
|
Would this fix #3729? |
|
I think we could probably close that if this were merged. Maybe But with cull mode exposed for the user, it may not be an issue, and my motivation for opening the bug would be resolved at least. |
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Mar 5, 2022
This makes it possible for materials to configure front or back face culling, or disable culling. Initially I looked at specializing the Mesh which currently controls this state but conceptually it seems more appropriate to control this at the material level, not the mesh level. _Just for reference this also seems to be consistent with Unity where materials/shaders can configure the culling mode between front/back/off - as opposed to configuring any culling state when importing a mesh._ After some archaeology, trying to understand how this might relate to the existing 'double_sided' option, it was determined that double_sided is a more high level lighting option originally from Filament that will cause the normals for back faces to be flipped. For sake of avoiding complexity, but keeping control this currently keeps the options orthogonal, and adds some clarifying documentation for `double_sided`. This won't affect any existing apps since there hasn't been a way to disable backface culling up until now, so the option was essentially redundant. double_sided support could potentially be updated to imply disabling of backface culling. For reference https://github.com/bevyengine/bevy/pull/3734/commits also looks at exposing cull mode control. I think the main difference here is that this patch handles RenderPipelineDescriptor specialization directly within the StandardMaterial implementation instead of communicating info back to the Mesh via the `queue_material_meshes` system. With the way material.rs builds up the final RenderPipelineDescriptor first by calling specialize for the MeshPipeline followed by specialize for the material then it seems like we have a natural place to override anything in the descriptor that's first configured for the mesh state.
bors
bot
changed the title
kurtkuehnert
pushed a commit
to kurtkuehnert/bevy
that referenced
this pull request
Mar 6, 2022
This makes it possible for materials to configure front or back face culling, or disable culling. Initially I looked at specializing the Mesh which currently controls this state but conceptually it seems more appropriate to control this at the material level, not the mesh level. _Just for reference this also seems to be consistent with Unity where materials/shaders can configure the culling mode between front/back/off - as opposed to configuring any culling state when importing a mesh._ After some archaeology, trying to understand how this might relate to the existing 'double_sided' option, it was determined that double_sided is a more high level lighting option originally from Filament that will cause the normals for back faces to be flipped. For sake of avoiding complexity, but keeping control this currently keeps the options orthogonal, and adds some clarifying documentation for `double_sided`. This won't affect any existing apps since there hasn't been a way to disable backface culling up until now, so the option was essentially redundant. double_sided support could potentially be updated to imply disabling of backface culling. For reference https://github.com/bevyengine/bevy/pull/3734/commits also looks at exposing cull mode control. I think the main difference here is that this patch handles RenderPipelineDescriptor specialization directly within the StandardMaterial implementation instead of communicating info back to the Mesh via the `queue_material_meshes` system. With the way material.rs builds up the final RenderPipelineDescriptor first by calling specialize for the MeshPipeline followed by specialize for the material then it seems like we have a natural place to override anything in the descriptor that's first configured for the mesh state.
aevyrie
pushed a commit
to aevyrie/bevy
that referenced
this pull request
Jun 7, 2022
This makes it possible for materials to configure front or back face culling, or disable culling. Initially I looked at specializing the Mesh which currently controls this state but conceptually it seems more appropriate to control this at the material level, not the mesh level. _Just for reference this also seems to be consistent with Unity where materials/shaders can configure the culling mode between front/back/off - as opposed to configuring any culling state when importing a mesh._ After some archaeology, trying to understand how this might relate to the existing 'double_sided' option, it was determined that double_sided is a more high level lighting option originally from Filament that will cause the normals for back faces to be flipped. For sake of avoiding complexity, but keeping control this currently keeps the options orthogonal, and adds some clarifying documentation for `double_sided`. This won't affect any existing apps since there hasn't been a way to disable backface culling up until now, so the option was essentially redundant. double_sided support could potentially be updated to imply disabling of backface culling. For reference https://github.com/bevyengine/bevy/pull/3734/commits also looks at exposing cull mode control. I think the main difference here is that this patch handles RenderPipelineDescriptor specialization directly within the StandardMaterial implementation instead of communicating info back to the Mesh via the `queue_material_meshes` system. With the way material.rs builds up the final RenderPipelineDescriptor first by calling specialize for the MeshPipeline followed by specialize for the material then it seems like we have a natural place to override anything in the descriptor that's first configured for the mesh state.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This makes it possible for materials to configure front or
back face culling, or disable culling.
Initially I looked at specializing the Mesh which currently
controls this state but conceptually it seems more appropriate
to control this at the material level, not the mesh level.
Just for reference this also seems to be consistent with Unity
where materials/shaders can configure the culling mode between
front/back/off - as opposed to configuring any culling state
when importing a mesh.
After some archaeology, trying to understand how this might
relate to the existing 'double_sided' option, it was determined
that double_sided is a more high level lighting option originally
from Filament that will cause the normals for back faces to be
flipped.
For sake of avoiding complexity, but keeping control this
currently keeps the options orthogonal, and adds some clarifying
documentation for
double_sided. This won't affect any existingapps since there hasn't been a way to disable backface culling
up until now, so the option was essentially redundant.
double_sided support could potentially be updated to imply
disabling of backface culling.
For reference https://github.com/bevyengine/bevy/pull/3734/commits also looks at exposing cull mode control. I think the main difference here is that this patch handles RenderPipelineDescriptor specialization directly within the StandardMaterial implementation instead of communicating info back to the Mesh via the
queue_material_meshessystem.With the way material.rs builds up the final RenderPipelineDescriptor first by calling specialize for the MeshPipeline followed by specialize for the material then it seems like we have a natural place to override anything in the descriptor that's first configured for the mesh state.