Fix bug #968 - Roads are no longer allowed to be in no-mans land or i…

[Xellzul]

Fix bug #968 - Roads are no longer allowed to be in no-mans land or in border

The code is ugly so if you have any suggestions, feel free to fix it.

This should fix all problems, where roads were in places, where they shouldn't be.

If you need, I can provide few examples (savegames)

[Flamefire]

Very good actually, thanks!
For completeness maybe a test for the new function (at https://github.com/Return-To-The-Roots/s25client/blob/master/tests/s25Main/simple/testMapBase.cpp) would be good. I guess just 2 hard-coded, neighbouring points, first each one separately then their union without the duplicates, again with a hard-coded list of the expected result, should be easy enough and show what is done.

It also needs recreating the test replays (the test is only run when built in release mode) because the point order is now different. See

s25client/tests/s25Main/autoplay/main.cpp

Lines 105 to 123 in fd16215

	BOOST_AUTO_TEST_CASE(Play200kReplay)
	{
	// Map: Big Slaughter v2
	// 7 x Hard KI
	// 2 KIs each in Teams 1-3, 1 in Team 4
	// 200k GFs run (+ a bit)
	const boost::filesystem::path replayPath = rttr::test::rttrBaseDir / "tests" / "testData" / "200kGFs.rpl";
	playReplay(replayPath);
	}
	BOOST_AUTO_TEST_CASE(PlaySeaReplay)
	{
	// Map: Island by Island
	// 2 x Hard KI + Player KI
	// No teams, Sea attacks enabled, ships fast
	// 300k GFs run (+ a bit)
	const boost::filesystem::path replayPath = rttr::test::rttrBaseDir / "tests" / "testData" / "SeaMap300kGfs.rpl";
	playReplay(replayPath);
	}

Basically: Start the game normally with the described configuration, fast forward/skip to 200/300k GF and then copy the created replays into the test folder: https://github.com/Return-To-The-Roots/s25client/tree/fd16215cfb8b6b3cb35fb002578358d942600757/tests/testData

[Xellzul]

@Flamefire Everything except the test should be addressed.

[Flamefire]

@Flamefire Everything except the test should be addressed.

I see perfect thanks! Are you going to fix the test too or do you need help with that?

[Xellzul]

@Flamefire Everything except the test should be addressed.

I see perfect thanks! Are you going to fix the test too or do you need help with that?

i will try to add the test on my own later today or tomorow

[Flamefire]

Well done test, only a few minor improvements!

Check them and note that you can apply each suggestion from this page on Github directly without having to apply them locally.

[Flamefire]

What do "components" and "vertices" mean here? I see 3 points.

[Xellzul]

Well, best explained here:
https://en.wikipedia.org/wiki/Component_(graph_theory)
https://en.wikipedia.org/wiki/Vertex_(graph_theory)

vertex is basically point
components are all connected points - two road networks in S2 are two components

can rewrite the comment to something else.

Maybe: Two sets of points

[Flamefire]

I meant what you refer with "vertex" here. Because the function takes as inputs a list of points. For the purpose of this function the points are not connected, i.e. there is no vertex between (10,10) and (10,11) here, they are just neighbours. Of course you could refer to the points as nodes in a graph which basically is the map and vertices would be the relationships, i.e. what makes points neighbours. So there is a vertex between (10,10) and (10,11) but none of either to (1,1) (unless the map is small)

But I wouldn't get to technical in the comment here. So maybe:

Suggested change

	// Two compontents of 1 and 2 vertices
	// 3 points where the last 2 share neighbours and are neighbours to each other

Because that is the important thing for the tested function: We get the neighbours but remove and duplicates and additionally also the points itself, but also avoid duplicates if one is the neighbour of another given point

[Xellzul]

Yea, your comment works better.

[Flamefire]

Suggested change

	std::vector<MapPoint> expectedResultPoints{
	const std::vector<MapPoint> expectedResultPoints{

[Xellzul]

@Flamefire
So I did some more testing and found out one new bug.
Border is considered as player territory..
Should be fixed in last commit

[Xellzul]

Not sure what exactly is the failing test 'ProductivityStats' testing.
However it probably fails, because last created building Harbour is recalculating border and destroying buildings.

Just skipping the creation of harbour will fix the test

[Flamefire]

That would make sense, yes. Maybe we need a comment why this is OK though:

Suggested change

	if(!BuildingProperties::IsValid(bldType) || bldType == BuildingType::HarborBuilding)
	if(!BuildingProperties::IsValid(bldType))
	continue;
	if(bldType == BuildingType::HarborBuilding)
	{
	// Creating a harbor would recalculate the territory and destroy some buildings already added in the calculation, but we don't have productivity for it so no need to test
	BOOST_TEST_REQUIRE(!helpers::contains(iwBuildingProductivities::icons, bldType))
	continue
	}

However I don't feel so good about excluding (real) buildings here as we might miss bugs. Maybe a better solution would be to convert helpers::EnumRange<BuildingType> into a vector and sort such that all "usual" buildings come last, i.e. BuildingProperties::IsUsual

This way territory changes only happen at the start

[Flamefire]

Even better (and faster):

Suggested change

	if(flag->GetPlayer() + 1 == owner && !IsBorderNode(curMapPt, owner))
	if(flag->GetPlayer() + 1 == owner && IsPlayerTerritory(curMapPt, owner))

IsBorderNode = GetNode(pt).owner == owner && !IsPlayerTerritory(pt, owner))
--> !IsBorderNode = GetNode(pt).owner != owner || IsPlayerTerritory(pt, owner)) and the first part can never be true here

[Flamefire]

We use C++11/14 so you never need this for(it=..., ++it) but can always use the range-based-for: for(auto t: buildingTypesEnum)

However here you can just use the range constructor:

Suggested change

	std::vector<BuildingType> buildingTypes;
	const auto buildingTypesEnum = helpers::EnumRange<BuildingType>{};
	for(auto it = buildingTypesEnum.begin(); it != buildingTypesEnum.end(); ++it)
	{
	buildingTypes.push_back(*it);
	}
	const auto buildingTypesEnum = helpers::EnumRange<BuildingType>{};
	std::vector<BuildingType> buildingTypes(buildingTypesEnum.begin(), buildingTypesEnum.end());

[Xellzul]

I looked into this, and didnt manage to make it work.

believe you have to implement https://en.cppreference.com/w/cpp/iterator/iterator_tags

E0289 no instance of constructor "std::vector<_Ty, _Alloc>::vector [with _Ty=BuildingType, _Alloc=std::allocator<BuildingType>]" matches the argument list Test_integration \s25client\tests\s25Main\integration\testGamePlayer.cpp 73

[Flamefire]

True. I have a PR for this #1665