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

bevy_render: Centralize shader View uniform type #5535

Closed
wants to merge 1 commit into from

Conversation

superdump
Copy link
Contributor

Objective

Solution

  • Move shader View struct to a shader import at bevy_render::view_types
  • Make shader view bindings for binding the view uniform to group 0 binding 0 as this is very common, at bevy_render::view_bindings - note that this itself imports bevy_render::view_types for convenience as we have done elsewhere
  • Remove the shader View struct from mesh_view_types.wgsl in bevy_pbr, mesh2d_view_types.wgsl and sprite.wgsl in bevy_sprite and ui.wgsl in bevy_ui and instead make use of the above imports.

Changelog

  • Removed: bevy_sprite::mesh2d_view_types and bevy_sprite::mesh2d_view_bindings
  • Added: bevy_render::view_types and bevy_render::view_bindings
  • Changed: All of bevy_pbr, bevy_sprite, and bevy_ui now use bevy_render::view_types and bevy_render::view_bindings to keep the shader View struct in-sync

Migration Guide

If you were using the bevy_sprite::mesh2d_view_types and/or bevy_sprite::mesh2d_view_bindings shader imports, now use bevy_render::view_types and/or bevy_render::view_bindings instead.

@superdump superdump requested review from mockersf and cart August 1, 2022 17:14
@superdump superdump added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change labels Aug 1, 2022
@superdump
Copy link
Contributor Author

I tested lighting, mesh2d, many_sprites, and ui and all look ok to me.

@nicopap
Copy link
Contributor

nicopap commented Jul 31, 2023

Is this still relevant? Kind of a nice small change

@superdump
Copy link
Contributor Author

I forgot that this was never merged. Definitely something we want to do in my opinion because of how ViewUniform is reused.

@cart
Copy link
Member

cart commented Aug 1, 2023

We have centralized the View type under bevy_render::view. With the new import system, it looks like #import bevy_render::view View. However binding imports are still separated inside bevy_pbr::mesh_view_bindings and bevy_sprite::mesh2d_view_bindings.

I like the idea of standardizing the view binding position and consolidating the import under bevy_render. We might also want to standardize the globals binding position to group(0) binding(1) and also place it in bevy_render?

@richchurcher
Copy link
Contributor

Backlog cleanup: hasn't seen any activity in awhile, migrating to issue #15645 as likely to still be useful.

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 C-Feature A new feature, making something new possible
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants