Merge lp:~kxq/widelands/bug-1348800-forester_adds_weight into lp:widelands

[Teppo Mäenpää]

Forester now favours the most suitable tree(s).

[lp:~kxq/widelands/bug-1348800-forester_adds_weight]

7348. By Teppo Mäenpää on 2015-01-12

Seemingly, I messed something up just before previous commit. Now this might even compile.

[SirVer (sirver)]

some nits and some need for explanation.

review: Needs Information

[lp:~kxq/widelands/bug-1348800-forester_adds_weight]

7349. By Teppo Mäenpää on 2015-01-14

nits..

[Teppo Mäenpää (kxq)]

code comments seem to have disappeared.

I put the additional addition to "choice", to ensure that it is always positive and therefore state.ivar2 is always assigned.

[lp:~kxq/widelands/bug-1348800-forester_adds_weight]

7350. By Teppo Mäenpää on 2015-01-14

Removed the epsilon.

[SirVer (sirver)]

> code comments seem to have disappeared.

you have to select the diff you want to look at below. Unfortunately, the diff comments are bound to the diff and not listed again. I added one more to the old diff.

review: Needs Fixing

[Teppo Mäenpää (kxq)]

Would you mind writing it here? For some reason, I cannot see any new comments.

Maybe Launchpad is still updating itself.

[lp:~kxq/widelands/bug-1348800-forester_adds_weight]

7351. By Teppo Mäenpää on 2015-01-14

Even smaller tuning.

[Teppo Mäenpää (kxq)]

>you have to select the diff you want to look at below.
...
I added one more to the old diff.

Are you sure you clicked the "save comment" button? I still cannot find the new comment; guessing that it should have propagated through launchpad by now.

[SirVer (sirver)]

Sure.

> for (auto bsii
use const auto& to avoid reconstructing the tuple. for both loops. Please also add {} for the second for.

> Adding epsilon
I was not concerned about performance - this is not hot inner loop code anyways. I just didn't get it. In fact, I still do not get it - espilon() is 1e-18 or so, why does adding it turn choice into a positive number?

logic_rand_as_double always returns a positive number so it is probably not needed. I just want to understand it.

[SirVer (sirver)]

In fact I did not save the comment before ... sorry :/

[lp:~kxq/widelands/bug-1348800-forester_adds_weight]

7352. By Teppo Mäenpää on 2015-01-15

A performance improvement of dozens of percents (in a minuscule loop).

[Teppo Mäenpää (kxq)]

Now I have solved all the issues I am aware of.

[Teppo Mäenpää (kxq)]

Ready for a re-review.

[SirVer (sirver)]

looks good to me. Could you merge?

review: Approve

[lp:~kxq/widelands/bug-1348800-forester_adds_weight]

7353. By Teppo Mäenpää on 2015-01-16

merged trunk

=== modified file 'src/logic/worker.cc'
--- src/logic/worker.cc	2014-12-28 16:45:37 +0000
+++ src/logic/worker.cc	2015-01-16 19:01:32 +0000
@@ -843,8 +843,27 @@
 	}
 
 	// Randomly pick one of the immovables to be planted.
-	const uint32_t idx = game.logic_rand() % best_suited_immovables_index.size();
-	state.ivar2 = std::get<1>(*std::next(best_suited_immovables_index.begin(), idx));
+
+	// Each candidate is weighted by its probability to grow.
+	double total_weight = 0.0;
+	for (const auto& bsii : best_suited_immovables_index)
+	{
+		double weight = std::get<0>(bsii);
+		total_weight += weight * weight;
+	}
+
+	double choice = logic_rand_as_double(&game) * total_weight;
+
+	for (const auto& bsii : best_suited_immovables_index)
+	{
+		double weight = std::get<0>(bsii);
+		state.ivar2 = std::get<1>(bsii);
+		choice -= weight * weight;
+		if (0 > choice)
+		{
+			break;
+		}
+	}
 
 	Immovable& newimm =
 	   game.create_immovable(pos, state.ivar2, state.svar1 == "tribe" ? &descr().tribe() : nullptr);