Page MenuHomeWildfire Games

Speed up the water manager recomputations
ClosedPublic

Authored by wraitii on Jan 20 2017, 11:27 AM.

Details

Reviewers
vladislavbelov
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22006: Clean up and speed up the water manager distance computations
Summary

The water manager computations for "fancy water effects" have always been quite slow. I've updated one of the functions to be much faster, and the other doesn't need to be called (apparently, since I removed coastal foam, which tbh I don't remember doing).

This should all be redone entirely to be honest, as it's generally terrible, but in the short-term this makes this function almost usable in real-time.

Test Plan

Start a few maps, verify if HQ water effects make waves show up and that it doesn't crash.

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
vladislavbelov added inline comments.Apr 10 2017, 8:19 PM
source/simulation2/components/CCmpWaterManager.cpp
100 ↗(On Diff #1198)

So is enough to call this function only from CCmpTerrain?

Want to commit this soon, @vladislavbelov care to accept? I'll check the blur situation later when I improve the functions themselves because RecomputeWindStrength is a bit buggy too…

leper added inline comments.
source/renderer/WaterManager.cpp
442 ↗(On Diff #1198)

Why isn't this in the loop condition?

444 ↗(On Diff #1198)

Why isn't this next to ++i?

509 ↗(On Diff #1198)

Why isn't that done before the possibly pointless copy?

533 ↗(On Diff #1198)

Why does this use the other variable if they have the exact same value at this point?

424 ↗(On Diff #292)

new float[SideSize*SideSize]()

value initialization to the rescue.

437 ↗(On Diff #292)

Using lambdas for this seems like someone wanted to use lambdas for this. Not because they make the code more readable or likely to be nicely optimized.

In D78#14425, @wraitii wrote:

Want to commit this soon, @vladislavbelov care to accept? I'll check the blur situation later when I improve the functions themselves because RecomputeWindStrength is a bit buggy too…

Ok, I'll accept, but when it'll be refactored?

mimo added a subscriber: mimo.Apr 20 2017, 8:04 PM

while at it, here are a few defects i reported to you a long time ago on irc. Would be good to have a look at it some time

source/renderer/WaterManager.cpp
1091 ↗(On Diff #1198)

if (a > b) a = b (or even a = min(a, b) although the former is faster) would be better than a = a > b ? a : b;

1101 ↗(On Diff #1198)

these lines smell like a copy-paste bug (the first index should certainly vary from 0 to 3). If that is intended, an easy optimization is possible :)

wraitii updated this revision to Diff 1394.Apr 21 2017, 10:44 AM

Change the refactor following leper's comment so that we only have one lambda-ed loop called twice and transposed. Fix style issues.

@mimo: noted, but I think I'll rewrite this whole thing so I won't do it in this revision anyhow.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/838/ for more details.

leper added inline comments.Apr 21 2017, 1:08 PM
source/renderer/WaterManager.cpp
448 ↗(On Diff #1394)

Given that i == lookahead at every point in this loop (if we consider incrementing them both to be one atomic step, which we'll do for the sake of the argument), you can just ignore having i at all and just use lookahead instead.

450 ↗(On Diff #1394)

You'd want ++i, ++lookahead here, but that is just for the sake of not letting that ugly piece of code uncommented.

465 ↗(On Diff #1394)

i == lookahead hold for the whole loop.

477 ↗(On Diff #1394)

Given that if/else part in there I guess this would either be nicer with a templated bool parameter (which would lead any sane compiler to remove those if-else blocks within loops), or trying to do this with a define.

511 ↗(On Diff #1394)

I'd not hava a change in this line at all, as adding those spaces distracts from < which is the important operation in that line.

512 ↗(On Diff #1394)

Same as above.

857 ↗(On Diff #1394)

If we are reserving above we should do that here too.

(I'm not exactly sure what this code tries to do, but do we have to do this reversal here instead of constructing things as expected?)

509 ↗(On Diff #1198)

*cough*

424 ↗(On Diff #292)

{} works too, but for consistency with the rest of the codebase I'd go with ().

leper changed the visibility from "All Users" to "Public (No Login Required)".Apr 21 2017, 1:14 PM
wraitii updated this revision to Diff 1403.Apr 21 2017, 4:27 PM

Initialize the array with maxLevel which I should have done from the start, removing the need for one if, clean the for loops, and use std::reverse instead of custom code for some reason.

Also there have been a few comments that might be useful earlier.

source/renderer/WaterManager.cpp
420 ↗(On Diff #1403)

Why isn't this const? (Since maxLevel below is also const, might as well do that here too.)

452 ↗(On Diff #1403)

That one evil voice in my head that likes templates would just pass the transpose part to a templated abovewater function and let that handle that.

The one voice liking macros would just pass transpose to the macro and let that handle it, keeping the loop condition short and readable.

453 ↗(On Diff #1403)

{}; is a very strange construct, which is neither for(...;...;...){} or the better for(...;...;...); where the ; should be on the next line to avoid confusion (and silence warnings for a few compilers with nice warnings).

wraitii added inline comments.Apr 21 2017, 4:50 PM
source/renderer/WaterManager.cpp
452 ↗(On Diff #1403)

I would have to define the whole function as a macro for it to work though no? Unless I'm missing something. And that's a lot of "\" which I don't really like all that much.

The template argument is possible but I'd have to put the function in the global scope (C++ doesn't handle templated-structs-inside-a-function-scope)... I don't really feel strongly either way, and I have an unverified hunch that a compiler would generate clever assembly anyways.

leper added inline comments.Apr 21 2017, 5:04 PM
source/renderer/WaterManager.cpp
452 ↗(On Diff #1403)

For a macro, yes, unless the transpose part is a templated parameter.

I guess a static templated function doesn't sound that bad here (or a templated member function). Especially considering that most of the function is now that lambda, and a tiny amount of init before that and then two calls afterwards.

The main point of doing either of these would be to not have !a && foo(b, c) || a && foo(c,b) with longer variable names repeated a lot, and then hoping that the compiler will inline that enough to be sure that one of those two conditions is always false and just drop it. That doesn't sound that nice if we could just write this in a way that the compiler has to do just that, does it?

wraitii updated this revision to Diff 1406.Apr 21 2017, 5:28 PM

I suppose that's fair, indeed.

Used a static templated function.

Starts to resemble something one could try to understand (not that I intend to do so, but you must admit that it is a lot more readable now, isn't it?).

source/renderer/WaterManager.cpp
430 ↗(On Diff #1406)

Now using another define for ((!Transpose && ABOVEWATER(lookahead, id1)) || (Transpose && ABOVEWATER(id1, lookahead))) would be great.

509 ↗(On Diff #1198)

*cough*

wraitii updated this revision to Diff 1407.Apr 21 2017, 6:10 PM

Abstract the ugly loop away, and add a tiny algorithm comment in case somebody ever wants to change this.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/841/ for more details.

I guess I have complained enough, get someone to actually test that and figure out what this actually does.

source/renderer/WaterManager.cpp
419 ↗(On Diff #1407)

Not entirely sure but I think adding () around the expansion could prevent unintended issues when someone adds another use of this.

420 ↗(On Diff #1407)

No need for the () here.

Also \ instead of \. (Same for the below.)

454 ↗(On Diff #1407)

#undef UPDATELOOKAHEAD

857 ↗(On Diff #1394)

Ah, the old code reverses blocks of size 9, so this is a behaviour change (maybe not an issue, I'll leave that for someone else to decide).

wraitii added inline comments.Apr 21 2017, 6:22 PM
source/renderer/WaterManager.cpp
419 ↗(On Diff #1407)

Right.

420 ↗(On Diff #1407)

Added it to make it look like a function but it is definitely unnecessary.

857 ↗(On Diff #1394)

Mh, you're right... I think it's to avoid a counter-clock-wise type of issue, but I am not sure… Might be easier to just disable the face culling when rendering if that's the case...

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/844/ for more details.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/845/ for more details.

wraitii updated this revision to Diff 1537.Apr 29 2017, 5:29 PM

Fix Leper's last remarks. RC.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/926/ for more details.

vladislavbelov requested changes to this revision.Jun 25 2017, 12:09 PM
vladislavbelov added inline comments.
source/renderer/WaterManager.cpp
429 ↗(On Diff #1537)
const size_t& ...
433 ↗(On Diff #1537)

This could be written as:

size_t lookahead = level > 0;
441 ↗(On Diff #1537)

And here we could use ternary:

level = ABOVEWATER(x, z) ? 0 : std::min(level+1, maxLevel);
This revision now requires changes to proceed.Jun 25 2017, 12:10 PM
wraitii updated this revision to Diff 4015.Oct 29 2017, 2:37 PM

Rebase, address comments.

Build is green

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/2188/ for more details.

Executing section Default...
Executing section Source...
Executing section JS...

http://jenkins-master:8080/job/phabricator_lint/644/ for more details.

wraitii added a reviewer: Restricted Owners Package.Nov 25 2017, 3:28 PM

Bumping in hopes of a review

source/renderer/WaterManager.cpp
441 ↗(On Diff #1537)

not much more readable imo.

wraitii added a subscriber: temple.Jan 21 2018, 4:25 PM

@temple or @vladislavbelov would you mind looking at this?

Pink Floyd's famous 1979 'Another patch in review'

aeonios added a subscriber: aeonios.May 5 2018, 6:00 PM

I compiled this and it crashed when I tried to load med cove in atlas. I didn't get anything specific, just an access violation.

wraitii updated this revision to Diff 6550.May 15 2018, 8:34 PM

Yup, it appears I'd forgotten to remove some piece of code or something at some point in the diff updates. Fixed.

wraitii updated this revision to Diff 6551.May 15 2018, 8:36 PM

With the changes this time -_-

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

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

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

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

wraitii updated this revision to Diff 7150.Dec 30 2018, 6:24 PM

Rebase, clarify a comment.

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

Link to build: https://jenkins.wildfiregames.com/job/differential/860/

This revision was not accepted when it landed; it landed in state Needs Review.Jan 2 2019, 4:23 PM
This revision was automatically updated to reflect the committed changes.
In D78#14568, @mimo wrote:

while at it, here are a few defects i reported to you a long time ago on irc. Would be good to have a look at it some time

For reference: 2016-06/2016-06-23-QuakeNet-#0ad-dev.log

18:33 < mimo> @tell wraitii source/renderer/WaterManager.cpp has what looks like a wrong copy-and-paste. Lines 1082-1085 the first index of blurKernel is certainly to be increased on each line

In D78#14745, @wraitii wrote:

mimo: noted, but I think I'll rewrite this whole thing so I won't do it in this revision anyhow.

Followed up by:

(08:38:55 PM) wraitii: vladislavbelov[: removed by https://code.wildfiregames.com/D1721 anyways

source/renderer/WaterManager.cpp
509 ↗(On Diff #1198)

(was taken into account)

source/simulation2/components/CCmpWaterManager.cpp
100 ↗(On Diff #1198)

It is not called from CCmpTerrain though, now it's called from nowhere, the function should be removed if it's useless, or fixed if it is useful.