Page MenuHomeWildfire Games

Gamesetup - Optionally allow players to setup the game
Changes PlannedPublic

Authored by Imarok on May 6 2017, 12:49 AM.

Details

Reviewers
None
Trac Tickets
#3806
Summary

Allow players to change some options. The host can control with a setting which options players can change (none/color/color & civ/color, civ & team/all).
This patch is currently WIP.
(Seems to work for everything besides the mapSelection dropdown. I know, this contains one commented warning for debugging atm.)

Test Plan

Test every setting is changed correctly and let the host send a correct gameAttributes message. Players shouldn't not be able to modify settings they aren't allowed to.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
https://svn.wildfiregames.com/public/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 1478
Build 2334: Vulcan BuildJenkins
Build 2333: arc lint + arc unit

Event Timeline

Imarok created this revision.May 6 2017, 12:49 AM
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.May 6 2017, 12:49 AM
Imarok updated the Trac tickets for this revision.May 6 2017, 12:50 AM
Imarok edited the summary of this revision. (Show Details)
Imarok edited the summary of this revision. (Show Details)May 6 2017, 1:02 AM
elexis added a subscriber: elexis.May 6 2017, 1:14 AM

Perhaps this would be better off not introducing a new method to change a setting, but waiting for a gamesetup rewrite x) so that only the settings (but not derived settings) are saved?

Then every client could (attempt to) broadcast gamesettings, the NetServer could then broadcast these with the sender ID and then clients could decide which of these settings they accept and which to reject.
This way the NetServer is still gamesetting agnostic, has minimal changes and only one function to send gamesettings.

binaries/data/mods/public/gui/common/settings.js
36

that file wasnt uploaded

Is the build server down or broken, I haven't got any new pyrogenesis.exe and I've seen that a lot has changed lately?

elexis added a comment.May 6 2017, 2:56 AM

Is the build server down or broken, I haven't got any new pyrogenesis.exe and I've seen that a lot has changed lately?

I guess that question is not related to the discussion of the patch, but yes, we're working on it.

Vulcan added a subscriber: Vulcan.May 6 2017, 6:36 AM

Build is green

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

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

Imarok updated this revision to Diff 1682.May 6 2017, 12:26 PM

Unify change of settings

Owners added a subscriber: Restricted Owners Package.May 6 2017, 12:26 PM
Vulcan added a comment.May 6 2017, 1:13 PM

Build is green

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

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

Imarok planned changes to this revision.Jun 16 2017, 7:29 PM

Might be a bit easier with rP25077

This will need to be completely rewritten after D3714 and the various gamesetup rewrites that happened in between, of course, but it should now be reasonably easy to do.

I think using a different message is a good idea -> it can make the C++ enforce that only the controller can send actual gamesetup messages, which is a good idea, as we need a single source of truth to maintain message ordering & thus synching clients

You don't need a custom message class, since you could just pass a string instead of a JS::RootedValue (I may or may not change the existing gamesetup message in the meantime).