Map XML Changes/Deletions Request

[Cernelius]

Note: This post has been edited as per summing up everything relevant at the following discussion. This first post is supposed to be self-sufficient; meaning that you don't need to read any part of the following discussion to integrate what at this first post; but I advise you to read the whole discussion, anyway, to have more details.

This post consists mainly of two lists. Both lists are self sufficient and have the same items.
The first list aims at making simple the most to know where and how the single items' changes are to be made.
The second list groups the items and provides some explanations.
Practically, the second listing aims at making the items fairly understendeable, and the first listing aims at directing the changes as much clearly as possible.
All the items sharing the same number are supposed to be interrelated: it would make no sense making only part of them.

Also, the first list is for tracking what has been already done [done].

In addition to the already made:
Attatch->Attach
attatch->attach

I wish to propose the following changes and deletions, to be included in the 1.9 engine (the numbers at the first listing refer primarily to the second listing):

XML related engine reductions:
Means that these ones should be value restricted.
7A)victoryCity": stop supporting boolean only for this option
8B)"giveUnitControl": stop supporting boolean only for this option

XML related engine deletions (if not already done):
Means that these ones should be just fully deleted in the engine only, but not in any maps' xml.
8A)"takeUnitControl"
9A)"occupiedTerrOf"
10A)"isParatroop"
11A)"isMechanized"
12A)"isTwoHit"
13A)option name="conditionType" value="XOR"
14A)option name="trigger"
15A)"SBR Affects Unit Production"

XML related engine only changes:
Means that these ones should be changed in the engine only, but not in any maps' xml.
1A)Naval Bombard Casualties Return Fire->Naval Bombard Casualties Do Not Return Fire

XML related engine changes and XML changes:
Means that these ones should be name-changed both in the engine and in any maps' xml having them.
[done]2A)isImpassible->isImpassable
3A)offence->offensive
3B)defence->defensive
4)Units repair at end of round->Units Repair Hits End Turn
5)Units repair at beginning of round->Units Repair Hits Start Turn
6)option name="turns"->option name="rounds"

XML only deletions:
Means that these ones should be removed in any maps' xml having them, but not in the engine.
8C)option name="takeUnitControl" value="true"
8C)option name="takeUnitControl" value="false"
8C)option name="giveUnitControl" value="true"
8C)option name="giveUnitControl" value="false"
(all and only the boolean occurrences of giveUnitControl must be deleted)

XML only changes:
Means that these ones should be name-changed in any maps' xml having them, but not in the engine.
7B)option name="victoryCity" value="true"->option name="victoryCity" value="1"
7B)option name="victoryCity" value="false"->option name="victoryCity" value="0"
9B)"occupiedTerrOf"->"originalOwner"
10B)"isParatroop"->"isAirTransportable"
11B)"isMechanized"->"isInfantry"
12B)"isTwoHit" value="true"->"hitPoints" value="2"
12B)"isTwoHit" value="false"->"hitPoints" value="1"
13B)option name="conditionType" value="XOR"->option name="conditionType" value="1"
14B)"trigger"->"conditions"
1B)"Naval Bombard Casualties Return Fire" value="true"->"Naval Bombard Casualties Do Not Return Fire" value="false"
1B)"Naval Bombard Casualties Return Fire" value="false"->"Naval Bombard Casualties Do Not Return Fire" value="true"
15B)"SBR Affects Unit Production"->"Damage From Bombing Done To Units Instead Of Territories"

Engine behaviour changes:
Means that these ones are changes to the engine, not to any names.
1C)Naval Bombard Casualties Do Not Return Fire: invert behaviour (to behave as true like it now does as false, and vice versa)
1D)property name="WW2V2": update so that when it is true it will change the property "Naval Bombard Casualties Do Not Return Fire" to be true too, as long as this property is not specifically set in the xml.

map.properties related engine changes and map.properties changes:
Means that these ones should be changed in any maps' map.properties having them.
[done]2B)color.Impassible->color.Impassable

(alternatively to "Naval Bombard Casualties Do Not Return Fire" it would be also fine "Naval Bombard Casualties Return Fire Restricted", that was the previous property's name)

And the current pos2 descriptions should be updated accordingly, in several of the above cases.

I suggest these changes not being backward compatible (removed, not deprecated), just like the attatch to attach case.

I believe this should reduce incorrect or redundant or deprecated elements to only "artillerySupportable", "artillery", and "unitSupportCount", that, albeit duplicative of what done via supportAttachment, I definitively suggest being kept, for some reasons.

Please, if any developer willingly to do any of the above changes don't want to make some of them or want to make some of them in a different way or just don't get the point of some of them, let me know specifically which ones you are in disagreement or in doubt with and I will explain you why you should do them the way I said.
While I'm of course favourable if all the above changes are fully made exactly the way I said, I may be contrary to make any of them if only some of them are made or if they are made in a different way than I said.

Thanks!

References:
http://tripleadev.1671093.n2.nabble.com/Attatch-incorrect-spelling-fixed-tp7591892.html;cid=1470329274322-906
http://tripleadev.1671093.n2.nabble.com/PRECONDITIONS-FOR-MAP-UPLOAD-tp7587348.html;cid=1470329274322-906

Following, is a more lengty listing that categorises all the interrelated changes and explains them to some extent (what listed below are exactly the same items as above, differently organised to be more explicative, and with some additional explanations):

• Updates to behaviour and name so the behaviour matches v1 rules default (when false) and the behaviour matches the name

1A) Engine only:
Naval Bombard Casualties Return Fire->Naval Bombard Casualties Do Not Return Fire

1B) Maps' xml only:
property name="Naval Bombard Casualties Return Fire" value="true"->property name="Naval Bombard Casualties Do Not Return Fire" value="false"
property name="Naval Bombard Casualties Return Fire" value="false"->property name="Naval Bombard Casualties Do Not Return Fire" value="true"

1C) Engine only:
property name="Naval Bombard Casualties Do Not Return Fire": invert behaviour (to behave as true like it now does as false, and vice versa)

1D) Engine only:
property name="WW2V2": update so that when it is true it will change the property "Naval Bombard Casualties Do Not Return Fire" to be true too, as long as this property is not specifically set in the xml.

Note:
The property name and it being false are both correct with respect to v1 Rules (TripleA default), but the behaviour is exactly the opposite.
The property value has to be consequently changed in all maps having it set (from "true" to "false" and from "false" to "true"), on the assumption that the mapmakers set the option on what it does, not on what it says.
The "WW2V2" is a general property that turns the game from v1 Rules (default) to v2 Rules.

• Updates to naming so the name matches the behaviour

2A) Engine and Maps' xml:
isImpassible->isImpassable

2B) Maps' map.properties:
color.Impassible->color.Impassable

3A) Engine and Maps' xml:
offence->offensive

3B) Engine and Maps' xml:
defence->defensive

Note:
The only case in the xml where the terms "offence" and "defence" are used are for the supportAttachment, namely something like this:

    <attachment name="supportAttachmentRussiansArtillery1" attachTo="artillery" javaClass="games.strategy.triplea.attachments.UnitSupportAttachment" type="unitType">
        <option name="unitType" value="infantry"/>
        <option name="faction" value="allied"/>
        <option name="side" value="offence"/>
        <option name="dice" value="strength"/>
        <option name="bonus" value="1"/>
        <option name="number" value="1"/>
        <option name="bonusType" value="ArtyOld"/>
        <option name="impArtTech" value="true"/>
    </attachment>

My suggestion is to change (both in the engine and in each map's xml), more in detail:
option name="side" value="offence"->option name="side" value="offensive"
option name="side" value="defence"->option name="side" value="defensive"
option name="side" value="offence:defence"->option name="side" value="offensive:defensive"

The main reason for this change is that we already have an option called:
option name="defense"
which is the very basic option that decides at what strength the related unit will defend, upon rolling dice (it is commonly 2 for the infantry, 4 for the battleship, etc.).
It is very stupid having an option called "defense" and another option called "defence", thus the option called "defence" has to be renamed to "defensive".
Similarly, "offence" has to be renamed to "offensive".
For more reasons behind this change, please read some other post of mine at this Issue (I would say not needed to give other reasons, as the main one is enough).
An alternative suggestion, from Frostion, has been to change "offence" and "defence" to "offense" and "defense", and I've argumented why this would be a bad choice, in a previous post of mine.
Side note, I'm not going to suggest any changes from British English to American English, so I would not have given any suggestions at all, in this case, if the issue would have been only this being a British English spelling (meaning that I would definitively suggest the change to offensive/defensive even if the current names would be offense/defense (please refer to some other post at this Issue for more clarifications on this matter)).

4. Engine and Maps' xml:
   Units repair at end of round->Units Repair Hits End Turn

Note:
Here there are 3 issues:
This option applies to all units having multiple HitPoints, not just to the ones named "Battleship".
It is needed to somehow clarify we are talking of repairing HitPoints damages, not damages from SBR etc..
This option takes effect at any EndTurn, not end of round.

5. Engine and Maps' xml:
   Units repair at beginning of round->Units Repair Hits Start Turn

Note:
Here there are 3 issues:
This option applies to all units having multiple HitPoints, not just to the ones named "Battleship".
It is needed to somehow clarify we are talking of repairing HitPoints damages, not damages from SBR etc..
This option takes effect at the first "move" phase of the unit's owner, not at beginning of round.

6. Engine and Maps' xml:
   option name="turns"->option name="rounds"

Note:
This option is (seldom) used in cases like (from Napoleonic Empires):

            <attachment name="conditionAttachmentTurn2" attachTo="France" javaClass="games.strategy.triplea.attachments.RulesAttachment" type="player">
                <option name="turns" value="2"/>
            </attachment>

But, despite what the option says, it is verified for the whole second round, not just for the second turn of "France", thus the name must be changed accordingly, from "turns" to "rounds", as it means "rounds", not "turns".
I believe only a few maps use this option, and it is an option only for conditions or objectives (so, I'm not suggesting any general change from "turns" to "rounds": this one is a very simple and limited change too, not harder than the isImpassible to isImpassable one).

• Obsolete or redundant setting removal and related naming changes to maps having the obsolete reference

7A) Engine only:
option name="victoryCity" value="true"
option name="victoryCity" value="false"
Stop supporting boolean only.

7B) Map's xml only:
option name="victoryCity" value="true"->option name="victoryCity" value="1"
option name="victoryCity" value="false"->option name="victoryCity" value="0"

Note:
Numbered values are already supported, making boolean redundant, as "0" is the same as "false" and "1" is the same as "true".
This is a very unimportant change. Numbered value are already supported, but the old boolean is still supported too, which is redundant.

8A) Engine only:
"takeUnitControl"
Remove this option from the engine.

8B) Engine only:
giveUnitControl
Stop supporting boolean only for this option.

8C) Maps' xml only:
option name="takeUnitControl" value="true"
option name="takeUnitControl" value="false"
option name="giveUnitControl" value="true"
option name="giveUnitControl" value="false"
Remove these options from all maps having them.

Note:
All and only the boolean occurrences of "giveUnitControl" must be deleted.
While "takeUnitControl" is wholly obsolete, "giveUnitControl" is still used, with player names as value, not boolean.

9A) Engine only:
"occupiedTerrOf"
Remove this option from the engine.

9B) Maps' xml only:
"occupiedTerrOf"->"originalOwner"

10A) Engine only:
"isParatroop"
Remove this option from the engine.

10B) Map' xml only:
"isParatroop"->"isAirTransportable"

11A) Engine only:
"isMechanized"
Remove this option from the engine.

11B) Map' xml only:
"isMechanized"->"isInfantry"

12A) Engine only:
"isTwoHit"
Remove this option from the engine.

12B) Map's xml only:
"isTwoHit" value="true"->"hitPoints" value="2"
"isTwoHit" value="false"->"hitPoints" value="1"

Note:
"isTwoHit" is an obsolete boolean option that used to define whether an unit would have had 2 HitPoints or 1, 1 being the default.
Currently, this is instead set via the numbered "hitPoints" option, and all that "isTwoHit" is left doing is setting the value of "hitPoints" to 2, if true, at start game.
Practically, "isTwoHit" is still supported only to keep supporting maps using the obsolete option, thus I suggest getting rid of it, and update all maps as to use the new "hitPoints" option, instead.
Side note, I think this was the very last major engine change developed by Veqryn, I suppose for his own "Galaxy" map.

13A) Engine only:
option name="conditionType" value="XOR"
Remove this option's value from the engine.

13B) Map's xml only:
option name="conditionType" value="XOR"->option name="conditionType" value="1"

Note:
This is not a deprecated element, as far as I know, but the "XOR" case it is exactly the same as "1"; thus, being covered by the already supported numbered values, there is no need of having this duplicate.
The "1" is even better telling than "XOR", since "1" obviously means that exactly "1" of the conditions must be verified, while "XOR" is definitively not a common word (unlike "OR" or "AND").
Anyway, this is very unimportant; thus keeping the useless "XOR" is not a real issue.

14A) Engine only:
option name="trigger"
Remove this option from the engine.

14B) Map's xml only:
"trigger"->"conditions"

Note:
The removal of "trigger" is very limited in scope, and, to be clear, refers only to options like this:
option name="trigger" value="conditionAttachmentXXX"
that are deprecated, and are supposed to be written this way, instead:
option name="conditions" value="conditionAttachmentXXX"
My suggestion is simply to discontinue the support to the already long time deprecated "trigger" option, and only admit the new "conditions" one; then, rename
option name="trigger"->option name="conditions"
in all xml still having this obsolete option.

15A) Engine only:
"SBR Affects Unit Production"
Remove this option from the engine.

15B) Map's xml only:
"SBR Affects Unit Production"->"Damage From Bombing Done To Units Instead Of Territories"

Note:
SBR Affects Unit Production no longer does anything at all. Instead it just sets to true: "Damage From Bombing Done To Units Instead Of Territories".
Thus, I advise to get rid of the old and duplicative property, and update all xml still having it.

General Note (IMPORTANT!):
I might not have caught all duplicative / obsolete / deprecated option, thus I encourage the developers to delve into the code and see if there are others, discussing their addition to the list at this Issue.
Anyway, I believe all the above elements are self-sufficient, thus this issue is already fully defined, not requiring additions (but it would be nice not missing something analogue to the above point).
It would be nice to take the 1.9 release as on opportunity to fully clean up all the old rubbish from the engine, and update all xml.
Other than what listed, I know there are the following 3 duplicative options:
"artillerySupportable"
"artillery"
"unitSupportCount"
that, albeit duplicative of what done via supportAttachment, I definitively suggest them being kept, as they are, because they make both mapmaking and tooltip display easier.
I definitively request such changes, as well as any changes breaking compatibility between the release and the prerelease, being either done only shortly before a new release (and surely not more than 1 month beforehand) or not done at all, to reduce mapmaking problems, particularly referring to playtesting of new maps/mods.
It would be also highly advisable to do all such changes in a single moment in time or closely so, not scattering them amongst various versions with other unrelated changes in between.

Latest changes already done and not part of this Request (History Record) (this issue has been updated to take into account them):
Naval Bombard Casualty Return Fire Restricted->Naval Bombard Casualty Return Fire
Battleships repair at end of round->Units repair at end of round
Battleships repair at beginning of round->Units repair at beginning of round

Note:
The current property "Naval Bombard Casualty Return Fire" was previously named "Naval Bombard Casualty Return Fire Restricted", thus it already had the same meaning of the proposed change.

[FrostionAAA]

isImpassible -> isImpassable and color.Impassible=->color.Impassable=. Would make sense.

option name="turns"-> option name="rounds". Would make sense.

offence/defence might as well change to offense/defense. Why the –sive part? Keep it simple and short.

Battleships repair at end of round->Units Repair Hits End Turn and Battleships repair at beginning of round->Units Repair Hits Start Turn. Technically it is not "Hits" but "Hit Points" that are repaired. Also, that would be in line with option name="hitPoints" value="X"

Naval Bombard Casualties Return Fire Restricted->Naval Bombard Casualties Return Fire + true/false swap is also a good idea. But can this change be done by a script/bot fixing all the XMLs at Github?

[Cernelius]

To better explain the above points:

The
Naval Bombard Casualties Return Fire Restricted->Naval Bombard Casualties Return Fire
should be kept doing what it does (while changing its name, because it currently does the opposite of what it says), but its default in the engine should be changed from "false" to "true" (currently you have it false if you don't specify it in the xml, and, instead, it should be true, because in v1 units hit by naval bombard are indeed able to return fire).
Instead, only the name change should be made in the map's xml, and the values kept unchanged there (the "false" to "true" is a change only for the default in the engine and nothing else, affecting only the maps having this property not directly or indirectly specified in the xml), on the assumption that the mapmakers set it on what it does, not on what it says.

Relatively to the deletions, those should all be deleted in the engine, but in the map's xml they should be changed this way:
"trigger"->"conditions"
"occupiedTerrOf"->"originalOwner"
"isParatroop"->"isAirTransportable"
"isMechanized"->"isInfantry"
"isTwoHit" value="true"->"hitPoints" value="2"

And the following should be just deleted in all xml too, if any (I guess only very few xml have them):
option name="takeUnitControl" value="true"
option name="takeUnitControl" value="false"

Moreover, and this is a tricky thing I forgot, "giveUnitControl" must stop supporting boolean values (if it still does), and supporting only player names.
Then, the following should be deleted in all xml, if any:
option name="giveUnitControl" value="true"
option name="giveUnitControl" value="false"

You are supposed to find it in a format like:

                     <attachment name="playerAttachment" attachTo="British" javaClass="games.strategy.triplea.attachments.PlayerAttachment" type="player">
                          <option name="takeUnitControl" value="true"/>
                     </attachment>
                     <attachment name="playerAttachment" attachTo="Australians" javaClass="games.strategy.triplea.attachments.PlayerAttachment" type="player">
                          <option name="giveUnitControl" value="true"/>
                     </attachment>

So, all this above should be manually deleted in the (likely) very few maps having them.

But notice that, instead:
option name="giveUnitControl"
is still an used option, as long as its value is a player's name (not boolean), and should not be deleted (not delete it from Total World War!).
The legit "giveUnitControl" can be found as something like this:

                     <attachment name="playerAttachment" attachTo="Britain" javaClass="games.strategy.triplea.attachments.PlayerAttachment" type="player">
                          <option name="retainCapitalNumber" value="1"/>
                          <option name="giveUnitControl" value="Australia"/>
                          <option name="giveUnitControl" value="Canada"/>
                          <option name="giveUnitControl" value="Egypt"/>
                          <option name="giveUnitControl" value="India"/>
                          <option name="giveUnitControl" value="SouthAfrica"/>
                          <option name="giveUnitControl" value="ExiledAllies"/>
                          <option name="destroysPUs" value="true"/>
                     </attachment>

Doing all this would allow cleaning up all the old rubbish and restarting up with a fresh engine with no really deprecated or pointless or wrong or misspelled elements, and keeping only 3 redundant ones (the three simplified support options, for easier mapmaking and more user-friendly tooltip info in basic maps (redrum said they are deprecated and should not be used in new maps, but I disagree, and think they are still good to have, even tho they are surely duplicative)).

Finally, two other very minor things are that, currently:

option name="victoryCity" value="true"->option name="victoryCity" value="1"
Supports both boolean and numbered values, the "1" being the same as the "true", but I'm not sure if it would be good to drop the boolean and convert all to 1.

option name="conditionType" value="XOR"->option name="conditionType" value="1"
XOR and 1 are exactly the same thing, so you might want to stop supporting XOR and converting all xml from XOR to 1.

The only question mark left would be, then, only the weird
property name="Pacific Theater"
and related stuff, that I don't know what they do.

[Cernelius]

@FrostionAAA
Hit and HitPoints are both fine; "Repair HitPoints" sounds a bit silly to me, because you rather repair stuff and regenerate or heal HitPoints, but if it sounds fine to the rest of you, nevermind, especially since "Repair Hits" doesn't sound that great, either, so whatevah.
You just be sure that it doesn't get confusing with the other repair, namely the damages from SBR or rockets, and it's fine.

[Cernelius]

@FrostionAAA
There are several reasons why I'd change it to offensive/defensive, mostly detailed in the first reference given, namely:

1. I'm just not going to suggest changes from British English to American English.

2. We already have attack/defense for the basic stats, thus offense/defense would be both duplicative of defense and, on the other hand, incoherent, since defense would be the same amongst the two options, while attack and offense would not; I suggest avoiding using the same names for different things, especially if done partially or incoherently.
   Side note, I would advise against renaming it attack/defense too, as this would be semantically wrong, since the opposite of defense is offense, not attack (and, traditionally, the military opposite of attack is disengage, since the meaning of attack is making contact with the enemy, which you can do both on the offensive and on the defensive, strategically), and contrasting with the traditional strategic Clausewitzian theory, where it is also specified that the attack belongs more to the defensive than to the offensive, even tho this is not really anymore the case at least since WWI (I've pasted an except in the first reference).

3. Literally, since we are saying "side", we should say "offensive side" or "defensive side", not "offense side" or "defense side"

4. Since the option is not referring to the units receiving support, that are the ones having their attack/defense stats modified, but to the units giving support (the "side" is the supporting side, not the supported side), the name should not be the same of some stat it doesn't necessarily modify.
   Thus, offensive/defensive would make the most sense, regarding of how the option works, since it is an option giving supporting ability to units belonging either to the offensive or the defensive side, not directly influencing any attack or defense stats itself. For example, if the side is "defence" and the faction is "enemy", you will be changing the "attack" (not the "defense"!) value of enemy units; thus changing to "defensive" would make the most sense, otherwise you would have something called "defense" that it is changing something called "attack" and not at all changing something else called "defense" all the same, which would be quite strange (anyone would imagine that "defense" support would be all about changing the "defense" stats, but it is not!).
   Moreover, offensive/defensive, as being both distinct from the related attack/defense stats, would help cutting down confusion about the fact that offensive supports are not necessarily given to units in attack and defensive supports are not necessarily given to units in defense; if you say, like, "defense" people will likely understand that you are changing the "defense" value of units (because both are called exactly the same), while this is not necessarily true, as said; what that means is that the units giving support are the ones on the defensive, which in turn implies that you will be modifying defense value if the faction is allied, but you will be modifying attack values if the faction is enemy; same thing, mutatis mutandis, for the offensive.
   From pact of steel 2:
   values: "offence" or "defence", support when attacking or defending, can be both (example: value="offence:defence"). The side the attached-to unit is on when giving this support.
   should be changed to:
   values: "offensive" or "defensive", support when attacking or defending, can be both (example: value="offensive:defensive"). The side the attached-to unit is on when giving this support.

5. If feasible, I suggest avoiding terms that are different between British and American English, to minimise confusion, one way or the other.

Because of this, I insist on my suggestion of:

offence->offensive
defence->defensive

Generally speaking, I think this change would be good if the above were currently called offense/defense, as well.

But, yeah, while I'm not proposing changes from British English to American English, having "defense" in an option and "defence" in another one is just ridiculous. If it was made on purpose, to not having the exactly same name in both (only Veqryn could tell, I guess), then I would say definitively better offensive/defensive, instead.

Also, rather, I would change "attack" to "offense" for the basic stats (since "offense", not "attack", is the opposite of "defense"), but I didn't propose that because attack/defense is a very traditional TripleA thing, so I guess we don't want to be so much correct there.

[Cernelius]

Also either change in the xml:
"isTwoHit" value="false"->"hitPoints" value="1"
or delete them.

[Cernelius]

I highly suggest at least the:
Naval Bombard Casualties Return Fire Restricted->Naval Bombard Casualties Return Fire
plus the change from false to true default in the engine (not in the xml) be made for the 1.9, because it is really bad to have an option doing the opposite of what it says and not being default v1 rules, while anything else is and it is supposed to be.
This is much more important than changing attatch to attach.
This was a thing that I also told Veqryn back then; reference:
http://sourceforge.net/p/triplea/bugs/1082/
He tagged it as "closed-fixed", but it is not true (he should have tagged it as "closed-wont-fix"); only the default for the specific "WWII Classic" games and mods was fixed, but the default is still wrong in the engine and the property still does the opposite of what it says.

[ron-murhammer]

@Cernelius We want all properties to default to 'false' for consistency.

[Cernelius]

Thanks for clarifying, then my suggestion is, on that point:

Keep the option
Naval Bombard Casualties Return Fire Restricted
named the same.

Keep the default in the engine "false", as it is now.

Change the behaviour of this option upside down from now, so that it will finally do what it actually says and always said (meaning that when it is false the casualties from naval bombard will be able to return fire, because if something called "Naval Bombard Casualties Return Fire Restricted" is false, it means that the naval bombard casualties are not restricted from returning fire, which means that they can return fire).

Change in all current xml:
"Naval Bombard Casualties Return Fire Restricted" value="true"->"Naval Bombard Casualties Return Fire Restricted" value="false"
"Naval Bombard Casualties Return Fire Restricted" value="false"->"Naval Bombard Casualties Return Fire Restricted" value="true"
(on the assumption that the mapmakers set the option on what it does, not on what it says)

I think this is the most important thing to change.
Of my other starting points, only the two repair options are relevant for users.
Anything else is just for making life easier for the mapmakers or power users, which is not high priority, arguably (but surely not low priority).

[Cernelius]

Then, also pact_of_steel_2.xml would be to be updated, as it currently says:

    <!-- Despite what this one says, it means the opposite.  If true, casualties from naval bombard will get to return fire. -->
    <property name="Naval Bombard Casualties Return Fire Restricted" value="true" editable="false">
        <boolean/>
    </property>

[ron-murhammer]

@Cernelius Yep I'd agree with that change now. Can you update your first post to have the complete list based on the discussion.

[Cernelius]

Do you prefer Repair Hits or Repair HitPoints or what? I don't really like either, but not much idea.
It can't be just Repair, cause there's also the SBR thing.

[Cernelius]

Just updated.

If you don't intend to close this issue, you should mark it as "Include with 1.9 release", as it would be not advisable to do it thereafter (nor it would be advisable doing it anytime but briefly before a new release).

I want also to point out that the attatch->attach change has been a significant burden; I definitively advise, for the future, any such changes being made shortly before a new release only, and surely no more than 1 month beforehand.

[Cernelius]

This commit:
#1047

has been merged, but I'm strongly against the:
Naval Bombard Casualty Return Fire Restricted -> Naval Bombard Casualty Return Fire
change, if done without also changing the engine default from "false" to "true" (meaning better you change nothing, if you don't change the default too!).

Moreover, as we discussed and you can read in the first post of the Issue, this is not even anymore my current suggestion.

Another thing that I need to be confirmed by somebody with knowledge of how the code works is that, after having changed upside down the behaviour of
"Naval Bombard Casualty Return Fire Restricted"
then, it has to be assured that when:
property name="WW2V2" value="true"
this property will change the previous property from false to true, if it has not been specifically set in the xml.

Please I need a developer to confirm the above point (regarding "WW2V2"), then I will update my first post accordingly.

Thanks.

[DanVanAtta]

The property name at least makes sense called: "Naval Bombard Casualty Return Fire", right? When set to false, it means that casualties will not return fire.

[Cernelius]

Yes, but the other main problem is that the engine default is set as working as opposite to v1 rules, which is bad, as this is the opposite of everything else, and it is likely to induce mapmakers to set their rules wrongly, in the case they want to have the Classic ruleset, unless they specifically know of this 1 exception case only (the Game of Thrones map is one example in which the mapmaker just chose the Classic ruleset and got bombard casualties unable to return fire).

Both the "Naval Bombard Casualty Return Fire Restricted" and its default as "false" were correct; what was wrong was that the engine were doing the opposite of what it should have had. So, if you are not changing the default from "false" to "true", then the behaviour, not the name, should be changed upside down.

This is particularly relevant, as the Classic ruleset is often chose by mapmarkers that don't know much about TripleA, thus they are unlikely to go after single-case bugs and inconsistencies.

Thanks again.

[DanVanAtta]

Naval Bombard Casualties Return Fire Restricted invert behaviour (to behave as true like it now does as false, and vice versa)

I do not plan on doing this, it is a good sized change in the game engine, and does not seem all that beneficial.

XML related engine deletions (if not already done (I don't know)):
"takeUnitControl"
"occupiedTerrOf"
"isParatroop"
"isMechanized"
"isTwoHit"
"trigger"
"SBR Affects Unit Production"

What do you mean by deletions, the XML code for these can just be removed?

XML related engine changes and XML changes:
isImpassible->isImpassable

This is done

option name="victoryCity" value="true"->option name="victoryCity" value="1"
(stop supporting boolean only)

Is this a very valuable change?

offence->offensive
defence->defensive

This change needs to be done selectively depending upon the context. I do not plan on doing this.

Battleships repair at end of round->Units Repair Hits End Turn
Battleships repair at beginning of round->Units Repair Hits Start Turn

The word battleships has been generated to units.

option name="turns"->option name="rounds"

Bit of a big change, hard to script this one. Not planning to do .

option name="conditionType" value="XOR"->option name="conditionType" value="1"

I do not agree with this one. "value=1" is meaningless to me as a conditionType, XOR has at least meaning.

XML only deletions:
option name="takeUnitControl" value="true"
option name="takeUnitControl" value="false"
option name="giveUnitControl" value="true"
option name="giveUnitControl" value="false"
(all and only the boolean occurrences of giveUnitControl must be deleted)

I think I only spotted a couple of maps that had this, we can address it in the maps and remove the game engine code in patch versions.

XML only changes:
"occupiedTerrOf"->"originalOwner"
"isParatroop"->"isAirTransportable"
"isMechanized"->"isInfantry"

These are under consideration. Are they really just search and replace operations?

"isTwoHit" value="true"->"hitPoints" value="2"
"isTwoHit" value="false"->"hitPoints" value="1"

value change from boolean to int more than I'm willing to tackle.

"trigger"->"conditions"

Context dependent change, it's a lot of work. Needs to be very valuable to be worth doing.

"Naval Bombard Casualties Return Fire Restricted" value="true"->"Naval Bombard Casualties Return Fire Restricted" value="false"
"Naval Bombard Casualties Return Fire Restricted" value="false"->"Naval Bombard Casualties Return Fire Restricted" value="true"

The flip of the default value is not happening in any way, but am happy to do a simple rename, as was done. "Naval Bombard Casualties Return Fire" if i understand right does better describe what the property does.

"SBR Affects Unit Production"->"Damage From Bombing Done To Units Instead Of Territories"

this should be considered, looks like a simple search/replace

color.Impassible=->color.Impassable=

I might have missed updates to properties files. With some luck I did hit this, but would be good to verify. This should be done. (I think i did a global search/replace on all file types, in game engine code was just xml and java files)

[DanVanAtta]

When "Naval Bombard Casualty Return Fire Restricted" is set to false, naval casualties do not fire back, correct, that is the current behavior? i want the name to first match the behavior, then we can look at changing the default or behavior+name.

[Cernelius]

Ah ok, I guessed that was not really a question. Sorry.

Yes, that is correct (the option currently does the opposite of what it says and if you would remove the " Restricted" part, then it would do exactly what it says).

[DanVanAtta]

Okay, then seems like it would be better if: "Naval Bombard Casualty Return Fire" was renamed to: "Immediately Remove Naval Bombard Casualty "

[Cernelius]

"Naval Bombard Casualty Return Fire" -> "Naval Bombard Casualty Removed Immediately"

I agree (and default false), but in this case you should also change the engine behaviour upside down, since "Naval Bombard Casualty Removed Immediately" means the opposite of "Naval Bombard Casualty Return Fire" (saying "Naval Bombard Casualty Removed Immediately" is the same as saying "Naval Bombard Casualty Return Fire Restricted").

[DanVanAtta]

I'm confused why the engine behavior should change. Isn't the point of updating the name only to make it more clear what it does, it should match the behavior as is, right? The name change will only be a clarification?

[Cernelius]

No, because both the name and the default relatively to the name are fine, but it is the behaviour that it is messed up upside down. And this is because all options are supposed to work default as per v1 rules, and I highly advise following this fully, for several reasons I've already explained.

Basically, if you change the name as doing what the property does, you are legalising a bug.

Since originally TripleA supported only "Classic" (=v1) rules (so, I assume there was not this option, that I guess was added upon supporting "Revised" (=v2) too), I guess this bug was introduced along the way, by someone that guessed v2 and v1 were supposed to work the same way, on this regard (I'm just surmising).

On the above points, I fear there are a lot of misunderstandings going on; so I suggest you wait some time before pushing any other changes, and I will try to clarify what I was trying to say in a few hours from now.

Thanks again.

But, first of all, I want to make clear what the single sections mean:

The:
Naval Bombard Casualties Return Fire Restricted
is the only option I'm suggesting an engine behaviour change (for absolutely nothing else I'm suggesting anything else but naming changes or engine deletions of already deprecated elements).

XML related engine deletions
Means that these ones should be just fully deleted in the engine only, but not in any maps' xml.

XML related engine changes and XML changes
Means that these ones should be name-changed both in the engine and in any maps' xml having them.

XML only deletions
Means that these ones should be removed in any map's xml having them, but not in the engine.

XML only changes
Means that these should be name-changed in any map's xml having them, but not in the engine.

Please, can you confirm that you understood all the above correctly; and if not, please redetail fully all points you are unwillingly to change the way I suggested, to make sure you are not thinking I'm proposing something different from what I've in mind and am trying to explain.

Thank you again.

[DanVanAtta]

XML related engine deletions
Means that these ones should be just fully deleted in the engine only, but not in any maps' xml.

Why keep it in the map XML? If not in the engine, it will not do anything if in the XML.

XML only deletions
Means that these ones should be removed in any map's xml having them, but not in the engine.

Opposite question, why keep unused features in the engine?

XML only changes
Means that these should be name-changed in any map's xml having them, but not in the engine.

It does not work like this, pretty much every map XML entity maps to something in the game engine. The names need to be kept in sync or transitions need to be managed on the code side.

[DanVanAtta]

The game behavior matches the naval bombard property. The fact that does not match v1 I'm not terribly upset about. Regardless, I would like to focus on the 1.9 release. The fact 1.8.0.9 map process is frozen has caused a lot of tension. Cernel, please summarize any remaining changes you want, and split them between:

• updates to naming so the name match behavior
• updates to behavior so the behavior matches the name
• obsolete setting removal

Please also number each change requested so it can be discussed more easily.

[Cernelius]

• Updates to behaviour so the behaviour matches the name

1A) Engine only:
property name="Naval Bombard Casualties Return Fire Restricted": invert behaviour (to behave as true like it now does as false, and vice versa)

1B) Maps' xml only:
property name="Naval Bombard Casualties Return Fire Restricted" value="true"->property name="Naval Bombard Casualties Return Fire Restricted" value="false"
property name="Naval Bombard Casualties Return Fire Restricted" value="false"->property name="Naval Bombard Casualties Return Fire Restricted" value="true"

1C) Engine only:
property name="WW2V2" has to be updated so that when it is true it will change the property "Naval Bombard Casualties Return Fire Restricted" to be true too, as long as this property is not specifically set in the xml.

Note:
The property name and it being false are both correct with respect to v1 Rules (TripleA default), but the behaviour is exactly the opposite.
The property value has to be consequently changed in all maps having it set (from "true" to "false" and from "false" to "true"), on the assumption that the mapmakers set the option on what it does, not on what it says.
The "WW2V2" is a general property that turns the game from v1 Rules (default) to v2 Rules.

• Updates to naming so the name matches the behaviour

2A) Engine and Maps' xml:
isImpassible->isImpassable

2B) Maps' map.properties:
color.Impassible->color.Impassable

3A) Engine and Maps' xml:
offence->offensive

3B) Engine and Maps' xml:
defence->defensive

Note:
The only case in the xml where the terms "offence" and "defence" are used are for the supportAttachment, namely something like this:

    <attachment name="supportAttachmentRussiansArtillery1" attachTo="artillery" javaClass="games.strategy.triplea.attachments.UnitSupportAttachment" type="unitType">
        <option name="unitType" value="infantry"/>
        <option name="faction" value="allied"/>
        <option name="side" value="offence"/>
        <option name="dice" value="strength"/>
        <option name="bonus" value="1"/>
        <option name="number" value="1"/>
        <option name="bonusType" value="ArtyOld"/>
        <option name="impArtTech" value="true"/>
    </attachment>

My suggestion is to change (both in the engine and in each map's xml), more in detail:
option name="side" value="offence"->option name="side" value="offensive"
option name="side" value="defence"->option name="side" value="defensive"
option name="side" value="offence:defence"->option name="side" value="offensive:defensive"

The main reason for this change is that we already have an option called:
option name="defense"
which is the very basic option that decides at what strength the related unit will defend, upon rolling dice (it is commonly 2 for the infantry, 4 for the battleship, etc.).
It is very stupid having an option called "defense" and another option called "defence", thus the option called "defence" has to be renamed to "defensive".
Similarly, "offence" has to be renamed to "offensive".
For more reasons behind this change, please read some other post of mine at this Issue (I would say not needed to give other reasons, as the main one is enough).
An alternative suggestion, from Frostion, has been to change "offence" and "defence" to "offense" and "defense", and I've argumented why this would be a bad choice, in a previous post of mine.
Side note, I'm not going to suggest any changes from British English to American English, so I would not have given any suggestions at all, in this case, if the issue would have been only this being a British English spelling (meaning that I would definitively suggest the change to offensive/defensive even if the current names would be offense/defense (please refer to some other post at this Issue for more clarifications on this matter)).

4. Engine and Maps' xml:
   Battleships repair at end of round->Units Repair Hits End Turn

Note:
Here there are 3 issues:
This option applies to all units having multiple HitPoints, not just to the ones named "Battleship".
It is needed to somehow clarify we are talking of repairing HitPoints damages, not damages from SBR etc..
This option takes effect at any EndTurn, not end of round.

5. Engine and Maps' xml:
   Battleships repair at beginning of round->Units Repair Hits Start Turn

Note:
Here there are 3 issues:
This option applies to all units having multiple HitPoints, not just to the ones named "Battleship".
It is needed to somehow clarify we are talking of repairing HitPoints damages, not damages from SBR etc..
This option takes effect at the first "move" phase of the unit's owner, not at beginning of round.

6. Engine and Maps' xml:
   option name="turns"->option name="rounds"

Note:
This option is (seldom) used in cases like (from Napoleonic Empires):

            <attachment name="conditionAttachmentTurn2" attachTo="France" javaClass="games.strategy.triplea.attachments.RulesAttachment" type="player">
                <option name="turns" value="2"/>
            </attachment>

But, despite what the option says, it is verified for the whole second round, not just for the second turn of "France", thus the name must be changed accordingly, from "turns" to "rounds", as it means "rounds", not "turns".
I believe only a few maps use this option, and it is an option only for conditions or objectives (so, I'm not suggesting any general change from "turns" to "rounds": this one is a very simple and limited change too, not harder than the isImpassible to isImpassable one).

• Obsolete or redundant setting removal and related naming changes to maps having the obsolete reference

7A) Engine only:
option name="victoryCity" value="true"
option name="victoryCity" value="false"
Stop supporting boolean only (numbered are already supported, making boolean redundant, as "0" is the same as "false" and "1" is the same as "true").

7B) Map's xml only:
option name="victoryCity" value="true"->option name="victoryCity" value="1"
option name="victoryCity" value="false"->option name="victoryCity" value="0"

Note:
This is a very unimportant change. Numbered value are already supported, but the old boolean is still supported too, which is redundant.

8A) Engine only:
"takeUnitControl"
Remove this option from the engine.

8B) Maps' xml only:
option name="takeUnitControl" value="true"
option name="takeUnitControl" value="false"
option name="giveUnitControl" value="true"
option name="giveUnitControl" value="false"
Remove these options from all maps having them.

Note:
All and only the boolean occurrences of "giveUnitControl" must be deleted.
While "takeUnitControl" is wholly obsolete, "giveUnitControl" is still used, with player names as value, not boolean.

9A) Engine only:
"occupiedTerrOf"
Remove this option from the engine.

9B) Maps' xml only:
"occupiedTerrOf"->"originalOwner"

10A) Engine only:
"isParatroop"
Remove this option from the engine.

10B) Map' xml only:
"isParatroop"->"isAirTransportable"

11A) Engine only:
"isMechanized"
Remove this option from the engine.

11B) Map' xml only:
"isMechanized"->"isInfantry"

12A) Engine only:
"isTwoHit"
Remove this option from the engine.

12B) Map's xml only:
"isTwoHit" value="true"->"hitPoints" value="2"
"isTwoHit" value="false"->"hitPoints" value="1"

Note:
"isTwoHit" is an obsolete boolean option that used to define whether an unit would have had 2 HitPoints or 1, 1 being the default.
Currently, this is instead set via the numbered "hitPoints" option, and all that "isTwoHit" is left doing is setting the value of "hitPoints" to 2, if true, at start game.
Practically, "isTwoHit" is still supported only to keep supporting maps using the obsolete option, thus I suggest getting rid of it, and update all maps as to use the new "hitPoints" option, instead.
Side note, I think this was the very last major engine change developed by Veqryn, I suppose for his own "Galaxy" map.

13A) Engine only:
option name="conditionType" value="XOR"
Remove this option's value from the engine.

13B) Map's xml only:
option name="conditionType" value="XOR"->option name="conditionType" value="1"

Note:
This is not a deprecated element, as far as I know, but the "XOR" case it is exactly the same as "1"; thus, being covered by the already supported numbered values, there is no need of having this duplicate.
The "1" is even better telling than "XOR", since "1" obviously means that exactly "1" of the conditions must be verified, while "XOR" is definitively not a common word (unlike "OR" or "AND").
Anyway, this is very unimportant; thus keeping the useless "XOR" is not a real issue.

14A) Engine only:
"trigger"
Remove this option from the engine.

14B) Map's xml only:
"trigger"->"conditions"

Note:
The removal of "trigger" is very limited in scope, and, to be clear, refers only to options like this:
option name="trigger" value="conditionAttachmentXXX"
that are deprecated, and are supposed to be written this way, instead:
option name="conditions" value="conditionAttachmentXXX"
My suggestion is simply to discontinue the support to the already long time deprecated "trigger" option, and only admit the new "conditions" one; then, rename
option name="trigger"->option name="conditions"
in all xml still having this obsolete option.

15A) Engine only:
"SBR Affects Unit Production"
Remove this option from the engine.

15B) Map's xml only:
"SBR Affects Unit Production"->"Damage From Bombing Done To Units Instead Of Territories"

Note:
SBR Affects Unit Production no longer does anything at all. Instead it just sets to true: "Damage From Bombing Done To Units Instead Of Territories".
Thus, I advise to get rid of the old and duplicative property, and update all xml still having it.

General Note (IMPORTANT!):
I might not have caught all duplicative / obsolete / deprecated option, thus I encourage the developers to delve into the code and see if there are others, discussing their addition to the list at this Issue.
It would be nice to take the 1.9 release as on opportunity to fully clean up all the old rubbish from the engine, and update all xml.
Other than what listed, I know there are the following 3 duplicative options:
"artillerySupportable"
"artillery"
"unitSupportCount"
that, albeit duplicative of what done via supportAttachment, I definitively suggest them being kept, as they are, because they make both mapmaking and tooltip display easier.

[FrostionAAA]

Concerning “SBR Affects Unit Production” and “Damage From Bombing Done To Units Instead Of Territories”:

A better and more descriptive name could be “Damage From Bombing Is Done To Damageable Units Instead Of Unit Production Capabilities Of Territories”. I know that the description is a bit long, but if this is what happens then it would be better. Also the whole unit damage and unit production thing is very complicated and therefore terms should maybe be more self explaining.

Another thing that would help understand the system and what is going on concerning territory PU and unit production could be some alterations in the basic names of Territory Attachments. Like these:

production -> PUAndUnitProduction

productionOnly -> PUProductionOnly

unitProduction -> unitProductionOnly

There are a lot of lines in PoS2.xml that explains how a territory’s “production” value also sets unitProduction to the same value. I would guess that many of these sentences could become obsolete if the Territory Attachment names became more self explaining.

I think the whole bombing, damage and unit production system is very cryptic, probably because it must support a variety of settings, so I might not understand the system to its full extent. So if my proposal does not make sense, please disregard ;-)

[ZjelcoP]

I there or will there be a complete list with all changes that will have to be made to a map to have it 1.9.0.0 compatible? As some old maps and new maps will be added after the big changeover that will need conversion.
Or is there a script to be run that will convert a map?

[Cernelius]

Sorry @FrostionAAA but “Damage From Bombing Is Done To Damageable Units Instead Of Unit Production Capabilities Of Territories” is horribly long, which means a "Game options" window having it listed would become ridiculously wide, and this is bad.
I don't think the "production" stuff really needs changes, but I'm not against your proposals, yet I can't say I like the proposed names. There are at least about a dozen other things that I would change (comprising shortening some of the longest properties or better naming things like "attackAA" to "defensiveAttackAA"), but I limited my list to only what I believe definitively needs it.

I agree with you @ZjelcoP , and, if no developers do it, I may see to do that list myself, for reference; maybe also a list for changing back maps from 1.9 to 1.8.0.9. I'm guessing the wiki here would be the best place to fit it?

Up until the changes related to this Issue, to convert a map's xml you needed:

1.8.0.9->1.9
Attatch->Attach
attatch->attach

1.9->1.8.0.9
attachmentList->attatchmentList
attachment name=->attatchment name=
attachTo=->attatchTo=
triplea.attachments->triplea.attatchments
/attachment->/attatchment
"techAttachment->"techAttatchment
"unitAttachment->"unitAttatchment
"territoryAttachment->"territoryAttatchment
"canalAttachment->"canalAttatchment
"rulesAttachment->"rulesAttatchment
"playerAttachment->"playerAttatchment

[DanVanAtta]

Maps should be updated now, hopefully all is playable.

[simon33-2]

@ALL - I'm interested to know if anyone else is willing/wanting to also learn how to make mass map updates. It would be good for me to not be the only one.

Hi @DanVanAtta
Is this offer still on the table? I know a bit about UNIX and am willing to help.

[DanVanAtta]

@simon33-2 https://github.com/triplea-maps/Project/blob/master/README.md
that would get you started. As we do more and more with maps we should probably keep adding to that list.

[simon33-2]

I already know how to do all that.

What I don't know how to do: get a list of all repos under triplea-maps in a machine readable form.

[DanVanAtta]

Check the contents of the clone_all script: https://github.com/triplea-maps/Project/blob/master/bin/clone_all.sh

[simon33-2]

@DanVanAtta

Regarding the idea of someone else being able to do bulk updates, wouldn't that require someone else having write access to all the map repos? I don't think you want to merge 20+ PRs for a bulk update.