triplea-game / triplea

TripleA is a turn based strategy game and board game engine, similar to Axis & Allies or Risk.
https://triplea-game.org/
GNU General Public License v3.0
1.33k stars 392 forks source link

Units on transports are hidden from conditions #441

Closed panguitch closed 7 years ago

panguitch commented 8 years ago

Conditions cannot see units on transports, but triggers can affect them.

Example: a condition looks to make sure you still have a particular unit. If you don't, a trigger fires. As long as you have that unit anywhere on the map, you're good. But if it dies, the trigger activates.

However, if you move the unit onto a transport and leave it there, it is effectively invisible, doesn't meet the condition, and the trigger activates.

While playing around with this, I've further discovered that this is just conditions. Units on transports can still be affected by triggers.

Example: A trigger removes all of Unit B from the entire map, if the condition is met that there is no Unit A anywhere on the map. Say you have a Unit B on a land territory, and one sitting on a transport. If you have Unit A on land, nothing happens. But if you put Unit A on a transport and leave him there, then he becomes invisible, the condition is met, and the Unit B is removed from both land and transport.

This should be changed so that conditions can take into account units on transports.

Original forum post: http://tripleadev.1671093.n2.nabble.com/Units-on-transports-are-hidden-from-conditions-td7588107.html Another forum post: http://tripleadev.1671093.n2.nabble.com/directPresenceTerritories-trigger-including-all-the-map-sea-zones-td7591146.html#a7591183

ron-murhammer commented 8 years ago

@panguitch - Yeah, my concern here is if any maps with conditions assume that it isn't checking transports so if we just change it then we could break them.

panguitch commented 8 years ago

I can see that could possibly be a problem. Perhaps make it an optional fix with a new game property: "units on transports are hidden from conditions" which defaults to true if not included in a map. That way legacy maps are unaffected, and new maps can choose.

FrostionAAA commented 8 years ago

This bug really bugs me ;-)

I am having trouble with option name="directPresenceTerritories" value="map" on a map of mine.

Taking into account that there are alteratives to the "map" value, like “originalNoWater” and “controlledNoWater”, I don’t think there are any question about this being a bug- It should look in the Water for units.

If any map have set up their functioning to be dependent on a bug like this, then it is a problem for those maps. Should people not just accept a change and expect bugs to be fixed one day?

Anyway …. Is there some way to investigate what maps make use of "directPresenceTerritories"? And from there investigate if any maps will have problems if this is fixed? Can someone make some sort of Github bot that searches through all maps to see which ones make use of "directPresenceTerritories"?

Also … Is the bug only a directPresenceTerritories + map thing. Or is it related to any and all instances of PresenceTerritories / ExclusionTerritoriest where the condition are looking at the map and forgetting to look at the sea territories?

DanVanAtta commented 8 years ago

Here is the list of maps impacted:

~/work/maps$ grep -lr "directPresenceTerritories" | sed 's|/.*||' | sort | uniq
age_of_tribes
big_world_2
blue_vs_gray
civil_war
dragon_war
elemental_forces
empire
feudal_japan
feudal_japan_warlords
greyhawk_wars
invasion_usa
operation_market_garden
pacific_challenge
Project
star_trek_dilithium_war
stellar_forces
the_pact_of_steel
total_ancient_war
total_world_war
tutorial
war_of_the_relics
world_war_ii_europe
world_war_ii_g40_balanced
world_war_ii_global
world_war_ii_pacific
ron-murhammer commented 7 years ago

@FrostionAAA @panguitch I'm guessing this is still an issue? I'm going to take a more serious look at this and see if we can make a change so conditions can see transported units. Any additional input is appreciated.

ron-murhammer commented 7 years ago

@FrostionAAA @panguitch It appears that counting units on transports was considered a bug and was fixed in this change: https://github.com/triplea-game/triplea/commit/db17ead6f015fc05dc53d2fad267392787f180ea

@veqryn Do you remember why you decided to filter out transported units from the unitPresence conditions?

veqryn commented 7 years ago

A dependent unit should not count towards "unit presence". I believe there were some better reasons than the fact that it was the most logical choice. I know it was considered a bug when they accidentally were. But I can't remember far back enough to say the exact situation I was in at the time. I believe it mattered for regular maps, such as Pacific or Global, as that was what we were coding towards at the time.

My recommendation would be adding a flag that defaults to false, that would allow the consideration dependent units towards unit presence conditions. The flag could be global or it could be on a per-condition basis, up to you.

FrostionAAA commented 7 years ago

I noticed this issue because in Age of Tribes there are conditions that look for the presence of certain land units that triggers want to remove, and they were hidden if they had boarded a ship. The current system does not spoil the AoT map. I have not tested it recently, but the only thing I think this does to AoT is that a player can "protect" outdated units (from being removed from the map by triggers as the ages progress) if you move them to ships. So you could in theory protect a Caveman from being removed and keep him up to modern times. No real harm here.

But If the issue still remains I think it sets limitations on maps having special units like heros, generals, special objects etc. and at the same time wanting conditions and triggers do something if these special units are suddenly not present. I one tried making heroes in a star wars map and if they died it should open up new purchase options, but as I remember the purchase options opened up when the hero boarded a ship. But it is so long ago that I don't even know if what I am saying is false or outdate. I had forgotten about this issue totally.

panguitch commented 7 years ago

There are several "unit presence" triggers in Greyhawk Wars. This issue results in suboptimal outcomes for some triggers, similar to Frostion's Age of Tribes. But others are more serious: the map and its underlying play strategies are based on hero and leader units. If you have only one left and you leave it on a ship, the game thinks you are dead and enforces significant consequences (production profiles are changed, notifications sent, diplomatic relationships are changed, all PUs are removed, etc.). Currently this is explained by some handwavium in a notification about there being rumors of the leader's demise and you better get him back on land quick. Not exactly elegant.

I think the clearest logic for "unit presence" is whether there are any of those units present, dependent or not.

But in any case I agree the solution is a flag that governs whether conditions consider dependent units. By default that flag should be set to ignore dependent units, just so that existing maps and workarounds aren't disrupted, but maps where this is a problem can flip it to include dependent units and then jettison their complicated workarounds and explanations.

ron-murhammer commented 7 years ago

@veqryn I looked through the unitPresence conditions in global and didn't see any of them where transported units should matter. In general, regular sea transports with land units shouldn't need to filter out transportedBy since you can specify to only check sea unit types. The only cases that might matter are air/land transports if for some reason you wanted to count only the non-transported units in a land territory where air/land transport with loaded units is present.

@panguitch I tend to agree that the clearest logic is to include all units whether they are dependent or not.

I think the main question is are there cases where you wouldn't want to include units that are being transported in the condition? Air/land transports over land were the only ones I could think of though I'm not sure exactly how those systems work and when the units even have transportedBy attribute set. So that may not be valid.

ron-murhammer commented 7 years ago

@FrostionAAA @panguitch @veqryn Any final thoughts on this? Seems the 3 options are:

  1. Change to always consider transported units
  2. Add an additional boolean parameter like "includeDependentUnits" to conditions
  3. Do nothing and close this issue

I don't particularly like adding more parameters for conditions especially if they are rarely used but will if enough folks feel that is the route to go.

veqryn commented 7 years ago

The only maps I would imagine this check might matter for are: Total World War Global 1940 Pacific 1940 Europe 1940

If it matters for any of those maps, then I'd say go route number 2. If it doesn't matter for TWW/Global/Pacific/Europe/vX, then go route number 1.

Cernelius commented 7 years ago

I think even in case it matters for some maps, number 1 is still viable. Just change to always consider transported units and, if a mapmaker wants to just test for land units that are not on transports, he can easily do it by having a condition that tests in all land territories only, by listing all of them. If you want, you can add a "land" option, that will test in all and only land territories on the map; that may be useful for other matters too. If there are maps that need to test only for any units not inside transports (I've no idea why), that condition could, then, be substituted with two conditions, one testing for any land units in "land" (or the full list of all land territories in the game) and the other one testing for any other units in "map"; then a super-condition, with OR, between these two.

An important other side of the coin is that, when you remove a transport with triggers, also any units inside that transport should be removed too. Currently they don't, and this makes the game crash. There is only a partial work around for this (being sure to remove all land units in any sea zones), and it is not really functional, especially because like an Italians transport may have a Germans armour in it (that will make the game crash, if you remove the transport with a trigger). So, if you can also assure all loaded / dependent units being removed when you remove a transport with trigger, that would be really good (no reasons not to, here, because the game just crashes, currently). The hack that I use is to place units that kill all naval units of a player, as that assures any cargo going too, but it would be surely better if it works with normal triggering.

Cernelius commented 7 years ago

Basically, since this Issue started as:

Conditions cannot see units on transports, but triggers can affect them.

if then we go with 1 or 2, we could open another one saying:

Conditions can see dependent units, but triggers don't remove dependent units when removing the units they are dependent from (thus crash)

But I'm thinking I should open another issue for this. I'll wait to see how this one ends, I guess. Obviously, the triggers for removing units were never meant to be used to remove any units having dependent units, as well as the conditions were not meant to see dependent units.

FrostionAAA commented 7 years ago

I would like number 1. I can't think of situations where I have seen the current behaviour logical or practical.

@Cernelius I hope you open the other issue about the transported units not being removed along with their transport. The current behaviour and problem is the only reason why one can sail around with a galley in Age of Tribes far into the future. It has always been a bit strange since land units are outdated with time. But If this was to be a rule, what about carriers? Should their air units also be dependent and removed with their ship? I would find it strange since their survival would realistically depend on the manner of the situation the trigger tries to simulate. Like if a space carrier is blown up by a sabotage chance trigger, then logical the stuff in the hangar could not be saved, but if it is a wooden fantasy ship with a friendly dragon sitting on the deck goes down in a storm somehow, then the dragon could probably save itself and fly to nearest land, like when fighters go for shore if the carrier is lost in battle. A case by case handling of this mig be nice.

Cernelius commented 7 years ago

@Cernelius I hope you open the other issue about the transported units not being removed along with their transport. The current behaviour and problem is the only reason why one can sail around with a galley in Age of Tribes far into the future.

You could obtain it by having an "AI" player that spams suicide units that offensiveAA kill the units (Galley) you want to and, then, remove all those units with triggers. Of course, this works fine, but it is a hack. I would still do that, if no developers fix how triggers work for units having dependent ones.

But If this was to be a rule, what about carriers? Should their air units also be dependent and removed with their ship?

Yes, unless the trigger fires during the turn of the owner of those units, as, at that point, they are not dependent. If a trigger removes an Americans carrier having an Americans fighter and a British fighter during the Americans turn, the British fighter should be removed with the Americans carrier, but not the Americans fighter, which is not actually on the carrier, at the point, having already taken off. A side note would be that, for properly supporting fuel or carriers being faster than air units, it should be possible to decide not to take off from a carrier, as it is currently automatic (since you always want to, if the fighter is faster than the carrier and has no fuel costs, as per in the common games).

panguitch commented 7 years ago

1 or 2. Whichever causes the least trouble.

ron-murhammer commented 7 years ago

Let's go with 1 then.

@panguitch @FrostionAAA @Cernelius Can you double check that changing this wouldn't have any negative impact on any of these maps (I looked them over and didn't see any reason that it should): Total World War Global 1940 Pacific 1940 Europe 1940

And if there is an issue with triggers not handling dependent units when removing transports then please open a new issue with the details and a way to reproduce it.

ron-murhammer commented 7 years ago

Submitted PR #1930 to remove the transported by limitation here.

@panguitch @FrostionAAA @Cernelius Please test once its merged and feel free to open additional issues for remaining issues.

FrostionAAA commented 7 years ago

I feel bad about not having tested this change, but as I think @panguitch map could be directly affected by the change or edited to make positiv use of the change, I think he would be the best tester. I don't think Age of Tribes will be changed because of this.

panguitch commented 7 years ago

Tested this on Greyhawk Wars and it successfully resolves the issue there. This will allow the map to function as intended and I can remove the handwavium in notifications that explained why things weren't working in certain situations.

Many thanks!