[Widelands-dev] [Merge] lp:~widelands-dev/widelands/handling-various-fs-errors into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/handling-various-fs-errors into lp:widelands has been updated. Commit message changed to: Improved error handling for file save dialogs In Load/Save menu for games/replays: - All deletion errors are caught, player gets a message. - When deleting multiplayer replays, also the corresponding syncstream file is deleted. - After deleting replays, the table now respects the Show Filenames setting. In Save menu for maps: - Errors when trying to create a directory are now caught, player gets a message. - Directory creation now doesn't allow names with map extensions, because the game would just trying to interpret them as maps and not show as directories. A tooltip is shown. - Directory creation can now also be triggered by pressing Enter in the name edit box. Other file errors are caught and logged: - syncstream deletion in SyncWrapper destructor - in Map::get_correct_loader - file deletion/renaming in GameClient::handle_packet For more details, see: https://code.launchpad.net/~widelands-dev/widelands/handling-various-fs-errors/+merge/358767 -- Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/handling-various-fs-errors. ___ 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/handling-various-fs-errors into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/handling-various-fs-errors into lp:widelands has been updated. Commit message changed to: Improved error handling for file save dialogs For more details, see: https://code.launchpad.net/~widelands-dev/widelands/handling-various-fs-errors/+merge/358767 -- Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/handling-various-fs-errors. ___ 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/handling-various-fs-errors into lp:widelands
Review: Approve Tested and all good now :) @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/handling-various-fs-errors/+merge/358767 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/handling-various-fs-errors. ___ 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/ferry into lp:widelands
Continuous integration builds have changed state: Travis build 4264. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/458580181. Appveyor build 4058. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_ferry-4058. -- https://code.launchpad.net/~widelands-dev/widelands/ferry/+merge/351880 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/ferry 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/empire04_unused_key_return_on_dismantle into lp:widelands
Review: Approve Perhaps it might be worth having it without the string . (showing nothing beneath the Headings) -- 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/empire04_unused_key_return_on_dismantle into lp:widelands
Tested the branch. Works like it should do. no difference found in empire 04. ALthough the message for no wares returned looks unfamiliar and is to long for my taste. For me we should have the same headings as for wares returned. and a short message like "no wares" instead of the waremap to richtext. resulting in: Dismantle Returns: "no wares" anyhow as this is a matter of taste I will approve this. Question is do we want to have it for b20? Problem is that it will have at least one new string to translate. -- 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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/terrain_affinity_as_int into lp:widelands
Yes, the exponential function is the problem. Approximating it with highish
precision using only integer calculations would require a lot of computation,
which isn't really feasible. There are reasonable options though if we don't
mind straying a little. The function we use here is basically a
three-dimensional Gauß-curve, and I think we could find an alternative that
roughly preserves the shape and can be done with a small number of integer
operations.
It's probably not necessary though, but if we keep getting desyncs because of
such platform/compiler-dependent computation differences, this would provide an
option.
I also replied to your replies.
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) {
> 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) +
The operator "+" is left-to-right associative in C++
(https://en.cppreference.com/w/cpp/language/operator_precedence) so without
those extra brackets the evaluation (inside the "exp(-(...))") would always be
"(fertility_part + humidity_part) + temperature_part".
for every compiler. You are simply forcing it to always be
"fertility_part + (humidity_part + temperature_part)", which is different but
not inherently better.
Of course those two different evaluation orders could have slightly different
results, but the order is specified by the expression, so every compiler would
use the same order anyway.
Operator associativity is defined by the C++ standard, so compiler
optimizations should not be able to change the order, but if they were then the
extra brackets wouldn't stop them either.
Anyway, it doesn't really matter which way, I just think that with such a big
expression we don't need to make it even harder to read by more brackets,
especially not to separate those three semantically similar terms.
> +pow2(((affinity.preferred_temperature() -
> terrain_temperature) / kTemperatureWeight) / sigma))) / 2.0);
> +
> +return static_cast(std::max(0.0, std::floor(result *
> static_cast(TerrainAffinity::kPrecisionFactor;
> }
>
> } //
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/ferry into lp:widelands
Continuous integration builds have changed state: Travis build 4263. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/458376614. Appveyor build 4057. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_ferry-4057. -- https://code.launchpad.net/~widelands-dev/widelands/ferry/+merge/351880 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/ferry 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-website/remove_djangoratings into lp:widelands-website
Cleaning up unused stuff is always a good idea :) -- https://code.launchpad.net/~widelands-dev/widelands-website/remove_djangoratings/+merge/358960 Your team Widelands Developers is subscribed to branch lp:widelands-website. ___ 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
thanks Arty for your comments and solutions. I would be happy to test this
again after the fixes.
Nice spot with the resulting farm1 issue after this finally works.
Diff comments:
>
> === modified file 'data/campaigns/emp04.wmf/scripting/tribes/farm1.lua'
> --- data/campaigns/emp04.wmf/scripting/tribes/farm1.lua 2018-09-09
> 17:58:55 +
> +++ data/campaigns/emp04.wmf/scripting/tribes/farm1.lua 2018-11-21
> 09:48:10 +
> @@ -10,10 +10,6 @@
> enhancement = "empire_farm2",
>
> return_on_dismantle = {
> - planks = 0,
> - granite = 0,
> - marble = 0,
> - marble_column = 0
> },
correct. Thanks Arty. The table has to be removed completely, as the farm
shouldn't be dismantable at all.
>
> animations = {
--
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
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/terrain_affinity_as_int into lp:widelands
Thanks for the excellent review :)
I have addressed most of your comments and left replies to the rest.
One of our math wizards spent a long time tweaking the system and the function,
so I'm hesitant to change any semantics here. There is an e function involved,
so there is no way to avoid floating point completely unless we radically
change the system.
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) {
> 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) +
Unlike real numbers, floating point multiplication/division is neither
associative nor commutative, so I am forcing the same order of calculation here
for all compilers and optimization settings. I am adding a comment to the code
to make sure that nobody will "clean it up."
> +pow2(((affinity.preferred_temperature() -
> terrain_temperature) / kTemperatureWeight) / sigma))) / 2.0);
> +
> +return static_cast(std::max(0.0, std::floor(result *
> static_cast(TerrainAffinity::kPrecisionFactor;
> }
>
> } // namespace
>
> TerrainAffinity::TerrainAffinity(const LuaTable& table, const std::string&
> immovable_name)
> - : preferred_fertility_(table.get_double("preferred_fertility")),
> - preferred_humidity_(table.get_double("preferred_humidity")),
> - preferred_temperature_(table.get_double("preferred_temperature")),
> - pickiness_(table.get_double("pickiness")) {
> - if (!(0 <= preferred_fertility_ && preferred_fertility_ <= 1.)) {
> - throw GameDataError("%s: preferred_fertility is not in [0,
> 1].", immovable_name.c_str());
> - }
> - if (!(0 <= preferred_humidity_ && preferred_humidity_ <= 1.)) {
> - throw GameDataError("%s: preferred_humidity is not in [0, 1].",
> immovable_name.c_str());
> - }
> - if (!(0 <= pickiness_ && pickiness_ <= 1.)) {
> - throw GameDataError("%s: pickiness is not in [0, 1].",
> immovable_name.c_str());
> + : preferred_fertility_(table.get_int("preferred_fertility")),
> + preferred_humidity_(table.get_int("preferred_humidity")),
> +
