Page MenuHomeWildfire Games

Do not send Reflection and Refraction matrices to shader when not needed
Needs ReviewPublic

Authored by Angen on Jun 5 2019, 11:43 AM.

Details

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 OK
Unit
No Unit Test Coverage
Build Status
Buildable 7784
Build 12674: Vulcan BuildJenkins
Build 12673: arc lint + arc unit

Event Timeline

Angen created this revision.Jun 5 2019, 11:43 AM
Owners added a subscriber: Restricted Owners Package.Jun 5 2019, 11:43 AM

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

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

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
Angen 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: Angen.
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?

Angen commandeered this revision.Jun 5 2019, 4:26 PM
Angen edited reviewers, added: vladislavbelov; removed: Angen.

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

Angen updated this revision to Diff 8324.Jun 5 2019, 5:44 PM
Angen 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

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.

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

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

@gameboy not related to water problem

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

Angen 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.

@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 ?