Page MenuHomeWildfire Games

Add spying to the game
ClosedPublic

Authored by mimo on Feb 1 2017, 9:21 PM.

Details

Summary

With https://code.wildfiregames.com/rP19175 it is now easy to play with vision at the entity level. I've used it to test the addition of spying into the game. The basic idea is simple: you have first to research a tech (available in phase 3, espionage), and then you can bribe a random unit (defined as Bribable in its template) from a chosen player and share its vision during 10s.
There is also an option in the gamesetup to enable this feature (off by default for the time being), analoguous to the treasure disabling option.
This patch uses temporarily the coinage.png tech image for the espionage technology, but a better one would be nice. In addition, the button in the diplomacy panel to request a spy uses the economics.png icon, but a better one would be nice too.
In the current version, only traders (land and naval ones) are bribables.

Test Plan

Check that everything works as expected.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 594
Build 940: Vulcan BuildJenkins
Build 939: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
elexis added inline comments.Feb 22 2017, 4:24 AM
binaries/data/mods/public/gui/gamesetup/gamesetup.xml
410–420

(double negations should be avoided, see #4493, but I agree with the argument to have it consistent with the disableTreasure option. We can invert the boolean in 4493)

binaries/data/mods/public/gui/session/diplomacy_window.xml
4

(Merge conflict incoming with D88 but won't be hard to solve for him, just adding 10px or something)

binaries/data/mods/public/gui/session/menu.js
317

(This call is actually needed as it updates the dialog even before the next sim update has arrived, which can take at an arbitrary amount of time if the game is paused or lagging and since the dialog isn't updated if closed)

337

(guess I should have moved that variable to diplomacyFormatAttackRequestButton, as you are right that the subfunctions should determine specific uncommon hidden logic themselves)

345

TODO: We want that button disabled but not hidden if we don't have the tech researched, so we can read the tooltip (which also states that it requires Espionage. Therefore we don't need the hasSharedLos variable at all.

(And if the button was to be hidden if we don't have the tech, it shouldt that be hidden if ... || !hasSharedLos)

(The isCeasefire makes me question whether we should prohibit the bribing while ceasefire is active. The answer is no though :P)

Do these GUI checks have equivalent simulation checks? Guess players would only shoot themselves in the foot if they would send such a command for an ally or themselves, so not important.

526

(Urgh, disabledTemplates is an object not an array, so this is a correct check even)

532

unit -> trader? As we were considering to add a new button if we want other units to be bribed and as the GUI nowhere states that only traders are affected and players will totally not get that that's the case!

(There is also no hint that players have to use the diplo panel for that feature, but as that is opened frequently even by newbies, they will likely discover that on their own)

"during" seems wrong, perhaps 'share its vision briefly'?

(Notice we can't display the exact timespan as that can differ depending on the template.)

569

tiny todo: onPress (engine doesn't complain, still preferable case and we should fix all occurances sometime)

binaries/data/mods/public/simulation/data/technologies/unlock_spies.json
5

Seems a bit expensive, maybe 500 food 500 metal? Discussions would be welcome, in particular with those who play/ed this game frequently

9

"and" -> "to"?
units -> traders (as the description already adds that limitation and as players should be aware what this thing actually does)

10

TODO: Upgrade sound not really fitting, why not alarmresearchtech_1.ogg ?

binaries/data/mods/public/simulation/helpers/Commands.js
754

TODO: cmd.target is associated with entities in other commands like attack, heal, repair, garrison and more.

The right name would be cmd.player (like with tributes and diplomacy changes) in my opinion. (The attack request one has that collision too)

758

tiny todo: avoid variable which is used only once

let ent = pickRandom(rangeManStuff.filter(...))

\n

832

tiny todo:

Do we need a function for this? It's only used in one place and not sure if it's going to be reused

838

TODO: markForTranslation!

binaries/data/mods/public/simulation/helpers/Player.js
123–129

todolittle: Seems more clean to concat to disabledTemplates

binaries/data/mods/public/simulation/templates/special/spy.xml
12

TODO: 1500 metal is very expensive and doesn't relate to the gain of the vision (seen on the screenshot of a moving unupgraded trade cart)

Discussed this feature with some lobby fanatics @borg- @Grugnas and @Hannibal_Barca and they said they likely wouldn't use it even if they had thousands of resources in the bank. Making Citizen Soldiers bribable would be much more useful borg said, which I agree with, but on the other hand I'm perfectly fine with seeing bribable traders in Alpha 22 and if required, change it for Alpha 23.

We have to think about the use case of this feature and that most of the time, we can send in two or three disposable citizen soldier cavalry (100 food 40 wood each) that are likely to survive and reach far into the enemy land (if its not a map whose access is limited by few chokepoints or a fully walled city). If the city is fortified and well defended, cavalry champions (in particular if speed upgraded at the corral) costing 250 food 100 metal may still yield much more vision at a lower price. Even a hero unit costing about 100 food 200 metal could be used.

I propose 600 metal, which is still quite expensive (4 expensive champion or 8 cheap champions or 3 battering rams) and prevents overusage of the tech.

17

todolittle:

Afaik this should be a VisibleClass, so that it appears in the selection details, informing players which units can become spies

Somewhere there was a list of classes that must be extended iirc

This revision now requires changes to proceed.Feb 22 2017, 4:24 AM
elexis added inline comments.Feb 22 2017, 4:28 AM
binaries/data/mods/public/simulation/templates/special/spy.xml
17

Identity.js

mimo added a comment.Feb 22 2017, 12:27 PM

Thanks for the extensive review. I will provide a new patch soon.

binaries/data/mods/public/gui/gamesetup/gamesetup.xml
416

ok

binaries/data/mods/public/gui/session/diplomacy_window.xml
71

I guess it would be nicer to increase a little bit the gap between tribute and attack-request (so that it is the same as between the A-N-E and tributes, and have the same one between attack and spy.

binaries/data/mods/public/gui/session/menu.js
345

The idea here is to have the same behaviour as for the attackRequest: the button is hidden for allies, so here it is also hidden for allies when you have the sharedlos as spies are then useless.

532

but trader is specific to the public mod. Others could do differently, so the tooltip have to stay generic. Correct one would be "Bribe a random bribable unit", but that should be implicit.
i'm also not too fond of the 'share its vision briefly' as the duration could be changed in the spy.xml.

569

ok, and will fix a few other occurences of it in the same file.

binaries/data/mods/public/gui/session/messages.js
517–518

ok

binaries/data/mods/public/gui/session/session.js
851

ok

binaries/data/mods/public/simulation/components/VisionSharing.js
8

ok

9

Usually? all BuildTime or tech ResearchTime are in second. But i agree that we always have this ambiguity when reading templates which is not nice. Maybe we should decide on one time convention at least for future additions. I've no strong opinion about second or millisecond.

141

while scaling is not really needed with bribable traders (land and naval traders have nearly the same vision), it would be unfair to pay the same price for a female (vision=32) and a cav (vision=90) if we add more bribable units. We could replace this scaling by a price depending on vision of the unit which will be selected, but i'm not sure it is worth the complication.
Permanent spies is not the purpose of this patch (i've written the code as generically as possible to allow it, but never tested it and that will require the addition as a new permanentSpy template or the modification of the current spy template).

binaries/data/mods/public/simulation/data/technologies/unlock_spies.json
5

yes, agreed that it's a bit expensive, i will switch to 500-500-300-300 (or 500-500-0-500 if preferred?)

9

ok for and -> to
i'm not convinced that the tooltip of this tech should have any apriori on which units are bribable.

10

ok

binaries/data/mods/public/simulation/helpers/Commands.js
754

ok

758

agree to avoid it in general, but here i think it make it more readable.

832

ok that was copied from other notifyFailure in this function, but agreed that it's not needed.

838

ok

binaries/data/mods/public/simulation/helpers/Player.js
123–129

For readability, i find clearer to add it separately. And that avoid some useless array manipulation from concat (even if it doesn't matter here) as in the player component these are objects not arrays.

binaries/data/mods/public/simulation/templates/special/spy.xml
12

agreed that it is too expensive. I proposed in the forum to decrease it to 1200, but let's bargain it at 900 (as you propose 600). But anyway, i expect that we will have a balance patch after a bit of testing.
On the other comments, i don't believe this feature will be heavily (if any) used in competitive games. I rather see it useful for players who like longer games and appreciate to take time to build their cities and armies. So although I value inputs from "competitive players" which are very useful, the details of features should not be dictated by their needs.

17

No because no unit will ever have this template. If there is some information to make visible, it is the Bribable one. I will add it in the Trader's VisibleClass template.

fatherbushido added inline comments.Feb 22 2017, 1:10 PM
binaries/data/mods/public/simulation/templates/special/spy.xml
12

"The details of features should not be dictated by their needs" even more from "lobby fanatics".

Imarok added a subscriber: Imarok.Feb 22 2017, 1:47 PM

After a successful bribe the bribe unit should be focussed. Otherwise its difficult to find the bribed unit.
It would be nice to not close the diplomacy dialog, when the bribe fails.

binaries/data/mods/public/simulation/helpers/Commands.js
757

What about filtering out the units that are already spying for the player?
(So that no unit gets bribed twice without a benefit for the player)

It seems like your patch breaks the tooltip when you try to tribute lets say 1500 food at once with shift clicking multiple times.

In D117#5897, @Imarok wrote:

After a successful bribe the bribe unit should be focussed. Otherwise its difficult to find the bribed unit.

Agree! In particular since the time is running out. Can be done with setCameraFollow(ent); (potentially also g_Selection.reset(); g_Selection.addList([ent] false, false); but probably not)

binaries/data/mods/public/gui/session/diplomacy_window.xml
71

ack

binaries/data/mods/public/gui/session/menu.js
345

Ah yes, I was confused that the button is visible to allies, but it's indeed hidden if sharedlos is researched, ack.
(The term might still be moved to that function as updateDiplomacyPanel() so as to reduce feature specific logic)

532

Okay, good enough as the tech explicitly refers to traders and we indeed don't want to complicate things for modders.

binaries/data/mods/public/simulation/components/VisionSharing.js
141

Shouldn't the template specify the duration, thus the use/cost ratio?
Permanent spies, well, guess that could be added by those who want it then.

binaries/data/mods/public/simulation/data/technologies/unlock_spies.json
5

pickRandom :-)

9

Why does the description do that then?

binaries/data/mods/public/simulation/helpers/Commands.js
758

(Not sure why let ent = pickRandom(cmpRangeManager.GetEntitiesByPlayer(cmd.target).filter(ent => { would be harder to read, but not important)

binaries/data/mods/public/simulation/helpers/Player.js
123–129

Agree. Didn't read properly, if it were two disabled templates it could have become

if (settings.DisableSpies)
   disabledTemplates.push("temp1", "temp2")
binaries/data/mods/public/simulation/templates/special/spy.xml
12

The cost 600 is still a significantly high, 900 very high and I'm not sure casual or new players have the resources for that.
The use is only significant if the enemy has fully walled their city.

IMO the trailer should show this feature as it will be a highlight of the new alpha and I don't want the majority of players to become disappointed when they notice the cost/use ratio.

Maybe @scythetwirler has an opinion about the price? (900 metal for 15 seconds visibility of a trader being proposed)

Also fanatic seems to be the right word: https://en.wikipedia.org/wiki/Fan_(person)

mimo added a comment.Feb 22 2017, 9:03 PM
In D117#5900, @Imarok wrote:

It seems like your patch breaks the tooltip when you try to tribute lets say 1500 food at once with shift clicking multiple times.

In D117#5900, @Imarok wrote:

It seems like your patch breaks the tooltip when you try to tribute lets say 1500 food at once with shift clicking multiple times.

good catch, will fix it by not updating the tribute buttons which don't need to be and should not be when mass-tributing

mimo added a comment.Feb 22 2017, 9:16 PM
In D117#5897, @Imarok wrote:

After a successful bribe the bribe unit should be focussed. Otherwise its difficult to find the bribed unit.
It would be nice to not close the diplomacy dialog, when the bribe fails.

Yes, but i was not sure if people would agree changing the focus without the player agreement, and using CameraFollow as suggested by elexis would be an easy change, but what after: do we follow it for the whole spy duration and then go back to the original camera view? or let the player change it back when it wants to?
As it is quite independent of the current patch, I'd rather have it in a following "correction patch" when the idea has matured a bit more and we reach an agreement.

binaries/data/mods/public/simulation/helpers/Commands.js
757

ok, i will add a filter for units already sharing los with the player.

In D117#5964, @mimo wrote:
In D117#5897, @Imarok wrote:

After a successful bribe the bribe unit should be focussed. Otherwise its difficult to find the bribed unit.
It would be nice to not close the diplomacy dialog, when the bribe fails.

Yes, but i was not sure if people would agree changing the focus without the player agreement, and using CameraFollow as suggested by elexis would be an easy change, but what after: do we follow it for the whole spy duration and then go back to the original camera view? or let the player change it back when it wants to?

I'd say just follow and select the spy trader once and then do nothing else. (So the player can decide what he wants to do)

mimo updated this revision to Diff 579.Feb 22 2017, 10:45 PM
mimo edited edge metadata.

Fixes most comments

Build is green

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

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

elexis added a comment.EditedFeb 25 2017, 7:12 AM

(rebase is trivial)

Following the bribed entity is the capstone to this patch and IMO should be done in the same move.

On successful bribe, a GUI notification of a new "spy-response" type can be sent that contains the playerID that sent the request and the entity ID of the bribed unit.
That notification arrives in messages.js and if g_ViewedPlayer is that player, the follow command can be executed (this way it works in observermode too).

(The dialog should be closed in all cases as we want to avoid players clicking multiple times per turn on it until receiving the notification)

binaries/data/mods/public/gui/session/diplomacy_window.xml
70

Ack on the new size

binaries/data/mods/public/gui/session/menu.js
320

Spaces, somewhat meaningless variable name, how about updateTributeButton?

465

thx

binaries/data/mods/public/simulation/components/VisionSharing.js
151

trailing whitespace

165
/**
 *
 */
binaries/data/mods/public/simulation/helpers/Commands.js
762

(Is this visible in 1024x768 if the diplo dialog is opened? Might not be in the future. Anyway, ack)

binaries/data/mods/public/simulation/templates/special/spy.xml
12

(Yadda yadda, you have misunderstood my point. IMO 1500 metal is too much for all players independent of skill. Competitive players have more metal and might use it more often than newcomers. I have pinged people as I want a discussion, not a dictatation. I think experienced players are more capable to see shortcomings than than inexperienced players. The balancing requirements are mostly the same for all players. Where are those 900 metal coming from? It's a 1/5 metal mine and military units are expensive and mandatory. I don't insist on anything, chose as you wish, I just want to give a recommendation. 900 is much better than 1500 and it opens actual use cases in late game if one has thousands of excess metal in the bank (which is actually more likely for experienced than inexperienced players). (It's nothing emotional, I just think its the truth,, so I feel obliged to state it))

mimo added a comment.Feb 25 2017, 11:08 AM
In D117#6294, @elexis wrote:

(rebase is trivial)

Following the bribed entity is the capstone to this patch and IMO should be done in the same move.

Why should it be in the same patch? previously (when more people shared my ideas of a sane development, while it seems that now there is a drift towards committing only complete and full features patches which is just unproductive and a misuse of a svn development), the goal was to separate big patches in several smaller parts more manageable and easier to review. That's typically something applicable for that feature: the code part involved is nearly disconnected from the rest.

binaries/data/mods/public/gui/session/menu.js
320

ok for spaces, not about "meaningless". The important thing here is to say if we are really updating the panel, or opening it. And in the future, we may have other buttons which do not need to be updated. So i would be ok for "init" or something like that, but certainly not updateTributeButton.

binaries/data/mods/public/simulation/templates/special/spy.xml
12

The use of this feature should stay exceptionnal, not be overused, otherwise it is all the concept of vision which could be distorted. So the price should stay big enough.
But once again, i think i have said several times here or on the forum that:

  • it would be nice to complement it by a counter-espionnage tech which increases the price for civs spying you. When that is done, we could decrease the initial price
  • all the numbers (prices, durations, ...) should be experimented really in game, and only then we can have a balance patch when everything is in place. Now we are just of the level of implementing the functionnality.

That's a bit connected to what i answered to the comment on "following the bribed unit": we do not have to wait to have a fully, complete and well-balanced patch to test a new feature.

mimo updated this revision to Diff 609.Feb 25 2017, 11:27 AM

Update patch

Build is green

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

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

elexis accepted this revision.Feb 25 2017, 5:30 PM

I'm thankful that you addressed the issues noticed. (My stance of the new commit guidelines is also nack and I wouldn't mind not being the only one complaining about it in the relevant topics or staff meetings)
The affected update for the unit following is really small:

(Not a proper incremental diff)

diff --git a/binaries/data/mods/public/gui/session/messages.js b/binaries/data/mods/public/gui/session/messages.js
index c01d747..a74c526 100644
--- a/binaries/data/mods/public/gui/session/messages.js
+++ b/binaries/data/mods/public/gui/session/messages.js
@@ -378,6 +378,11 @@ var g_NotificationsTypes =
                        "targetIsDomesticAnimal": notification.targetIsDomesticAnimal
                });
        },
+       "spy-response": function(notification, player)
+       {
+               if (g_ViewedPlayer == player)
+                       setCameraFollow(notification.entity);
+       },
        "dialog": function(notification, player)
        {
                if (player == Engine.GetPlayerID())
diff --git a/binaries/data/mods/public/simulation/helpers/Commands.js b/binaries/data/mods/public/simulation/helpers/Commands.js
index 2145d38..789be85 100644
--- a/binaries/data/mods/public/simulation/helpers/Commands.js
+++ b/binaries/data/mods/public/simulation/helpers/Commands.js
@@ -767,6 +767,33 @@ var g_Commands = {
                        });
        },
 
+       "spy-request": function(player, cmd, data)
+       {
+               let cmpRangeManager = Engine.QueryInterface(SYSTEM_ENTITY, IID_RangeManager);
+               let ents = cmpRangeManager.GetEntitiesByPlayer(cmd.player).filter(ent => {
+                       let cmpVisionSharing = Engine.QueryInterface(ent, IID_VisionSharing);
+                       return cmpVisionSharing && cmpVisionSharing.IsBribable() && !cmpVisionSharing.ShareVisionWith(player);
+               });
+               let ent = pickRandom(ents);
+               if (ent)
+               {
+                       Engine.QueryInterface(ent, IID_VisionSharing).AddSpy(cmd.source);
+
+                       Engine.QueryInterface(SYSTEM_ENTITY, IID_GuiInterface).PushNotification({
+                               "type": "spy-response",
+                               "players": [player],
+                               "entity": ent
+                       });
+               }
+               else
+                       Engine.QueryInterface(SYSTEM_ENTITY, IID_GuiInterface).PushNotification({
+                               "type": "text",
+                               "players": [player],
+                               "message": markForTranslation("There are no bribable units"),
+                               "translateMessage": true
+                       });
+       },
+
        "dialog-answer": function(player, cmd, data)
        {
                // Currently nothing. Triggers can read it anyway, and send this

Can be added to the patch or not, idc, but should be done this alpha.
About the design part of that: One could argue that allies should also focus on the spied trader on first sight, so that the entire team gets notified, but actually disagreeing with that as the allies could be in the middle of a fight, so the diff should be ok like that.

binaries/data/mods/public/gui/session/menu.js
320

say if we are really updating the panel, or opening

ok then

This revision is now accepted and ready to land.Feb 25 2017, 5:30 PM
fatherbushido added inline comments.Feb 25 2017, 6:05 PM
binaries/data/mods/public/simulation/components/VisionSharing.js
110

Just note for myself:
(When msg.to == -1,
The CheckVisionSharings() calls will just set this.shared to an empty Set() so it doesn't do weird stuff.
So it's ok).

mimo updated this revision to Diff 624.EditedFeb 26 2017, 8:12 PM

update patch as fatherbushido noticed VisionSharing does not need to listen to OnDiplomacyChanged.
Thanks elexis for the code to follow the bribed unit, i'll make a new patch as soon as this one is commited (currently waiting for some tests on VisionSharing component that fatherbushido proposed to provide).

Build is green

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

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

This revision was automatically updated to reflect the committed changes.

Thanks for the nice feature and for going through all these iterations and remarks!

Focus on spies: D169
Vision test: D170

Nice idea the one to spy enemies, it can really come in handy expecially in team games where the trade is really essential. Since the "Espionage" is a tech aviable in the market and the bribe is a feature aviable after have researched for that technology, have you planned to add more features like bribing or even adding an extra effect to the current bribing? For example while in some situations could be useful have a look at enemy base and organization by bribing an enemy trade cart, in some situations could even come in handy having other useful infos like enemy current population or resources amount.