[Widelands-dev] [Merge] lp:~widelands-dev/widelands/lua_AI_fixes into lp:widelands
Continuous integration builds have changed state: Travis build 4255. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/457654319. Appveyor build 4049. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_lua_AI_fixes-4049. -- https://code.launchpad.net/~widelands-dev/widelands/lua_AI_fixes/+merge/359082 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/lua_AI_fixes into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/terrain_affinity_as_int into lp:widelands
Review: Needs Fixing
I reviewed the code and also tested somewhat. Still has a few issues, most of
them minor though. See diff comments.
The file data\world\immovables\bush1\init.lua still has a few comments (lines
86-94) referring to the floating point values, this should also be changed in
this branch.
Overall it should be much less likely now to get different results based on
platform/compiler dependent precision choices, but they might still happen.
There are still things based on floating point calculations (like the main
probability calculation in calculate_probability_to_grow) but they might be
difficult to replace with pure integer based stuff.
That said, considering that the ratios of the final probabilities of the six
best immovables already change significantly with this branch, we could
possibly also change the basic probability calculation to somewhat that uses
only integer operations but is not too far off from the current approach (which
seems to have been based roughly on multivariate normal distributions).
Btw, where did those three weight constants in calculate_probability_to_grow
originally come from? Their precision is too high to be from a simple "try and
error until it feels right" approach. They still feel quite arbitrary though.
Diff comments:
>
> === modified file 'src/logic/map_objects/terrain_affinity.cc'
> --- src/logic/map_objects/terrain_affinity.cc 2018-04-07 16:59:00 +
> +++ src/logic/map_objects/terrain_affinity.cc 2018-11-16 06:27:07 +
> @@ -33,85 +33,92 @@
>
> namespace {
>
> +// Literature on cross-platform floating point precision-problems:
> +// https://arxiv.org/abs/cs/0701192
> +// Monniaux, David (2008): "The pitfalls of verifying floating-point
> computations",
> +// in: ACM Transactions on Programming Languages and Systems 30, 3 (2008) 12.
> +//
> +// Recommends using heximal float constants, but we'd need to switch to
> C++17 for that.
> +//
> +//
> https://randomascii.wordpress.com/2012/03/21/intermediate-floating-point-precision/
> +
> constexpr double pow2(const double& a) {
Can you please rename that to "sqr" or some such? ("pow2" usually stands for
computing powers of two. It's really irritating here.) The function is only
used three times in the function below, so a renaming is also quick to apply.
> return a * a;
> }
>
> // Helper function for probability_to_grow
> // Calculates the probability to grow for the given affinity and terrain
> values
> -double calculate_probability_to_grow(const TerrainAffinity& affinity,
> - double terrain_humidity,
> - double terrain_fertility,
> - double terrain_temperature) {
> -
> - constexpr double kHumidityWeight = 0.500086642549548;
> - constexpr double kFertilityWeight = 0.5292268046607387;
> - constexpr double kTemperatureWeight = 61.31300863608306;
> -
> - const double sigma_humidity = (1. - affinity.pickiness());
> - const double sigma_temperature = (1. - affinity.pickiness());
> - const double sigma_fertility = (1. - affinity.pickiness());
> -
> - return exp((-pow2((affinity.preferred_fertility() - terrain_fertility) /
> - (kFertilityWeight * sigma_fertility)) -
> - pow2((affinity.preferred_humidity() - terrain_humidity) /
> - (kHumidityWeight * sigma_humidity)) -
> - pow2((affinity.preferred_temperature() -
> terrain_temperature) /
> - (kTemperatureWeight * sigma_temperature))) /
> -2);
> +inline unsigned int calculate_probability_to_grow(const TerrainAffinity&
> affinity,
> + int terrain_humidity,
> + int terrain_fertility,
> + int terrain_temperature) {
> + constexpr double kHumidityWeight = 5.00086642549548;
> + constexpr double kFertilityWeight = 5.292268046607387;
> + constexpr double kTemperatureWeight = 0.6131300863608306;
> +
> +// Avoid division by 0
> +assert(affinity.pickiness() < 100);
> + const double sigma = std::floor(100.0 - affinity.pickiness());
> +
> + const double result = exp(-
> + (pow2(((affinity.preferred_fertility() -
> terrain_fertility) / kFertilityWeight) / sigma) +
> + (pow2(((affinity.preferred_humidity() -
> terrain_humidity) / kHumidityWeight) / sigma) +
One pair of brackets is obsolete: The opening bracket before "pow2" in the line
above, and the corresponding closing bracket in the line below. (The just group
the humidity and temperature part together in the sum, but the swapped
summation order won't give any benefit here.)
> +pow2(((affinity.preferred_temperature() -
> terrain_temperature) / kTemperatureWeight) / sigma))) / 2.0);
> +
> +return
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1801767-tutorial2-ai into lp:widelands
ok I have added the fixes for this branch to my new branch cause branching takes a lot of time on my old machine (due to a lot of write operations on my hard disk) so this branch is probably superseded -- https://code.launchpad.net/~widelands-dev/widelands/bug-1801767-tutorial2-ai/+merge/358363 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1801767-tutorial2-ai. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/lua_AI_fixes into lp:widelands
hessenfarmer has proposed merging lp:~widelands-dev/widelands/lua_AI_fixes into
lp:widelands.
Commit message:
fixes 2 small bugs in lua related to AI behaviour
bug #1804039
bug #1801767
Requested reviews:
Widelands Developers (widelands-dev)
Related bugs:
Bug #1801767 in widelands: "Tutorial 2 (warfare): AI abandons sentry"
https://bugs.launchpad.net/widelands/+bug/1801767
Bug #1804039 in widelands: "Atlanteans AI deadlock due to no well in basic
economy"
https://bugs.launchpad.net/widelands/+bug/1804039
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/lua_AI_fixes/+merge/359082
assign empty AI to p2 in warfare tutorial
add roads to p2 in warfare tutorial due to no AI can build them.
add one well to the atlantean basic economy as water is needed for crucial
spidercloth production.
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/lua_AI_fixes into lp:widelands.
=== modified file 'data/campaigns/tutorial02_warfare.wmf/player_names'
--- data/campaigns/tutorial02_warfare.wmf/player_names 2014-10-26 09:05:11 +
+++ data/campaigns/tutorial02_warfare.wmf/player_names 2018-11-20 20:35:50 +
@@ -12,5 +12,5 @@
[player_2]
name="Sparring Partner"
tribe="empire"
-ai=
+ai="empty"
closeable="false"
=== modified file 'data/campaigns/tutorial02_warfare.wmf/scripting/mission_thread.lua'
--- data/campaigns/tutorial02_warfare.wmf/scripting/mission_thread.lua 2017-12-19 17:04:30 +
+++ data/campaigns/tutorial02_warfare.wmf/scripting/mission_thread.lua 2018-11-20 20:35:50 +
@@ -78,7 +78,9 @@
end
function create_enemy()
- prefilled_buildings(wl.Game().players[2],
+ local map = wl.Game().map
+ local p2 = wl.Game().players[2]
+ prefilled_buildings(p2,
{"empire_barrier", 24, 7},
{"empire_sentry", 29, 16},
{"empire_tower", 30, 21},
@@ -88,7 +90,10 @@
}
}
)
- wl.Game().players[2]:forbid_buildings("all")
+ connected_road(p2,map:get_field(29,17).immovable,"tr,tl|tl,tl|tl,tl|tl,tl|tl,l")
+ connected_road(p2,map:get_field(31,22).immovable,"tr,tl|tl,tl,tl")
+ connected_road(p2,map:get_field(31,28).immovable,"tr,tr|tr,tl|tl,tl")
+ p2:forbid_buildings("all")
end
function attack()
=== modified file 'data/tribes/buildings/productionsites/atlanteans/well/init.lua'
--- data/tribes/buildings/productionsites/atlanteans/well/init.lua 2018-05-24 10:19:21 +
+++ data/tribes/buildings/productionsites/atlanteans/well/init.lua 2018-11-20 20:35:50 +
@@ -31,6 +31,7 @@
},
aihints = {
+ basic_amount = 1,
collects_ware_from_map = "water"
},
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1801767-tutorial2-ai into lp:widelands
I will have a look tonight to fix this bug and the missing atlantean well in basic economy in one branch as both are somewhat related to AI. Having roads would look nicer and somewhat more normal. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1801767-tutorial2-ai/+merge/358363 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1801767-tutorial2-ai. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/empire04_unused_key_return_on_dismantle into lp:widelands
I got ninja'd yesterday when I reviewed the code. :-) As for the "needs positive return on dismantle" condition, I don't have a strong opinion, but I'd prefer to not have it, even though in most circumstances dismantling without return would be useless. My reasons: 1) The condition - even though it makes sense - doesn't really add anything. The game would work fine without it. And regular buildings (and most special builidings in campaigns) have proper return on dismantle amounts anyway, so I wouldn't expect "false" bug reports about this. 2) Not having the condition allows for a little more flavour in missions. Like in emp04. Or maybe there'll be a mission where destroying would come with a fire hazard to neighboring buildings, possibly destroying them, too. (Not sure this is possibly atm, i.e., whether we can detect the difference between dismantling and destroying via mission scripts. Just a general idea.) 3) If the building is not far from a warehouse with a builder available, dismantling without return is even a little faster than destroying. Difference is minor though. -- https://code.launchpad.net/~widelands-dev/widelands/empire04_unused_key_return_on_dismantle/+merge/358273 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/empire04_unused_key_return_on_dismantle. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1798297-locale-C into lp:widelands
Refusing to merge, since Travis is not green. Use @bunnybot merge force for merging anyways. Travis build 4235. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/455848130. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1798297-locale-C/+merge/358364 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1798297-locale-C into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1798297-locale-C into lp:widelands
I tested this pretty thoroughly on Linux, so it should be fine. Bunnybot will take care of the indentation. Thanks for the review! :) @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/bug-1798297-locale-C/+merge/358364 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1798297-locale-C into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1801767-tutorial2-ai into lp:widelands
Do we care if there are reinforcements coming? The purpose here is that the player learns how to use the Attack button. I would no be against defining some roads though. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1801767-tutorial2-ai/+merge/358363 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1801767-tutorial2-ai. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1801767-tutorial2-ai into lp:widelands
Review: Needs Fixing just had a look on the scripting code of the scenario. I think empty AI would be ok but we need to define some roads to connect the military buildings of p2 in this case. otherwise there will be no reinforcements for the military buildings coming from HQ -- https://code.launchpad.net/~widelands-dev/widelands/bug-1801767-tutorial2-ai/+merge/358363 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1801767-tutorial2-ai. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/empire04_unused_key_return_on_dismantle into lp:widelands
I am ok with that. Unfortunately I overlooked some code issues in the toher branch which Arty found. we should make sure we will fix them with this branch at the latest. Perhaps it will be even recommendable to do it now in a fixing round -- https://code.launchpad.net/~widelands-dev/widelands/empire04_unused_key_return_on_dismantle/+merge/358273 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/empire04_unused_key_return_on_dismantle. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
