Page MenuHomeWildfire Games
Differential D166

PickNonGaiaEntitiesInRect cleanup
ClosedPublic
Actions

Authored by elexis on Feb 26 2017, 7:24 AM.

Details

Reviewers
• leper
Commits
rP19344: Cleanup the rest of the mess introduced with the NonGaiaEntities functions in…
Summary

rP17623 fixing #3215 was broken and messy:

  1. Broken: GetNonGaiaEntities returns entities by Gaia (reported in #4495, fix at D164, found out on the polar sea map D156 where gaia wolves look for non-gaia targets but loop over gaia too causing lag) The ownership mask was taken without further examination (only playtest) from an irc recommendation on 2015-06-03-QuakeNet-#0ad-dev.log (17:31 to 17:57).
  1. Messy: a) Bad code location: There is code in ScriptFunctions.cpp that should have been in Selection.cpp. b) Bad loop: Querying the rangemanage for entities owned by each player consecutively and then concatenating the results instead of doing one query to get all non-gaia entities. As a result we can get rid of that playermanager reference again. c) Bad alias: A script function calls another script function, while every script function should just call one entity selection function and be done with it. d) Bad virtual keywords as pointed out in lepers quick-review wave on 2016-01-10-QuakeNet-#0ad-dev.log removed in rP17663

    Lame excuse: 7 months of begging and still not getting a complete review.

    This patch proposes to fix the messy code, as the broken part is fixed in the other proposal.
Test Plan

Applying this patch but not D164 reveals that bug. Having both applied shows that it still works in a whitebox test.

Confirm that the patch doesn't add unit tests that would be useful here and potentially complain. Not sure about the simulation helper, but the rangemanager ought to have one.

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

elexis created this revision.Feb 26 2017, 7:24 AM
Owners added a subscriber: Restricted Owners Package.Feb 26 2017, 7:24 AM
elexis mentioned this in D164: RangeManager returns Gaia entities in GetNonGaiaEntities.Feb 26 2017, 7:31 AM
elexis updated this revision to Diff 616.Feb 26 2017, 7:38 AM

Argh

elexis updated this revision to Diff 617.Feb 26 2017, 7:42 AM

Remove commented out debug code

Vulcan added a subscriber: Vulcan.Feb 26 2017, 8:17 AM

Build is green

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

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

Harbormaster completed remote builds in B599: Diff 615.Feb 26 2017, 8:17 AM
Vulcan added a comment.Feb 26 2017, 9:05 AM

Build is green

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

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

Harbormaster completed remote builds in B600: Diff 616.Feb 26 2017, 9:05 AM
Vulcan added a comment.Feb 26 2017, 9:52 AM

Build is green

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

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

Harbormaster completed remote builds in B601: Diff 617.Feb 26 2017, 9:52 AM
• leper requested changes to this revision.Feb 26 2017, 7:29 PM
• leper added a subscriber: • leper.

Some tests to make sure those do the right thing might be nice.

source/simulation2/helpers/Selection.cpp
133 ↗(On Diff #617)

Does changing this order have any impact on performance?

157 ↗(On Diff #617)

If you remove those braces, change this to a range-based loop.

164 ↗(On Diff #617)

and I guess same here.

185 ↗(On Diff #617)

One would have to check, but moving this before the first variable might help with return value optimizations.

189 ↗(On Diff #617)

Range-based for might be nicer.

source/simulation2/helpers/Selection.h
71 ↗(On Diff #617)

I'm not really sure if having the boolean here is useful, and shouldn't be added later if we really need it, or not.

This revision now requires changes to proceed.Feb 26 2017, 7:29 PM
elexis marked 3 inline comments as done.Feb 27 2017, 10:44 PM
In D166#6447, @leper wrote:

Some tests to make sure those do the right thing might be nice.

Added a test for the GetNonGaiaEntities part of the rangemanager (which also seems to be the only component that tests a constructed unit) to cover the bug I had added at the time.
Our C++ test files are really desolate.

source/simulation2/helpers/Selection.cpp
133 ↗(On Diff #617)

With few units around, CheckEntityInRect is insignificantly faster than GetLosVisibility, otherwise its significantly (factor 2 to 10) slower.
So the proposed change should actually improve performance unless I miss something.

I kept the performance test code in there so people can reproduce, it won't be committed.
Also profiler2 somehow not showing the entries I had added in that place.

157 ↗(On Diff #617)

Also everywhere else in the file while at it

185 ↗(On Diff #617)

You mean moving ents before hitEnts? Inlined ents altogether now

source/simulation2/helpers/Selection.h
71 ↗(On Diff #617)

Me neither, but I thought I keep it consistent.

elexis updated this revision to Diff 633.Feb 27 2017, 10:46 PM
elexis edited edge metadata.
elexis marked an inline comment as done.

Add rangemanager test and performance test, use range-based loops.

Harbormaster completed remote builds in B618: Diff 633.Feb 27 2017, 10:46 PM
• leper requested changes to this revision.Feb 27 2017, 11:01 PM

Lots of unrelated changes in here, code that isn't meant to be there (put that perf code in a test if you want to, or post a diff somewhere), making this thing unreadable, the test looks ok, apart from that the previous iteration was a lot nicer given that it didn't change unrelated code.

source/simulation2/components/CCmpRangeManager.cpp
1053 ↗(On Diff #633)

What is this doing in here?

source/simulation2/helpers/Selection.cpp
53 ↗(On Diff #633)

That's not an ent, also const &.

72 ↗(On Diff #633)

doesn't really seem related to the change at hand, same as for the above.

224 ↗(On Diff #633)

const&, but this also seems unrelated.

This revision now requires changes to proceed.Feb 27 2017, 11:01 PM
Harbormaster completed remote builds in B618: Diff 633.Feb 28 2017, 3:15 AM
Vulcan added a comment.Feb 28 2017, 3:15 AM

Build is green

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

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

• leper added a comment.Mar 24 2017, 11:19 PM

Can we get an update of this without all the unrelated code changes?

elexis updated this revision to Diff 937.Mar 25 2017, 1:08 AM
elexis edited edge metadata.

Remove all cleanup of lines not actually relevant to the task of this proposal.

• leper accepted this revision.Mar 25 2017, 2:34 AM

Thanks.

This revision is now accepted and ready to land.Mar 25 2017, 2:34 AM
elexis added a comment.Mar 25 2017, 3:19 AM

Thanks for the review(s)!

source/simulation2/components/tests/test_RangeManager.h
144 ↗(On Diff #937)

Changing this and the one below to player_id_t before a concern gets raised ;)

source/simulation2/helpers/Selection.cpp
177 ↗(On Diff #937)

guess this comment isn't needed anymore

• leper added inline comments.Mar 25 2017, 3:32 AM
source/simulation2/components/tests/test_RangeManager.h
144 ↗(On Diff #937)

Good catch!

source/simulation2/helpers/Selection.cpp
177 ↗(On Diff #937)

I'd keep it, mostly for consistency with the other function.

Closed by commit rP19344: Cleanup the rest of the mess introduced with the NonGaiaEntities functions in… (authored by elexis). · Explain WhyMar 25 2017, 3:43 AM
This revision was automatically updated to reflect the committed changes.
Vulcan added a comment.Mar 25 2017, 5:08 AM

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/596/ for more details.

Harbormaster completed remote builds in B883: Diff 937.Mar 25 2017, 5:08 AM

Revision Contents
Changeset List

PathSize
ps/
trunk/
source/
gui/
scripting/
22 lines
simulation2/
components/
tests/
16 lines
helpers/
8 lines
45 lines

Diff 943

View Options

ps/trunk/source/gui/scripting/ScriptFunctions.cpp

Loading...
View Options

ps/trunk/source/simulation2/components/tests/test_RangeManager.h

Loading...
View Options

ps/trunk/source/simulation2/helpers/Selection.h

Loading...
View Options

ps/trunk/source/simulation2/helpers/Selection.cpp

Loading...
Wildfire Games · Phabricator