bkaradzic / bgfx

Cross-platform, graphics API agnostic, "Bring Your Own Engine/Framework" style rendering library.
https://bkaradzic.github.io/bgfx/overview.html
BSD 2-Clause "Simplified" License
14.86k stars 1.93k forks source link

OpenGL shaders with derivatives cause fatal errors (using shader profiles higher than 1.0) #3085

Open EvilTrev opened 1 year ago

EvilTrev commented 1 year ago

Describe the bug Using derivatives with OpenGL ES shaders (version 3.2 on Android in this case) leads to runtime errors because extensions are being defined after other preprocessor lines.

Shader output starts:

#version 320 es
#define attribute in
#define varying out
precision highp float;
precision highp int;
#extension GL_OES_standard_derivatives : enable

Which leads to this fatal error:

BGFX FATAL 0x00000001: Failed to compile shader. 0: ERROR: 0:6: '' :     GLSL compile error:  Extension directives must occur before any non-preprocessor tokens.

To Reproduce Create shader using derivatives and attempt to load.

Expected behavior That it would not define them after other preprocessor defines.

EvilTrev commented 1 year ago

The fix appears to be in shaderc.cpp, where the logic to append precision should exist much further down

bkaradzic commented 1 year ago

Provide shader source that has this issue.

EvilTrev commented 1 year ago

Any shader that uses dFdx or dFdy, for example you could try adding something like this to one of the shaders:

float textureMipLevel( sampler2D tex, vec2 uv )
{
    // Estimate using OpenGL spec: http://www.opengl.org/registry/doc/glspec42.core.20120427.pdf
    vec2 texel = vec2( textureSize( tex, 0 ).xy ) * uv;
    vec2 dx_vtc = dFdx( texel );
    vec2 dy_vtc = dFdy( texel );
    float delta_max_sqr = max( dot( dx_vtc, dx_vtc ), dot( dy_vtc, dy_vtc ) );
    return 0.5 * log2( delta_max_sqr );
}

But there isn't much need, the issue starts at line 2373 of shaderc.cpp, where precision is injected before many other extensions. The same problems would occur with GL_EXT_shader_texture_lod, GL_OES_texture_3D and so on.

bkaradzic commented 1 year ago

See example 42-bunny...

Type make TARGET=4 rebuild, and there is no issue.

EvilTrev commented 1 year ago

Did you use target profile 310_es or 320_es? It won’t occur for 100_es due to the logic surrounding precision injection in shaderc.cpp

EvilTrev commented 1 year ago

Target 4 will build glsl, for android gles it is:

make TARGET=3

And the parameter to shaderc would need to be -p 300_es, so in my makefile I have something like:

ifeq ($(TARGET), 3)
VS_FLAGS=--platform android -p 300_es
FS_FLAGS=--platform android -p 300_es
CS_FLAGS=--platform android -p 300_es
SHADER_PATH=shaders/essl

Which should be fine since the default for BGFX_CONFIG_RENDERER_OPENGLES_MIN_VERSION is 30.

jsm174 commented 2 months ago

I'm having this same exact issue with GLES shaders while trying to get our BGFX port of Visual Pinball running on Android. (I'm testing on a Pixel 7)

I quickly hacked shaderc.cpp, to move all #extension to the top, and it started working.

            if (usesTextureArray)
            {
                bx::stringPrintf(code
                    , "#extension GL_EXT_texture_array : enable\n"
                    );
            }

            std::istringstream stream(code);
            std::string line;
            std::vector<std::string> extensions;
            std::vector<std::string> otherLines;
            std::string versionLine;

            bool versionLineFound = false;

            while (std::getline(stream, line)) {
                if (line.find("#version") == 0) {
                    versionLine = line;
                    versionLineFound = true;
                } else if (line.find("#extension") == 0) {
                    extensions.push_back(line);
                } else {
                    otherLines.push_back(line);
                }
            }

            std::ostringstream output;
            if (versionLineFound) {
                output << versionLine << "\n";
            }
            for (const auto& ext : extensions) {
                output << ext << "\n";
            }
            for (const auto& other : otherLines) {
                output << other << "\n";
            }

            code = output.str();

            if (glsl_profile == 100)
            {

Would a nicer fix be considered for inclusion in shaderc?