Page MenuHomeWildfire Games

Upgrade engine to handle up to 30 players
Needs ReviewPublic

Authored by nani on Wed, Mar 25, 12:44 AM.
Tags
None
Subscribers
elexis, Restricted Owners Package, Restricted Owners Package and 2 others
Tokens
"The World Burns" token, awarded by nani."Yellow Medal" token, awarded by Freagarach.

Details

Reviewers
Angen
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Trac Tickets
#4004
Summary

Take (2nd try) on enabling games with more than 8 players.

The diff can be condensed in two main changes:

  • Unify all data structures and methods used in the c++ range manager component in a class that handles the players/ownership data and the bit-wise operator it uses.
  • Allow to specify the number of GUI entries to be repeated before the <repeat> tag is expanded.

This diff is only meant to upgrade the engine to handle 30 players and capable of launching from the command line not from the GUI. That will need another patch.

At first sight the changes seems to not have affected the performance but the increase of players will probably make it more noticeable.

Add @Angen as reviewer as he (if I recall correctly) already worked on this similar code with data that this diff touches.

Test Plan

Look for logic inconsistencies on the code passed to the class, possible better approaches and performance concerns.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 11359
Build 20573: arc lint + arc unit

Event Timeline

nani created this revision.Wed, Mar 25, 12:44 AM
Owners added subscribers: Restricted Owners Package, Restricted Owners Package, Restricted Owners Package.Wed, Mar 25, 12:44 AM
nani edited the summary of this revision. (Show Details)Wed, Mar 25, 12:47 AM
elexis updated the Trac tickets for this revision.Wed, Mar 25, 1:43 AM
elexis added a subscriber: elexis.Thu, Mar 26, 12:00 AM

Previous revision D1662.

The objective of the ticket is to remove technical limitations of the playerlimit, thus maximizing versatility of the engine.
30 players is an improvement, but what if someone wants more playerIDs than 30 (without necessarily increasing the total entity count).
So the ideal implementation for that task description would work with something like 2^16, 2^32, 2^64 players as a limit with the least amount of performance decrease.

nani tested other data structures, and apparently there was some performance consideration:

vector<bool> is 7x
svn vs vector<uint16_t> is 5x
svn vs uint64_t is smorewhat similar
but wrose by 1.5 i would say
los update prfiler

But those numbers may not represent the optimized implementation, just saying there are more possibilities for the data structure.

(If the performance decreases linearily with increased playercount due to the ownershipmask bitsize, perhaps one could chose a data structure. Perhaps some std::map<playerID, entities> could be possible? I didn't check, but perhaps optimizations would be possible that dont add a performance regression while maximizing playercount. 30 players is very good, but it would have to be revised if someone wants the 31st plaer.)

GUI repeat diff is logically independent from the simulation diff.

nani updated this revision to Diff 11569.Thu, Mar 26, 8:34 PM

Removed const and gave the child template parameter a different name so it wont complain in gcc.

Did not have time to get into it yet, so pointing only style.

source/simulation2/components/CCmpRangeManager.cpp
68

IsValidPlayer or IsPlayerValid

73

IsValidOwner or IsOwnerValid

85

if (

120

PlayerData& dirty

193

^ comments not valid

221

, own

697

,

713

, 1

715

, 0

2066

, LOS

2427–2428

It would be good to avoid auto we know that there are player_id_t no?

2428

j * m_Ter

source/simulation2/components/ICmpRangeManager.h
1

years

271

j * m_Ver

283

j * m_Ver

296

j * m_Ver

309

j * m_Ver

source/simulation2/helpers/ArrayData.cpp
2

copyright

35

for ( ++i

36

i, ve

75

ArrayData<extra>&

81

ArrayData<extra>&

87

ArrayData<extra>&

93

ArrayData<extra>&

107

2 * i

113

2 * i

source/simulation2/helpers/ArrayData.h
2

copyright

31

ArrayData<extra>&

32

ArrayData<extra>&

34

ArrayData<extra>&

Angen added a reviewer: Restricted Owners Package.Fri, Mar 27, 3:08 PM
Angen added inline comments.Fri, Mar 27, 3:47 PM
source/simulation2/helpers/ArrayData.h
18

I would be careful using u64 as we still compile with 32 bit compiler, that means storing that value in 2 registers instead just one what makes reading and writing slower. (just a note, did not check if we can do something about it to not use u64)

Angen added inline comments.Fri, Mar 27, 4:02 PM
source/graphics/tests/test_LOSTexture.h
34–45

also 0, 0 same below

nani updated this revision to Diff 11587.Fri, Mar 27, 7:52 PM
nani added inline comments.Fri, Mar 27, 7:53 PM
source/simulation2/helpers/ArrayData.h
18

ok

nani awarded a token.Fri, Mar 27, 7:57 PM
nani marked 32 inline comments as done.Fri, Mar 27, 8:01 PM
nani added inline comments.Fri, Mar 27, 8:03 PM
source/simulation2/helpers/ArrayData.cpp
75

The const is needed.

some more comments

source/simulation2/helpers/ArrayData.cpp
42

If you expect here i = -1 (Comment at line 119). that is not possible. size_t is non negative.

51

for ( i += 2

52

value & 0x3 always returns the same result, should be computed before loop

112

static_cast<u64>(0x3) << 2 * i static_cast<u64>(v & 0x3) << 2 * i