Add a cost for unsuccessful bribe attempts. So that you don't this check for bribe-able units for free. 25% of the bribe cost will be incurred on the player.
Details
Details
- Reviewers
mimo - Commits
- rP19967: Add a cost for unsuccessful bribe attempts.
- Trac Tickets
- #4623
Start a game and research the espionage technologies, then try to spy on a player. If the attempt failed you should lose 0.25 * 500 = 125 metal.
Diff Detail
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
Comment Actions
Note: 1 thing is missing here; how to tell the player that he is going to pay some cost if the operation failed? I think this can be added in the tooltip when hovering over the bribe player button in diplomacy screen, any ideas?
See menu.js of rP19357
| binaries/data/mods/public/simulation/data/technologies/spy_counter.json | ||
|---|---|---|
| 10 ↗ | (On Diff #2502) | FailedSpyCostMultiplier is 0.25 * 0.375 after this, not 0.375 right? |
Comment Actions
Build has FAILED
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (306 tests).WARNING: JavaScript warning: simulation/components/Player.js line 63 reference to undefined property this.template.FailedSpyCostMultiplier In TestComponentScripts::test_scripts: ../../../source/test_setup.cpp:134: Error: Test failed: L"Stack trace:\n@simulation/components/tests/test_Player.js:66:1\nExpected equal, got NaN !== 0.25" ................................................................................................................................................................................................................................................................................................................ Failed 1 and Skipped 0 of 306 tests Success rate: 99%
Link to build: http://jw:8080/job/phabricator/1527/
See console output for more information: http://jw:8080/job/phabricator/1527/console
Comment Actions
Update the tooltip for the bribe button to show that a failed bribe will have a cost.
Also fixed a failed test case in test_player.js
Comment Actions
Yes, I also think it should go here. Just not sure how this should be presented? I updated the diff with a proposal, please tell me if its okay to present it this way or if there is something better.
| binaries/data/mods/public/simulation/data/technologies/spy_counter.json | ||
|---|---|---|
| 10 ↗ | (On Diff #2502) | The base price is 0.25, shouldn't it increase 50% with this tech? |
| binaries/data/mods/public/simulation/data/technologies/spy_counter.json | ||
|---|---|---|
| 10 ↗ | (On Diff #2502) | As elexis said, "multiply" means that you multiply the raw value by this quantity, not what you want. You should multiply by 1.5 as is done for SpyCostMultiplier. But anyway, why not take 0.25 of the full spy cost (not of the raw cost from template). In that way, you would not have to modify FailedSpyCostMultiplier each time the SpyCostMultiplier changes. IMO, it's not important for gameplay (when this tech is available, all players should already have traders or they will certainly loose the game) and could even just be a constant without need to have it in all player's template: that would simplify this patch a lot. |
| binaries/data/mods/public/gui/session/menu.js | ||
|---|---|---|
| 546 ↗ | (On Diff #2506) | why changing it here? i don't care if round or floor, but should be done consistently with VisionSharing. |
| binaries/data/mods/public/simulation/data/technologies/spy_counter.json | ||
| 10 ↗ | (On Diff #2502) | usually, when asking a question, i expect an answer! explain why such complications is needed compared to a simple approach (25% of the modified cost instead of a modified % of the original cost). Futhermore, with your approach, you'd have still to supplement the patch as if both multipliers can live an independent life, you should take the max of both costs when checking if a bribe is possible (you should never assume what a modder will do). |
Comment Actions
Thanks, the patch looks much nicer (and simpler) now. There are still two possible enhancements if you are up to:
- put the 0.25 in a template if a mod want to change it. Most logical would look to me in VisionSharing where we already have the Bribable and Duration properties.
- addSpy and IncurUnsuccessfulBribeCost share some code for the bribe cost. Could be put in a common fonction.
Comment Actions
Sure, I will make a common function to calculate the cost of a bribe. For the first 1st one, IMO we can't add it to the VisionSharing template, cause its a components associated with the units that can be bribed. The cause for a failed bribe is that we don't have a unit that can be bribed and therefore we can't store this value in that template.
That mainly what I thought when I added it to the player's template in the first place.
What about adding it in the spy template, where the cost of the spy is defined?
| binaries/data/mods/public/simulation/data/technologies/spy_counter.json | ||
|---|---|---|
| 10 ↗ | (On Diff #2502) | Okay got it. |
Comment Actions
yes, that's what i meant, but i was not clear sorry, when i said with the Duration property.
| binaries/data/mods/public/gui/session/menu.js | ||
|---|---|---|
| 545 ↗ | (On Diff #2530) | (D23 should have touched this line and some others too, same applies to one new line introduced here, but I can fix that elsewhere) |
| 556 ↗ | (On Diff #2530) | sprintf(translate("A failed bribe will cost you: %(cost)s") so that it will work with right-to-left and other unexpected languages |
| binaries/data/mods/public/simulation/components/VisionSharing.js | ||
| 128 ↗ | (On Diff #2530) | (no whitespace in empty lines) |
| binaries/data/mods/public/simulation/components/tests/test_VisionSharing.js | ||
| 112 ↗ | (On Diff #2530) | Engine.LoadHelperScript("Commands.js"); so that we don't have to carry around this half copy and can even test the helper function |
| binaries/data/mods/public/simulation/helpers/Commands.js | ||
| 1688 ↗ | (On Diff #2530) | 1 space before the * |
| binaries/data/mods/public/gui/session/menu.js | ||
|---|---|---|
| 553 ↗ | (On Diff #2530) | you can use template or modifiedTemplate instead of Engine.GetTemplate("special/spy") |
| binaries/data/mods/public/simulation/components/VisionSharing.js | ||
| 13 ↗ | (On Diff #2530) | As you say, it's a ratio of costs rather than a multiplier. FailureCostRatio or something like that would look to me more adequate. |
Comment Actions
There is still the comment on line 552. No need to make a new call to Engine.GetTemplate
Apart from that, looks good to me, but need to wait for after A22 is released to be commited as we are in feature freeze now.
Comment Actions
@mimo the templates already present are formatted; contain only small set of attributes and GetTemplateData is used elsewhere, so I assume it filter the template for only specific attributes.
| binaries/data/mods/public/gui/session/menu.js | ||
|---|---|---|
| 553 ↗ | (On Diff #2530) | GetTemplateData("special/spy"); used to get template doesn't return all attributes in the template only a subset. |
Comment Actions
With some delay because of holidays!
I missed the fact that GetTemplateData was used for the first case (i'll add a comment when commiting).