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

Replace glsl-to-spirv and spirv-reflect with Naga #209

Conversation

lachlansneff
Copy link
Contributor

@lachlansneff lachlansneff commented Aug 16, 2020

Replace both bevy-glsl-to-spirv (a fork of glsl-to-spirv) and spirv-reflect with naga

This should reduce compile times a lot, as well make it easier to cross-compile for platforms like android, ios, and webassembly.

The glsl frontends and spirv backend for naga are not very mature yet. They're progressing pretty quickly, but they're still not usable for real applications yet. Hopefully this will change in the upcoming weeks.

@cart
Copy link
Member

cart commented Aug 16, 2020

I love this. Naga is a big step forward in terms of build complexity and compile times. I'll try to review as soon as I can.

@lachlansneff lachlansneff force-pushed the friendship-ended-with-spirv-reflect-now-naga-is-my-best-friend branch from 22e6ff1 to 9a1a498 Compare August 16, 2020 19:49
@lachlansneff
Copy link
Contributor Author

I also just added a preprocessor in the naga-glsl path (since the glsl-new glsl naga front-end doesn't have one yet) and it adds support for shader includes! Not exposed yet, of course though.

@lachlansneff lachlansneff force-pushed the friendship-ended-with-spirv-reflect-now-naga-is-my-best-friend branch from a47749c to 1355128 Compare August 25, 2020 23:07
@Waridley
Copy link
Contributor

Waridley commented Sep 7, 2020

FYI I just tried rebasing this onto the current master commit, applied the fixes I've been experimenting with for wasm for the non-rendering-related stuff, and managed to compile all crates to wasm! No idea yet if rendering will actually work on the web, but the tests still pass and examples still run on Linux, and without this PR I couldn't get anything depending on the renderer to compile to wasm at all.

@karroffel karroffel added A-Build-System Related to build systems or continuous integration A-Rendering Drawing game state to the screen C-Dependencies A change to the crates that Bevy depends on labels Sep 7, 2020
Copy link
Member

@mrk-its mrk-its left a comment

Choose a reason for hiding this comment

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

@lachlansneff Great job! I'm just going to give it a chance in webgl2_renderer backend. Could you rebase it on current master? Do you have an idea will it work with "300 es" shaders? Thanks!

EDIT:
I played a bit with this PR and bevy's 2d sprite and sprite_sheet examples. First I've rebased it on current master and changed naga to latest version (some minor changes were required: mrk-its@14ada61). Then I was able to compile sprite example. Unfortunately it panics a lot:

  • vertex sprite shader was parsed, but it failed while writing spirv data.
  • after disabling conversion to spirv it failed on parsing fragment shader.

(Float, 4) => VertexFormat::Float,
_ => return Err(()),
},
Vector { size, kind, width } => match (size, kind, width) {
Copy link
Member

Choose a reason for hiding this comment

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

It would be great to have this (size, kind, width) information preserved. Take a look on my webgl2_renderer: https://github.com/bevyengine/bevy/pull/613/files#diff-d4ccfb870bfebbfae7f1b458a69aed63 - I have to recreate this information from this enum :)

@alice-i-cecile alice-i-cecile 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 Feb 17, 2021
Base automatically changed from master to main February 19, 2021 20:44
@alice-i-cecile alice-i-cecile removed 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 Feb 23, 2021
@mockersf
Copy link
Member

I tried updating this PR to current bevy main and released naga, it is failing on gfx-rs/naga#321

@cart
Copy link
Member

cart commented Mar 14, 2021

Dang. Thanks for checking. Hopefully we'll be able to use it Soon™ 😄

@NathanSWard
Copy link
Contributor

Closing this issue as discussed here.
This appears to be a goal of the new rendering rework planned for 0.6.
As well have #2130 which appears to be a alternative to this PR.

Please feel free to reopen if I'm incorrect or future development spawns from this PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Build-System Related to build systems or continuous integration A-Rendering Drawing game state to the screen C-Dependencies A change to the crates that Bevy depends on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants