Page MenuHomeWildfire Games

Adds #include directive support to shaders
Needs ReviewPublic

Authored by vladislavbelov on Sun, Oct 11, 3:10 PM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Trac Tickets
#3640
Summary

The #include directive allows to significantly reduce size of the shader code duplication.

There was a try to implement #include: https://trac.wildfiregames.com/attachment/ticket/3640/preprocessor_include_with_hotload_impl.patch. But it was incorrect in terms of macro-context, also it has some design and leak issues. The current patch was inspired by that one though.

The patch doesn't change Ogre code to reduce third_party changes, and it completely follows original Ogre approach. They use the same technique to parse #include directives before the main preprocessor pass: https://github.com/OGRECave/ogre/blob/155b2385de92e81ce7688888ff327d7d27992e45/RenderSystems/GLSupport/src/GLSL/OgreGLSLShaderCommon.cpp#L82, _resolveIncludes: https://github.com/OGRECave/ogre/blob/155b2385de92e81ce7688888ff327d7d27992e45/OgreMain/src/OgreHighLevelGpuProgram.cpp#L263

The algorithm is similar but allows some white space padding (like for a usual preprocessor directive).

Test Plan
  1. Apply the patch and compile the game and tests
  2. Make sure that tests are passed
  3. Run the game and make sure that there is no shader error (for GLSL and ARB shaders)

Event Timeline

vladislavbelov created this revision.Sun, Oct 11, 3:10 PM
vladislavbelov edited the summary of this revision. (Show Details)Sun, Oct 11, 3:13 PM

Successful build - Chance fights ever on the side of the prudent.

builderr-debug-macos.txt
../../../source/graphics/ShaderProgram.cpp:85:15: warning: 'Reload' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
        virtual void Reload()
                     ^
../../../source/graphics/ShaderProgram.h:124:15: note: overridden virtual function is here
        virtual void Reload() = 0;
                     ^
../../../source/graphics/ShaderProgram.cpp:120:15: warning: 'Bind' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
        virtual void Bind()
                     ^
../../../source/graphics/ShaderProgram.h:135:15: note: overridden virtual function is here
        virtual void Bind() = 0;
                     ^
../../../source/graphics/ShaderProgram.cpp:130:15: warning: 'Unbind' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
        virtual void Unbind()
                     ^
../../../source/graphics/ShaderProgram.h:140:15: note: overridden virtual function is here
        virtual void Unbind() = 0;
                     ^
../../../source/graphics/ShaderProgram.cpp:158:18: warning: 'GetTextureBinding' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
        virtual Binding GetTextureBinding(texture_id_t id)
                        ^
../../../source/graphics/ShaderProgram.h:149:18: note: overridden virtual function is here
        virtual Binding GetTextureBinding(texture_id_t id) = 0;
                        ^
../../../source/graphics/ShaderProgram.cpp:168:15: warning: 'BindTexture' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
        virtual void BindTexture(texture_id_t id, Handle tex)
                     ^
../../../source/graphics/ShaderProgram.h:153:15: note: overridden virtual function is here
        virtual void BindTexture(texture_id_t id, Handle tex) = 0;
                     ^
../../../source/graphics/ShaderProgram.cpp:181:15: warning: 'BindTexture' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
        virtual void BindTexture(texture_id_t id, GLuint tex)
                     ^
../../../source/graphics/ShaderProgram.h:154:15: note: overridden virtual function is here
        virtual void BindTexture(texture_id_t id, GLuint tex) = 0;
                     ^
../../../source/graphics/ShaderProgram.cpp:192:15: warning: 'BindTexture' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
        virtual void BindTexture(Binding id, Handle tex)
                     ^
../../../source/graphics/ShaderProgram.h:155:15: note: overridden virtual function is here
        virtual void BindTexture(Binding id, Handle tex) = 0;
                     ^
../../../source/graphics/ShaderProgram.cpp:199:18: warning: 'GetUniformBinding' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
        virtual Binding GetUniformBinding(uniform_id_t id)
                        ^
../../../source/graphics/ShaderProgram.h:158:18: note: overridden virtual function is here
        virtual Binding GetUniformBinding(uniform_id_t id) = 0;
                        ^
../../../source/graphics/ShaderProgram.cpp:204:15: warning: 'Uniform' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
        virtual void Uniform(Binding id, float v0, float v1, float v2, float v3)
                     ^
../../../source/graphics/ShaderProgram.h:161:15: note: overridden virtual function is here
        virtual void Uniform(Binding id, float v0, float v1, float v2, float v3) = 0;
                     ^
../../../source/graphics/ShaderProgram.cpp:213:15: warning: 'Uniform' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
        virtual void Uniform(Binding id, const CMatrix3D& v)
                     ^
../../../source/graphics/ShaderProgram.h:162:15: note: overridden virtual function is here
        virtual void Uniform(Binding id, const CMatrix3D& v) = 0;
                     ^
../../../source/graphics/ShaderProgram.cpp:232:15: warning: 'Uniform' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
        virtual void Uniform(Binding id, size_t count, const CMatrix3D* v)
                     ^
../../../source/graphics/ShaderProgram.h:163:15: note: overridden virtual function is here
        virtual void Uniform(Binding id, size_t count, const CMatrix3D* v) = 0;
                     ^
11 warnings generated.
In file included from ../../../source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.cpp:17:
/Users/wfg/Jenkins/workspace/macos-differential/source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h:202:17: warning: unused parameter 'includePath' [-Wunused-parameter]
                                const CStr& includePath, CStr& out) {
                                            ^
/Users/wfg/Jenkins/workspace/macos-differential/source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h:219:17: warning: unused parameter 'includePath' [-Wunused-parameter]
                                const CStr& includePath, CStr& out) {
                                            ^
/Users/wfg/Jenkins/workspace/macos-differential/source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h:219:36: warning: unused parameter 'out' [-Wunused-parameter]
                                const CStr& includePath, CStr& out) {
                                                               ^
/Users/wfg/Jenkins/workspace/macos-differential/source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h:233:17: warning: unused parameter 'includePath' [-Wunused-parameter]
                                const CStr& includePath, CStr& out) {
                                            ^
4 warnings generated.
builderr-release-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libsimulation2.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libengine.a(precompiled.o) has no symbols
../../../source/graphics/ShaderProgram.cpp:85:15: warning: 'Reload' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
        virtual void Reload()
                     ^
../../../source/graphics/ShaderProgram.h:124:15: note: overridden virtual function is here
        virtual void Reload() = 0;
                     ^
../../../source/graphics/ShaderProgram.cpp:120:15: warning: 'Bind' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
        virtual void Bind()
                     ^
../../../source/graphics/ShaderProgram.h:135:15: note: overridden virtual function is here
        virtual void Bind() = 0;
                     ^
../../../source/graphics/ShaderProgram.cpp:130:15: warning: 'Unbind' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
        virtual void Unbind()
                     ^
../../../source/graphics/ShaderProgram.h:140:15: note: overridden virtual function is here
        virtual void Unbind() = 0;
                     ^
../../../source/graphics/ShaderProgram.cpp:158:18: warning: 'GetTextureBinding' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
        virtual Binding GetTextureBinding(texture_id_t id)
                        ^
../../../source/graphics/ShaderProgram.h:149:18: note: overridden virtual function is here
        virtual Binding GetTextureBinding(texture_id_t id) = 0;
                        ^
../../../source/graphics/ShaderProgram.cpp:168:15: warning: 'BindTexture' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
        virtual void BindTexture(texture_id_t id, Handle tex)
                     ^
../../../source/graphics/ShaderProgram.h:153:15: note: overridden virtual function is here
        virtual void BindTexture(texture_id_t id, Handle tex) = 0;
                     ^
../../../source/graphics/ShaderProgram.cpp:181:15: warning: 'BindTexture' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
        virtual void BindTexture(texture_id_t id, GLuint tex)
                     ^
../../../source/graphics/ShaderProgram.h:154:15: note: overridden virtual function is here
        virtual void BindTexture(texture_id_t id, GLuint tex) = 0;
                     ^
../../../source/graphics/ShaderProgram.cpp:192:15: warning: 'BindTexture' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
        virtual void BindTexture(Binding id, Handle tex)
                     ^
../../../source/graphics/ShaderProgram.h:155:15: note: overridden virtual function is here
        virtual void BindTexture(Binding id, Handle tex) = 0;
                     ^
../../../source/graphics/ShaderProgram.cpp:199:18: warning: 'GetUniformBinding' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
        virtual Binding GetUniformBinding(uniform_id_t id)
                        ^
../../../source/graphics/ShaderProgram.h:158:18: note: overridden virtual function is here
        virtual Binding GetUniformBinding(uniform_id_t id) = 0;
                        ^
../../../source/graphics/ShaderProgram.cpp:204:15: warning: 'Uniform' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
        virtual void Uniform(Binding id, float v0, float v1, float v2, float v3)
                     ^
../../../source/graphics/ShaderProgram.h:161:15: note: overridden virtual function is here
        virtual void Uniform(Binding id, float v0, float v1, float v2, float v3) = 0;
                     ^
../../../source/graphics/ShaderProgram.cpp:213:15: warning: 'Uniform' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
        virtual void Uniform(Binding id, const CMatrix3D& v)
                     ^
../../../source/graphics/ShaderProgram.h:162:15: note: overridden virtual function is here
        virtual void Uniform(Binding id, const CMatrix3D& v) = 0;
                     ^
../../../source/graphics/ShaderProgram.cpp:232:15: warning: 'Uniform' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
        virtual void Uniform(Binding id, size_t count, const CMatrix3D* v)
                     ^
../../../source/graphics/ShaderProgram.h:163:15: note: overridden virtual function is here
        virtual void Uniform(Binding id, size_t count, const CMatrix3D* v) = 0;
                     ^
11 warnings generated.
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libgraphics.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libatlas.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libgui.a(precompiled.o) has no symbols
In file included from ../../../source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.cpp:17:
/Users/wfg/Jenkins/workspace/macos-differential/source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h:202:17: warning: unused parameter 'includePath' [-Wunused-parameter]
                                const CStr& includePath, CStr& out) {
                                            ^
/Users/wfg/Jenkins/workspace/macos-differential/source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h:219:17: warning: unused parameter 'includePath' [-Wunused-parameter]
                                const CStr& includePath, CStr& out) {
                                            ^
/Users/wfg/Jenkins/workspace/macos-differential/source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h:219:36: warning: unused parameter 'out' [-Wunused-parameter]
                                const CStr& includePath, CStr& out) {
                                                               ^
/Users/wfg/Jenkins/workspace/macos-differential/source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h:233:17: warning: unused parameter 'includePath' [-Wunused-parameter]
                                const CStr& includePath, CStr& out) {
                                            ^
4 warnings generated.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1638/display/redirect

vladislavbelov edited the test plan for this revision. (Show Details)Sun, Oct 11, 3:33 PM

Successful build - Chance fights ever on the side of the prudent.

builderr-debug-gcc6.txt
In file included from ../../../source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.cpp:17:0:
/zpool0/gcc6/source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h: In lambda function:
/zpool0/gcc6/source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h:202:17: warning: unused parameter 'includePath' [-Wunused-parameter]
     const CStr& includePath, CStr& out) {
                 ^~~~~~~~~~~
/zpool0/gcc6/source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h: In lambda function:
/zpool0/gcc6/source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h:219:17: warning: unused parameter 'includePath' [-Wunused-parameter]
     const CStr& includePath, CStr& out) {
                 ^~~~~~~~~~~
/zpool0/gcc6/source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h:219:36: warning: unused parameter 'out' [-Wunused-parameter]
     const CStr& includePath, CStr& out) {
                                    ^~~
/zpool0/gcc6/source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h: In lambda function:
/zpool0/gcc6/source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h:233:17: warning: unused parameter 'includePath' [-Wunused-parameter]
     const CStr& includePath, CStr& out) {
                 ^~~~~~~~~~~
builderr-release-gcc6.txt
In file included from ../../../source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.cpp:17:0:
/zpool0/gcc6/source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h: In lambda function:
/zpool0/gcc6/source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h:202:17: warning: unused parameter 'includePath' [-Wunused-parameter]
     const CStr& includePath, CStr& out) {
                 ^~~~~~~~~~~
/zpool0/gcc6/source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h: In lambda function:
/zpool0/gcc6/source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h:219:17: warning: unused parameter 'includePath' [-Wunused-parameter]
     const CStr& includePath, CStr& out) {
                 ^~~~~~~~~~~
/zpool0/gcc6/source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h:219:36: warning: unused parameter 'out' [-Wunused-parameter]
     const CStr& includePath, CStr& out) {
                                    ^~~
/zpool0/gcc6/source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h: In lambda function:
/zpool0/gcc6/source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h:233:17: warning: unused parameter 'includePath' [-Wunused-parameter]
     const CStr& includePath, CStr& out) {
                 ^~~~~~~~~~~

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/3290/display/redirect

vladislavbelov requested review of this revision.Sun, Oct 11, 3:50 PM
Stan added a subscriber: Stan.Sun, Oct 11, 4:05 PM

You should probably add them to the credits if you reused code from the patch? Seems fair. I guess you might want to link that revision in the ticket and vice versa. It also fixes it since elif was implemented?

source/graphics/PreprocessorWrapper.cpp
39

std::find maybe ? or indexOf -1 ?

225

const ?

source/graphics/ShaderProgram.cpp
444

Warning?

In D3030#132999, @Stan wrote:

You should probably add them to the credits if you reused code from the patch?

Mikita Hradovich? Does he agree to be mentioned?

I guess you might want to link that revision in the ticket and vice versa.

Revision of what?

It also fixes it since elif was implemented?

elif changes are independent.

source/graphics/PreprocessorWrapper.cpp
39

std::endl isn't a character, it's I/O manipulator.

std::find might work, there is no indexOf in CStr.

225

It returns char*, but might be const here in-place.

source/graphics/ShaderProgram.cpp
444

It will be generated inside, and it also will be detailed in the preprocessor.

Stan added a comment.Sun, Oct 11, 4:37 PM
In D3030#132999, @Stan wrote:

You should probably add them to the credits if you reused code from the patch?

Mikita Hradovich? Does he agree to be mentioned?

Well you use his code, so dunno ^^

I guess you might want to link that revision in the ticket and vice versa.

Revision of what?

The diff in the patch field of the trac ticket

It also fixes it since elif was implemented?

elif changes are independent.

I mean that since ELIF was done, the ticket can probably be closed after this patch

While building:

In file included from ../../../source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.cpp:17:0:
/media/,,,/0AD/SVN/source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h: In lambda function:
/media/,,,/0AD/SVN/source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h:202:17: warning: unused parameter ‘includePath’ [-Wunused-parameter]
     const CStr& includePath, CStr& out) {
                 ^~~~~~~~~~~
/media/,,,/0AD/SVN/source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h: In lambda function:
/media/,,,/0AD/SVN/source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h:219:17: warning: unused parameter ‘includePath’ [-Wunused-parameter]
     const CStr& includePath, CStr& out) {
                 ^~~~~~~~~~~
/media/,,,/0AD/SVN/source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h:219:36: warning: unused parameter ‘out’ [-Wunused-parameter]
     const CStr& includePath, CStr& out) {
                                    ^~~
/media/,,,/0AD/SVN/source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h: In lambda function:
/media/,,,/0AD/SVN/source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h:233:17: warning: unused parameter ‘includePath’ [-Wunused-parameter]
     const CStr& includePath, CStr& out) {
                 ^~~~~~~~~~~

In-game no oddities, tests are green.

Stan added inline comments.Mon, Oct 12, 10:00 AM
source/graphics/PreprocessorWrapper.h
59

Do you need ordering?

vladislavbelov added inline comments.Mon, Oct 12, 9:27 PM
source/graphics/PreprocessorWrapper.h
59

No, but afaik we don't have std::hash for CStr, so I'm planning to add that in a separate patch.