Battle Calculator: Allow fighting any player #9532
Conversation
When changing a player, do not set the other player automatically to an enemy. This allows fights between any two players. Moreover, it does not erase the battle configuration in case of a misclick.
Codecov Report
@@ Coverage Diff @@
## master #9532 +/- ##
============================================
- Coverage 28.19% 27.12% -1.07%
+ Complexity 8339 7799 -540
============================================
Files 1317 1200 -117
Lines 81036 79276 -1760
Branches 11040 10940 -100
============================================
- Hits 22845 21506 -1339
+ Misses 56023 55703 -320
+ Partials 2168 2067 -101
Continue to review full report at Codecov.
|
|
It has been a long time since I complained that it was a wrong behaviour for FFA not to allow to calculate battles between allies, as your ally this round can be your enemy subsequently. Despite guessing this being an easy fix, I never found a developer interested in making it (erasing the limitation by which you are not allowed to calculate between allies), so it is nice to see this very old problem being finally solved. Moreover, another thing that I recall to have reported a long time ago is that (as it can fairly easily happen in some FFA), if it happens that everyone is allied with everyone, the battlecalculator becomes simply unusable and errors out (you cannot even open the battlecalculator when everyone is allied with everyone). So, if this problem is still present, this change should fix it too. However, the concept of not allowing battlecalculating between players that are never supposed to fight each other is sound. If you go with simply allowing everyone to battlecalculate against everyone, I can easily see a developer in the future seeing, for example, the battlecalculator in Revised allowing calculating Russians fighting Americans as a wrong allowance so reintroducing what is being eliminated with this change. The point is that, instead of disallowing battlecalculating between players in an allied relationship, the program should disallow battlecalculating within the same alliance. I mean looking at As a side note, there may be some weird map that allows fighting between same-alliance players, but I'm not aware there is any such map currently. The conclusion is that I prefer this change over the current problematic behaviour, yet I would rather prefer keeping the feature but substituting checking for allied relationships with looking at shared alliances. Additionally, I don't feel like the issue of mistakenly erasing your selection for selecting a player you are supposed never to have need to select is correctly addressed by allowing calculating for something you should never need to calculate. This may be better addressed by changing the behaviour of the feature rather than by completely removing it. For example, by making the button starting the process unavailable when all defenders are players of the alliance of the attacker. |
Glad I could help. :-)
Do I understand you correctly that you want to gray out the player entries in the dropdown list if they are in the same alliance?
The reference you posted (https://github.com/triplea-maps/the_pact_of_steel/blob/master/map/games/pact_of_steel_2.xml#L635) states:
I'm not opposed to your suggestion but this sounds to me as you're suggesting to promote a discouraged feature over a preferred one which doesn't seem right to me. Or is relation yet another state two parties can have? If you want some additional notice that allied players (or players in an alliance) are fighting against each other, we could display a small warning sign somewhere, e.g. a yellow exclamation mark icon on the calculate button. |
No.
Yes.
It would be just weird that in all basic games you are allowed to calculate for the Germans fighting the Japanese and such. Whilst I'm not bothered by this nonsense, I think there is a high likehood some developer will, so he/she will somehow disable such option in the future if it is left, possibly introducing or reintroducing issues for non-basic games. Moreover, if some noob sees that he can calculate for the Germans to fight the Japanese, he/she might think that is something which can actually happen.
You'll have to get back to whoever wrote that line to try to have any kind of explanation. I suppose you can easily find out who was the lead developer when 1.3.3.0 was released and try to get back to him/her, asking him/her what he/she exactly meant there. Obviously, to say "it is preferred to use relationships instead of alliances" doesn't make any sense, because you simply have to use alliances for the players panel and victory conditions. So I would say that the closest way to make any sense to the line you quoted is to rewrite it as "it is preferred to use relationships in addition to alliances". I don't think that alliances are currently deprecated, if this is what you mean. In my opinion, any serious program should have a clear list of what is actually deprecated. So, unless you show me something that states that alliances are deprecated, I believe it makes the only sense to assume they are not. Usually, but not necessarily, players of the same alliance all share allied relationships with one an other, yet you can make a game in which all players of the same alliance are at war with one an other. In this case, my suggestion would make you unable to battlecalculate for that, but I think it is a much smaller limitation than being generally unable to battlecalculate for allies which may be not anymore allies in the future. To clarify what I understand (from someone who didn't make the system), the relationships define what is going on between two powers, whereas the alliances define the grouping of powers which win or lose the game together (and can be played all together by only one player). |
|
Thanks for explaining.
Now I understand where you are heading to. So, what are your thoughts on, instead of disabling the button, showing an additional notice that allied players (or players in an alliance) are fighting against each other. We could display a small warning sign somewhere, e.g. a yellow exclamation mark icon on the calculate button and an explanatory tooltip, or – more intrusive – an "Are you sure? Those players are allied." dialog box. or maybe even something else. |
I cannot think of a single map in which players of the same alliance are ever going to fight each other. If you want to leave the option, maybe add it to engine preferences. If you are playing Revised, being asked if you are sure you want to battlecalculate between Germans and Japanese would make just about as much sense as being simply allowed to.
No: you are confusing alliance state with allied relationships. Powers part of a same alliance are not necessarily allied. That you cannot battlecalculate between allied players is how it works now and how I think it should not work: to disable it between powers of the same alliance, not between allied players, is my suggestion. |
|
I was wondering if the calc would work having two allied factions fighting against each other but it appears it does. Checking this out, I found the initial state is a bit odd where both defender and attacker are populated from the same territory: It's hard to say whether that might be 'better' as most calc's will be of similar units vs similar units, so it might save some clicks. On the other hand, selecting an enemy faction first is now an extra step and the typical player will want it to be two opposing factions. By that reasoning it would make sense to keep the initial behavior where when the battle calc window loads that an opposing faction is automatically selected, but if a player changes one side or the other then we do not automatically update the opposing side. WDYT? |
|
As it should be clear from what I've already written, I believe it needs to be clarified what exactly are "two opposing factions" before anyone can reliably agree or disagree with any proposal. Ideally, every game should have a full list of each couple of factions for which there is no need to battlecalculate, or the program should be very smart and read through all the triggers and actions in the game file and sort it out automatically. Realistically, my suggestion is that opposing factions are every couple of so-called players which do not belong to the same alliance (whether or not they are sharing an allied relationship at the start or at any point during the game). As for the example, I think the "Calculate Odds" button should not be actionable when you have a situation like that. By the way, the "-1" things as to mean infinite in the two retreat options are some terrible UI. The second one doesn't even make sense, because, if "-1" is infinite, then "Retreat When X Units Left" would mean "infinite units left"! There should be a tick mark for each of them (never retreating if deselected) and the ability to have only positive integers only if ticked. |
If you select a territory, where you fight against another player that fight is calculated (true for both, if you attack them on their grounds, and if they attack you on your grounds, and even for 3rd partyA vs 3rd partyB). The scenario you highlighted will only happen when you select your own territory with no fight going on. a) you want to calculate a defence on that territory => your units should be the defenders It is impossible to know which you want and setting the calculator up for both (i.e. you are attacker and defender) is probably better than assuming an intention and randomly select an opponent (most of the time the opponent select is the wrong one anyway). |
|
@emmanuel-perlow there are some assumptions that are hard to truly make without data. Given that I would be hesitant to change the existing behavior too much. Players are used to it, for better or worse, and changing that expectation will take players out of the game flow and force some relearning (so better to not change some things unless it really is justified). Battle calc is used very heavily in multiplayer games and rarely is it for a battle where all the units are moved. If anything, because you don't want to give away to your opponent exactly which battles you are considering and secondarily doing all of the movement for the sake of calc is too time consuming. Hence, it's my experience that battle calc almost always starts with 'ctrl-b' on units in one territory and then the other side is manually added or added using 'ctrl-a' or 'ctrl-d'. Also to consider, having the territory come up automatically as attacker or defender makes sense, if it's another players turn then a person is probably more interested in the defensive values and vice versa. Otherwise, there is the 'swap' sides expressly to change things around after the initial 'ctrl-b' press.
Players are used to this, so FWIW that is a reason to keep it how it was unless something much obviously better is found. Second, usually the opposing faction has similar units across any faction, so whether a calc is done vs 'Germany' and not 'Japan' often does not matter. IMO we should keep some of the original behavior but still allow for calc's to be done against arbitrary factions (as implemented here) |
Mimic the legacy behaviour and force the two parties in the initial setup to be non-allied. This legacy behaviour was requested in triplea-game#9532.
Mimic the legacy behaviour and force the two parties in the initial setup to be non-allied. This legacy behaviour was requested in triplea-game#9532.
emmanuel-perlow
force-pushed
the
battle-simulator/allow-fighting-any-player
branch
from
fecbd05 to
7d0d44b
Compare
Mimic the legacy behaviour and force the two parties in the initial setup to be non-allied. This legacy behaviour was requested in triplea-game#9532.
emmanuel-perlow
force-pushed
the
battle-simulator/allow-fighting-any-player
branch
from
7d0d44b to
ebdfe15
Compare
|
Ok, I included some mimicry code. |
| // Mimic the legacy behavior and force the parties to be non-allied modifying either | ||
| // attacker or defender like the old code did). This legacy behavior was requested in #9532. |
There was a problem hiding this comment.
A potential maintainer reading this comment might be curious what "legacy behavior" refers to, it's not clear without context of a PR. Requiring a reader to reference a PR is a smell, PRs are ephemeral (and subsequent PRs can change things).
This comment perhaps should focus on explaining what we are doing in this code, what the behavior is, rather than it is simply "legacy" (albeit, still the current behavior).
...pp/game-core/src/main/java/games/strategy/triplea/odds/calculator/BattleCalculatorPanel.java
Show resolved
Hide resolved
| if (location.isWater() && players.isEmpty()) { | ||
| getEnemy(getAttacker()).ifPresent(this::setDefender); | ||
| } else { | ||
| getEnemy(getDefender()).ifPresent(this::setAttacker); |
There was a problem hiding this comment.
Can you clarify why the battle location and players being empty or not would change whether we need to set the 'defender' or 'attacker'?
There was a problem hiding this comment.
Sure, that's what the old code did. If you asking why the old code chose those conditions and why they chose to alter attacker over defender or vice versa, then you have to ask the person who initially wrote that.
There was a problem hiding this comment.
Walking through this in a debugger, I think the comment no longer agrees with the if/else statement. Essentially the logic is says:
- if we have not found two opposing factions
- if we are water, then the attacker is correct and find an enemy faction
- else, defender is correct and find an enemy faction
Basically this seems to be an extension of the previous logic that is assigning an attacker and a defending player. Hence, it's odd to have a comment stating that we are going to choose two opposing sides here to keep with a users expectations when the previous 30 lines of code are also doing exactly that but for different cases. There is an implication that this could never have been removed and/or the preceding logic is dead code.
Does it make sense why I say the comment and this if statement do not jive together?
I think we are also violating the DRY principle where we do the same thing in multiple places. Notably if we assign an attacker then the same assignment earlier would be a dead store. Seemingly this if block needs to be merged with preceding code for it to make more sense.
There was a problem hiding this comment.
Basically this seems to be an extension of the previous logic that is assigning an attacker and a defending player.
It's not an extension, the same thing was also done by the old code but was well hidden behind a chain of EventListeners and was removed in this PR:
Basically this seems to be an extension of the previous logic that is assigning an attacker and a defending player. Hence, it's odd to have a comment stating that we are going to choose two opposing sides here to keep with a users expectations when the previous 30 lines of code are also doing exactly that but for different cases. There is an implication that this could never have been removed and/or the preceding logic is dead code.
Does it make sense why I say the comment and this if statement do not jive together?
Sorry, can't follow you there. I think maybe the confusion arises from the fact that the code change in the event listener affected this logic? Since this code is now gone, we have to move parts of the event listener logic here in the setup code.
I think we are also violating the DRY principle where we do the same thing in multiple places.
Yes, it does but moving this code into the above stacked if-cases would duplicate this if three times (we cannot join the isWater() and isEmpty() case). I contemplated about it but decided for 2 as it is smaller than 3. Essentially we have a 3x2 grid guarded by ifs and I don't think we can avoid that.
Notably if we assign an attacker then the same assignment earlier would be a dead store.
I can understand your hesitation about dead store but
a) it is already dead store, e.g. attacker get assigned (potentially) multiple times:
- (jepp, that one did in the old code reset the attacker via the event listener)
- (dito)
- (dito)
b) I cannot see how to make a single assignment for attacker/defender in all cases, e.g. the land case would become
if (!location.isWater()) {
defenderCombo.setSelectedItem(location.getOwner());
boolean setAttacker = false;
for (final GamePlayer player : players) {
if (Matches.isAtWar(getDefender(), data.getRelationshipTracker()).test(player)) {
attackerCombo.setSelectedItem(player);
setAttacker = true;
break;
}
}
if (!setAttacker) {
// Currently player has priority if at war.
if (currentPlayer.isPresent() && Matches.isAtWar(getDefender(), data.getRelationshipTracker()).test(currentPlayer.get())) {
attackerCombo.setSelectedItem(currentPlayer.get());
} else {
// find an enemy if possible
Optional<Player> enemy = getEnemy(getDefender());
if (enemy.isPresent()) {
attackerCombo.setSelectedItem(enemy.get());
} else {
// Fall back to current player (they are allied but there are no enemies anyway)
if (currentPlayer.isPresent()) {
attackerCombo.setSelectedItem(currentPlayer.get());
} else {
attackerCombo.setSelectedItem(getFirstPlayer();
}
}
}
}
} else {...}(pseudo code might not compile)
So what should we do?
There was a problem hiding this comment.
Thank you for the response. To answer your question, we either spend quite a bit more time refactoring to get maybe cleaner code, or we live with where we are. I am concerned by both scope creep and ROI if we spend a lot more time now, on this. So, having been given more context, I think we can move on and call things good enough. I re-read the diff and it's quite reasonable.
Mimic the legacy behaviour and force the two parties in the initial setup to be non-allied. This legacy behaviour was requested in triplea-game#9532.
emmanuel-perlow
force-pushed
the
battle-simulator/allow-fighting-any-player
branch
from
ebdfe15 to
ff8d143
Compare
emmanuel-perlow
deleted the
battle-simulator/allow-fighting-any-player
branch
When changing a player, do not set the other player automatically to an
enemy. This allows fights between any two players. Moreover, it does
not erase the battle configuration in case of a missclick.
The later point is for me a pain, as I set up the unit configuration for one
party and then set the other party to the other player and from time to
time is miss the tiny entry with the player and select an ally and just
like that is all the preparation gone.
The first point is important for FFA maps as you might still be allied
with someone but an inevitable war is looming, so you want to be
prepared.
Note: It is not always possible to use a substitute, that is a enemy player
which has the same or similar enough units, to do the calculation as
there are quite some maps with "asymmetric" units, e.g. 270BC.
Most of the time (i.e. n-1 out of n) the automatically selected player was
the wrong enemy, so this feature wasn't that useful after all in my opinion.
Additional Notes to Reviewer
I hope this is the only place where the setting of the other player is done.
Release Note
CHANGE|The battle calculator now allows to select any two players to battle against not only enemies