Page MenuHomeWildfire Games
Differential D164

RangeManager returns Gaia entities in GetNonGaiaEntities
ClosedPublic
Actions

Authored by mimo on Feb 25 2017, 9:12 PM.

Details

Reviewers
• leper
elexis
Commits
rP19245: RangeManager returns Gaia entities in GetNonGaiaEntities
Trac Tickets
#4495
Summary

The code to select the bit mask for non gaia entities in CCmprangeManager is wrong.

  • as bit 0 is for owner=-1 and bit 1 is for gaia, it should shift by 2 and not by 1
  • futhermore, if playing with MAX_LOS_PLAYER_ID players, the last one won't be included.

as we want to negate the first two bits, we can simply use ~3 instead of the complicated expression.

Test Plan

Just add a printout of GetNonGaiaEntities().length for example in GuiInterface.GetSimulationState, and start a game. Without the fix, it is several thousands as gaia are included, and fixed by the patch.

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

mimo created this revision.Feb 25 2017, 9:12 PM
Owners added a subscriber: Restricted Owners Package.Feb 25 2017, 9:12 PM
Vulcan added a subscriber: Vulcan.Feb 25 2017, 9:56 PM

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

Harbormaster completed remote builds in B597: Diff 613.Feb 25 2017, 9:56 PM
• leper accepted this revision.Feb 25 2017, 10:34 PM
This revision is now accepted and ready to land.Feb 25 2017, 10:34 PM
elexis mentioned this in D166: PickNonGaiaEntitiesInRect cleanup.Feb 26 2017, 7:24 AM
elexis accepted this revision.Feb 26 2017, 7:31 AM
elexis added a subscriber: elexis.

GetNonGaiaEntities and it's broken ownership mask was introduced in rP17623 to fix #3215 (which was about displaying the status bars and hiding them again while changing the perspective)

I wasn't too comfortable with that code and the ownermask computation at the time and didn't get the reviews that should have been there (the patch was uploaded shortly after I installed 0ad and merged the patch myself without proper review after I was added to the team) .
Didn't notice the recommended ownership mask was off by 1 (refs 2015-06-03-QuakeNet-#0ad-dev.log 17:31 to 17:57).

The commit rP17623 only works because GetNonGaiaEntities is used to only remove status bars and returning too many entities there means still every previously shown one will be removed.

If you had thought that an unreviewed commit by someone who was never entirely sure what he did there, that already had one fix-the-previous commit and now this one might have more issues, you guessed right.
Hope D166 addresses everything that's bad about it.

source/simulation2/components/CCmpRangeManager.cpp
69 ↗(On Diff #613)

The operation x << n shifts the value of x left by n bits.

So,
for owner = -1 (invalid), CalcOwnerMask returns 1 << (1+owner) ==1 << (1+-1) == 1 << 0 = 0x1 << 0 == 0x1
for owner = 0 (gaia), CalcOwnerMask returns 0x1 << 1 == 0x10
for owner = 1 (player 1), CalcOwnerMask returns 0x1 << 2 ==0x100
...
i.e. the first bit is set for invalid, the second bit for gaia, third for player 1, ...., 32nd for the 30th player

// owner == INVALID_PLAYER; Used when selecting units in Atlas or other mods that allow all kinds of selectables to be selected.

Also notice the "invalid" player is not an invalid owner :P

1053 ↗(On Diff #613)

Mask is ((1 << MAX_LOS_PLAYER_ID) - 1) << 1

((1 << 16) - 1) << 1

((0x10000000000000000) - 1) << 1

0x1111111111111111 << 1

0x11111111111111110

i.e. returns all gaia entities and only excludes entities owned by -1 (INVALID_PLAYER) and hence ((1 << MAX_LOS_PLAYER_ID) - 1) << 2 could have worked.

1053 ↗(On Diff #613)

~3 = ~(0x11) = ~(0x 0000.0000 0000.0000 0000.0000 0000.0011) = ~(0x 1111.1111 1111.1111 1111.1111 1111.1100)

i.e. everyone but the first two, which are -1 (invalid) and 0(gaia), i.e. player 1 to player 30, i.e. passes a mask that requests entities owned by those players as defined by the function name (except invalid, which we likely don't need, as every entity we are interested in has an owner (actors don't have owners I guess, but there is nothing currently that could use a GetNonGaiaEntities that includes actors).

Also I agree that not3 is more readable than ((1 << MAX_LOS_PLAYER_ID) - 1) << 2, and it is also independent of the maximum, so if we want to support more than 30 players in the mask, we only have to increase the size of that variable, at least in this occurance.

mimo added inline comments.Feb 26 2017, 11:23 AM
source/simulation2/components/CCmpRangeManager.cpp
1053 ↗(On Diff #613)

I don't think ((1 << MAX_LOS_PLAYER_ID) - 1) << 2 would have worked if you were to play with the maximum number of players allowed: you should then shift by MAX_LOS_PLAYER_ID+1 or even by MAX_LOS_PLAYER_ID+2 (if MAX_xxx does not includes gaia, which i've not checked).
And then you would have to add some protection if that goes bigger than 32

Closed by commit rP19245: RangeManager returns Gaia entities in GetNonGaiaEntities (authored by mimo). · Explain WhyFeb 26 2017, 11:26 AM
This revision was automatically updated to reflect the committed changes.
elexis added a comment.Feb 26 2017, 4:36 PM

Thanks for the patch and shame on me! :-)

• leper added a comment.Feb 26 2017, 7:05 PM
In D164#6444, @elexis wrote:

Thanks for the patch and shame on me! :-)

Bugs happen. The important thing is getting them fixed.

elexis mentioned this in D156: Polar Sea Random Map.Feb 28 2017, 10:50 PM
elexis mentioned this in rP19256: Remove a TODO which was fixed by rP19245 / D164..Mar 1 2017, 12:22 PM

Revision Contents
Changeset List

PathSize
ps/
trunk/
source/
simulation2/
components/
2 lines

Diff 620

View Options

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

Loading...
Wildfire Games · Phabricator