Merge lp:~widelands-dev/widelands/bug543113 into lp:widelands

[Jens Beyer]

Implemented variant 2 of the bug description

percentage of total land mass (then, only land fields should be counted)

It uses the same counting method as the Territorial Lord win condition (which is the most useful statistic, I think).

[Shevonar (shevonar)]

Instead of your nested if and else statement you can use just two if statement.

if ((fc.field->nodecaps() & ~MOVECAPS_SWIM) and ((fc.field->nodecaps() & MOVECAPS_WALK) or fc.field->get_immovable())) {
    m_land_size_total++;
    if (Player_Number const owner = fc.field->get_owned_by())
        ++land_size[owner - 1];
}

Is it even necessary to check if the field is not "swimmable"? Can a field be walkable and swimmable (I don't think so) or swimmable and have and immovable (I am not sure about that)?

review: Needs Fixing

[Shevonar (shevonar)]

I just noticed that I missplaced the ~. It must be ~(fc.field->nodecaps() & MOVECAPS_SWIM) of course. Sorry!

[Jens Beyer (qcumber-some)]

ok i can change this tonight, but i left the design of the whole conditions construct to the designer of the territorial lord win condition. if your arguments are valid (and I don't know enough of this part of the game to say they aren't), the win condition should be fixed too. maybe someone else can enlighten us here.

[Nasenbaer (nasenbaer)]

as far as i can remember, the check in territorial lord was created that way, that only "usable" land was counted, so in other words: lava, swamp, ice floats and of course water should be excluded. But it is of course possible, that there is a bug in that win condition as well. If yes, feel free to fix it, or even better change it to use the new statistics once they are implemented :).

[Jens Beyer (qcumber-some)]

As far as I could find out, a field is indeed either walkable or swimmable, neither both.
Immovables can only be placed on walkable fields.
So it seems to be enough to check either is walkable or has an immovable.

[Frank Pieper (frank-pieper-1)]

> Instead of your nested if and else statement you can use just two if
> statement.
>
> if ((fc.field->nodecaps() & ~MOVECAPS_SWIM) and ((fc.field->nodecaps() &
> MOVECAPS_WALK) or fc.field->get_immovable())) {
> m_land_size_total++;
> if (Player_Number const owner = fc.field->get_owned_by())
> ++land_size[owner - 1];
> }
>
> Is it even necessary to check if the field is not "swimmable"? Can a field be
> walkable and swimmable (I don't think so) or swimmable and have and immovable
> (I am not sure about that)?

ShallowWater could be passable for Ships and LandUnits.

[SirVer (sirver)]

What is the state of this? It still seems desirable to get this in and have the win conditions use this statistics.

[Jens Beyer (qcumber-some)]

Unchanged. Somehow I lost track of this in a heavy-work phase ;-)

Problem is still the "Needs fixing" topic.
I could merge the two if statements to one, no problem.

But I can not decide on the design of the win condition. All I did was, implement what I found in the win condition.

Someone has to decide if the win condition should be reworked (and if it should, then this statistics should be updated too). Maybe it is a topic for a different bug entry? Then we could merge this one (it still merges without errors.. ;-) )

[Jens Beyer (qcumber-some)]

I moved the branch to ~widelands-dev

[cghislai (charlyghislain)]

immovables can be placed on swimmable tiles
the portdock is an example (maybe the only one?)

[SirVer (sirver)]

I think it is fine not to count non walkable fields - my issue is more that we really should not reimplement this logic in lua as well, instead make this available to the win condition(s) via the statistics somehow. I do not remember if there is already some logic that can access the statistics from Lua.

[SirVer (sirver)]

> Is it even necessary to check if the field is not "swimmable"?
I think it is a good idea to check - for being defensive of future changes.

> Can a field be walkable and swimmable (I don't think so) or swimmable and have and immovable
> (I am not sure about that)?
I am not sure about the portdocks these days. I think the check doesn't hurt.

[SirVer (sirver)]

I set this to rejected for now. It has been sitting idle for a while and it confuses me in the review queue. If anybody picks up on this again, feel free to set to in-progress or ready for review.

[GunChleoc (gunchleoc)]

Since the bug has been marked as "Won't Fix", would everybody be OK with my setting the branch to "Abandoned"?

=== modified file 'src/logic/game.cc'
--- src/logic/game.cc	2012-09-21 21:36:07 +0000
+++ src/logic/game.cc	2012-10-04 22:23:25 +0000
@@ -905,9 +905,25 @@
 	//  We walk the map, to gain all needed information.
 	Map const &  themap = map();
 	Extent const extent = themap.extent();
+	m_land_size_total = 0;
 	iterate_Map_FCoords(themap, extent, fc) {
-		if (Player_Number const owner = fc.field->get_owned_by())
-			++land_size[owner - 1];
+		// use the same method of counting fields as in Territorial Lord win condition
+		// a countable field is not swimmable; but walkable or has an immovable
+		// a countable field belongs to a player if it is owned by the player
+		bool count = false;
+		if (!(fc.field->nodecaps() & MOVECAPS_SWIM)) {
+			if (fc.field->nodecaps() & MOVECAPS_WALK) {
+				count = true;
+			}
+			else if (fc.field->get_immovable()) {
+				count = true;
+			}
+		}
+		if (count) {
+			m_land_size_total++;
+			if (Player_Number const owner = fc.field->get_owned_by())
+			      ++land_size[owner - 1];
+		}
 
 			// Get the immovable
 		if (upcast(Building, building, fc.field->get_immovable()))
@@ -986,7 +1002,7 @@
 	// Now, push this on the general statistics
 	m_general_stats.resize(map().get_nrplayers());
 	for (uint32_t i = 0; i < map().get_nrplayers(); ++i) {
-		m_general_stats[i].land_size       .push_back(land_size       [i]);
+		m_general_stats[i].land_size       .push_back(100 * land_size       [i] / m_land_size_total);
 		m_general_stats[i].nr_buildings    .push_back(nr_buildings    [i]);
 		m_general_stats[i].nr_casualties   .push_back(nr_casualties   [i]);
 		m_general_stats[i].nr_kills        .push_back(nr_kills        [i]);

=== modified file 'src/logic/game.h'
--- src/logic/game.h	2012-02-15 21:25:34 +0000
+++ src/logic/game.h	2012-10-04 22:23:25 +0000
@@ -245,6 +245,9 @@
 
 	/// For save games and statistics generation
 	std::string          m_win_condition_string;
+
+	// used in statistics building
+	uint32_t m_land_size_total;
 };
 
 inline Coords Game::random_location(Coords location, uint8_t radius) {