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.35k stars 399 forks source link

Maps Issues For Conversion Of Disabled Elements Remaining Items (1.9.0.0.3307) #1332

Closed Cernelius closed 6 years ago

Cernelius commented 8 years ago

The following xml mass changes:

find . -name "*xml" -type f -or -name "*java" -type f 2> /dev/null | xargs sed -i 's/isParatroop/isAirTransportable/'
find . -name "*xml" -type f -or -name "*java" -type f 2> /dev/null | xargs sed -i 's/isMechanized/isInfantry/'

that are items at the following Issue: https://github.com/triplea-game/triplea/issues/1033

may create problems (actually, un-bury them) for some badly coded maps (as an element that was deprecated and disabled would be totally or partially brought back, thus potentially changing the maps behaviour). I suggested that change and, as I said, the problem is only for blatantly badly coded maps (so, I still think it made sense to go that way).

This issue is meant to be a collection of whatever remaining map (not engine) issues related to the above changes.

As I said at this Issue: https://github.com/triplea-game/triplea/issues/1190

Can you also verify that no unitAttachment of the repository xml currently have multiple occurrence of:

name="isInfantry"
name="isAirTransportable"

For example, you could have had, inside a single unitAttachment:

<option name="isInfantry" value="true"/>
<option name="isMechanized" value="false"/>

And now you might have:

<option name="isInfantry" value="true"/>
<option name="isInfantry" value="false"/>

In this case, it should be reduced to:

<option name="isInfantry" value="true"/>

Meaning possibly check that the changes of these long disabled options didn't cause duplications of the same options in single attachments.


I'm also undecided whether we should suggest regular makers to do this:

name="isParatroop":name="isAirTransportable" name="isMechanized":name="isInfantry"

or this

name="isParatroop": name="isMechanized":

Meaning changing these disabled options to the closest supported ones or just deleting them. I believe the second solution (deleting) is the correct one. They are currently doing nothing, thus deleting them would be the most correct way to go, by default, preserving the current (absence of) functionalities. I just suggested them to be changed to the closest supported option, for the maps in the TripleA repository, to somewhat preserve the underlying info in the likely very long time abandoned maps having them. On the other hand, I believe that, in perspective of updating from 1.8.0.9 to 1.9.0.1 (not from some older version), it would be coherent the most to just suggesting deleting all already disabled options. Thus, I'm oriented for suggesting deleting all the options of any "isParatroop" and "isMechanized", because changing them like we did would not be strictly an update from 1.8.0.9, but a factual change of functionalities.

Example:

https://github.com/triplea-maps/big_world/blob/master/map/games/Big_World_1942_v3rules.xml Big_World_1942_v3rules.xml is currently bugged and both the land and air transports are not currently working as intended:

Old:

                <attatchment name="unitAttatchment" attatchTo="infantry" javaClass="games.strategy.triplea.attatchments.UnitAttachment" type="unitType">
                         <option name="movement" value="1"/>
                         <option name="transportCost" value="2"/>
                         <option name="isInfantry" value="true"/>
                         <option name="isAirTransportable" value="true"/>
                         <option name="isParatroop" value="false"/>
                         <option name="isMechanized" value="false"/>
                         <option name="attack" value="1"/>
                         <option name="defense" value="2"/>
                         <option name="artillerySupportable" value="true"/>
                </attatchment>

New:

                <attachment name="unitAttachment" attachTo="infantry" javaClass="games.strategy.triplea.attachments.UnitAttachment" type="unitType">
                         <option name="movement" value="1"/>
                         <option name="transportCost" value="2"/>
                         <option name="isInfantry" value="true"/>
                         <option name="isAirTransportable" value="true"/>
                         <option name="isAirTransportable" value="false"/>
                         <option name="isInfantry" value="false"/>
                         <option name="attack" value="1"/>
                         <option name="defense" value="2"/>
                         <option name="artillerySupportable" value="true"/>
                </attachment>

Why any mapmakers would choose to set deprecated and disabled items beside the correct ones, meant to do the same things, and even having a setting opposite to them, and even setting disabled and deprecated items with a value equal to false, which is the default behaviour of an option that would do absolutely nothing anyway is totally beyond me, but this is the root of the problem here. You would imagine that it would be safe to assume that new maps coded or modified with the new elements would not leave or add the previous deprecated and disabled ones beside them, but, apparently, this is the case here, instead.

As far as that map only is concerned, the bug (and the previously buried nonsense) can be saned changing the attachment to:

                <attachment name="unitAttachment" attachTo="infantry" javaClass="games.strategy.triplea.attachments.UnitAttachment" type="unitType">
                         <option name="movement" value="1"/>
                         <option name="transportCost" value="2"/>
                         <option name="isInfantry" value="true"/>
                         <option name="isAirTransportable" value="true"/>
                         <option name="attack" value="1"/>
                         <option name="defense" value="2"/>
                         <option name="artillerySupportable" value="true"/>
                </attachment>

In general, as I said, I suggest running something to check in all xml in the repository the presence of duplicate ="isInfantry" ="isAirTransportable" inside the same attachment.

Side note, this is not an issue for the Wiki/Upgrade Maps Information, since there I've suggested to just delete the already disabled items, instead (as I said): https://github.com/triplea-game/triplea/wiki/Upgrade-Maps-Information

Cernelius commented 8 years ago

As now, the only reported / known bugged game is:

Big_World_1942_v3rules.xml https://github.com/triplea-maps/big_world

That has both "mechanized" and "paratroopers" techs not working as intended.

Needing having the attachment name="unitAttachment" attachTo="infantry" changed to:

                <attachment name="unitAttachment" attachTo="infantry" javaClass="games.strategy.triplea.attachments.UnitAttachment" type="unitType">
                         <option name="movement" value="1"/>
                         <option name="transportCost" value="2"/>
                         <option name="isInfantry" value="true"/>
                         <option name="isAirTransportable" value="true"/>
                         <option name="attack" value="1"/>
                         <option name="defense" value="2"/>
                         <option name="artillerySupportable" value="true"/>
                </attachment>

Bug report from lobby:

(22:53:42) FieldCommanderCohen: hey all (22:53:58) FieldCommanderCohen: is there atrick to the mecanized infantry? (22:54:01) FieldCommanderCohen: a trick (22:54:08) Cernel: what do you mean? (22:54:13) FieldCommanderCohen: its not working for me on bw v3 (22:54:18) Cernel: let me check (22:54:20) FieldCommanderCohen: flaw in new version??? (22:54:27) Navalland: brb (22:54:47) samstreeter: how long nav? (22:54:59) FieldCommanderCohen: can't carry infantry two spaces (22:55:03) FieldCommanderCohen: won't work (22:55:10) samstreeter: give me soem advice real quick imbaked panzerman has joined (22:55:20) prastle: lol (22:55:37) Imbaked (1): sure (22:55:41) DayMar: youtube upload (22:55:46) panzerman: up 2 3 4 pm loves the marine core (22:56:11) Master_Blaster: from the halls of montezuma.... (22:56:20) panzerman: forgot it was Vet. day (22:56:22) Cernel: need 2 in my multi (22:56:28) FieldCommanderCohen: anyone? dubstep has left (22:56:43) panzerman: but no free oil change for pm :( (22:56:44) FieldCommanderCohen: hey champ (22:56:57) champ73: yes (22:56:59) FieldCommanderCohen: can't carry infantry two spaces with tanks (22:57:05) FieldCommanderCohen: something i'm missing (22:57:11) Master_Blaster: u should go short for the dollar (22:57:11) FieldCommanderCohen: flaw in version 9?? (22:57:20) champ73: there are many bugs, check out new website (22:57:23) FieldCommanderCohen: i have mech infantry (22:57:23) Cernel: it's not an engine flaw (22:57:36) Cernel: it works for me on another map (22:57:39) panzerman: never short the Donald pm has learned (22:57:49) panzerman: stupid dow (22:57:51) FieldCommanderCohen: pretty sure i have the newest version (22:57:51) champ73: Bug Reports: https://github.com/triplea-game/triplea/issues (22:57:59) FieldCommanderCohen: ok thanks

Cernelius commented 7 years ago

I overlooked that in "Big_World_1942_v3rules.xml" also the Marine units are supposed, or at least were able, to be land-transported. So here they are the updated changes I suggest, relatively to this issue only:

Change this:

                <attachment name="unitAttachment" attachTo="infantry" javaClass="games.strategy.triplea.attachments.UnitAttachment" type="unitType">
                         <option name="movement" value="1"/>
                         <option name="transportCost" value="2"/>
                         <option name="isInfantry" value="true"/>
                         <option name="isAirTransportable" value="true"/>
                         <option name="isAirTransportable" value="false"/>
                         <option name="isInfantry" value="false"/>
                         <option name="attack" value="1"/>
                         <option name="defense" value="2"/>
                         <option name="artillerySupportable" value="true"/>
                </attachment>

to:

                <attachment name="unitAttachment" attachTo="infantry" javaClass="games.strategy.triplea.attachments.UnitAttachment" type="unitType">
                         <option name="movement" value="1"/>
                         <option name="transportCost" value="2"/>
                         <option name="attack" value="1"/>
                         <option name="defense" value="2"/>
                         <option name="isInfantry" value="true"/>
                         <option name="isAirTransportable" value="true"/>
                         <option name="artillerySupportable" value="true"/>
                </attachment>

Change this:

        <attachment name="unitAttachment" attachTo="marine" javaClass="games.strategy.triplea.attachments.UnitAttachment" type="unitType">
             <option name="movement" value="1"/>
             <option name="transportCost" value="2"/>
             <option name="attack" value="2"/>
             <option name="defense" value="2"/>
                         <option name="isInfantry" value="true"/>
             <option name="isMarine" value="true"/>
                         <option name="isAirTransportable" value="false"/>
                         <option name="isInfantry" value="false"/> 
                         <option name="artillerySupportable" value="true"/>  
        </attachment>

either to:

        <attachment name="unitAttachment" attachTo="marine" javaClass="games.strategy.triplea.attachments.UnitAttachment" type="unitType">
             <option name="movement" value="1"/>
             <option name="transportCost" value="2"/>
             <option name="attack" value="2"/>
             <option name="defense" value="2"/>
             <option name="isMarine" value="true"/>
                         <option name="isInfantry" value="true"/>
                         <option name="isAirTransportable" value="false"/>
                         <option name="artillerySupportable" value="true"/>  
        </attachment>

or change the functionalities to (I would do this):

        <attachment name="unitAttachment" attachTo="marine" javaClass="games.strategy.triplea.attachments.UnitAttachment" type="unitType">
             <option name="movement" value="1"/>
             <option name="transportCost" value="2"/>
             <option name="attack" value="2"/>
             <option name="defense" value="2"/>
             <option name="isMarine" value="true"/>
                         <option name="isInfantry" value="true"/>
                         <option name="isAirTransportable" value="true"/>
                         <option name="artillerySupportable" value="true"/>  
        </attachment>

Regarding the "marine" unit, it should be firstly ascertained if either:

A) The "marine" is supposed to be land-transportable but not air-transportable. B) The "marine" is supposed to be air-transportable but not land-transportable. C) The "marine" is supposed to be land-transportable and air-transportable. D) The "marine" is supposed to be not land-transportable and not air-transportable.

By looking at the old release code, meaning this one:

        <attatchment name="unitAttatchment" attatchTo="marine" javaClass="games.strategy.triplea.attatchments.UnitAttachment" type="unitType">
             <option name="movement" value="1"/>
             <option name="transportCost" value="2"/>
             <option name="attack" value="2"/>
             <option name="defense" value="2"/>
                         <option name="isInfantry" value="true"/>
             <option name="isMarine" value="true"/>
                         <option name="isParatroop" value="false"/>
                         <option name="isMechanized" value="false"/> 
                         <option name="artillerySupportable" value="true"/>  
        </attatchment>

it seems that "A" was how it worked, but that doesn't seem sensible to me, so here I fear we might be confirming a pre-existent bug. I tend to think that if the "marine" is land-transportable (as this is how it worked, at least), which I think it makes sense, then it should be air-transportable too; thus I advice to go with "C", surmising the old absence of the air-transport option for the "marine" unit was a bug too.

As a matter of what the game tells in the notes, here it is the only relevant part of them:

Follows v3 Rules (except that there are 2 new units) Based on Prussia's Big World : June mod (only difference between Prussia's 3.5 and this mod is full compliance with v3 rules, and 2 german subs and 1 japanese sub are deleted from this version, to account for transports having no defense in v3 rules)

Based on this, you should, then, assume that the marine should be both not land-transportable and not air-transportable, and in this case the game would have been bugged as to allowing the marine to be land transported (but "correctly" not air transported), instead. However, the notes for the game are poor, thus I would not just follow them like they are a rulebook.

Regarding the reference to Prussia's "Big World : June mod", I would not even look at that, as I'm against having the notes dispersed on multiple games (I surely believe each game should be self sufficient plus a referring ruleset, not sending you to look the notes of some other games, that may be independently updated meanwhile), for what this strange thing is worth, assuming the reference is to "Big World : June 1942", that map is just totally silent about this matter.

So, to sum it up:

The past behaviour was that the "marine" was land transportable but not air transportable (while the "infantry" was correctly both land transportable and air transportable) (I also tested this on the old game coming with the 1.8.0.9 release).

Based on the game notes, plus the game notes of the other referred game, plus the reference to the basic ruleset (not having marine at all), then the marine should be not land transportable and not air transportable.

On the other hand, it is my personal opinion / guess that the absence of the air transportable ability for the marine was a missing item (a map bug), and the game notes just failed to clarify this difference from the basic referring game (WWIIv3) as well as the, on the other hand, actually implemented ability of the marine to be land transported.

Opinions?

As this is an abandoned custom map, I believe (aside from finding some specific rules somewhere) only the repository admins can decide upon this, maybe after consulting the Big World players in forum, on the matter. My suggestion is to go with the "C" behaviour, as I believe the "marine" is traditionally intended as a unit able to do everything the "infantry" can and more (meaning it is intended to be absolutely superior or equal to the infantry in all aspects). If you are not sure, my suggestion is opening a Topic, to discuss if "marine" should be air-transportable or not, in the "WarClub" forum.

I suggest making the above changes together with those eventually required for "Big_World_1942_v3rules.xml" at this issue: https://github.com/triplea-game/triplea/issues/1330

as in that map the bombing raid is bugged too.

While at the root of these problems are my suggested mass-changes, I still believe that was the right thing to do, as this issue is the consequence of extremely bad (and buried) xml coding (he/they put both the correct properties and the deprecated and useless ones, previously doing the same thing, inside the same attachment and with settings one opposite to the other).

Cernelius commented 7 years ago

Just quoting here the opinion at the previously referenced closed Issue:

@Cochise77 :

Having the marines land or air transportable would certainly increase the utility of the marine, and make it more of a value compared to artillery.

Right now, I only use the marines early/mid game for the US, or late game Germany/Japan (when conquering Great Britain or the Eastern US). I've never found them to be a good value for the UK or Italy (where production constraints push me towards Infantry+Artillery), and can't ever see myself buying them for Russia or China.

For paratroopers (or para-marines), I don't think there's much benefit unless you could drop them in the 2nd enemy territory (i.e. drop them one spot behind enemy lines). Requiring them to drop in the first enemy territory negates their usefulness.

Mechanized marines could be really interesting for Russia. They'd also be useful for mainland Asia (Japan, USA, & UK). They could also tip the value scale for Italy.

So blah, blah, blah, long way of saying it would be interesting.

@Cochise77 :

On the marine question, I should have stated it more plainly, yes, IMO, the marine should be air/land/sea transportable.

ron-murhammer commented 7 years ago

@Cernelius I'm not really sure what we are tracking in this issue at this point. I believe there are still paratrooper issues with some maps? Maybe update this issue's summary or created new issues to track what we need to fix?

Cernelius commented 7 years ago

This issue is still meant to track all the bugs / behaviour changes of the recent changes documented at Upgrade Maps Information.

However, if you go that way, then you should bring BWv3 back with marine that can be land-transported but not air-transported. That is fine, but I'm just suggesting to make it both land-transported and air-transported.

But if you want to adhere to the Issue strictly, then nevermind about the air-transport for the marine. That can be made in a second moment if any BWv3 player pushes it.

My guess is that we have just a bug on top of another bug, and the marine should be both air-transported and land-transported, not only land-transported (as it seems it used to be), but, if we consider the referring rules and the notes, then the bug would be that it should be neither of the two, instead (so, I surmise that the game was bugged and the info in Note are missing).

Just saying, not sure if we want to restore the functionality to likely just bring back an old bug, tho it is not certain it is a bug (but it would be a very strange decision, on the side of the mapmaker, if it is not).

Yes, the @Cochise77 comments I quoted don't really belong to this issue. Rather better to stay in a WarClub forum post about if the BWv3 marine should or should not be able to be air transported.

Practically, I just gave 2 possible solutions, one with and one without the air-transportable marine (the one without is the one restoring the previous behaviour); just pick at will or open a new issue or a topic in the WarClub on this (not requesting it), if you so prefer. I've not a strong opinion on any solutions (and I'm not a BWv3 player).

DanVanAtta commented 7 years ago

@Cernelius I don't think anyone is quite clear on what needs to be done. I went through maps and found cases where we had 'isInfantry=true' and also 'isInfantry=false' on the same unit. That was causing issues and should be cleaned up.

With respect to marine behavior, I'm not sure if the existing behavior was bugged or intended, but it was the behavior that people knew and expected. Anything different would be a rule change. The cleanups we did should not have changed the rules, so let's keep the configurations working as they were.

I am curious if you are aware of any items that still need to be done from this issue?

Cernelius commented 7 years ago

Yeah, this was my bad (too optimist about the xml codings making sense) as, as I said, I assumed no maps would have had both the legit and the disabled option in the same attachment with settings one opposite of the other. Instead, it turns out that even the flagship Big World has such absurd coding in the xml.

So, since we have sorted out that we don't want to address the marine weirdness, I suggest doing this, if possible:

Run a search of all maps having these codes:

name="isInfantry" value="false" name="isAirTransportable" value="false"

(hopefully, only a few will have them, as there are no reasons for setting them false)

then, either:

A) Just go ahead removing those everywhere (there is no point in having them false, anyway).

XOR

B) Tell me here the list, then I will look into the xml myself and and tell what maps should have the above deleted from the attachment.

XOR

C1) You delete the whole line of:

name="isInfantry" value="false"

from all attachments also having an occurrence of

name="isInfantry" value="true"

C2) You delete the whole line of:

name="isAirTransportable" value="false"

from all attachments also having an occurrence of

name="isAirTransportable" value="true"

Sadly I now fear that some mapmakers might have just copy pasted such codings from Big World for their own maps.

Big World ended up having a bunch of bugs, at the release, for various different reasons. I don't remember if this would be the last one to sane.

DanVanAtta commented 7 years ago

@Cernelius is there anything more to do or track in this ticket? What are next steps ? Can this issue be closed if none?

Cernelius commented 7 years ago

If nobody did any of the things at my post above or the matter wasn't otherwise solved, I suppose it is still open.

My suggestion is first, as I said:

Run a search of all maps having these codes:

name="isInfantry" value="false" name="isAirTransportable" value="false"

Then I guess I may take a look myself and see what to suggest.

I want also to reference this: https://forums.triplea-game.org/topic/180/land-transport-improvements/2 in which @ron-murhammer appears wanting to deprecate the "isInfantry" property, anyways.

Cernelius commented 7 years ago

To be clear, I'm suggesting someone to run a search of the above and give here the full list of all games in which it is positive. Then I can take a look at each one if, say, they are about a score.

Cernelius commented 7 years ago

And I think it is needed getting around sorting this out and, possibly, solving and closing it.

Cernelius commented 7 years ago

Especially if they would be too many, it would be actually preferable:

Search all games having these codes in the same attachment:

name="isInfantry" value="false" name="isInfantry" value="true"

Search all games having these codes in the same attachment:

name="isAirTransportable" value="false" name="isAirTransportable" value="true"

That would show exactly what we need to know; I just don't know if this is feasible.

More in general, it would be good to search for any boolean options that are repeated twice or more in the same attachment, as that should never happen.

Cernelius commented 7 years ago

I went through maps and found cases where we had 'isInfantry=true' and also 'isInfantry=false' on the same unit. That was causing issues and should be cleaned up.

Do you mean that you have cleaned it up somehow already or that it is to be cleaned up yet? I understood that you meant it has to be, not that you did it. The problem is that one plus "isAirTransportable" alike.

prastle commented 7 years ago

your a trooper cernel :)