Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configurable number of islands for water & mixed maps #1369

Merged

Conversation

vader1986
Copy link
Contributor

@vader1986 vader1986 commented Feb 11, 2021

First, I fixed the size of players' home islands to a value depending on the map size (200-1200 nodes). Also, I added configurable "extra" islands (few/medium/many) of random size (range between 200 & 1200 nodes). This makes water & mixed maps slightly more interesting to play I think.

At the moment, I did not bind the central island to a specific size. I liked the idea of having a big island where multiple players can meet eventually. But up to discussion, could also make it optional/configurable as well.

Also, I have to increase the test coverage, I'll look at that this weekend.

@Flamefire
Copy link
Member

Compilation fails because of missing output operators (I.e. operator<<). You may include GameTypesOutput.h for most of the types and/or use code like:

// LCOV_EXCL_START
static std::ostream& operator<<(std::ostream& out, const YOURENUM e)
{
    return out << static_cast<unsigned>(e);
}
// LCOV_EXCL_STOP

or BOOST_TEST_DONT_PRINT_LOG_VALUE(YOURENUM)

@@ -72,7 +72,7 @@ BOOST_AUTO_TEST_CASE(GenerateRandomMap_returns_valid_water_map)
loadGameData(worldDesc);
MapSettings settings;

settings.size = getRandomMapSize(83, 100); // Need enough space for player islands
settings.size = getRandomMapSize(84, 100); // Need enough space for player islands
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems this is still not enough. Have you tried it locally with the minimum size and MAX_PLAYERS? Maybe the change here makes the player islands to small? Because previously this test worked with those sizes, didn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that fixed minimum island size of 200 might be too small. I'll try playing around locally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or 200 is too large which would lead to a similar result (e.g. 6 players get a large island and there's not enough space for the remaining player).

Copy link
Member

@Flamefire Flamefire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far. A few minor notes to polish it up. Last ones here, I promise ;)

const auto islandRadius = GetIslandRadius(map_.size);
const auto islandAmount = static_cast<double>(settings_.islands) / 100;
auto islandNodes = static_cast<unsigned>(islandAmount * waterNodes);
auto islandSize = rnd_.RandomValue(200u, GetIslandSize(map_.size));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to make the 200 a named constant. I was wondering if this part is correct and GetIslandSize always returns a value >= 200. It does, so its not a bug, but it isn't obvious and I'm not sure what happens if that ever was changed and you call e.g. RandomValue(200, 100)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might also make sense to allow smaller islands than 200 nodes. At least for the last or only island. E.g. use a min size of something like min(200u, islandNodes). Could at least be decorative ;)

libs/s25main/mapGenerator/RandomMap.cpp Outdated Show resolved Hide resolved
libs/s25main/mapGenerator/RandomMap.cpp Outdated Show resolved Hide resolved
tests/s25Main/UI/testIngameWindow.cpp Outdated Show resolved Hide resolved
Timm Hoffmeister added 17 commits February 20, 2021 18:05
First, I fixed the size of players' home islands to a value depending on the map size (200-1200 nodes). Also, I added configurable "extra" islands (few/medium/many) of random size (range between 200 & 1200 nodes). This makes water & mixed maps slightly more interesting to play I think.
Increase island size range for testing water maps

Fixed smoothing iterations for 512x512 map size

island-test-map-size++ :-/

Fixed warning
* avoid duplication of enum in iwMapGenerator
* convert combined sizes into product of width & height
* added kind of stupid integration test for islands amount settings
Copy link
Member

@Flamefire Flamefire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@Flamefire Flamefire merged commit 442b5cd into Return-To-The-Roots:master Feb 21, 2021
@vader1986 vader1986 deleted the feature/configurable-islands branch July 20, 2022 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants