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

Feature : New Trees Placement Algorithm "Forest". #6848

Closed
wants to merge 9 commits into from

Conversation

@SylvainDevidal
Copy link

SylvainDevidal commented Jul 4, 2018

It creates new trees only in existing forest, preserving meadows.

@SylvainDevidal SylvainDevidal changed the title New Trees Placement Algorithm "Forest". Feature : New Trees Placement Algorithm "Forest". Jul 4, 2018
@LordAro

This comment has been minimized.

Copy link
Member

LordAro commented Jul 4, 2018

Without looking at the actual code, a couple of things are immediately obvious:

See CONTRIBUTING.md for more details

@LordAro

This comment has been minimized.

Copy link
Member

LordAro commented Jul 4, 2018

Thanks very much! I'd recommend using rebase to squash everything together - the build system will have a go at building it then, once it's happy with the commit messages!

I'm not sure about that last commit, the one about docker complaining about lack of newline at EoL - the git diff looks much like you've removed them rather than added them. Regardless, try not to touch stuff that's unrelated to the PR

@SylvainDevidal

This comment has been minimized.

Copy link
Author

SylvainDevidal commented Jul 4, 2018

Hello LordAro,
Thank you for you quick comments.
I think I fixed most of the issues:

  • Reverted changes to lang files except "english.txt"
  • Fixed (and simplified) coding style
  • Changed the commit message

But there is still an issue with docker.
It complains about new line at end of files that I have not changed (in fact, I changed them accidentely and reverted them).
I tried to fix the issue, but the message presists:

`[commit-checker] Running shell script

  • docker logs --follow 152ab8ac1e672452fdb2f1e999af1f32b71c52757e2278b776c3adb6076469aa
    Validating commits
    Branch is on top of master
    *** b/projects/generate_vs141.vcxproj: No newline at end of file
    *** b/projects/langs_vs141.vcxproj: No newline at end of file
    *** b/projects/openttd_vs141.vcxproj: No newline at end of file
    *** b/projects/settings_vs141.vcxproj: No newline at end of file
    *** b/projects/settingsgen_vs141.vcxproj: No newline at end of file
    *** b/projects/strgen_vs141.vcxproj: No newline at end of file
  • docker wait 152ab8ac1e672452fdb2f1e999af1f32b71c52757e2278b776c3adb6076469aa
  • exit 1
    script returned exit code 1`
@LordAro

This comment has been minimized.

Copy link
Member

LordAro commented Jul 4, 2018

Weird. Regardless, if you rebase your PR (something like git rebase -i origin/master) and squash all the commits together, that should remove any changes to the project files, meaning that the checker won't complain

@SylvainDevidal

This comment has been minimized.

Copy link
Author

SylvainDevidal commented Jul 4, 2018

Well, I really Don't understand how to do.
When I rebase + squash all my commits, I get a nice log in my local copy with only the desired files updated.
But when I try to push it, it claims I need to pull before, and then I get some spaghetti… And still the error :(

@LordAro

This comment has been minimized.

Copy link
Member

LordAro commented Jul 4, 2018

Yeah, you're rewriting the commit history, so you'll need to "force-push" when pushing to a remote - just add -f to the command

@SylvainDevidal SylvainDevidal force-pushed the SylvainDevidal:TreeSpreading branch from 8bcfa0d to 49a7627 Jul 4, 2018
@SylvainDevidal

This comment has been minimized.

Copy link
Author

SylvainDevidal commented Jul 4, 2018

Oh, ok.
Thank you :)
It looks like to work better now!

Update: Translations from eints
@@ -305,6 +333,7 @@ void GenerateTrees()

switch (_settings_game.game_creation.tree_placer) {
case TP_ORIGINAL: i = _settings_game.game_creation.landscape == LT_ARCTIC ? 15 : 6; break;
case TP_FOREST: /* FALL THROUGH */

This comment has been minimized.

Copy link
@michicc

michicc Jul 7, 2018

Member

We have a proper macro for this now (FALLTHROUGH), to take advantage of some C++17 stuff if supported.

@michicc

This comment has been minimized.

Copy link
Member

michicc commented Jul 7, 2018

In general, you shouldn't have merges in PRs. We will always squash or rebase a PR anyway to keep a linear history.

A merge only complicates reviewing as it can make the changes less clear and harder to check if the resulting rebase/squash will result in a working state. GitHub checks for conflicts, but this is is invariably only a textual check and not a logical one.

@SylvainDevidal SylvainDevidal force-pushed the SylvainDevidal:TreeSpreading branch from 7b1a87c to 9bebe2d Jul 8, 2018
@SylvainDevidal

This comment has been minimized.

Copy link
Author

SylvainDevidal commented Jul 8, 2018

Hello michicc,

I Added the FALLTHROUGH macro instead of the comment.
The "coding style" section should be updated in Wiki then.

@andythenorth

This comment has been minimized.

Copy link
Contributor

andythenorth commented Jul 19, 2018

forest_patch_2

forest_patch_1

@nielsmh

This comment has been minimized.

Copy link
Contributor

nielsmh commented Jul 19, 2018

Those screenshots makes it look a bit strange, forests forming in perfect rectangles. The rest of the land also looks unnecessarily barren.

Can I suggest adding some more fuzziness so forest spread can be more uneven, and also have a minimal chance trees can spring up in the middle of nowhere to jumpstart new forests occasionally?

@andythenorth

This comment has been minimized.

Copy link
Contributor

andythenorth commented Jul 19, 2018

Hi Sylvain!

Thanks for this, trees do need some love.

I'm not a core dev, but I tested your PR, and here are my observations :)

  • as nielsm said, the forests make quite regular diamond shapes, with no trees at all on other tiles, this looks odd in the map / minimap, more randomness would be nice
  • the world-gen menu items need some attention, 'none', 'original', 'improved', 'forest' is now quite confusing, hard for the player to understand which to pick

I hope you can keep contributing and the end result is nicer trees for the game!

@SylvainDevidal

This comment has been minimized.

Copy link
Author

SylvainDevidal commented Jul 20, 2018

Hello andythenorth and nielsmh,

Thank you for your feedback.

I agree "Improved" and "Forest" are quite confusing. So I updated the "Improved" feature instead of creating a new one. From my point of view, the initial "Improved" and "Original" features were not making big difference as the trees were planted on all tiles of the map after a few décades.

So I changed this:

  • Improved now plant fewer trees alone during map generation (16 times less)
  • Improved now doesn't make trees spreading if there are not other near by trees

As a result, you can see now a few trees appearing in meadows, but they won't spread to cover the whole map.

For the diamond aspect of forests, if you look closely to the original "improved" forests, you'll see they were already diamonds. It was less ugly as there were also many random trees everywhere, so the forests limits were not much visible.

The problem is located in the function PlaceTreeGroups that I didn't changed. It chooses a tile, then plant many trees arround, but there is no "shape", so the result produces a square.
I currently don't have idea how to improve it.

@SylvainDevidal

This comment has been minimized.

Copy link
Author

SylvainDevidal commented Jul 20, 2018

I added some randomness to the forest tree placement. It looks like to produce something better.

@SylvainDevidal SylvainDevidal force-pushed the SylvainDevidal:TreeSpreading branch from b71fc99 to a3e8c85 Jul 20, 2018
@@ -2216,7 +2216,7 @@ from = 30
guiflags = SGF_MULTISTRING | SGF_NEWGAME_ONLY | SGF_SCENEDIT_TOO
def = 2
min = 0
max = 2
max = 3

This comment has been minimized.

Copy link
@TrueBrain

TrueBrain Jul 20, 2018

Member

Think this is a remainder of a past version now? :)

Copy link
Member

TrueBrain left a comment

Do you happen to have a screenshot how the latest result looks? :D

@@ -193,11 +220,16 @@ static void PlaceTreeGroups(uint num_groups)
int x = GB(r, 0, 5) - 16;
int y = GB(r, 8, 5) - 16;
uint dist = abs(x) + abs(y);
uint max_dist = (_settings_newgame.game_creation.tree_placer == TP_IMPROVED) ? GB(r, 0, 3) + 8 : MAX_DISTANCE_FROM_CENTER;

if (dist > max_dist)

This comment has been minimized.

Copy link
@TrueBrain

TrueBrain Jul 20, 2018

Member

Please either put this on a single line, or add {} around it. This form only leads to errors over time :)

This comment has been minimized.

Copy link
@SylvainDevidal

SylvainDevidal Jul 20, 2018

Author

I splitted this line, now it's more clear

This comment has been minimized.

Copy link
@michicc

michicc Jul 20, 2018

Member

I think TrueBrain meant the coding style for the if. Either make it a single line without any linebreaks, or use {}.

@SylvainDevidal SylvainDevidal force-pushed the SylvainDevidal:TreeSpreading branch from a3e8c85 to 4cd943f Jul 20, 2018
Codechange: Added some alone trees during generation.
Codechange: Added some randomness to the tree placement distance around forests.
@SylvainDevidal SylvainDevidal force-pushed the SylvainDevidal:TreeSpreading branch from 4cd943f to 697d479 Jul 20, 2018
@SylvainDevidal

This comment has been minimized.

Copy link
Author

SylvainDevidal commented Jul 20, 2018

Here is a new commit with a new picture :

tree1

The "diamond" layout is attenuated

@andythenorth

This comment has been minimized.

Copy link
Contributor

andythenorth commented Jul 20, 2018

Looks better to me. What's it like in sub-tropic and arctic climates? Sub-tropic has extra complication, no trees in desert :)

uint max_dist = MAX_DISTANCE_FROM_CENTER;

if (_settings_newgame.game_creation.tree_placer == TP_IMPROVED)
max_dist = GB(r, 16, 3) + 8;

This comment has been minimized.

Copy link
@TrueBrain

TrueBrain Jul 22, 2018

Member

Exactly what @michicc said; coding style, please never put a if statement with a single line body over two lines without {}. So either:

if (_settings_newgame.game_creation.tree_placer == TP_IMPROVED) max_dist = GB(r, 16, 3) + 8;

or

if (_settings_newgame.game_creation.tree_placer == TP_IMPROVED) {
    max_dist = GB(r, 16, 3) + 8;
}

Same goes for the line below :)
This to prevent future errors, where someone mistakes the indentation for a start of a new scope.

This comment has been minimized.

Copy link
@SylvainDevidal

SylvainDevidal Jul 22, 2018

Author

Oh, ok, got it!

@TrueBrain TrueBrain dismissed their stale review Jul 22, 2018

All fixed :) Tnx!

@andythenorth

This comment has been minimized.

Copy link
Contributor

andythenorth commented Jul 23, 2018

Thx ;)

I tested this some more. I think the game is now not replacing trees that have died. I tested this by running the game on fast forward for a while. On my test map, most of the trees were gone (and not replaced) after about 50 years, with no replacements. I didn't screenshot before/after, but I can run this again if that helps.

@SylvainDevidal

This comment has been minimized.

Copy link
Author

SylvainDevidal commented Jul 23, 2018

@andythenorth is there a way to "really fast forward" a game ?

When running in fast mode, one year still takes several minutes so it's very hard to check the effects of the patch on the long term

@nielsmh

This comment has been minimized.

Copy link
Contributor

nielsmh commented Jul 23, 2018

Integrate the frame rate window changes that were merged a few days ago and check why it's running slow :)
If you zoom all the way in, make the game window small, and place the view in a corner of the map, you usually waste the least time on graphics rendering, which gets you the fastest passage of time in fast-forward.

@andythenorth

This comment has been minimized.

Copy link
Contributor

andythenorth commented Jul 23, 2018

As nielsm mentioned, I place the view in a corner of the map.

I also disable 'full animation' as it slows fast-forward on some platforms. I also close all game windows (minimap etc).

It took around 10 minutes to run 50 years for me.

… on their own tile.
@SylvainDevidal

This comment has been minimized.

Copy link
Author

SylvainDevidal commented Jul 23, 2018

Here is a quick fix for the "alone" tree disapearing.
1/ Now they can spread on their own tile : we get small grove that is much realistic (IMHO)
2/ Thus when they get old, they are replaced with a new one

@andythenorth

This comment has been minimized.

Copy link
Contributor

andythenorth commented Sep 7, 2018

I've tested this again.

  • the changes from 23 July have improved the disappearing trees issue
  • testing on FFWD, I'm not convinced that the issue is completely mitigated: 'forests' start out quite dense and uniform, then seem to thin out as time progresses in the game (see screenshots, 100 years of game time)

Is this better than trunk 'improved' algorithm?

  • trunk 'improved' spams far too many trees on the map IMHO, it's overwhelming (I always toggle trees off in transparency for this reason)
  • but the trees in this patch seem a bit too sparse IMHO

6848-jan-1900

6848-jan-2000

@TrueBrain

This comment has been minimized.

Copy link
Member

TrueBrain commented Jan 5, 2019

We recently switched from Jenkins as CI to Azure Pipelines as CI. This means you need to rebase before this Pull Request will pass its checks. Sorry for the troubles!

@andythenorth andythenorth added the stale label Jan 6, 2019
@andythenorth

This comment has been minimized.

Copy link
Contributor

andythenorth commented Jan 9, 2019

Thanks for this. There's been no activity on this for some time, and as it stands, it doesn't look likely that it will go any further. I'm closing it as we try to keep the open PR count low for OpenTTD, it helps us focus on things that are important and fun. Feel free to discuss in irc or request re-opening if you disagree. Thanks for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.