Triplea: Strategic Bombing Bug (3307)

[panther2]

Reported on aaa.org:

http://www.axisandallies.org/forums/index.php?topic=38761.msg1595640#msg1595640

I only have been playing the WWII v5 1942 2nd Edition map, I don't know the other maps.

Unfortunately, with the new version (1.9.0.0.3307), there seems to be the following problems with the stratetic bombing:
-stratetic bombing doesn't damage a factory, even it was a hit. The factory stays undamaged.
-stratetic bombing suddenly costs (shouldn't it be for free on the WWII map?)
-there isn't even the possibility to edit the factory damage ("change unit bombing damage") in the Edit Mode, the buttom doesn't appear.

With version 1.8.0.9, those things were working fine!

I was able to reproduce it:
Just perform a strategic bombing. Instead of damaging the factory, the damage is substracted from the fabric owning nation's PU balance. The factory is undamaged. No damage can be edited.

For reference, this most likely was broken in #1133

[panther2]

I can confirm the issue for the following maps:

• wwIIv3
• wwIIv4
• wwIIv5

(wwIIv6 does not include strategic bombing and I did not test older map-versions)

It appears that wwII_Global is the only (wwII) map not affected by this bug, so I assume that the
"Strategic Bombing Fix" possibly introduced problems on the other maps.

[simon33-2]

Problem comes from the missing map property "Damage From Bombing Done To Units Instead Of Territories", at least in v5.

I'll add a PR.

Sorry this took so long to look at. I've moved cities and started a new job.

[simon33-2]

Ok, 3 PRs added.

Only changed 1942 version in both v3 and v4.

If I should change the 6 army and 1941 variants can you let me know? Can someone familiar with the correct behaviour please test these after they've merged? I've tested the v5 version and it works.

I also left the ...v3_11n map repo alone.

@ron-murhammer

[panther2]

Thanks, but please include at least the v3-1941, too. It has always been more popular.

On the other hand can't this issue be fixed on the engine-level (where it possibly has been introduced)? Who knows which maps are affected - apart from those mentioned above (the only ones I tested)?

[simon33-2]

Not really I would think. The maps affected have always lacked the correct option but it didn't matter previously apparently. Perhaps it was a different change which caused this and not the Strat bombing bug fix. Anyway, I'm not really that worried about which change caused the problem. We seem to have a fix now.

[panther2]

Interesting... thanks.

[simon33-2]

Ok, v3-1941 is in the PR for v3.

[panther2]

Great , thanks.

So chances are, that a lot of maps are affected by this issue. Maybe all maps including strategic bombing - except wwII_global and those fixed by now ...

[simon33-2]

Well, all the ones besides global, v3, v4, v5 which damage factories rather than deduct income.

So Classic would be fine.

The problem with an engine fix is that it would need to be able to determine the correct behaviour some other way.

[simon33-2]

Should the six army variants of v4 be changed?

[simon33-2]

Had a quick look 1.809 and I can't at all see how it worked for v5 at least.

[ron-murhammer]

@simon33-2 So I'm fine with adding the appropriate property to maps but think we need to also update the engine to make it would for those maps without the property like it did previously. The main reason being its going to be very difficult to determine all the maps that need updated. Most likely to do that you'd need to run a search to see which maps don't have the property defined.

I think we need to re-review #1133 and hopefully fix it. Otherwise consider reverting it and attempt to redo the change.

[panther2]

So do we touch the map repos solving this issue, or don't we?

(Referring to
https://github.com/triplea-maps/world_war_ii_v3/pull/1
https://github.com/triplea-maps/world_war_ii_v4/pull/1
https://github.com/triplea-maps/world_war_ii_v5_1942/pull/1)

In case we do we need sort of a list of all affected maps/variants...

[ron-murhammer]

@panther2 The preference would be that we shouldn't need to change the map repos. Otherwise its going to be difficult to track down all the maps that need changed.

[simon33-2]

I can't see how it worked in 1.809.

Anyway, as I've noted above, the only way we can avoid touching the maps is if we can identify some other attribute to look at.

Revert #1133? You've got to be kidding. There is no work around for that. This is functionally identical if the bombee repairs immediately and if not there is an edit mode fix.

[simon33-2]

Regarding #1133, I've just tested this on v5 with the map as it was before in build 2882 (before #1133 was merged), and it incorrectly deducted the damage from the PUs of the bombee nation.

Whatever broke this between 1.809 and 1.900.3010+, it wasn't #1133.

[ron-murhammer]

@simon33-2 Interesting. I guess I'll have to take a closer look then at this issue and see what changes were made before #1133.

@panther2 I went ahead and merged the specific map fixes in the mean time:
triplea-maps/world_war_ii_v3#1
triplea-maps/world_war_ii_v4#1
triplea-maps/world_war_ii_v5_1942#1

Please test those to make sure they work properly while we track down the engine bug.

[simon33-2]

Hi Ron,
Something appears to have changed with the way the release of maps is incorporated. We don't need to update that any more? Cool.

[panther2]

@simon33-2
When you download a map for the first time you always get the latest version regardless of the versioning in the triplea_maps.yaml. When already installed, TripleA does not offer to update the maps, as long as the version number in the triplea_maps.yaml has not been changed. So updating the map's version number in the triplea_maps.yaml is still necessary to be informed about map-updates using the ingame downloader.

( So it is still the same process as described here https://github.com/triplea-game/triplea/issues/1266#issuecomment-249708291 )

[panther2]

@ron-murhammer
I have tested the new versions of wwii_v3_1941, wwii_v3_1942, wwii_v4 and wwii_v5 now.
I can confirm that damage is now attached to the fabrics correctly, also damage is editable again.

[panther2]

Pull request for updating triplea_maps.yaml is here: #1347

[Cernelius]

As @ron-murhammer pointed out, of course, the change, whatever it is, cannot be at a map level, to start with, as all these maps (adding up BWv3 too) were working in the previous release (it is just not possible that they were already all broken and nobody noticed LOL), thus the issue is at the engine level, definitely not only at a map level, and should be firstly either addressed at the engine level or by giving confirmation (and upgrading the "Upgrade Maps Instruction") that the change (making those maps as coded not anymore working as intended with the new release) is intended, and only then touching any maps.

I believe these updates to maps you are making are wrongly aimed, one way or the other.

You should not leave a not working option beside adding another one meant to do the same thing, if that was even needed.

It should be firstly ascertained if the not working option is a bug or communication fault then, either:

1- if it is the first case, either:

1A- leave the map totally alone, and correct the bug in the engine, if no matter at all;

1B- substituting the option, if it is supposed to work but be deprecated (not adding the working one beside the deprecated one!);

2- if it is the second case, substituting the option (not adding the working one beside the unsupported one!).

In particular, it is needed to know if this is an engine bug or if this is has been intended or what. If it is not a bug, then the:
https://github.com/triplea-game/triplea/wiki/Upgrade-Maps-Information
will need updating.

I also fear that patching up just some pernsonal favourites might just result in burying up the rest, to be progressively reported (in a confusingly manner, most likely) over the years, or maybe never, especially since the engine doesn't validate properties.

Also, generally speaking, you should not add properties of stuff at the end of all others, but, instead, adding them by the order specified by @veqryn , unless the current developers don't want it to be followed anymore (please let us know, in this case) (for example, "Damage From Bombing Done To Units Instead Of Territories" should not stay after "Neutrals Are Blitzable", in v3, unless the directives have changed).

I'm also curious, since I asked that property be removed from the engine, a few months ago; so maybe there has been a miscommunication there and also in the making of the Wiki thereafter? Definitely this is urgent, also for the very popular "Big World : 1942 v3 Rules" (which is more played than the sum of v3, v4 and v5, in lobby!).

Of course, I'm not saying that this is the fault of anyone making the changes (!), as I believe here we have some underlying problem (either it is a bug or the Wiki is lacking information about what to do and how to do it correctly).

So, first item to address or clarify here is:

Can any developer communicate if the fact that the property "SBR Affects Unit Production" appears to be currently ineffective is wanted or not and, then, either confirm that such property has been totally removed from the engine or, instead, this is just a bug?

If any code should be changed or it is anyway advisable to be changed, better to do it as a consistent and serious one-time mass-change for all cases in the repository, instead of spuriously running around pushing changes for this or that map (and that you are doing wrong anyway, since you are adding the working property beside the one that has stopped working, instead of correctly substituting it).

In any case, as I said months ago, my suggestion is still fully removing the property "SBR Affects Unit Production" from the engine (or just update the Wiki if it has already been), and mass changing it in all maps to "Damage From Bombing Done To Units Instead Of Territories". Then, checking for duplicates (it will be duplicated in all the maps referenced at this Issue, unless you firstly revert all map's changes done thus far, as I suggest), and removing them.

Thanks.

[panther2]

We should definitely try to find out what broke the maps on the engine level.
I have understood the latest map fixes as sort of "workaround" until the engine has been fixed.

@Cernelius
You said

Also, generally speaking, you should not add properties of stuff at the end of all others, but, instead, adding them by the order specified by @veqryn , unless the current developers don't want it to be followed anymore (please let us know, in this case) (for example, "Damage From Bombing Done To Units Instead Of Territories" should not stay after "Neutrals Are Blitzable", in v3, unless the directives have changed).

Can you please point me to the said specifications and directives? I am searching for any documentation about the engine and the maps' game.xml with all those attachments, properties, options and so on...
and their effects on the game.
Thank you.

[simon33-2]

@Cernelius
Look, there clearly needs to be some sort of marker at the map level for this unless it was hard coded. If you want to work out how this worked in 1.809, feel free to spend time on that.

I'm happy to update Big World when I get back home probably.

And what panther said.

[Cernelius]

https://github.com/triplea-maps/the_pact_of_steel/blob/master/map/games/pact_of_steel_2.xml

PLEASE KEEP YOUR PROPERTIES IN THIS ORDER!!!

Anyway, I tend to interpret that not as a binding order, but more as a guideline, maybe to be strictly followed only regarding maps you don't have the ownership of, as I have to admit that sometimes I put properties in different orders, if I think it will make easier for people to manage them (when editable) before playing. But definitively, I would say not just adding them at random. That would just make the xml harder to read for anyone else. But, as I said, if the new developers disagree with @veqryn , and no order is asked for anymore, just let us know; not a big deal.

Generally, I would anyway advise to at least putting properties beside the logically similar ones.

Aside from this, that should have been made by substituting the "Damage From Bombing Done To Units Instead Of Territories" property to the "SBR Affects Unit Production" property, not adding it beside it, as I've already explained.

The @simon33-2 answer (which makes sense, as nobody can be expected to know everything) is one of the reasons why I requested the recent engine updates, to make stuff some more logical for everyone not having all history since 2001, and this one was actually one of those few that have not been accepted / made, because "SBR caused an odd test failure" (or at least @DanVanAtta said so).
You can see here all the ones made (by @DanVanAtta ) and communicated, as far as I know:
https://github.com/triplea-game/triplea/wiki/Upgrade-Maps-Information

And, yes, it is obvious that to "work out how this worked in 1.809" is something that (if not you (not saying you must)) somebody has to do, eventually (just my suggestion). Anyway, I've already explained how it worked at my previous post, and now also linked the full info here; so now the matter is just to wait for a developer to explain why it doesn't work anymore (and if it is a not well documented change or a bug).

Thanks.

p.s.: As much as I think a quick fix for BWv3 will be surely appreciated by the playing community, I still suggest waiting for the main developers to clarify the matter some, at least knowing if this is a bug or a communication breakdown or what else, but not up to me to decide. I would not tell people a "new" version of BWv3 is out, if this is just a bug that can be corrected in the engine only soon, or maybe wait to bundle it with the other bugs Big World currently has (just my advice). But all good, just push it, if you want; then it will be up to the developers to merge it anyways, if they think so.

p.p.s.: I wish to quote myself, in case it was overlooked:

Of course, I'm not saying that this is the fault of anyone making the changes (!), as I believe here we have some underlying problem (either it is a bug or the Wiki is lacking information about what to do and how to do it correctly).

[simon33-2]

I'm not sure what the actual correct order is??? Perhaps explain it slower. I didn't see in either link where it was explained.

I've updated v3 in the (apparently) incorrect order. PR here: https://github.com/triplea-maps/big_world/pull/4

Happy to update all these to apply a correct order later.

[Cernelius]

In this particular case, as well as all the ones related to this issue, as long as the game worked fine with the 1.8.0.9, you don't actually need to know the order, as I'm suggesting to revert what already done, and do these changes (as I already said multiple times), instead:

="SBR Affects Unit Production" : ="Damage From Bombing Done To Units Instead Of Territories"

Also, that one of putting it in the correct order was a side matter, and mostly a matter if the new developers still wants that as a guideline. I would not be surprised if the existent "SBR Affects Unit Production" is not in the right place, either.

Also, as I said, as a map / mod maker, I don't always follow the @veqryn order, either.

But what I'm mainly saying is doing nothing at all till the main developers clarify, but this is just what I would do myself. If the developers say this is just an engine bug, then no need to touch maps.

[simon33-2]

I've done a quick test on 1.809 and removing the "Damage From Bombing Done To Units Instead Of Territories" causes the problem.

So I think the fix for this issue would be to look at that attribute instead. I'm looking at coding that right now.

[Cernelius]

But since @ron-murhammer said:

So I'm fine with adding the appropriate property to maps but think we need to also update the engine to make it would for those maps without the property like it did previously. The main reason being its going to be very difficult to determine all the maps that need updated. Most likely to do that you'd need to run a search to see which maps don't have the property defined.

Then go ahead adding the property to all those maps you think are the most popular ones (I wouldn't, but no matter), if you wish to; I'm just saying to do it as per my previous post (and I said it in the others too); otherwise, either you keep around a property doing nothing or, if this is a bug and gets corrected, you will have a same property set twice (once directly and once indirectly), which should not give problems, but makes no sense; thus why I'm suggesting doing it in a better / more consistent / long term safer manner.

As I said, not saying it is your fault, as this is either a bug or we are missing it in the Upgrade Maps Information.

I guess you did that test on a map that had "Damage From Bombing Done To Units Instead Of Territories" and not "SBR Affects Unit Production".

As I've already said, multiple times, and it is explained at the link I gave, "SBR Affects Unit Production" and "Damage From Bombing Done To Units Instead Of Territories" obtain the same final effect, an the issue is that "SBR Affects Unit Production" stopped working, and now it is up to the developers to clarify if this is wanted (either now or already) or it is only a bug.

[Cernelius]

As I said, my suggestion was and it is still:

1) Removing the property "SBR Affects Unit Production" from the engine entirely.

2) Mass updating all xml this way:
="SBR Affects Unit Production" : ="Damage From Bombing Done To Units Instead Of Territories"

(but before doing that, revert all changes made at this Issue, or you will end up with a duplicate property)

3) (Optional) Testing no maps now have multiple occurences of:
="Damage From Bombing Done To Units Instead Of Territories"

I suggested that monts ago, and, if it was made, we would not be having this discussion now.

@DanVanAtta agreed on this proposal, and wanted to do it, but he encountered problems (an odd test failure, reportedly), as far as I know. This is why you don't see that listed in the current "Upgrade Maps Information" (all those other items listed I believe were made by @DanVanAtta ).

[simon33-2]

Ahh, I don't think I've understood that until now.

Well, congratulations on forecasting this issue. Sounds a good idea that you are proposing which should be rammed in regardless of test failures - these can be nullified by removing the relevant assert lines if the test can't be fixed.

[Cernelius]

Ok, all good.

To be clearer, what at my previous post was one of the few items that were not made at:
https://github.com/triplea-game/triplea/issues/1033
because deemed too problematic / time expensive to be worth tackling at that point:

@DanVanAtta :

All but 1-3, and 14+ are now done.

I would have liked to have gotten them all, but the remaining ones are bit more involved (SBR caused an odd test failure, the others need some deeper engine work). I think this is a decent stopping point for before 1.9.

There is some follow up works on the maps themselves that needs to take place now.

The remaining items can be re-summarized in the header, or in a new issue. If it's not terribly important to follow up, we can close this out. I do think there should be a drop down option for 'v1 / v2 / v3' settings. In that case we would very much need to make sure the values/behavior were accurate to the rule set.

Maybe better opening a new Issue for this, if a developer wants to do what I'm proposing at my previous post. That would also automatically solve al problems at this Issue, that can be then closed.
Just remember to revert all changes done here, before that.

I would then update:
https://github.com/triplea-game/triplea/wiki/Upgrade-Maps-Information

Thanks.

[Cernelius]

Side off topic note, if a developer is disposable tackling all left at:
https://github.com/triplea-game/triplea/issues/1033
especially or even exclusively the wrong default for the naval bombardment, please open an issue about it. I would just ask that to be done all in 1 go and shortly before a new official release everybody has to download (meaning that you officially release it a few days after and the Upgrade Maps Information gets updated), to not give problems / confusion to mapmakers / players over a too long timeline (especially since now there is not the different map folder (maps vs downloadedMaps) thing anymore).

But, actually, the only 1 thing left that I really dislike is the wrong default for the naval bombardment (default you don't return fine, while you should be able to).

[simon33-2]

In fact, to fully mirror the 1.809 behaviour any map with both attributes should remain as is. Only the maps with "SBR Affects unit production" and without "Damage From Bombing Done To Units Instead Of Territories" should be updated. In 1.809 if you set both, the latter attribute took precedence.

I'm happy to work out how to apply this change if there is an easy way to do it? Might be easier for someone with write access to do it though. Not too keen on logging 20 pull requests.

[Cernelius]

The maps with both should have (and should have had) "SBR Affects Unit Production" fully deleted.

As is said, the point 1 at my proposal is (was):

Removing the property "SBR Affects Unit Production" from the engine entirely.

Even tho currently the properties are not validated, I would surely not leave a pointless one there, especially considering that it may be editable (unlikely).

[Cernelius]

Theorically, you should also check if the property is referenced in a condition, but I would not bother about this, as I don't believe you would find a case.

[simon33-2]

Ok, I've done a search in the 1.809 maps and found all the repos which appear to be updated. These are:
big_world
world_war_ii_europe
world_war_ii_global
world_war_ii_pacific
world_war_ii_v3
world_war_ii_v4
ww2v3_11n

I've updated all of these now and tested two. Pull requests have been created in the above areas. The v5 one didn't need to be changed.

[Cernelius]

Not trying to criticise your work or saying it is not good to push some specific changes meanwhile, but, to be just clear, when I said _Mass updating_ I meant automatically doing that, like @DanVanAtta did for "attatch" to "attach". Almost surely, there are other less popular maps that would need that update, unless this is just a bug, and the main developers prefer to restore the functionality of the "SBR Affects Unit Production" property, which is why I said wait for their opinion on the matter (but no problem pushing requests, since it is up to them to accept, anyway (it just seems obvious to me that it would be better done as a mass update)).

[Cernelius]

Regarding that list only, I just think it would be confusing updating these two:
https://github.com/triplea-maps/world_war_ii_v3
https://github.com/triplea-maps/ww2v3_11n

while not updating this one:
https://github.com/triplea-maps/ww2v3_variants

Also, just for general information, @DanVanAtta asked for a developer to side him in managing the repository (like being also able to do mass updates); I've no clue if the offer is still there; just saying, in case.

https://github.com/triplea-game/triplea/issues/1033
_@DanVanAtta :_

@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.

[simon33-2]

FWIW, I've added updates for those maps.

The point is though, that every map which worked in 1.809 (barring non standard activity) should work in 1.9 now. This wasn't previously the case. That's why I bothered.

I've put my hand up for helping out with the maps.

[simon33-2]

I've downloaded about half of the map repos and it seems about half of them need updating. I think the only way it is going to be reasonable to do this update myself is if I'm given write access to all the maps. In that case I would be happy to do it but otherwise I think @DanVanAtta will have to do it.

[DanVanAtta]

What's the update that needs to be run @simon33-2 ?

Bit as another topic - Any luck tracking the problem back to the code? I think Cernel had some good points that if we can fix the code then we can avoid the map updates. In the meantime many maps were fixed, which is important to keep the game working as it is supposed to.

[DanVanAtta]

Found:

-    <property name="SBR Affects Unit Production" value="true" editable="false">
 +    <property name="Damage From Bombing Done To Units Instead Of Territories" value="true" editable="false">

How do we know which maps this would need to be done for?

[simon33-2]

1.809 referred to the sbr property if the damage property wasn't set.

We could change 1.9 to do the same thing but then we have a redundant property in the system. Better to :
Search for the damage property. If not found, update the sbr property to the damage property. If damage property found, remove sbr property.

[simon33-2]

BTW, I'm pretty sure cernel was arguing the exact opposite, unless I completely misunderstand him.

[simon33-2]

When I get home I should be able to give you some Unix to do the above.

[simon33-2]

Here's some UNIX which should make all the necessary updates (and no more) unless there is some very tricky lines in the .xml files:
grep -l "SBR Affects Unit Production" */map/games/*.xml > list
for f in `cat list`;do if grep '\ x!=1 { print $0 }
/<\/property>/ { if(x==1) print substr($0,index($0,"\")+11); x=0; } ' $f > tmp1 ; mv tmp1 $f ; else sed -i 's/SBR Affects Unit Production/Damage From Bombing Done To Units Instead Of Territories/' $f;fi ;done
source_dir=$PWD
cut -d/ -f1 list | sort -u ;while read dir;do cd ${source_dir}/${dir} ; git commit -a -m"Replace SBR Affects Unit Production
For Damage From Bombing Done To Units Instead Of Territories";
git push
done

Note that I had to put a couple of backslashes in the above to get past the significance of the less than sign to HTML.

I am assuming you have already cloned every map repo to a subdirectory of the current directory

[panther2]

I see many map repos have been updated - maybe wwii_v3 has been missed?
https://github.com/triplea-maps/world_war_ii_v3/pull/2

[DanVanAtta]

Biggest round of updates were for paratroop bug. As soon as the SBR fixes are confirmed they're getting merged. Chances are we just need to apply the latest SBR fixes to that map (world war ii v3)

[simon33-2]

I count 55 repos needing updates to retire the SBR attribute.

Really create 55 PRs?

[DanVanAtta]

Let's first focus on the the half dozen most popular maps out of the 55 @simon33-2 . Once we've ensured that we have understood and verified the fixes there, I think we'll be in a better position to successfully mass update the 55 maps.

[DanVanAtta]

It would be very useful if we had a way to identify v3 repo's, though I'm not sure if there is. Regardless v3 maps are the priority since their behavior is currently incorrect and would update with this fix.

simon33-2, if you bulk apply the fix, and consistently put it on the same branch name. If agreeable I can pull the updates from your remote. I'll then also test a dozen or of the maps in addition to analyzing XML differences.

[simon33-2]

Not sure what you mean about the v3 variants. I think we've fixed all the repos with v3 in their name. I can have a look at this when I've back home (4 Jan) unless I can figure a way to do it from my holidays.

[simon33-2]

Ok, I've been able to push the changes for 20 repos (lack of disk space is the problem for the other 35). Pushed to a branch called "SBRPropChange" on my fork.

Repos:
big_world_2
diplomacy
camp_david
domination_1914_no_mans_land
hex_globe10
battle_of_jutland
star_wars_galactic_war
elemental_forces
large_middle_earth
world_war2010
blue_vs_gray
feudal_japan_warlords
ur_quan_war_masters_edition
sleeping_giant
pact_of_steel_variations
ultimate_world
total_ancient_war
star_wars_tatooine_war
great_lakes_war
global_war

[simon33-2]

Ok, I've been able to push the changes for 20 repos (lack of disk space is the problem for the other 35). Pushed to a branch called "SBRPropChange" on my fork.

Repos:
big_world_2
diplomacy
camp_david
domination_1914_no_mans_land
hex_globe10
battle_of_jutland
star_wars_galactic_war
elemental_forces
large_middle_earth
world_war2010
blue_vs_gray
feudal_japan_warlords
ur_quan_war_masters_edition
sleeping_giant
pact_of_steel_variations
ultimate_world
total_ancient_war
star_wars_tatooine_war
great_lakes_war
global_war

Pass 2:
1941
big_world_variations
caravan
caribbean_trade_war
civil_war
cold_war-1965
d-day
d-day2
domination_1914_blood_and_steel
eastern_front
europe
pacific_challenge
red_sun_over_china
small_balanced_2_or_4_player
steampunk
stellar_forces
tiberian_conquest_au
tiberian_conquest_eu
tiberian_conquest_sa
total_world_war
twilight_imperium
ultimate_world_variants
war_of_the_relics
world_war_ii_pacific
world_war_ii_revised_variations
zombieland

[simon33-2]

Added:
age_of_tribes
world_at_war_variants

I think that is all.

[veqryn]

@simon33-2
No offense meant, as I have been out of the loop for a long time now, but changing the maps to (maybe) fix a bug that was recently introduced into the engine is wrong.
The maps are fine, fix the bug in the engine instead.

[simon33-2]

Why is it a bug though? There should just be one property rather than two.

[veqryn]

If the maps were working in 1809 and the xml's were not changed, and the only thing that changed was the engine, then the bug must be in the engine.
Adding properties built for global into v3 will have unintended consequences.

[simon33-2]

Unless the maps were taking advantage of a bug in the engine.

Can't see a reason why there should be two properties to do the same thing.

[veqryn]

EDIT CORRECTION: the properties are equivalent and switching them will not fix the bug

You need to look into the code in the engine behind all these properties far more before switching them.

I'm personally rather curious how we managed to break one of the 3 most popular maps and not notice it, or even have a fix out yet...

@ron-murhammer @DanVanAtta: can you give an update on this?

[simon33-2]

I'm not particularly familiar with v3 but in Classic the damage is done to the PUs, and in Global it is done to the factory. Testing reveals no functional difference between v3 and Global in 1.809.

What should it be doing?

[veqryn]

EDIT CORRECTION: the properties are equivalent and switching them will not fix the bug

[simon33-2]

It was proposed to remove the options from the maps but it wasn't accepted. This caused the problems. What I have done is finish the change, as far as I can see. I've come in at the end of this process and don't particularly appreciate the suggestion that some different, but not explained, change should be made for reasons that also aren't explained.

There should be one attribute to achieve one goal, agreed? It's not clear to me why 1.809 did it differently.

[DanVanAtta]

@veqryn There were a set of XML updates for 1.9. We wanted to get them in since it was a good chance for backward incompatible changes to be fixed/introduced while everyone was re-downloading maps anyways. The 1.9 launch dragged out, community QA was enthusiastic to start but then dropped off, and then 1.9 was released.. Long story short there was a small mess at the end that was in large put due to trying to get so much legacy systems updated and having had so much time elapse since the last release.

We're fixing that by releasing after each update and making sure we keep engine compatibility, and also by doing a much more thorough job on each iteration to make sure that we keep from breaking items. At this point we do need to get over the mass breakage we had.

(side note: @veqryn - the reason for the delay on fix was bad timing for 1.9. I was busy the month after it was released, and there was not much bandwidth to fix resulting problems. We also suffered from no clear bug reports on this issue.)

@veqryn, @panther2 , @simon33-2 - we need now a clear summary of which maps are still broken.

[veqryn]

Took a look at the code, and the properties have not changed since 1809:

From properties.java:

  public static boolean getDamageFromBombingDoneToUnitsInsteadOfTerritories(final GameData data) {
    return data.getProperties().get(DAMAGE_FROM_BOMBING_DONE_TO_UNITS_INSTEAD_OF_TERRITORIES,
        data.getProperties().get(SBR_AFFECTS_UNIT_PRODUCTION, false));
  }

The properties are currently equivalent and switching them will not fix the bug.

The bug exists elsewhere, and updating the maps will only force everyone to redownload them, which is pointless and a major hassle for most users.

@simon33-2: This logic should have been clear:
engine 1809: the v3-type maps worked
engine 1900: the v3-type maps did not change (or at least, nothing to do with this), but no longer worked
conclusion: a bug must have been introduced into the engine

[simon33-2]

With all due respect @veqryn, your logic is overly simplistic and what are you doing to fix the issue besides sniping from the sidelines?

[veqryn]

World War II v3 has been one of the top 3 maps for the last 5-7 years, if not the very top one.

Changing game properties in an xml is a pretty big deal, as they directly alter the rules of the map, and are usually very interconnected with other parts of the xml such as unit and territory attachments, among many other things. It generally should not be done, especially on the main/popular maps, except after looking into other options or reasons why a bug was introduced, and even then should be done only by someone familiar with the code behind those properties and who has looked into what unintended side-effects might occur.

That is why I am "sniping from the sidelines", which I do believe I have earned the right to do, on rare occasion.

[DanVanAtta]

In short, this got messed up in the 1.9 release. We did some relatively late mass map changes to take advantage of the opportunity, there were a bunch of renames, and without fail there are some issues. At this point, unless we are seeking to fix a problem, there is a pretty high QA bar before we do any more mass map changes.

Regardless, at this point we need a clean summary of what is working, and what is not. AFAIK the latest Big worlds and Global should all have the correct rules.

[DanVanAtta]

Double checking some maps, things are looking correct:

big world 1942 -> damage to production
BW 1942v3 -> damage to units
1941v3 -> damage to units
WWII v4 -> damage to units
PoS -> damage to production
WWII Europe -> damage ot units

[veqryn]

Looking through the code, it looks like damage to territories and damage to units were combined several years ago, which leaves only 2 forms of damage (from sbr/rockets):

• Damage to PU's (ie: v1 and v2)
• Damage to Units (ie: v3 through global)

(If want to know which maps follow which, all the A&A map's rulebooks can be found here, which I don't think we've transferred to github yet: https://sourceforge.net/projects/tripleamaps/files/rules/ )

There is some fun, slightly convoluted logic to ensure that the defaults for damage to units follows v3's standard rules (ie: factories produce up to the territory production limit, and can be damaged up to 2x the territory production limit, etc).

It does Not look like any of the "renaming" PR's caused the bug (ie: the PR's that changed things inside xml's from territoryAttatchment to territoryAttachment, or from isTwoHit to hitpoints=2, etc).

The issue is probably some other PR.

[Cernelius]

BTW, I'm pretty sure cernel was arguing the exact opposite, unless I completely misunderstand him.

I'm not evil, just misunderstood...

What I was saying is that I would not have changed any maps, to start with, before knowing what the problem is (I can't, cause no developer), but, since a bunch of maps were changed already, I suggested the developers to possibly going ahead with this as a mass change, removing the duplicate, as I suggested to do before the 1.9, since everyone had to redownload all anyway.

Thanks @veqryn for having confirmed that now you remember that SBR_AFFECTS_UNIT_PRODUCTION does nothing else but setting DAMAGE_FROM_BOMBING_DONE_TO_UNITS_INSTEAD_OF_TERRITORIES, thus there can be no unintended consequences or any difference at all in having one or the other. I'm also relieved it does not look like any of the "renaming" PR's caused the bug.
This is actually one change that got refused, meaning it was agreed but there were problems, so it was not made. Ironically, as I said, if these mass changes would have been made before 1.9 (which they didn't), we would not be having this discussion, as everyone would have downloaded the maps with the working property, since the start of the 1.9 era (everyone had to redownload all maps anyway).

So, only partially involved in the matter, since I'm no developer:

A) If the developers prefer to go the way of getting rid of SBR_AFFECTS_UNIT_PRODUCTION from the engine, the:
https://github.com/triplea-game/triplea/wiki/Upgrade-Maps-Information
should be updated (I can do it, if a lead developer confirms; just waiting).

B) If the developers prefer to restore the functionality of SBR_AFFECTS_UNIT_PRODUCTION, I guess you have to decide if to revert all changes done so far or not? Also, please clarify if SBR_AFFECTS_UNIT_PRODUCTION is to be considered a deprecated property, if put back working.

p.s.: at @simon33-2 I'm assuming that Veqryn was being a bit vague because he is not sure on the matter and if he remembers all the story, not because he wants to keep everyone in dread suspense on the unintended consequences.

p.p.s.: Again, I'm no developer; so I don't really know what I'm talking about, and didn't actually want to join this discussion, since anyway I can't look at the engine, that it is what it is needed here (waited 1 month).

p.p.p.s.: I'm Cernel.

[simon33-2]

Sorry for the lack caps re:Cernel.

This is yet another example where timidity about making a change has caused problems. It's an old story in software development.

Anyway, I believe I've pushed all the needed changes and if they are merged I'll check again to make sure nothing has been missed.

[Cernelius]

LOL I meant Cernelius = Cernel, in case @veqryn was wondering.

First time I see him since I'm here.

Well, even if @veqryn would not have had any of them, I'm anyway positive with all the changes at
https://github.com/triplea-game/triplea/wiki/Upgrade-Maps-Information
as they may have caused a few problems (and not this one we are disscussing), but a bunch of rubbish is gone for good and, from now on, the mapmakers will have to waste a bit less time to know stuff or understand what is meaning turn and what is meaning round, etc..

(but I suggested them only because of the 1.9 folder change, making everyone redownloading all anyway, and actually the idea was not originally mine, but started from someone else in the forum, and was agreed, but was not tracked, at the end)

And I'm still bugged about the wrong Classic default for naval bombardment.

[simon33-2]

I'm not across what is wrong with naval bombardment in classic and can't it be fixed at the map level?

[Cernelius]

That was just an off topic, by bad. Shouldn't have mentioned it, since anyway this discussion is already heading for the 100 posts for sort of just saying that 1 property doing nothing else but setting another 1 stopped working, at the end.

Better that I just exit this discussion, since anyway it's only up to the developers. Just let me know if to update the Upgrade Information, in case.

[simon33-2]

Is there an issue for it?

[Cernelius]

Since there is talking about bumping up to 1.9.0.1, I don't know if that means before finishing up the milestone, but I suggest sorting this out before doing that (this is a redundant advice, since this is already a 1.9.0.1 milestone, but saying, just in case).
The upgrade maps information will need updating anyway, one way or the other (even if the functionality is restored, the version before 1.9.0.1 have been so bugged that it is better not referring to those), and I think it is better referring straight to 1.9.0.1, rather than maybe some development sub-version after that.
Just a side suggestion to keep the info logical; not very important (alternatively, I would then suggest bumping to 1.9.0.2 after this is solved, and referring to that).

@simon33-2 : I think, for that, the big chance was at the first 1.9.0.0 release, which was missed, sadly (no developers agreed, wanted or volunteered for doing it), since the map folder was new. With a normal release, addressing that rules bug would be messy, but maybe I will open an issue about that after 1.9.0.1, just to track it for the future (it is the only 1 case of a property that it is not default at v1 rules).

[Cernelius]

The main problem with having multiple redundant properties doing the same thing, and other rubbish like that (kept for backward compatibility), is that it is more frequent than you might think that practically about everyone but the most experienced mapmaker end up overlooking the matter and setting both in the xml, and sometimes even one opposite to the other, and you have to wonder what this will cause and what the mapmaker actually intended, if anything.

For example, surtur is quite the experienced mapmaker, yet in the map he posted you can see:
http://tripleadev.1671093.n2.nabble.com/Domination-1914-Weltpolitik-tp7595270.html;cid=1486520643202-38

                <property name="SBR Affects Unit Production" value="false" editable="false">
                        <boolean/>
                </property>

                <property name="Damage From Bombing Done To Units Instead Of Territories" value="true" editable="false">
                        <boolean/>
                </property>

Similarly, if you have only 1 property not working by v1 rules, you are almost assured that about everyone but the most experienced mapmakers will get it wrong (missing to specifically set it properly), if they want their games working by v1 rules.

[simon33-2]

@DanVanAtta 3 months later we're still waiting for all these repos to be merged:
1941
battle_of_jutland
big_world_2
big_world_variations
camp_david
caravan
civil_war
d-day2
diplomacy
domination_1914_blood_and_steel
domination_1914_no_mans_land
elemental_forces
feudal_japan_warlords
global_war
great_lakes_war
hex_globe10
invasion_usa
large_middle_earth
pacific_challenge
pact_of_steel_variations
red_sun_over_china
sleeping_giant
small_balanced_2_or_4_player
star_wars_galactic_war
star_wars_tatooine_war
stellar_forces
the_pact_of_steel
tiberian_conquest_au
tiberian_conquest_eu
tiberian_conquest_sa
total_ancient_war
total_world_war
twilight_imperium
ultimate_world
ur_quan_war_masters_edition
war_of_the_relics
world_at_war_variants
zombieland
world_war_ii_revised_variations

I also found another xml in v4 so I submitted a PR for that.

[ron-murhammer]

@simon33-2 @DanVanAtta Has this been resolved in all map repos?

[simon33-2]

No. Still waiting for the above changes to be merged in.

[ron-murhammer]

@simon33-2 I merged the one in v4 repo. I don't see PRs in the other repos listed.

[simon33-2]

Dan was going to merge them straight from simon33-2:SBRPropChange.

Wouldn't that be easier than 30+ merges through the web interface?

[ron-murhammer]

@simon33-2 Oh ok. Didn't realize that's where the changes were and yeah 30 PRs through github would be pretty painful.

@DanVanAtta Can you take a look at this when you have some time?

[simon33-2]

Two PRs on repos I can't update:
https://github.com/triplea-maps/iron_war/pull/1
https://github.com/triplea-maps/domination_1914-weltpolitik/pull/11

Then this will be done!

[DanVanAtta]

Excellent - Those two are merged.
@simon33-2 I added you to the map admin team. You would be able to push any outstanding fixes directly now. (As always, be careful about it, use PRs when you can, test thoroughly, check compatibility etc... - thank you for all the work you've put in!)

[simon33-2]

@panther2 I believe this one can be closed. When you get a chance, can you verify and close it?

[panther2]

Thanks for mentioning and all your efforts, @simon33-2 !