After discussing with @elexis we came to the conclusion that a menu item for this is rather unnecessary, but this will still allow people who are not content with the maximum selection size to change it without searching through the code.
Details
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 8847 Build 14511: Vulcan Build Jenkins Build 14510: arc lint + arc unit
Event Timeline
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/375/display/redirect
What I prewrote but not submit last time we spoke on IRC:
According to your testing it is not necessary to make this a user choice.
http://irclogs.wildfiregames.com/2019-08/2019-08-14-QuakeNet-%230ad-dev.log
19:10 < Freagarach> elexis: Back after crash ;) ~203k us for 1000 units (getmultipleselectionstates) vs ~25k us with 100 units.
I don't see any user that says "hey I want to limit my selection size to 50 units".
A mod might want to do that, but this patch makes it a user choice, not a mod choice.
For the user there is practically only a reason to increase it, not to decrease it.
And if the performance allows for basically infinite (as all the other bottlenecks hit sooner), then why not set this to say 3000 units and have every user satisfied without confusing people with a useless option? (its useless if everyone choses the same value)
In the ticket (#2609) there is an argument that the option would be useful for mods, but thats not really true either, because they can just change the constant in their mod, but the userconfig value cannot be changed by the mod and instead gives it into the users hands.
As mentioned most importantly it is in observermode and atlas selection. If there is an 8 player game with 200 pop, one can realistically select 1600 units.
Or imagine a gaia spam map with really many units (there is this one map with proportional attacker count depending on playercount and difficulty setting).
I had tested and successfully crashed the game by spawning too many units, that was the only effect.
I suppose I should spectate an alpha 23 game and try to select that many units when they were managed to be produced without crashing the game.
But also the time in GetEntityState should be measured. I mean the reviewer ought to repeat the experiment to compare the results to your measurement.
203k us - please dont mix units like that :P
k = kilo = 10^3
u = μ = micro = 10^-6
So thats 203ms.
Which is order of magnitude of turn length!
So in that case I would even say it would be disadvisable to grant the user to select so many units.
If some player did that in a MP game, he might, according to these numbers, additionally lag the game.
More detailed numbers would really help more say how far the selection can be pushed without hitting the performance ceiling.
I mean some stats that measure the number depending on amount of units.
This could actually be easier tested if there was a script to test that. One could then just run the test binary and get the results. But seems out of immediate reach.
So I agree with providing more freedom, but we have to ensure it doesn't add signfiicant lag. Perhaps I read the numbers wrong?
If the idea of the patch is to increase the nr 200 to a greater number, then the idea of the patch is correct, but it's time hasn't come yet if selecting many units is so slow that it delays the game (turn length time just for the selection).
Yeah I understand :) The idea of the patch was to make it a user choice, thus the idea is wrong, as you noticed :)