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

Version numbers and profile specifiers in shaders #587

Closed
MarcoHutter opened this issue Apr 30, 2016 · 8 comments
Closed

Version numbers and profile specifiers in shaders #587

MarcoHutter opened this issue Apr 30, 2016 · 8 comments

Comments

@MarcoHutter
Copy link

(This might be related to #405 )

Compiling the current shaders (e.g. from the "duck" example) on desktop (non-GLES) systems results in

0(1) : warning C7022: unrecognized profile specifier "highp"
0(1) : warning C7022: unrecognized profile specifier "precision"
0(18) : warning C7536: OpenGL does not allow matrix casts without #version 120 or later

The concerning thing is that the "matrix cast" warning may be considered as an error in some cases, and cause the compilation to fail. I'm not sooo familiar with the details, but know that this depends on the graphics card vendor / driver and potentially other factors (e.g. these "compatibility" and "core" settings...). At least, according to the GLSL ES Spec https://www.khronos.org/registry/gles/specs/3.2/GLSL_ES_Specification_3.20.pdf , Section 3.3, Version Declaration:

Shaders must declare the version of the language they are written to. The version is specified in the first line of a shader by a character string:

version number es

where number must be a version of the language, following the same convention as VERSION above. The directive “#version 320 es” is required in any shader that uses version 3.20 of the language. Any number representing a version of the language a compiler does not support will cause an error to be generated. Version 1.00 of the language does not require shaders to include this directive, and shaders that do not include a #version directive will be treated as targeting version 1.00.

So I guess that the version number must be included (in the RFC2119 sense), unless the shader really targets GLSL 1.00.

I fixed this for my local tests, by adding

#version 120
#ifdef GL_ES
    precision highp float;
#endif

at the beginning of the shaders, but am not sure whether this an appropriate or generally applicable solution.

@pjcozzi
Copy link
Member

pjcozzi commented May 11, 2016

Thanks for pointing this out, @MarcoHutter!

I think there's a few components here:

  • The sample models only have WebGL/ES-compatible shaders
  • COLLADA2GLTF only outputs WebGL/ES-compatible shaders
  • The glTF spec doesn't explicitly specify the GLSL version

This is potentially related to #236.

Labeling this 1.0.1 so we consider this in the near-term.

@pjcozzi
Copy link
Member

pjcozzi commented Jul 6, 2016

@lexaknyazev @MarcoHutter I think we have a few options here:

  • Push for after 1.0.1 since SIGGRAPH is less than three weeks away and this issue hasn't come up as much as others
  • Add a something like shadingLanguage and shadingLanguageVersion properties to asset.profile.schema.json.
  • Make the glTF spec more explicitly define the shading language version(s) based on the asset.profile properties (e.g., WebGL 1.0.1 uses GLSL ES 1.0).
  • Allow an asset to have different versions of shaders for different APIs.

I am leaning towards pushing post 1.0.1 this since I think it needs to be considered more carefully along with API profiles in glTF.

@MarcoHutter
Copy link
Author

Deferring this to a later version is certainly OK. There are more pressing issues, and one must indeed be careful with selecting the right approach.

(I ran into this issue from various directions, while playing with a Java based glTF viewer. There are interdependencies between GL profiles and GL API versions and the GLSL version and things like core and compatibilty specifiers. I thought it could be solved by adding the #version 120 and selecting a "backward compatibilty" GL3 profile, but GL3bc is not available for all cards (even when they support GL3 and GL4). The appearance of the precision keyword was considered as an error by some compilers as well. I still have to wrap my head around all this, but for most real (WebGL based) application cases, this is likely not so important)

@pjcozzi
Copy link
Member

pjcozzi commented Jul 6, 2016

Thanks for the quick response, @MarcoHutter.

@lexaknyazev do you agree with pushing?

@significant-bit
Copy link

Support for non-ES versions will be important to Blender in the future. For internal shaders we use a mix of versions from 120 to 150 compatibility (always declared atop the GLSL), and are transitioning everything to GL 3.2 core profile and GLSL 150 soon.

The ability to export these directly into glTF would be ✨ 👍 ✨

@lexaknyazev
Copy link
Member

I'm for pushing this after 1.0.1.

The issue is quite complicated and involves careful definition of asset.profile (we don't have one now).
We should keep in mind also that runtime may have to enable GLSL extensions before compilation.
The situation is even more complex with upcoming WebGL 2.

@pjcozzi
Copy link
Member

pjcozzi commented Jul 7, 2016

Pushing post 1.0.1. Thanks all for the input.

@significant-bit glTF will support non-ES versions, it just a matter of scope to finish 1.0.1 for SIGGRAPH. By the way, how is import/export glTF support coming in Blender? Is there a Google Summer of Code project?

@pjcozzi pjcozzi removed the 2.0 label Jul 7, 2016
javagl added a commit to javagl/JglTF that referenced this issue Aug 9, 2016
The version 120 specifier is not supported in current
WebGL implementations. Non-GLES runtimes will have to
cope with the missing version number (and the invalid
matrix casts) anyhow.

Related to KhronosGroup/glTF#587
@donmccurdy
Copy link
Contributor

Closing, this is currently obsolete in glTF 2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants