Hard Light Productions Forums

Modding, Mission Design, and Coding => FS2 Open Coding - The Source Code Project (SCP) => Test Builds => Topic started by: AdmiralRalwood on August 23, 2015, 12:24:21 pm

Title: Shader versioning test
Post by: AdmiralRalwood on August 23, 2015, 12:24:21 pm
There's a pull request (https://github.com/scp-fs2open/fs2open.github.com/pull/316) to hopefully fix shader compilation on certain drivers, but it may break others. In the interests of collecting data, we'd like people to run both a recent Nightly Build (http://www.hard-light.net/forums/index.php?topic=90347.0) and this test build with shaders enabled and report if there are any differences (e.g. if you can run a normal build with shaders, but can't run this build with shaders, or vice versa, let us know and share your debug log (http://www.hard-light.net/forums/index.php?topic=56279.msg1180359#msg1180359)).

SSE2: http://deviance.duckish.net/downloads/fs2_fred2_open_PR316_SSE2.7z
AVX: http://deviance.duckish.net/downloads/fs2_fred2_open_PR316_AVX.7z

Please note that since we want to test shader compilation, you will need to remove the -no_glsl ("Disable GLSL (shader) Support") command-line option (and probably -disable_glsl_model / "Don't use shaders for model rendering" as well) for this test.
Title: Re: Shader versioning test
Post by: m!m on August 23, 2015, 12:35:53 pm
If you disabled the commandline options AdmiralRalwood mentioned you should be able to test it by simply going to the Tech Room and looking at a ship model.
Title: Re: Shader versioning test
Post by: chief1983 on August 23, 2015, 12:44:38 pm
It would help to use the MediaVPs so that we actually are using most of the shaders too?
Title: Re: Shader versioning test
Post by: Echelon9 on August 23, 2015, 10:03:36 pm
The background context here is standards compliance with the OpenGL GLSL shader language.

Per the specifications, it is clear that the start of a shader (whether a fragment, vertex, or geometry shader etc) *must* begin with a version declaration which informs the driver which feature set is expected by the program / shader. e.g.

Code: [Select]
#version 120
// Shader code goes here

Now driver conformance with the OpenGL specification is a mixed bag. As engine developers, we have to work with these quirks in the same way web developers have worked around browser-specific interpretations.

In this case, the specification is clear we *must* include a #version directive. We want to understand from this test which combinations (hardware, driver and OS) correctly interpret this. i.e.
Now it may also be that our fs2open shaders are making errors too, which we can confirm by putting them through a reference GLSL compiler (https://www.khronos.org/opengles/sdk/tools/Reference-Compiler/).

As GPUs become fully programmable devices, the former fixed function pipeline approaches are less relevant and shader-driven approaches come to the fore. We'd like to better understand the environment we are working with at the moment.
Title: Re: Shader versioning test
Post by: niffiwan on August 24, 2015, 02:20:49 am
OK - recent master builds work, but a build from the PR does not. It exits as soon as I view a ship in the ship lab or tech room.

OS: Linux Mint 17 64bit
Card: GeForce GTS 450
NVIDIA Driver Version: 340.76 (closed source drivers supplied by distro)

Log here (http://pastebin.com/pDRpU3ui), as a wild guess I think my setup wants #version 140 in the shaders rather than #version 120 :)
Title: Re: Shader versioning test
Post by: AdmiralRalwood on August 24, 2015, 02:25:12 am
/me wonders if this means the AMD drivers would work with #version 140
Title: Re: Shader versioning test
Post by: m!m on August 24, 2015, 04:19:59 am
OK - recent master builds work, but a build from the PR does not. It exits as soon as I view a ship in the ship lab or tech room.

OS: Linux Mint 17 64bit
Card: GeForce GTS 450
NVIDIA Driver Version: 340.76 (closed source drivers supplied by distro)

Log here (http://pastebin.com/pDRpU3ui), as a wild guess I think my setup wants #version 140 in the shaders rather than #version 120 :)
Looks like another error on our side. I think Nvidia wants to have #extension GL_EXT_gpu_shader4 : enable or #version 140 in the shader. We use OpenGL 2.1 so enabling the extension seems like the way to go. Can you try inserting "#extension GL_EXT_gpu_shader4 : enable\n" in code/globalincs/def_files.cpp:1340 and see if that fixes the error? We might have to do additional extension checking if we want to do that though.
Title: Re: Shader versioning test
Post by: niffiwan on August 24, 2015, 05:47:14 am
OK, that removed one error, just leaving this:

Code: [Select]
0(28) : warning C7508: extension ARB_texture_buffer_object not supported
0(70) : error C7532: global function texelFetch requires "#version 130" or later

And for the heck of it, I tried setting #version 140, and even more errors spat out  :sigh:

Code: [Select]
0(28) : warning C7508: extension ARB_texture_buffer_object not supported
0(28) : warning C7555: 'attribute' is deprecated, use 'in/out' instead
0(35) : warning C7555: 'varying' is deprecated, use 'in/out' instead
0(61) : warning C7555: 'varying' is deprecated, use 'in/out' instead
0(64) : warning C7555: 'varying' is deprecated, use 'in/out' instead
0(65) : warning C7555: 'varying' is deprecated, use 'in/out' instead
0(102) : error C7616: global variable gl_TexCoord is removed after version 140
0(102) : error C7616: global variable gl_TextureMatrix is removed after version 140
0(102) : error C7616: global variable gl_MultiTexCoord0 is removed after version 140
0(103) : error C7616: global variable gl_Vertex is removed after version 140
0(109) : error C7616: global variable gl_ModelViewMatrix is removed after version 140
0(118) : error C7616: global variable gl_ProjectionMatrix is removed after version 140
0(120) : error C7616: global variable gl_Color is removed after version 140
0(123) : error C7616: global variable gl_NormalMatrix is removed after version 140
0(123) : error C7616: global variable gl_Normal is removed after version 140
0(156) : error C7616: global variable gl_ClipVertex is removed after version 140
Title: Re: Shader versioning test
Post by: tomimaki on August 25, 2015, 06:39:47 am
Seems both builds work on win 7 and ancient amd drivers 13.12.
Log from PR316 shows:
Code: [Select]
  Using OpenGL driver workarounds:
    No #version: Don't add #version to OpenGL shaders
    <none>
Title: Re: Shader versioning test
Post by: AdmiralRalwood on August 25, 2015, 06:42:35 am
Yes, the build currently does not add the #version line to any AMD drivers due to it already being known that they don't play nice with it.
Title: Re: Shader versioning test
Post by: m!m on August 25, 2015, 06:43:39 am
Did you try going to the tech room or playing a mission? The shader problems usually only happen when one of the main shaders is compiled.
Now that I think about it, it would be nice to add a way to override which workarounds are used for testing purposes. I'll try to add that to the PR.
Title: Re: Shader versioning test
Post by: tomimaki on August 25, 2015, 08:37:25 am
I checked Sathanas, Arcadia in Tech Room and Ship Lab, tested in mission and it works (no visible difference).

Tested on Xubuntu 15.04 64bit with Catalyst 15.3 and it works.
Title: Re: Shader versioning test
Post by: m!m on September 10, 2015, 12:09:22 pm
Thanks for all your help but I can't figure out how to properly fix this for Nvidia drivers. Our OpenGL rendering engine is pretty much FUBAR in my opinion and without in depth knowledge of the rendering code there is no way I can fix this.
Title: Re: Shader versioning test
Post by: The Dagger on September 11, 2015, 01:47:53 am
Have some strange issues with this build (SSE2 version) but none with nightlies. In the F3 lab, it looks like only subobjects detached from hull (like the cockpit glass or moving parts) are rendered correctly while the first subobject is fixed in a certain direction (original position IIRC, since I'm looking straight into the thrusters). In-mission, those objects are invisible.

OS: Windows 7
Card: Nvidia GeForce GTX 460M
Driver version: 355.60

Here's a log running the latest MediaVPSs: http://pastebin.com/iJMcvSz9 (http://pastebin.com/iJMcvSz9)

There was a HUGE amount of repeated
Code: [Select]
ERROR! Unable to create vertex shader!
Compiling new shader:
        Model Rendering
   Loading built-in default shader for: main-v.sdr
   Loading built-in default shader for: main-f.sdr
Vertex shader failed to compile:
0(34) : warning C7508: extension ARB_texture_buffer_object not supported
0(35) : error C7532: global type samplerBuffer requires "#version 140" or later
0(35) : error C0000: ... or #extension GL_EXT_gpu_shader4 : enable
0(75) : error C7532: global function texelFetch requires "#version 130" or later
that I deleted in order to upload it.
Title: Re: Shader versioning test
Post by: tomimaki on September 11, 2015, 09:02:56 am
I don't have account on Github so I write here.
Vendors for Mesa depends on GPU
and for Intel is:
Code: [Select]
OpenGL vendor string: Intel Open Source Technology Centerfor AMD opensource driver:
Code: [Select]
OpenGL vendor string: X.Orgfor NVIDIA opensource driver:
Code: [Select]
OpenGL vendor string: nouveau
So maybe easier add the same workaround for NVIDIA Corporation like for ATI Technologies Inc.? :)
Title: Re: Shader versioning test
Post by: m!m on September 11, 2015, 11:19:09 am
@tomimaki: Thanks for providing the vendor strings. Those are really hard to identify so I just enabled the version workaround for NVIDIA which should fix the issues without the need for more shader fixing.

@The Dagger: Thanks for the log, our shaders don't really like the #version at the top. The latest commit to the PR should fix those issues but I can't compile new builds right now.
Title: Re: Shader versioning test
Post by: AdmiralRalwood on September 13, 2015, 03:43:12 pm
Downloads have been updated with fresh builds.
Title: Re: Shader versioning test
Post by: The Dagger on October 06, 2015, 12:33:34 pm
Tested the new builds, it's all good ingame and no errors are reported on the debug log (NVidia GPU).
Title: Re: Shader versioning test
Post by: Echelon9 on November 05, 2015, 10:03:36 pm
Because we have a good group of motivated OpenGL testers here, thought I'd hijack this thread to provide an update on recent fs2open renderer refactoring that I've been looking at.

Branch for code here (no binaries as yet): https://github.com/Echelon9/fs2open.github.com/tree/fix/SHADER_MODEL-concept-removal

Background

On reviewing the OpenGL GLSL setup code in fs2open, it was apparent we use the 'Use_GLSL' variable to encapsulate three different concepts. This is a mess.

At present, depending on the value, this variable is used for:

The last bullet point is important, as the code readability is improved by removing legacy references to DirectX-only concepts. A broad mapping between DirectX Shader Model versions and GLSL versions can be found here: https://www.opengl.org/wiki/Detecting_the_Shader_Model.

Implementation: Refactoring 'Use_GLSL' variable

So, in order to do something about this, I've refactored the 'Use_GLSL' global variable to properly encapsulate only one concept. The branch does this by splitting the concept of (a) the GLSL version reported by the driver, and (b) whether a command line option has been used to disable GLSL shaders all together.

The third option, -disable_glsl_model appears less relevant today than when it was added. Today, most driver and GPU combinations will happily support > OpenGL GLSL 3.30 (cf. Steam Hardware Survey). There are a small number of users on older hardware that won't support GLSL shaders at all. There are significantly less in between, with partially working driver implementations. So I've taken this opportunity to remove a little used command line option.

The #version directive, which is required for standards conformance, is added in a dynamic manner. Am hoping this works across implementations, but I don't have all the hardware to test.

I'd be keen to see how users with a variety of GPUs and drivers handle this branch. Please test away.

Particularly keen to see if errors along the lines of "varying no longer support", or "global variable gl_TexCoord is removed after version 140".

Mesa open source drivers

Importantly, this branch was developed on an Intel GPU using the Linux Mesa open source drivers. The tooling and infrastructure is frankly fantastic. I want to be clear that the support and ability to see how the driver is internally handling the shaders is very helpful, and hopefully dispel some long-held, but now unsupported negative views on recent Intel GPUs.

I'll have a follow-up post that includes performance figures benchmarking the fs2open engine shaders through various Mesa versions and their shader-db tool, to see how they have improved of late.
Title: Re: Shader versioning test
Post by: Swifty on November 06, 2015, 04:40:10 pm
An idea I had floating in my mind was to have something called gr_is_capable() which would be some sort of function other parts of the code outside of the gr_screen specific code that would refer to when trying to figure out if a particular piece of code needed to be run or not.

gr_is_capable would accept a bunch of identifier constants like "CAPABILITY_DEFERED_LIGHTING", "CAPABILITY_ENV_MAPPING", "CAPABILITY_SOFT_PARTICLES", etc and would do all the work in determining if an end system is capable of a certain graphics feature.

Internally inside the gr_opengl code (and hypothetically the gr_vulkan code), we would have a table in the backend that lists all the pre-requisite extensions and shader versions needed for a particular graphics feature. So that way, the only code that needs to really know about extensions or a shader versions would be in the code that actually deals with OpenGL.

Echelon9's stuff seems to be a good step in the right direction.
Title: Re: Shader versioning test
Post by: The E on November 06, 2015, 05:12:38 pm
Random observations from testing this:

-Apparently GL_ARB_texture_buffer_object isn't supported by the driver (I'm pretty sure that it actually is present, but not at the #version used; this is on a Radeon R9 285 using Catalyst 15.8)
-The lightshaft fragment shader uses the name "sample" for one of its temp variables, this is a keyword in glsl4 and will cause compilation issues.
Title: Re: Shader versioning test
Post by: Echelon9 on November 06, 2015, 09:35:52 pm
Thanks for looking at this The E.
-Apparently GL_ARB_texture_buffer_object isn't supported by the driver (I'm pretty sure that it actually is present, but not at the #version used; this is on a Radeon R9 285 using Catalyst 15.8)

GL_ARB_texture_buffer_object is supported at OpenGL 3.1 / GLSL 1.40. Do you mind please sharing your fs2open.log for that particular hardware?

Quote
-The lightshaft fragment shader uses the name "sample" for one of its temp variables, this is a keyword in glsl4 and will cause compilation issues.

PR #419 (https://github.com/scp-fs2open/fs2open.github.com/pull/419) has been created on Github to fix this. [Fix merged into master.] Thanks for the report!
Title: Re: Shader versioning test
Post by: Echelon9 on November 06, 2015, 09:39:49 pm
An idea I had floating in my mind was to have something called gr_is_capable() which would be some sort of function other parts of the code outside of the gr_screen specific code that would refer to when trying to figure out if a particular piece of code needed to be run or not.

gr_is_capable would accept a bunch of identifier constants like "CAPABILITY_DEFERED_LIGHTING", "CAPABILITY_ENV_MAPPING", "CAPABILITY_SOFT_PARTICLES", etc and would do all the work in determining if an end system is capable of a certain graphics feature.

Internally inside the gr_opengl code (and hypothetically the gr_vulkan code), we would have a table in the backend that lists all the pre-requisite extensions and shader versions needed for a particular graphics feature. So that way, the only code that needs to really know about extensions or a shader versions would be in the code that actually deals with OpenGL.

I agree with and like the sound of this approach. Flexibility when we get the opportunity to include a Vulkan renderer would be useful. I'll think some more about the table concept and how it could be implemented efficiently.

Quote
Echelon9's stuff seems to be a good step in the right direction.

Thanks! I'll run it through a few more local tests, and then setup a Github PR for wider code review.
Title: Re: Shader versioning test
Post by: The E on November 07, 2015, 03:23:09 am
Did some further testing. Turns out that, at least for this particular hardware/driver combo, it is not necessary to enable GL_ARB_texture_buffer_object at all. Removing that line from the main vertex shader makes it compile cleanly.

The gl_init debug stuff:
Code: [Select]
Initializing OpenGL graphics device at 1280x800 with 32-bit color...
  Initializing WGL...
  Requested WGL Video values = R: 8, G: 8, B: 8, depth: 24, stencil: 8, double-buffer: 1
  Actual WGL Video values    = R: 8, G: 8, B: 8, depth: 24, stencil: 8, double-buffer: 1
  OpenGL Vendor    : ATI Technologies Inc.
  OpenGL Renderer  : AMD Radeon R9 200 Series
  OpenGL Version   : 4.5.13399 Compatibility Profile Context 15.201.1151.1007

  Using extension "GL_EXT_fog_coord".
  Using extension "GL_ARB_multitexture".
  Using extension "GL_ARB_texture_env_add".
  Using extension "GL_ARB_texture_compression".
  Using extension "GL_EXT_texture_compression_s3tc".
  Using extension "GL_EXT_texture_filter_anisotropic".
  Using extension "GL_ARB_texture_env_combine".
  Using extension "GL_EXT_compiled_vertex_array".
  Using extension "GL_EXT_draw_range_elements".
  Using extension "GL_ARB_texture_mirrored_repeat".
  Using extension "GL_ARB_texture_non_power_of_two".
  Using extension "GL_ARB_vertex_buffer_object".
  Using extension "GL_ARB_pixel_buffer_object".
  Using extension "GL_SGIS_generate_mipmap".
  Using extension "GL_EXT_framebuffer_object".
  Using extension "GL_ARB_texture_rectangle".
  Using extension "GL_EXT_bgra".
  Using extension "GL_ARB_texture_cube_map".
  Using extension "GL_EXT_texture_lod_bias".
  Using extension "GL_ARB_point_sprite".
  Using extension "GL_ARB_shading_language_100".
  Using extension "GL_ARB_shader_objects".
  Using extension "GL_ARB_vertex_shader".
  Using extension "GL_ARB_fragment_shader".
  Using extension "GL_ARB_shader_texture_lod".
  Using extension "GL_ARB_texture_float".
  Using extension "GL_ARB_draw_elements_base_vertex".
  Using extension "GL_EXT_framebuffer_blit".
  Using extension "GL_EXT_geometry_shader4".
  Using extension "GL_EXT_texture_array".
  Using extension "GL_ARB_uniform_buffer_object".
  Using extension "GL_EXT_transform_feedback".
  Using extension "GL_ARB_draw_instanced".
  Using extension "GL_ARB_texture_buffer_object".
  Found special extension function "wglSwapIntervalEXT".

Compiling new shader:
Particle Effects
   Loading built-in default shader for: effect-v.sdr
   Loading built-in default shader for: effect-particle-f.sdr
Shader Variant Features:
Compiling new shader:
Particle Effects
   Loading built-in default shader for: effect-v.sdr
   Loading built-in default shader for: effect-particle-f.sdr
   Loading built-in default shader for: effect-screen-g.sdr
Shader Variant Features:
Geometry shader point-based particles
Compiling new shader:
Distortion Effects
   Loading built-in default shader for: effect-distort-v.sdr
   Loading built-in default shader for: effect-distort-f.sdr
Shader Variant Features:
Compiling new shader:
Deferred Lighting
   Loading built-in default shader for: deferred-v.sdr
   Loading built-in default shader for: deferred-f.sdr
Shader Variant Features:
Compiling new shader:
Clear Deferred Lighting Buffer
   Loading built-in default shader for: deferred-clear-v.sdr
   Loading built-in default shader for: deferred-clear-f.sdr
Shader Variant Features:

Compiling new shader:
Post Processing
   Loading built-in default shader for: post-v.sdr
   Loading built-in default shader for: post-f.sdr
WARNING: Unable to get shader uniform location for "depth_tex"!
WARNING: Unable to get shader uniform location for "timer"!
Shader Variant Features:
Compiling new shader:
Bloom Brightpass
   Loading built-in default shader for: post-v.sdr
   Loading built-in default shader for: brightpass-f.sdr
Shader Variant Features:
Compiling new shader:
Gaussian Blur
   Loading built-in default shader for: post-v.sdr
   Loading built-in default shader for: blur-f.sdr
Shader Variant Features:
Horizontal blur pass
Compiling new shader:
Gaussian Blur
   Loading built-in default shader for: post-v.sdr
   Loading built-in default shader for: blur-f.sdr
Shader Variant Features:
Vertical blur pass
Compiling new shader:
FXAA
   Loading built-in default shader for: fxaa-v.sdr
   Loading built-in default shader for: fxaa-f.sdr
Shader Variant Features:
Compiling new shader:
FXAA Prepass
   Loading built-in default shader for: post-v.sdr
   Loading built-in default shader for: fxaapre-f.sdr
Shader Variant Features:
  Max texture units: 8 (32)
  Max client texture states: 8 (16)
  Max elements vertices: 2147483647
  Max elements indices: 16777215
  Max texture size: 16384x16384
  Max render buffer size: 16384x16384
  Can use compressed textures: YES
  Texture compression available: YES
  Post-processing enabled: YES
  Using trilinear texture filter.
  OpenGL Shader Version: 4.40
... OpenGL init is complete!

Title: Re: Shader versioning test
Post by: Talon 1024 on November 08, 2015, 03:03:20 am
I've just tried compiling a build from the repo linked from this post (http://www.hard-light.net/forums/index.php?topic=90348.msg1801982#msg1801982), and I don't think shader compilation is working with my setup. Some ship models, as well as the starfield.pof, will not move from the mission origin point; some ship models don't appear at all; The only parts of the ship that moves from the origin point are glowpoints; black or white boxes appear around thruster FX; and debris pieces will randomly appear or disappear, depending on how close the camera is to said debris piece.

Stuff on Google Drive (https://drive.google.com/folderview?id=0B01hVcJFK8KEbFBEem5nWnpDeE0&usp=sharing). Contains my fs2_open.log, glxinfo output, and some screenshots taken from the release build. I am unable to start playing if I use the debug build.
Title: Re: Shader versioning test
Post by: Echelon9 on November 08, 2015, 03:13:57 am
Did some further testing. Turns out that, at least for this particular hardware/driver combo, it is not necessary to enable GL_ARB_texture_buffer_object at all. Removing that line from the main vertex shader makes it compile cleanly.

The gl_init debug stuff:
Code: [Select]
<snip>

On review of the shader in globalincs/def_files.cpp it is a bit odd that we request GL_ARB_texture_buffer_object as an extension unconditionally, when there is core support as of OpenGL 3.1 - GLSL 1.40.

Perhaps we should wrap "#extension GL_ARB_texture_buffer_object : enable" in a #if __VERSION__ < 140?
Title: Re: Shader versioning test
Post by: Echelon9 on November 08, 2015, 03:22:37 am
Stuff on Google Drive (https://drive.google.com/folderview?id=0B01hVcJFK8KEbFBEem5nWnpDeE0&usp=sharing). Contains my fs2_open.log, glxinfo output, and some screenshots taken from the release build. I am unable to start playing if I use the debug build.

Thanks talon 1024, yes the shader compilation on NVIDIA's proprietary blob drivers is failing where we supply pre-GLSL 140 features and you have a modern GLSL 140+ card i.e.
Code: [Select]
  OpenGL Vendor    : NVIDIA Corporation
  OpenGL Renderer  : GeForce GTX 970/PCIe/SSE2
  OpenGL Version   : 4.5.0 NVIDIA 352.41
...
Compiling new shader:
Particle Effects
   Loading built-in default shader for: effect-v.sdr
   Loading built-in default shader for: effect-particle-f.sdr
Vertex shader failed to compile:
0(3) : warning C7555: 'attribute' is deprecated, use 'in/out' instead
0(9) : warning C7555: 'varying' is deprecated, use 'in/out' instead
0(10) : warning C7555: 'varying' is deprecated, use 'in/out' instead
0(24) : error C7616: global function ftransform is removed after version 140
0(25) : error C7616: global variable gl_ModelViewMatrix is removed after version 140
0(25) : error C7616: global variable gl_Vertex is removed after version 140
0(30) : error C7616: global variable gl_TexCoord is removed after version 140
0(30) : error C7616: global variable gl_MultiTexCoord0 is removed after version 140
0(31) : warning C7533: global variable gl_FrontColor is deprecated after version 120
0(31) : error C7616: global variable gl_Color is removed after version 140
0(32) : warning C7533: global variable gl_FrontSecondaryColor is deprecated after version 120
0(35) : error C7616: global variable gl_ClipVertex is removed after version 140

Given you presumably can play with a master release build, this refactor has identified some limitations with how scalable our shaders are at present.
Title: Re: Shader versioning test
Post by: m!m on November 08, 2015, 04:45:50 am
Those issues are similar to what I encountered when trying to implement the driver blacklist. As we don't support any OpenGL version beyond 2.1 the biggest #version directive we can use is #version 120. The rest of the shader code then needs to be adjusted to only use features available in that language version. The features from OpenGL 3 could be available via extensions but I am not sure how that would work.
Title: Re: Shader versioning test
Post by: Echelon9 on November 20, 2015, 05:36:09 am
Would talon and others on this thread re-test my PR branch here: https://github.com/scp-fs2open/fs2open.github.com/pull/425

I've removed the most problematic change, so hopeful this will aid merging the remaining valuable refactors.
Title: Re: Shader versioning test
Post by: Talon 1024 on November 20, 2015, 05:34:02 pm
I don't appear to be having any problems with these builds. Here are some debug logs:

This one is from "A Game of TAG" (https://drive.google.com/file/d/0B01hVcJFK8KEWGhCS1VNdDRvWWc/view?usp=sharing), and this one is from "A Lion at the Door" (https://drive.google.com/file/d/0B01hVcJFK8KEQmN6dzg4V1F1NW8/view?usp=sharing)
Title: Re: Shader versioning test
Post by: Echelon9 on November 24, 2015, 06:24:37 pm
Thanks. A favourable report is great to hear (and expected given the reduced set of refactors now present in the PR).
Title: Re: Shader versioning test
Post by: The E on November 25, 2015, 03:22:14 am
Actually, you do have problems. The FXAA shader failed to compile.

Code: [Select]
Compiling new shader:
FXAA
   Loading built-in default shader for: fxaa-v.sdr
   Loading built-in default shader for: fxaa-f.sdr
Fragment shader failed to compile:
0(201) : warning C7506: OpenGL does not define the global type float4
0(203) : warning C7506: OpenGL does not define the global type float2
0(204) : error C0000: syntax error, unexpected ',', expecting "::" at token ","
0(249) : error C0000: syntax error, unexpected reserved word "if" at token "if"
0(286) : error C0000: syntax error, unexpected reserved word "if" at token "if"
0(296) : error C0000: syntax error, unexpected reserved word "if" at token "if"
0(297) : warning C7506: OpenGL does not define the global function saturate
0(299) : error C0000: syntax error, unexpected '.', expecting "::" at token "."
0(302) : error C0000: syntax error, unexpected '.', expecting "::" at token "."
0(307) : error C0000: syntax error, unexpected '.', expecting "::" at token "."
0(310) : error C0000: syntax error, unexpected '.', expecting "::" at token "."
0(316) : error C0000: syntax error, unexpected reserved word "if" at token "if"
0(321) : error C0000: syntax error, unexpected "-=", expecting "::" at token "-="
0(325) : error C0000: syntax error, unexpected reserved word "if" at token "if"
0(328) : error C0000: syntax error, unexpected reserved word "if" at token "if"
0(495) : error C0000: syntax error, unexpected reserved word "if" at token "if"
0(509) : error C0000: syntax error, unexpected reserved word "if" at token "if"
0(520) : error C1121: main: function type parameters not allowed

ERROR! Unable to create fragment shader!
Error while compiling FXAA shaders. FXAA will be unavailable.
Title: Re: Shader versioning test
Post by: Echelon9 on November 26, 2015, 05:42:52 pm
Actually, you do have problems. The FXAA shader failed to compile.

Code: [Select]
Compiling new shader:
FXAA
   Loading built-in default shader for: fxaa-v.sdr
   Loading built-in default shader for: fxaa-f.sdr
Fragment shader failed to compile:
0(201) : warning C7506: OpenGL does not define the global type float4
0(203) : warning C7506: OpenGL does not define the global type float2
0(204) : error C0000: syntax error, unexpected ',', expecting "::" at token ","
0(249) : error C0000: syntax error, unexpected reserved word "if" at token "if"
0(286) : error C0000: syntax error, unexpected reserved word "if" at token "if"
0(296) : error C0000: syntax error, unexpected reserved word "if" at token "if"
0(297) : warning C7506: OpenGL does not define the global function saturate
0(299) : error C0000: syntax error, unexpected '.', expecting "::" at token "."
0(302) : error C0000: syntax error, unexpected '.', expecting "::" at token "."
0(307) : error C0000: syntax error, unexpected '.', expecting "::" at token "."
0(310) : error C0000: syntax error, unexpected '.', expecting "::" at token "."
0(316) : error C0000: syntax error, unexpected reserved word "if" at token "if"
0(321) : error C0000: syntax error, unexpected "-=", expecting "::" at token "-="
0(325) : error C0000: syntax error, unexpected reserved word "if" at token "if"
0(328) : error C0000: syntax error, unexpected reserved word "if" at token "if"
0(495) : error C0000: syntax error, unexpected reserved word "if" at token "if"
0(509) : error C0000: syntax error, unexpected reserved word "if" at token "if"
0(520) : error C1121: main: function type parameters not allowed

ERROR! Unable to create fragment shader!
Error while compiling FXAA shaders. FXAA will be unavailable.

Thanks for testing. Can you please provide the section of the fs2open.log that includes the OpenGL renderer string (i.e. need to know what OS, graphics card, and driver).
Title: Re: Shader versioning test
Post by: Talon 1024 on November 26, 2015, 09:17:39 pm
OS: Ubuntu Linux 15.10
Code: [Select]
OpenGL Vendor    : NVIDIA Corporation
OpenGL Renderer  : GeForce GTX 970/PCIe/SSE2
OpenGL Version   : 4.5.0 NVIDIA 352.63

BTW, the post above from The E was about the log that I had posted. FXAA is barely noticeable, so that's why I said I didn't appear to have any problems, despite the fact the FXAA shader failed to compile.
Title: Re: Shader versioning test
Post by: tomimaki on November 27, 2015, 09:17:13 am
Results on Ubuntu 15.10, GPU 6650M, driver Catalyst and Mesa.
Looks ok on Catalyst (FXAA shader fails), on Mesa it crash if I try to go to Tech Room or F3 Lab.

[attachment DELETED!! by Strong Bad]
Title: Re: Shader versioning test
Post by: Echelon9 on November 27, 2015, 11:53:08 pm
Thanks for all the testing, and logs reported. Overall it is progressing well.


[UPDATE]  To resolve the FXAA shader compile error, I've pushed a further commit to my testing branch. Lift up FXAA shader GLSL version check to opengl_post_load_shader(), out of the GLSL shader compiler preprocessor stage. (https://github.com/Echelon9/fs2open.github.com/commit/955b7c162d25507048e46f136ed057cb800ff811) Could those who saw this issue please retest?
Title: Re: Shader versioning test
Post by: tomimaki on November 28, 2015, 01:02:55 pm
Yep, FXAA works.

[attachment DELETED!! by Strong Bad]
Title: Re: Shader versioning test
Post by: The E on November 28, 2015, 01:14:57 pm
The lightshaft post process fragment shader uses "sample" as a variable name, this needs to be fixed too.

Code: [Select]
Compiling new shader:
Lightshafts
   Loading built-in default shader for: post-v.sdr
   Loading built-in default shader for: ls-f.sdr
Fragment shader failed to compile:
Fragment shader failed to compile with the following errors:
ERROR: 0:26: error(#132) Syntax error: "sample" parse error
ERROR: error(#273) 1 compilation errors.  No code generated


ERROR! Unable to create fragment shader!
ASSERTION: "handle >= 0" at gropenglshader.cpp:214
Freeing all existing models...
Title: Re: Shader versioning test
Post by: Echelon9 on December 01, 2015, 11:02:53 pm
Thanks for the lightshaft GLSL shader 'sample' error report.

The incorrect use of reserved keyword 'sample' was fixed in this commit (https://github.com/scp-fs2open/fs2open.github.com/commit/697c900a25bad266aab3e3f31d1f2ea1fd025a6b), so perhaps you need to refresh your local checkout of my PR?

Overall, this refactor is looking promising given positive feedback from a broad range of testers (and it's an added bonus where it is uncovering some other long latent bugs in our shaders).
Title: Re: Shader versioning test
Post by: Talon 1024 on December 22, 2015, 06:59:34 am
I just compiled and tested a brand new build from the latest revision of the GitHub repo, and I am getting no shader compilation errors on my 64-bit Ubuntu 15.10 setup with an NVidia GeForce GTX 970.
Title: Re: Shader versioning test
Post by: Echelon9 on January 03, 2016, 03:42:04 pm
Thanks for everyone's testing.

I'm comfortable with the variety of test coverage for the Pull Request to go forward for merge. If there are any further small issues shaken out, I'll address them in a later commit.

My next step will be to consider if we can adopt any of the dropped changes from this patch set with some further tweaks.

Update: Now merged into master. This refactor will be present in the next major Freespace 2 Open engine release.
Title: Re: Shader versioning test
Post by: AdmiralRalwood on January 17, 2016, 03:11:21 am
/me wonders if this means the AMD drivers would work with #version 140
So after a roundabout trip, it turns out the answer is "yes, if you also enable the GL_ARB_compatibility extension".

I tried valiantly to find a set of directives that would allow the Windows AMD drivers to work with a "#version 120" directive; one does not exist. The problem appears to be that it's lying about GL_ARB_texture_buffer_object support. The fs2_open.log clearly says this:
Code: [Select]
  Using extension "GL_ARB_texture_buffer_object".
But when it comes time to actually compile the shaders, we get this:
Code: [Select]
warning(#62) enable/warn/disable extension is not found. Extension "GL_ARB_texture_buffer_object" is not supported

Without a #version directive, this doesn't matter because the thing's basically running in maximum compatibility mode (instead of assuming GLSL 1.1, in clear violation of the spec but supporting the maximum number of shaders out-of-the-box, presumably), and the functionality of the GL_ARB_texture_buffer_object extension entered the core, with the bits we use in particular being added in... GLSL 1.4.

So yes, adding a #version 140 directive (plus using GL_ARB_compatibility to still have access to the deprecated pre-OpenGL 3.1 functionality) does, in fact, allow our current default shaders to compile on the Windows AMD drivers.

But we still can't apply the proper #version 120 directive to every system, and since Mesa drivers don't support the compatibility profile past OpenGL 3.0/GLSL 1.3, we can't just use GLSL 1.4 Compatibility on all platforms either.