Page MenuHomeWildfire Games
Differential D1954

Do not send Reflection and Refraction matrices to shader when not needed
ClosedPublic
Actions

Authored by Silier on Jun 5 2019, 11:43 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Sep 12, 9:52 PM
Unknown Object (File)
Mon, Sep 9, 9:43 AM
Unknown Object (File)
Mon, Sep 9, 1:41 AM
Unknown Object (File)
Wed, Aug 28, 5:05 PM
Unknown Object (File)
Sun, Aug 25, 7:42 PM
Subscribers
gameboy
Stan
vladislavbelov
wraitii
Restricted Owners Package

Details

Reviewers
wraitii
vladislavbelov
Commits
rP22971: Do not send Reflection and Refraction matrices to shader when not needed
Summary

We should avoid to send not needed data into shaders as much as possible.

Test Plan

enable reflection/refraction
move camera
disable them
move camera
enable them
check that effects are correctly applied

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 7776
Build 12660: Vulcan BuildJenkins
Build 12659: arc lint + arc unit

Event Timeline

Silier created this revision.Jun 5 2019, 11:43 AM
Owners added a subscriber: Restricted Owners Package.Jun 5 2019, 11:43 AM
Vulcan added a comment.Jun 5 2019, 11:45 AM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/1609/display/redirect

Harbormaster completed remote builds in B7776: Diff 8316.Jun 5 2019, 11:45 AM
wraitii requested changes to this revision.EditedJun 5 2019, 1:32 PM
wraitii added a subscriber: wraitii.
  • Indenting the water shader is wrong (the #ifs don't necessarily correspond to real ifs)

Edit: got confused about one point.

This revision now requires changes to proceed.Jun 5 2019, 1:32 PM
Silier added a comment.EditedJun 5 2019, 2:18 PM

I would leave indenting, because it is easier to read.
Used already in this file and some others: terrain_common.vs, terrain_common.fs, model_waterfall.vs, model_waterfall.fs, model_water.fs::get_shadow, model_common.*

wraitii resigned from this revision.Jun 5 2019, 2:28 PM

Resigning to un-request revision (hopefully)

This revision now requires review to proceed.Jun 5 2019, 2:28 PM
Stan added a subscriber: Stan.Jun 5 2019, 2:39 PM

Angen could just have requested review :)

vladislavbelov commandeered this revision.Jun 5 2019, 4:24 PM
vladislavbelov added a reviewer: Silier.
vladislavbelov added a subscriber: vladislavbelov.

I agree that it's harder to read the code without indents for some people. Only objection that I'm thinking about, if you remove #if with leaving the code you need to fix the indents again that also means you rewrite the blame again.

vladislavbelov added a comment.EditedJun 5 2019, 4:24 PM

WAT, why I commandeered it? Sorry anyway! @Angen could you take it back?

Silier commandeered this revision.Jun 5 2019, 4:26 PM
Silier edited reviewers, added: vladislavbelov; removed: Silier.
wraitii added a comment.Jun 5 2019, 4:33 PM

@Angen The "real" indentation of the if L315-330 conflicts with the #if indentation. That's one reason not to indent I think.

Silier updated this revision to Diff 8324.Jun 5 2019, 5:44 PM
Silier added inline comments.
binaries/data/mods/public/shaders/glsl/water_high.fs
269

just an idea to unify some lines

wraitii added inline comments.Jun 5 2019, 5:49 PM
binaries/data/mods/public/shaders/glsl/water_high.fs
272

Why is this removed?

Vulcan added a comment.Jun 5 2019, 6:13 PM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/1615/display/redirect

Harbormaster completed remote builds in B7784: Diff 8324.Jun 5 2019, 6:13 PM
wraitii added a comment.Jun 5 2019, 7:04 PM

Will test and commit after, this is a good idea

binaries/data/mods/public/shaders/glsl/water_high.fs
272

ok so it's merged with the same code below. LGTM I suppose.

gameboy added a subscriber: gameboy.Jun 8 2019, 10:17 AM

@Angen How is this going, my friend? Is there a good solution released? I can test it for you.

Silier added a comment.Jun 8 2019, 10:37 AM

@gameboy not related to water problem

gameboy added a comment.Jun 8 2019, 11:06 AM

@Angen When will there be a solution to the water problem?

Silier added a comment.Jun 8 2019, 11:17 AM

Vladislav told he will look into his older patch, I tried to but cannot solve it, unless we remove something but it might lower performance.

gameboy added a comment.Jun 8 2019, 11:23 AM

@Angen If you can launch this patch that may degrade performance, let me test it. I want to test this patch to help you.

Stan added a comment.Jun 20 2019, 3:45 PM

@wraitii Thoughts on this ?

wraitii added a comment.Sep 3 2019, 9:56 AM

Apparently when you resign things no longer show up in your review queue

wraitii accepted this revision.Sep 3 2019, 9:56 AM

Gonna accept to make it pop again, and I'll commit this later.

This revision is now accepted and ready to land.Sep 3 2019, 9:56 AM
wraitii updated this revision to Diff 9917.Sep 22 2019, 4:47 PM

Rebased before committing

Vulcan added a comment.Sep 22 2019, 4:48 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/288/display/redirect

Vulcan added a comment.Sep 22 2019, 4:52 PM

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

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

Harbormaster completed remote builds in B9550: Diff 9917.Sep 22 2019, 4:52 PM
Closed by commit rP22971: Do not send Reflection and Refraction matrices to shader when not needed (authored by wraitii). · Explain WhySep 22 2019, 4:56 PM
This revision was automatically updated to reflect the committed changes.
wraitii added a commit: rP22971: Do not send Reflection and Refraction matrices to shader when not needed.

Revision Contents
Changeset List

PathSize
binaries/
data/
mods/
public/
shaders/
glsl/
302 lines
8 lines
source/
renderer/
11 lines

Diff 8316

View Options

binaries/data/mods/public/shaders/glsl/water_high.fs

Loading...
View Options

binaries/data/mods/public/shaders/glsl/water_high.vs

Loading...
View Options

source/renderer/TerrainRenderer.cpp

Loading...
Wildfire Games · Phabricator