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

Add UV channel selection to StandardMaterial #13200

Merged
merged 5 commits into from
May 13, 2024

Conversation

geckoxx
Copy link
Contributor

@geckoxx geckoxx commented May 3, 2024

Objective

Solution

  • The StandardMaterial gets extended for each texture by an UvChannel enum. It defaults to Uv0 but can also be set to Uv1.
  • The gltf loader now handles the texcoord information. If the texcoord is not supported it creates a warning.
  • It uses StandardMaterial shader defs to define which attribute to use.

Testing

This fixes #12496 for example:
wall_fixed

For testing of all kind of textures I used the TextureTransformMultiTest from https://github.com/KhronosGroup/glTF-Sample-Assets/tree/main/Models/TextureTransformMultiTest
Its purpose is to test multiple texture transfroms but it is also a good test for different texcoords.
It also shows the issue with emission #13133.

Before:
TextureTransformMultiTest_main

After:
TextureTransformMultiTest_texcoord

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen M-Needs-Release-Note Work that should be called out in the blog due to impact X-Uncontroversial This work is generally agreed upon D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes D-Domain-Expert Requires deep knowledge in a given domain labels May 3, 2024
@geckoxx
Copy link
Contributor Author

geckoxx commented May 6, 2024

Added the same support for clearcoat textures and using shader defs instead of material flags.

@geckoxx geckoxx marked this pull request as ready for review May 6, 2024 14:29
@bugsweeper
Copy link
Contributor

@geckoxx You should add "Closes #13153" to your description

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.

nice. clean code and provides a good base to extend to more UVs in future.

we could potentially select which texcoords to put into our uv attributes based on usage rather than just taking 0/1, but if we do extend to more uvs that would be unnecessary anyway, and it's a clear improvement as is.

@robtfm
Copy link
Contributor

robtfm commented May 11, 2024

just a quick note about the shader defs - we could alternatively use indexes/bits in the material data to specify the uv indexes and avoid a proliferation of shader variants.

i think defs are probably better because

  • it will have no cost in the base case where only 1 set of texcoords are used
  • if you do need to use multiple sets, you likely have them all in the same locations, so only one variant would be needed
  • if they are not in the same locations, if you control the assets and you care enough, you can modify your assets to align them

whereas using data in the material would increase its size having a (marginal) effect on all uses of StandardMaterial, whether you want the flexibility or not.

@robtfm robtfm added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 11, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 13, 2024
Merged via the queue into bevyengine:main with commit 3f5a090 May 13, 2024
28 checks passed
@geckoxx geckoxx deleted the specific_uv_map branch May 14, 2024 07:11
@alice-i-cecile
Copy link
Member

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1313 if you'd like to help out.

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-Bug An unexpected or incorrect behavior C-Feature A new feature, making something new possible D-Domain-Expert Requires deep knowledge in a given domain D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-Release-Note Work that should be called out in the blog due to impact S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon
Projects
None yet
6 participants