Skip to content

Add missing Natural Wonders #5204

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

Merged
merged 10 commits into from
Sep 13, 2021
Merged

Add missing Natural Wonders #5204

merged 10 commits into from
Sep 13, 2021

Conversation

ravignir
Copy link
Contributor

This adds missing natural wonders from G&K and BNW. The latter are commented out, but I decided to leave them here anyway.

z

Copy link
Collaborator

@xlenstra xlenstra left a comment

Choose a reason for hiding this comment

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

Indentation is off a bit due to mixing of tabs & spaces, but we'll fix that later. Looks good otherwise!

@ravignir
Copy link
Contributor Author

ravignir commented Sep 13, 2021

ok, fixed that :)

It is weird tho, because it all looked ok on my computer, it broke after uploading to github.

@SomeTroglodyte
Copy link
Collaborator

Then you'll need a better editor - or better settings. Kotlin style guide expressly says do not use tabs and prescribes 4 spaces per indent. If your editor can handle python without the code breaking after a few edits, you should be fine...

@SomeTroglodyte
Copy link
Collaborator

  • What does "avoid ocean" mean? I see there's neither an implementation nor a todo.
  • "biggest landmass" can be done or soon - See City-States Influence rates; Wary status; Proximity calculations #5198 where we discussed how landmass size was already done (twice) and discarded by GameStarter. So now we know that code should be refactored in a way that it can run within map generation or on a map loaded from file, and in the case a generated map is not for the editor but 'started' directly, a side-channel to pass the info from map generation to gamestarter could save time and GC load.
  • The wonders urgently need "uniques" style conditions - one loop, no names special-cased as constants, no function per wonder, just json - should be possible, all those conditions are very similar.
  • && it.improvement == null - that's just copied - but why is this here? There can't be any improvements at this point - ancient ruins come last.
  • I see json and code, but no FantasyHex or atlas - but your screenshot must have had those, right?

That makes five 👎 so let me balance that out: 👍🏻👍🏿👍🏼👍🏽👍👍🏾 - there, almost neutral.

@ravignir
Copy link
Contributor Author

* What does "avoid ocean" mean? I see there's neither an implementation nor a todo.

That basically means Natural Wonder can't spawn next to coast, which was omitted up to this point - which is why i did not include it.
it.neighbors.none { neighbor -> neighbor.getBaseTerrain().name == Constants.coast }

* "biggest landmass" can be done or soon - See [City-States Influence rates; Wary status; Proximity calculations #5198](https://github.com/yairm210/Unciv/pull/5198) where we discussed how landmass size was already done (twice) and discarded by GameStarter. So now we know that code should be refactored in a way that it can run within map generation _or_ on a map loaded from file, and in the case a generated map is not for the editor but 'started' directly, a side-channel to pass the info from map generation to gamestarter could save time and GC load.

ok. So that is still ToDo later.

* The wonders urgently need "uniques" style conditions - one loop, no names special-cased as constants, no function per wonder, just json - should be possible, all those conditions are very similar.

Civ5 actually does that, but that would require quite a lot of uniques. Unfortunately, I don't feel I am capable of doing that (yet).

* `&& it.improvement == null` - that's just copied - but why is this here? There can't _be_ any improvements at this point - ancient ruins come last.

ok. I did not know that. But If I recall correctly, it used to be the case in past.

* I see json and code, but no FantasyHex or atlas - but your screenshot must have had those, right?

It was already added in #4916, so that is why there is only code and json :)

To sum up: I will add avoid ocean thing and check if reddit source is indeed right, because i recall Sri Pada spawning next to coast tiles in my civ5 games.

@SomeTroglodyte
Copy link
Collaborator

SomeTroglodyte commented Sep 13, 2021

it.neighbors.none { neighbor -> neighbor.getBaseTerrain().name == Constants.coast }

Now you mention it, neighbors is a little weird. The actual materialized list is nowhere 'kept' explicitly, and we trust the GC to not collect it by seeing the connection through that Sequence offer... Do I understand that? No.

Anyway, if you use neighbors you can also use isCoastalTile()

There can't be any improvements at this point

... at least as far as I see ... I hope I'm right. Perhaps someont thought of the deprecated fake StartingLocations, which at that point wouldn't have existed either. No matter, makes no difference.

already added

Senility at work here.

quite a lot of uniques

Fingertips tingling... Can't be that hard. This first.

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.

4 participants