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

Use bevy-glsl-to-spirv only on the supported targets, default to shaderc otherwise. #1348

Closed
inodentry opened this issue Jan 29, 2021 · 14 comments
Labels
A-Build-System Related to build systems or continuous integration C-Dependencies A change to the crates that Bevy depends on

Comments

@inodentry
Copy link
Contributor

The current configuration uses bevy-glsl-to-spirv by default, while selectively enabling shaderc for specific targets.

I think this is the wrong way around.

bevy-glsl-to-spirv provides precompiled binaries to help with builds on the officially supported targets. By design, it can only work for specifically-supported build targets.

shaderc has additional build-time requirements and complexity, but it supports a much wider range of compilation targets.

It doesn't make sense to me that bevy-glsl-to-spirv is the blanket default, and shaderc is on a special whitelist.

This results in compilation failures whenever anyone tries building bevy on a new target (aarch64-unknown-linux-gnu? x86_64-unknown-linux-musl? etc.), leading to issues like #898. I've seen many instances of similar reports; someone tries to compile bevy on their raspberry pi or whatever, only to hit the bevy-glsl-to-spirv error.

With shaderc, such platforms should be able to work (and would have likely worked out of the box for users attempting them for the first time).

Can we flip the configuration around? Make bevy-glsl-to-spirv conditionally-enabled on the platforms it supports, and default to shaderc otherwise?

This would keep builds exactly the same for any existing users of bevy, but it will help make bevy work on new targets more seamlessly.

@cart
Copy link
Member

cart commented Jan 29, 2021

Ooh i think that is a very good call. Thats the best (short term) solution to the problem I've heard to-date. I'm sold.

@inodentry
Copy link
Contributor Author

@cart where can I find a list of the exact targets that are supported with bevy-glsl-to-spirv?

I will go do this and make a PR, just want to make sure I don't miss anything.

@cart
Copy link
Member

cart commented Jan 29, 2021

You can see them outlined here: https://github.com/cart/glsl-to-spirv/blob/master/Cargo.toml
There is also some nuance after this pr: cart/glsl-to-spirv#7, which adds the option to build non-precompiled windows platforms from source (we will publish a new glsl-to-spirv version before the next release).

I think for the given "from source" windows platforms in glsl-to-spirv we will want to continue using glsl-to-spirv. The dependencies are already included in the VS C++ Build Tools, which is much more natural than the Ninja + Python requirement for shaderc+windows.

@GrygrFlzr what are your thoughts?

@inodentry
Copy link
Contributor Author

Yes, if building bevy-glsl-to-spirv is supported, and it is simpler (less dependencies) than building shaderc, seems logical that it should be preferred on that platform.

@inodentry
Copy link
Contributor Author

BTW, while we are at it, should I make a feature flag to opt into shaderc even on platforms where bevy-glsl-to-spirv is supported (and normally used)? I don't see why we should limit users' options. Maybe they have reasons to prefer shaderc.

@inodentry
Copy link
Contributor Author

Also, what prevents the "building from source" functionality of bevy-glsl-to-spirv from being enabled for even more targets?

@cart
Copy link
Member

cart commented Jan 29, 2021

hmm yeah bevy-glsl-to-spirv only requires cmake and cc, which is preferable to Cmake + Git + Python + (on windows: Ninja). the only reason i can think of to use shaderc over bevy-glsl-to-spirv is the fact that it is well tested and proven to work on the platforms we currently use it on.

We just need to go through the process of validating bevy-glsl-to-spirv on iOS and Apple Silicon MacOS.

@cart
Copy link
Member

cart commented Jan 29, 2021

should I make a feature flag to opt into shaderc even on platforms where bevy-glsl-to-spirv is supported (and normally used)

I vaguely remember some aspect of Cargo disallowing this functionality last time I looked in to it. Feel free to give it a whack though.

@inodentry
Copy link
Contributor Author

the only reason i can think of to use shaderc over bevy-glsl-to-spirv is the fact that it is well tested and proven to work on the platforms we currently use it on.

So, officially-supported platforms aside, if I understand you correctly, the only thing blocking support for unofficial platforms (like aarch64-unknown-linux-gnu or x86_64-unknown-linux-musl) for bevy-glsl-to-spirv with the new source-building configuration, is that it is not explicitly whitelisted / validated? I.e it is an artificially-imposed limitation?

If that is the case, then I'd want to reconsider this proposal of switching the default to shaderc in order to increase platform support (for not officially-supported targets). Should we not just lift the limitation from bevy-glsl-to-spirv, and let users use that?

@cart
Copy link
Member

cart commented Jan 29, 2021

Yup. Its a matter of validation / whitelisting. If we can prove bevy-glsl-to-spirv works on the relevant platforms then i think it makes sense to use that everywhere.

@inodentry
Copy link
Contributor Author

inodentry commented Jan 29, 2021

Well, my goal was to provide a "sane default" / fallback, so that people trying bevy on new platforms that haven't been "proven", have some chance of it just working. We shouldn't have to manually validate each and every target, before people are allowed to use it.

What's better for that? shaderc or bevy-glsl-to-spirv? That should be the default (unrestricted, allowed to build for any target), and the more limited one should be on a whitelist.

As it is, bevy is just outright artificially blocked from even trying to compile, on any target that isn't explicitly in either whitelist (as each of the options is only enabled for specific targets). This is not great. There is no fallback. I want to pick one of the two options as a fallback and allow it everywhere unconditionally.

Just tell me which one to pick, and I'll go remove the whitelist / un-restrict it.

@cart
Copy link
Member

cart commented Jan 29, 2021

We can't know which one to pick until we test the bevy-glsl-to-spirv changes on the relevant platforms.

If it works as expected, I think it's reasonable to assume it will work elsewhere. It would then be preferable to use glsl-to-spirv by default and opt in to shaderc, because then we can share the same code path across platforms.

However if it doesn't work then shaderc should probably be the default because we know it works well across many platforms

@inodentry
Copy link
Contributor Author

OK, now i understand you. 😄 I didn't understand you before, sorry.

I will try to compile bevy-glsl-to-spirv on some extra targets, whatever I have access to, to see if it works.

Those findings will inform the decision.

@karroffel karroffel added A-Build-System Related to build systems or continuous integration C-Enhancement A new feature C-Dependencies A change to the crates that Bevy depends on labels Jan 30, 2021
@alice-i-cecile
Copy link
Member

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 C-Dependencies A change to the crates that Bevy depends on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants