widelands-dev team mailing list archive

[cghislai <charlyghislain@xxxxxxxxx>]

cghislai has proposed merging lp:~widelands-dev/widelands/fishing into lp:widelands.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1094711 in widelands: "Fisher runs out of fish even with double breeders"
  https://bugs.launchpad.net/widelands/+bug/1094711
  Bug #1182010 in widelands: "fish breeder does not work"
  https://bugs.launchpad.net/widelands/+bug/1182010
  Bug #1205457 in widelands: "Fisher produces fish without decreasing resources"
  https://bugs.launchpad.net/widelands/+bug/1205457

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/fishing/+merge/177237

There are two changes in the logic
- When looking for a breedable node, accept as well if a direct neighbor is breedable. This allows breeders to breed a bit further than the shore.
- When looking for a node in the breed and fish programs, there are two loops but the mapregion was not reset in between. This lead a fisher walk to a node but fish several nodes further.

I tested the savegames from the bug report and it worked great.

Also, as explained in the FIXME in the code, there are two different logic when looking for a node to breed. In the findspace, we aim for non-empty, non-full nodes; while in the breed function, we only check for non-full ones. A breeder may thus put a new fish on an empty node if one neighbor is not full nor empty.
-- 
https://code.launchpad.net/~widelands-dev/widelands/fishing/+merge/177237
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/fishing into lp:widelands.

=== modified file 'src/logic/findnode.cc'
--- src/logic/findnode.cc	2013-07-26 20:19:36 +0000
+++ src/logic/findnode.cc	2013-07-27 00:19:28 +0000
@@ -119,13 +119,28 @@
 
 
 bool FindNodeResourceBreedable::accept
-	(const Map &, const FCoords & coord) const
+	(const Map & map, const FCoords & coord) const
 {
-	return
-		m_resource == coord.field->get_resources() &&
-		coord.field->get_resources_amount   ()
+	// Accept a tile that is full only if a neighbor
+	// neighbor also matches resource and is not full
+	if (m_resource != coord.field->get_resources()) {
+		return false;
+	}
+	if (coord.field->get_resources_amount   ()
 		<
-		coord.field->get_starting_res_amount();
+		coord.field->get_starting_res_amount()) {
+		return true;
+	}
+	for (Direction dir = FIRST_DIRECTION; dir <= LAST_DIRECTION; ++dir) {
+		FCoords neighb = map.get_neighbour(coord, dir);
+		if (m_resource == neighb.field->get_resources() &&
+			neighb.field->get_resources_amount   ()
+			<
+			neighb.field->get_starting_res_amount()) {
+			return true;
+		}
+	}
+	return false;
 }
 
 bool FindNodeShore::accept(const Map & map, const FCoords & coord) const

=== modified file 'src/logic/worker.cc'
--- src/logic/worker.cc	2013-07-26 20:19:36 +0000
+++ src/logic/worker.cc	2013-07-27 00:19:28 +0000
@@ -147,22 +147,23 @@
 		return true;
 	}
 
-	// Second pass through fields
+	// Second pass through fields - reset mr
 	pick = game.logic_rand() % totalchance;
-
+	mr = MapRegion<Area<FCoords> >
+		(map, Area<FCoords>(map.get_fcoords(get_position()), action.iparam1));
 	do {
 		uint8_t fres  = mr.location().field->get_resources();
+		if (fres != res) {
+			continue;
+		}
+
 		uint32_t amount = mr.location().field->get_resources_amount();
 
-		if (fres != res)
-			amount = 0;
-
 		pick -= 8 * amount;
 		if (pick < 0) {
 			assert(amount > 0);
 
 			--amount;
-
 			mr.location().field->set_resources(res, amount);
 			break;
 		}
@@ -192,6 +193,8 @@
  * \param action Which resource to breed (action.sparam1) and where to put
  * it (in a radius of action.iparam1 around current location)
  *
+ * FIXME: in FindNodeResourceBreedable, the node (or neighbors) is accepted if it is breedable.
+ * In here, breeding may happen on a node emptied of resource.
  * \todo Lots of magic numbers in here
  * \todo Document parameters g and state
  */
@@ -251,19 +254,21 @@
 		return true;
 	}
 
-	// Second pass through fields
+	// Second pass through fields - reset mr!
 	assert(totalchance);
 	pick = game.logic_rand() % totalchance;
+	mr = MapRegion<Area<FCoords> >
+		(map, Area<FCoords>(map.get_fcoords(get_position()), action.iparam1));
 
 	do {
 		uint8_t fres  = mr.location().field->get_resources();
+		if (fres != res)
+			continue;
+
 		uint32_t amount =
 			mr.location().field->get_starting_res_amount() -
 			mr.location().field->get_resources_amount   ();
 
-		if (fres != res)
-			amount = 0;
-
 		pick -= 8 * amount;
 		if (pick < 0) {
 			assert(amount > 0);