Details

Reviewers

Commits

rP20687: Add support for diplomacy changes for wonder victory, make notifications more…

Summary

Changing diplomacy has no affect on the timers in wondermode, so when a wonder is build by one of your allies and you (or (s)he) declares war, you still win when the timer has run out....
Fixing this with equivalent behaviour as D972

Support teams for wonder just as relic victory has, however a wonder is always owned by a specific player thus setting that in the string.

Test Plan

Perform the actions as above

check English
run a wonder game and change perspective
run the replay and do same

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

bb created this revision.Sep 7 2017, 12:34 AM

Owners added a subscriber: Restricted Owners Package.Sep 7 2017, 12:34 AM

bb added inline comments.Sep 7 2017, 12:35 AM

binaries/data/mods/public/maps/scripts/WonderVictory.js
27 ↗	(On Diff #3549)	could make a new function
44 ↗	(On Diff #3549)	didn't found anything better than his or her so went with that
56 ↗	(On Diff #3549)	your team includes the player self so not setting "you and your team"

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!
Checking XML files...

http://jenkins-master:8080/job/phabricator/1986/ for more details.

Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/maps/scripts/WonderVictory.js
|  25| »   let·cmpEndGameManager·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_EndGameManager)
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jenkins-master:8080/job/phabricator_lint/501/ for more details.

Harbormaster completed remote builds in B3048: Diff 3549.Sep 7 2017, 1:41 AM

elexis added a subscriber: elexis.Oct 22 2017, 6:23 PM

elexis added inline comments.

binaries/data/mods/public/maps/scripts/WonderVictory.js
44 ↗	(On Diff #3549)	his or her = their. Don't like that either as it sounds wrong for any gender. How about %(player)s team has a wonder and will win? Besides, I find the "has" a bit dry. (built or captured would be better, besides that we don't know which of the cases is true)

bb added inline comments.Oct 22 2017, 6:31 PM

binaries/data/mods/public/maps/scripts/WonderVictory.js
44 ↗	(On Diff #3549)	their is wrong following http://www.kentlaw.edu/academics/lrw/grinker/LwtaGender_Neutral_Language.htm can has => owns?

elexis added inline comments.Oct 22 2017, 6:41 PM

binaries/data/mods/public/maps/scripts/WonderVictory.js
44 ↗	(On Diff #3549)	Its correct according to https://en.wikipedia.org/wiki/Singular_they but I avoid it too. owns is probably better

has => owns
have => own
their => other sentence proposed by elexis + " 's "

Build is green

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/2150/ for more details.

Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/maps/scripts/WonderVictory.js
|  25| »   let·cmpEndGameManager·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_EndGameManager)
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
Executing section XML GUI...

http://jenkins-master:8080/job/phabricator_lint/618/ for more details.

Harbormaster completed remote builds in B3446: Diff 3938.Oct 22 2017, 8:15 PM

Owner allies can still see the timer going on despite a player loses the wonder.
I wonder if displaying the owner team number also may be useful to recognize which team is gonna win in case of more than 2 teams in a game.

yup wrong message name and missing reset, there for the allies message, nice find

For the teamNumber, can be for locked teams, but we should support diplo also, and there we don't really have teamNumers, so wouldn't we run into some inconsistencies there, when adding it only for locked teams?

fix ally message

Build is green

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/2157/ for more details.

Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/maps/scripts/WonderVictory.js
|  26| »   let·cmpEndGameManager·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_EndGameManager)
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
Executing section XML GUI...

http://jenkins-master:8080/job/phabricator_lint/625/ for more details.

Harbormaster completed remote builds in B3469: Diff 3956.Oct 24 2017, 7:43 PM

allies.length is always true since it always contains that one player, no?
strings are a bit false in case of diplomacy games where A has a wonder and mutual allies B and C, but B is not mutually allied with C. Not trivial to write strings that are both accurate and appealing
is there are lastmanstanding check missing?
(player color in case that got broken, didnt test)

In D881#43752, @elexis wrote:

allies.length is always true since it always contains that one player, no?

No (see that we fill it with the Exclusive thing), but there is a bug with it: the else ternary statement should be an empty array

strings are a bit false in case of diplomacy games where A has a wonder and mutual allies B and C, but B is not mutually allied with C. Not trivial to write strings that are both accurate and appealing

The own could sortof easily be fixed with s/team/allies
the others message is not too wrong (bb's team can be seen as me with my allies)
the allies message runs in some personalization issues, maybe "bb has a wonder, and bb and their allies will win in..." (albeit their is grammatically wrong!)

is there are lastmanstanding check missing?

enlighten me where (we check for alliedVictory already)

(player color in case that got broken, didnt test)

IIRC was ok, but will test

singular they grammatically wrong!

-> bb's allies

is there are lastmanstanding check missing?

alliedVictory

Ah that's why I didn't see it.

bb updated this revision to Diff 4718.Dec 11 2017, 6:06 PM

bb retitled this revision from Support teams for wonder victory messages to Add cdiplocamy changes support for wonder victory and add relevant messages.

bb edited the summary of this revision. (Show Details)

bb edited the test plan for this revision. (Show Details)

bb retitled this revision from Add cdiplocamy changes support for wonder victory and add relevant messages to Add diplomacy changes support for wonder victory and add relevant messages.Dec 11 2017, 6:20 PM

Executing section Default...
Executing section Source...
Executing section JS...

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

Harbormaster completed remote builds in B4092: Diff 4718.Dec 11 2017, 7:50 PM

elexis added inline comments.Dec 12 2017, 5:10 PM

binaries/data/mods/public/maps/scripts/WonderVictory.js
114 ↗	(On Diff #4718)	Should the messages become an array of 3 items that can be iterated to DeleteTimeNotification?

bb added inline comments.Dec 12 2017, 5:16 PM

binaries/data/mods/public/maps/scripts/WonderVictory.js
114 ↗	(On Diff #4718)	There indeed seems no point in having the keywords, since when changing one of the messages all are changed, so yup

bb mentioned this in D1137: GetNumPlayers and related cleanup.Dec 13 2017, 12:45 AM

Whenever I see this file I have to think about the fact that all the function names are kind of a workaround to prevent them from interfering with other TriggerScripts (D626).

binaries/data/mods/public/maps/scripts/WonderVictory.js
15 ↗	(On Diff #4718)	`= DeleteWonderVictoryMessages` afaics
31 ↗	(On Diff #4718)	`Set` has nicer existence checks and may be prefered over an Array when we are not going to use array functions on the thing anytime soon. But afaics includes comes with sm45.
54 ↗	(On Diff #4718)	Messages -> Notifications maybe StartWonderVictoryTimer? dunno
58 ↗	(On Diff #4718)	can be inlined (not only means that we have one line less, but also that the affected lines are easier to lookup)
85 ↗	(On Diff #4718)	(\n{
88 ↗	(On Diff #4718)	Should we prefer the proper noun Wonder?
95 ↗	(On Diff #4718)	},\n
114 ↗	(On Diff #4718)	We can keep it as an object too, the point is just that we can the statements with a for-loop

bb marked 7 inline comments as done.Dec 25 2017, 9:40 PM

bb added inline comments.

binaries/data/mods/public/maps/scripts/WonderVictory.js
15 ↗	(On Diff #4718)	there is a for loop in that function...

bb updated this revision to Diff 4953.Dec 25 2017, 9:40 PM

remove some bugs and duplication (based on comments by elexis)

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

Executing section Default...
Executing section Source...
Executing section JS...

Harbormaster completed remote builds in B4182: Diff 4953.Dec 25 2017, 10:41 PM

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

Executing section Default...
Executing section Source...
Executing section JS...

Harbormaster completed remote builds in B4183: Diff 4954.Dec 25 2017, 10:45 PM

fix and cleanup more

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

Executing section Default...
Executing section Source...
Executing section JS...

Harbormaster completed remote builds in B4184: Diff 4955.Dec 25 2017, 11:15 PM

various cleanups, gamesetup description, slightly changing strings

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/maps/scripts/WonderVictory.js
|  22| »   »   let·owner·=·this.wonderVictoryMessages[ent].playerID
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

Harbormaster completed remote builds in B4185: Diff 4956.Dec 26 2017, 12:38 AM

Very great patch, every line contains multiple enhancements!

Review:
3h static code analysis on IRC. Code reads entirely correct.
I didn't test it, make sure to cover the needed cases (also that case where a new wonder is built after a player has won)

Thank you very much for the endurance and the ideal code :-)

This revision is now accepted and ready to land.Dec 26 2017, 12:45 AM

Adding that vulcan semicolon

Couldn't find a way to bug...

Closed by commit rP20687: Add support for diplomacy changes for wonder victory, make notifications more… (authored by bb). · Explain WhyDec 26 2017, 1:59 PM

This revision was automatically updated to reflect the committed changes.

Add diplomacy changes support for wonder victory and add relevant messages
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 4959

ps/trunk/binaries/data/mods/public/maps/scripts/WonderVictory.js

ps/trunk/binaries/data/mods/public/simulation/data/settings/victory_conditions/wonder.json

Add diplomacy changes support for wonder victory and add relevant messagesClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 4959

ps/trunk/binaries/data/mods/public/maps/scripts/WonderVictory.js

ps/trunk/binaries/data/mods/public/simulation/data/settings/victory_conditions/wonder.json

Add diplomacy changes support for wonder victory and add relevant messages
ClosedPublic
Actions

Revision Contents
Changeset List