Page MenuHomeWildfire Games

Remove duplication of code from WaterManager
ClosedPublic

Authored by Angen on Nov 10 2019, 2:01 PM.

Details

Summary

Use already defined function in LoadWaterTextures with the same code.
Fixes the TODO from rP16388 / rP16389

Test Plan

Check replaced code is identical to one inside function and it works

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

Angen created this revision.Nov 10 2019, 2:01 PM
Owners added a subscriber: Restricted Owners Package.Nov 10 2019, 2:02 PM

Build failure - The Moirai have given mortals hearts that can endure.

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

elexis edited the summary of this revision. (Show Details)Nov 10 2019, 2:11 PM

Build failure - The Moirai have given mortals hearts that can endure.

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

vladislavbelov accepted this revision.Nov 10 2019, 3:08 PM
vladislavbelov added inline comments.
source/renderer/WaterManager.cpp
209 ↗(On Diff #10296)

Only disadvantage that I see, you don't reuse pathname. But it shouldn't touch performance.

379 ↗(On Diff #10296)

Could you also replace (int)i+1 by static_cast<int>(i) + 1 or replace format %02d by %02zu?

This revision is now accepted and ready to land.Nov 10 2019, 3:08 PM
Angen updated this revision to Diff 10298.Nov 10 2019, 3:23 PM

static cast

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

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

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

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

Stan added a subscriber: Stan.Nov 11 2019, 10:44 AM

Anyway we can test it actually works on GLES ?

Stan added inline comments.Nov 13 2019, 10:24 PM
source/renderer/WaterManager.cpp
377 ↗(On Diff #10298)

Any reason not to use int here ?

379 ↗(On Diff #10298)

Can't we replace ARRAY_SIZE(pathname) by PATH_MAX so we don't call a macro to do whatever each time ?

Angen added inline comments.Dec 2 2019, 2:06 PM
source/renderer/WaterManager.cpp
379 ↗(On Diff #10298)

It will be done in another patch as it is case in more places and kind of unrelated to reason of this patch.

Stan added inline comments.Dec 2 2019, 2:23 PM
source/renderer/WaterManager.cpp
372–373 ↗(On Diff #10298)

Doxygen.

379 ↗(On Diff #10298)

Sure, maybe it's just an optimization. I don't know if that macro call is cached...

This revision was automatically updated to reflect the committed changes.