Page MenuHomeWildfire Games
Differential D2557

Significantly improves Atlas terrain elevation performance
ClosedPublic
Actions

Authored by vladislavbelov on Jan 10 2020, 10:14 AM.

Details

Reviewers
Itms
Commits
rP23361: Improves performance of Atlas terrain elevation by updating the only changed…
Summary

We need to update passability only for modified terrain plus edges.

Test Plan
  1. Appy the patch and compile the game
  2. Run atlas and enable passability visualization (building-land)
  3. Make sure that terrain elevation worked faster and passability map is modified immediately

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.Jan 10 2020, 10:14 AM
vladislavbelov edited the test plan for this revision. (Show Details)
Vulcan added a comment.Jan 10 2020, 10:16 AM

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

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

Vulcan added a comment.Jan 10 2020, 10:19 AM

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

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/93/display/redirect

Vulcan added a comment.Jan 10 2020, 10:21 AM

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

Linter detected issues:
Executing section Source...

source/simulation2/components/CCmpPathfinder_Common.h
|  34| #include·"graphics/Overlay.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'template<...' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

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

Harbormaster completed remote builds in B10717: Diff 10956.Jan 10 2020, 10:21 AM
Itms edited reviewers, added: Itms; removed: Restricted Owners Package.Jan 10 2020, 11:09 AM
Itms added a subscriber: Itms.

Hey, this looks great! I didn't realize the message had that terrain information when I wrote the code!

Everything works fine for me and the code looks good, I just have a question about boundaries...

An extra test (which I didn't run but would be needed) is a serialization test on a map where the terrain changes during the game: typically Extinct Volcano.

source/simulation2/components/CCmpPathfinder.cpp
623 ↗(On Diff #10956)

why isn't itile1 + 1 needed? it's not clear to me when reading the comment.

vladislavbelov added a comment.Jan 10 2020, 12:35 PM
In D2557#106783, @Itms wrote:

An extra test (which I didn't run but would be needed) is a serialization test on a map where the terrain changes during the game: typically Extinct Volcano.

I think it's not enough, because it has only water level changes. That means the whole grid will be updated (no partial update).

source/simulation2/components/CCmpPathfinder.cpp
623 ↗(On Diff #10956)

You're right, there should be +1.

Itms added a comment.Jan 10 2020, 1:15 PM
In D2557#106789, @vladislavbelov wrote:

I think it's not enough, because it has only water level changes. That means the whole grid will be updated (no partial update).

That's true. I don't think we have other occurrences of in-game terrain changes though (especially since, before this patch, they would be quite expensive).

vladislavbelov added a comment.Jan 10 2020, 1:20 PM
In D2557#106791, @Itms wrote:

That's true. I don't think we have other occurrences of in-game terrain changes though (especially since, before this patch, they would be quite expensive).

I have few ideas how to speed up the complete update though.

Stan added a subscriber: Stan.Jan 10 2020, 3:49 PM
In D2557#106791, @Itms wrote:
In D2557#106789, @vladislavbelov wrote:

I think it's not enough, because it has only water level changes. That means the whole grid will be updated (no partial update).

That's true. I don't think we have other occurrences of in-game terrain changes though (especially since, before this patch, they would be quite expensive).

Terrain is not serialized anyway https://code.wildfiregames.com/D1642 and #2264

Stan added inline comments.Jan 10 2020, 3:53 PM
source/simulation2/components/CCmpPathfinder.cpp
575 ↗(On Diff #10956)

Why is the default param commented here? Shouldn't you comment the others as well? Wouldn't it be better if it matched the signature 1:1 ?

vladislavbelov added inline comments.Jan 10 2020, 4:15 PM
source/simulation2/components/CCmpPathfinder.cpp
575 ↗(On Diff #10956)

I think /* = true */ isn't needed here, since it doesn't give us a useful piece of information.

Itms added inline comments.Jan 10 2020, 4:35 PM
source/simulation2/components/CCmpPathfinder.cpp
575 ↗(On Diff #10956)

I like to put the default param like this in the implementation, because else you have to open the header to see what are the default values, if any. But with the new parameters it looks out of place. You can remove it if you want. Maybe expandPassability shouldn't be optional, I don't remember how many calls of the function there are.

vladislavbelov added inline comments.Jan 10 2020, 4:38 PM
source/simulation2/components/CCmpPathfinder.cpp
575 ↗(On Diff #10956)

I understand that it might be useful. But in which cases you need to know information about a default value inside a function? Usually it's needed for callers.

Itms added inline comments.Jan 10 2020, 4:41 PM
source/simulation2/components/CCmpPathfinder.cpp
575 ↗(On Diff #10956)

True. However for helper functions like this one usually all the callers are in the same .cpp file.

vladislavbelov added inline comments.Jan 10 2020, 4:47 PM
source/simulation2/components/CCmpPathfinder.cpp
575 ↗(On Diff #10956)

I was thinking about that too. Usually callers aren't aggregated in a place near to a function. A file is growing and there are lot of lines between a caller and the function. So it's easier to open a header in that case.

vladislavbelov updated this revision to Diff 10963.Jan 10 2020, 7:18 PM
vladislavbelov marked 2 inline comments as done.

Fixes notes.

Vulcan added a comment.Jan 10 2020, 7:20 PM

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

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

Vulcan added a comment.Jan 10 2020, 7:23 PM

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

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/97/display/redirect

Vulcan added a comment.Jan 10 2020, 7:23 PM

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

Linter detected issues:
Executing section Source...

source/simulation2/components/CCmpPathfinder_Common.h
|  34| #include·"graphics/Overlay.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'template<...' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

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

Harbormaster completed remote builds in B10724: Diff 10963.Jan 10 2020, 7:23 PM
Itms accepted this revision.Jan 10 2020, 8:15 PM

Excluding the in-game terrain updates, which are not actually implemented, everything works fine for me and the code looks good.

This revision is now accepted and ready to land.Jan 10 2020, 8:15 PM
Closed by commit rP23361: Improves performance of Atlas terrain elevation by updating the only changed… (authored by vladislavbelov). · Explain WhyJan 10 2020, 8:54 PM
This revision was automatically updated to reflect the committed changes.
vladislavbelov added a commit: rP23361: Improves performance of Atlas terrain elevation by updating the only changed….
Owners added a subscriber: Restricted Owners Package.Jan 10 2020, 8:54 PM
elexis added a subscriber: elexis.Jan 10 2020, 10:48 PM

when I wrote the code

(A commit reference would be useful to ease commit traversal while reviewing, auditing or revisioning)

in-game terrain updates, which are not actually implemented

(I remember that extinct volcano map with rising water that sent simulation terrain updates, also aren't atlas changes ingame terrain updates? I.e. suppose that map wouldnt benefit from performance improvments since water is everywhere, so it's never a partial update? And script components / trigger script partial terrain changes are the thing which is not implemented (rather than this patch not implementing the improvement there? I suppose I could read the code)

Itms added a comment.Jan 10 2020, 11:17 PM
In D2557#106874, @elexis wrote:

(A commit reference would be useful to ease commit traversal while reviewing, auditing or revisioning)

It was rP16824. And that is funny: looking at the commit, I had written a TODO, so I knew! I then removed the TODO in rP16827, just three revisions later, when I thought the improvements I made were enough.

also aren't atlas changes ingame terrain updates?

Well, by in-game, I meant literally in game, when playing. Indeed, from the point of view of the code, the simulation is running the same in Atlas and in-game, but that's not what I meant.

I remember that extinct volcano map with rising water that sent simulation terrain updates, I.e. suppose that map wouldnt benefit from performance improvments since water is everywhere, so it's never a partial update? And script components / trigger script partial terrain changes are the thing which is not implemented (rather than this patch not implementing the improvement there? I suppose I could read the code)

Yes to all of the questions here, and you can read the code if you want ?

vladislavbelov mentioned this in rP23364: Removes obsolete sys_cursor. Refs D2557..Jan 11 2020, 12:45 AM

Revision Contents
Changeset List

PathSize
ps/
trunk/
source/
simulation2/
components/
40 lines
6 lines

Diff 10965

View Options

ps/trunk/source/simulation2/components/CCmpPathfinder.cpp

Loading...
View Options

ps/trunk/source/simulation2/components/CCmpPathfinder_Common.h

Loading...
Wildfire Games · Phabricator