Page MenuHomeWildfire Games

Define the maximum number of players pyrogenesis can handle in a match.
AbandonedPublic

Authored by nani on Nov 1 2018, 11:37 AM.

Details

Reviewers
elexis
vladislavbelov
Trac Tickets
#4004
Summary

Makes matches with up to 16 players possible (and a upper limit set to 16 in c++).

Test Plan

If we want to support more than 16 we will have to discard the use of the u32 masking given that the CalcPlayerLosMask uses 2 bits per player.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

nani created this revision.Nov 1 2018, 11:37 AM
vladislavbelov requested changes to this revision.Nov 1 2018, 11:59 AM
vladislavbelov added a subscriber: vladislavbelov.

Why 32 in C++ and 24 in JS?

source/lib/alignment.h
58 ↗(On Diff #6956)

This and whole alignment.h shouldn't be modified, it's the internal constant. It's not the player count.

source/simulation2/components/CCmpRangeManager.cpp
82

32 should be replaced by a named constant.

This revision now requires changes to proceed.Nov 1 2018, 11:59 AM
Stan added a subscriber: Stan.Nov 1 2018, 12:05 PM
Stan added inline comments.
binaries/data/mods/public/gui/gamesetup/gamesetup.js
1082 ↗(On Diff #6956)

Can it somehow be dependent from the global max number of players ?

nani marked 2 inline comments as done.Nov 1 2018, 12:24 PM
nani added inline comments.
binaries/data/mods/public/gui/gamesetup/gamesetup.js
1082 ↗(On Diff #6956)

What do you mean by that, can you rephrase please? I don't get what you mean.

source/lib/alignment.h
58 ↗(On Diff #6956)

You're right.

source/simulation2/components/CCmpRangeManager.cpp
82

Ok.

nani marked 2 inline comments as done.Nov 1 2018, 12:26 PM

Why 32 in C++ and 24 in JS?

I initially wanted 32 for game setup too but the list won't visually fit nicely more than 24 slots.

nani updated this revision to Diff 6957.Nov 1 2018, 12:30 PM
Stan added a comment.Nov 1 2018, 12:36 PM

This patch would be fun with a Max pop of like 25 XD

binaries/data/mods/public/gui/gamesetup/gamesetup.js
1082 ↗(On Diff #6956)

As a ratio of g_maxNumberofplayers.

nani added inline comments.Nov 1 2018, 12:54 PM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
1082 ↗(On Diff #6956)

Ah I see, you mean. Yes could be done.

ex: height = Math.min(Math.max(30*currentNumPlayers/maxNumPlayers,30),15)

nani added a comment.Nov 1 2018, 12:56 PM

It could also be added a Engine.MatchGetMaxNumberOfPlayers()

In D1662#65753, @nani wrote:

It could also be added a Engine.MatchGetMaxNumberOfPlayers()

That'd be awesome, because we need to avoid a code duplication.

Also, isn't there any other place with the hardcoded max number of players?

nani added a comment.Nov 1 2018, 1:43 PM
In D1662#65753, @nani wrote:

It could also be added a Engine.MatchGetMaxNumberOfPlayers()

That'd be awesome, because we need to avoid a code duplication.

Also, isn't there any other place with the hardcoded max number of players?

I think the rmg library has some hardcodings too.
About the xml files with <repeat> tags, the hardcoding can't be avoided unless we add Engine.MatchGetMaxNumberOfPlayers() or similar that defines <repeat n="number"> in some way before the parsing or in the parsing.

In D1662#65755, @nani wrote:
In D1662#65753, @nani wrote:

It could also be added a Engine.MatchGetMaxNumberOfPlayers()

That'd be awesome, because we need to avoid a code duplication.

Also, isn't there any other place with the hardcoded max number of players?

I think the rmg library has some hardcodings too.
About the xml files with <repeat> tags, the hardcoding can't be avoided unless we add Engine.MatchGetMaxNumberOfPlayers() or similar that defines <repeat n="number"> in some way before the parsing or in the parsing.

Engine.GetMaxNumberOfPlayers() can be easily added through ScriptInterface.

nani added a comment.Nov 1 2018, 2:10 PM
In D1662#65757, @smiley wrote:

How can it be made so CCmpRangeManager.cpp and Player.cpp share the same #define MAX_NUMBER_OF_PLAYERS 32 ?

elexis updated the Trac tickets for this revision.Nov 1 2018, 2:26 PM
elexis added a comment.Nov 1 2018, 2:40 PM

Patches that should be committed should be correct and complete. I expect this to break on most random maps and in many GUI pages.
So it would probably be safer to first unify all constants in the engine to support arbitrary numbers and then only expose the > 8 in the UI if the UI and maps support it properly.
Also atlas.

Ultimately the maximum number of players should not be limited, so that game designers can use the Pyrogenesis engine for any games they come up with.
I see that you chose 32, since the RangeManager is limited to that, so that may be an improvement already.
The real question is whether those are all files that need this change (I wouldn't be surprised if there are many more cpp files surrounding the RangeManager or anything else that stores values depending on playerID).

binaries/data/mods/public/gui/common/settings.js
5 ↗(On Diff #6957)

This should be received from the engine, through JSInterface_... probably the Game one

binaries/data/mods/public/gui/gamesetup/gamesetup.xml
106 ↗(On Diff #6957)

100% height if this has to be changed

binaries/data/mods/public/gui/session/selection_panels_middle/multiple_details_area.xml
39 ↗(On Diff #6957)

<!-- MaxPlayers --> so people find it when editing it. (Would be ideal to have this a constant somewhere that is filled in in the C++/XML engine)

In D1662#65759, @nani wrote:
In D1662#65757, @smiley wrote:

How can it be made so CCmpRangeManager.cpp and Player.cpp share the same #define MAX_NUMBER_OF_PLAYERS 32 ?

I.e. static const int MAX_NUMBER_OF_PLAYERS.

Stan added inline comments.Nov 1 2018, 4:32 PM
binaries/data/mods/public/gui/common/settings.js
5 ↗(On Diff #6957)

Not sure if that shouldn't be the other way around and being sanitized by cpp if it's above 32 ?

nani retitled this revision from Increase maximum number of match players from 8 to 24 (32) to Increase maximum number of match players from 8 to 16.Nov 2 2018, 1:18 PM
nani edited the summary of this revision. (Show Details)
nani edited the test plan for this revision. (Show Details)
nani updated this revision to Diff 6961.EditedNov 2 2018, 1:21 PM

I made a blunder not seeing that LOS needs two bits per player. Given that the type is uint32_t that makes the upper bound in reality 16.

elexis added a comment.Nov 2 2018, 2:13 PM

If the engine already supports 16 max, then it should be documented well and propagated. New sim components should be written without being possible to assume that its max 8.

About exposing this to the players, I'm not sure, it would have to be very polished.
Currently 2-8 players is well known to produce good results, we don't to water down the experience.
It should be tested well that 16 max doesn't produce less entertaining results (not only random maps, but the entire match experience).
I'm sure it will work for 10 players max ingame, not sure about the dialog boxes. Beyond that it would require some testing to see if there are some problems or quality loss arising.
For example the current team chat is already difficult, how are players supposed to communicate with that many players.
(Pyrogenesis should support limit removals for sure, but increasing max for 0 A.D. needs separate consideration.)

binaries/data/mods/public/gui/common/settings.js
5 ↗(On Diff #6957)

The important part is that devs dont think they can just replace this one variable and their task is done.
A comment or math.min(16, engine_limit) would prevent that.
Speaking about comments, the one above would need an update.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
1083 ↗(On Diff #6961)

Can you show a screenshot how the gamesetup looks with 1024*768 for 16 playerslots with this patch?

source/simulation2/components/CCmpRangeManager.cpp
82

Don't think it should. Here 32 is used because it's u32. It's also not 2 * max number of players, but it should be capped by max_players.
For owner masks there are max 32 players but for LOS masks there are 16 max, no?

nani updated this revision to Diff 6962.Nov 2 2018, 2:47 PM
nani retitled this revision from Increase maximum number of match players from 8 to 16 to Define the maximum number of players pyrogenesis can handle in a match..

Changes concerning the number of players' limit in the 0ad public mod files should be made in a different diff.
This diff will only state the current engine players limit.

nani edited the summary of this revision. (Show Details)Nov 2 2018, 2:49 PM
nani edited the test plan for this revision. (Show Details)
nani marked an inline comment as done.Nov 2 2018, 2:56 PM
nani added inline comments.
binaries/data/mods/public/gui/gamesetup/gamesetup.js
1083 ↗(On Diff #6961)

The patch is not meant to change the GUI.

smiley added a comment.EditedNov 2 2018, 3:24 PM

I am a bit skeptical that the rangemanager is the only thing which needs change. There could be stray 8 or 16 s too.

smiley added a comment.EditedNov 2 2018, 5:53 PM

As mentioned in the lobby, you have to keep in mind the territory manager limitation. Posting here just for the record.
https://code.wildfiregames.com/source/0ad/browse/ps/trunk/source/simulation2/components/CCmpTerritoryManager.cpp$86
I guess that is Gaia + 31 players.

Stan added inline comments.Nov 2 2018, 6:12 PM
source/simulation2/components/CCmpRangeManager.cpp
72

It doesn't sound sane to do that.
Maybe introduce a constant called ABSOLUTE_MAX_NUMBER_OF_PLAYERS that's 32

nani added a comment.Nov 2 2018, 7:36 PM
In D1662#65837, @smiley wrote:

As mentioned in the lobby, you have to keep in mind the territory manager limitation. Posting here just for the record.
https://code.wildfiregames.com/source/0ad/browse/ps/trunk/source/simulation2/components/CCmpTerritoryManager.cpp$86
I guess that is Gaia + 31 players.

Seems m_Territories only gives 4 bits to player ID mask (owner value) , which was defined originally as a player_id_t (uint_32) in CCmpRangeMangaer.cpp giving the false impression that there can be 30 players.

In the end the maximum number of players seems to still be 8 with no margin for more due all player ID masking done with uint8 or uin16 depending of the case.

I think that as long as all data of each player is stored/packed into a single value it will not be really possible to do anything more given the current implementation.

nani abandoned this revision.Nov 2 2018, 7:36 PM
elexis added a comment.Nov 3 2018, 8:56 AM

Grid<u8>* m_Territories; could become one player_id_t sized grid for the owner and one u8 grid for the remaining flags. (But yes, noone said it would be easy.)