Page MenuHomeWildfire Games

Removes GLSL shaders code duplication for fog and shadows
ClosedPublic

Authored by vladislavbelov on Tue, Jan 12, 8:43 PM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP24598: Removes GLSL shaders code duplication for fog and shadows
Summary

Applies usages of the introduced #include directive. Significantly reduces code duplication for fog and shadows.

Test Plan
  1. Apply patch
  2. Make sure that objects with changed shaders look the same (w/ and w/o fog/shadows): water, terrain, models, particles, models with water, waterfalls.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vladislavbelov created this revision.Tue, Jan 12, 8:43 PM

Build is green

builderr-debug-macos.txt
../../../source/graphics/ShaderProgram.cpp:86: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:118: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:128:15: warning: 'Unbind' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
        virtual void Unbind()
                     ^
../../../source/graphics/ShaderProgram.h:140:

See https://jenkins.wildfiregames.com/job/macos-differential/2809/display/redirect for more details.

vladislavbelov requested review of this revision.Tue, Jan 12, 9:10 PM
Stan added a subscriber: Stan.Wed, Jan 13, 12:21 AM
Stan added inline comments.
binaries/data/mods/public/shaders/glsl/model_common.fs
3 ↗(On Diff #15209)

Missing the common folder?

228 ↗(On Diff #15209)

Why nuke macro? Is it unused?

binaries/data/mods/public/shaders/glsl/model_common.vs
176 ↗(On Diff #15209)

Same

binaries/data/mods/public/shaders/glsl/model_water.vs
63 ↗(On Diff #15209)

^

binaries/data/mods/public/shaders/glsl/model_waterfall.vs
46 ↗(On Diff #15209)

^

vladislavbelov marked an inline comment as done.

Fixes notes.

binaries/data/mods/public/shaders/glsl/model_common.fs
228 ↗(On Diff #15209)

No, it's moved in the function. It reduces the code duplication. Also we should think as a complete renderer as much possible to make it simpler to understand.

Stan added a comment.Wed, Jan 13, 12:44 AM

Should h files get copyright headers?
This patch makes the code much cleaner at least.

binaries/data/mods/public/shaders/glsl/common/shadows_fragment.h
4 ↗(On Diff #15220)

Why idented below, why don't do this elsewhere?

binaries/data/mods/public/shaders/glsl/model_common.fs
228 ↗(On Diff #15209)

Function call inexpensive?

Build is green

builderr-debug-macos.txt
../../../source/graphics/ShaderProgram.cpp:86: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:118: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:128:15: warning: 'Unbind' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
        virtual void Unbind()
                     ^
../../../source/graphics/ShaderProgram.h:140:

See https://jenkins.wildfiregames.com/job/macos-differential/2817/display/redirect for more details.

In D3340#148586, @Stan wrote:

Should h files get copyright headers?

I'm not sure, shaders don't have copyright headers.

binaries/data/mods/public/shaders/glsl/common/shadows_fragment.h
4 ↗(On Diff #15220)

Just copy, we have no standardized way to format nested macros.

binaries/data/mods/public/shaders/glsl/model_common.fs
228 ↗(On Diff #15209)

There is no function calling in core GLSL (without extensions), the whole code will be inlined, as well as no recursion, because of performance reasons.

This revision was not accepted when it landed; it landed in state Needs Review.Wed, Jan 13, 9:44 PM
This revision was automatically updated to reflect the committed changes.