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.

Sign up for GitHub

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

Jump to bottom

Great Sea Islands - Main #1827

Open
wants to merge 29 commits into
base: master
Choose a base branch
from
Open

Great Sea Islands - Main #1827

wants to merge 29 commits into from

Conversation

Malicos
Copy link
Contributor

@Malicos Malicos commented Nov 21, 2020 •
edited by sejtalk
Loading

Changelog:

  • Added islands around the Maelstrom, Nazjatar and Vashj'ir
  • Added event that turn Nazjatar and Maelstrom islands into wasteland.

Developer changelog:

  • Events that turn the provinces back into normal ones will come with separete branch.

How to test:

TitusVonDoom and others added 11 commits September 2, 2020 21:00
@Malicos Malicos requested a review from a team as a code owner November 21, 2020 14:43
@Malicos Malicos requested a review from zumbak04 November 21, 2020 14:44
@Malicos Malicos added 🎨 2D graphics 🖌️ Adding 2D graphic files or adjustments ❗ priority critical Issue or addition is of upmost importance 📄 localisation 🖊️ Adding or adjusting localisations ⭐ new feature 🆕 Adding a new feature to the mod 🚧 WIP 🚧 This pull request or issue's solution is a work in progress labels Nov 21, 2020
@Malicos Malicos added this to the 1.10 milestone Nov 21, 2020
Malicos and others added 9 commits November 21, 2020 15:58
@Malicos
Vashj'ir Snowfall
c38b40a 
Added snowfall to Vashj'ir
@sejtalk
sunken nazjatar and maelstrom underwater territories
0fc9989
@Malicos
syntax fixes
37fdcca 
fix barony syntax for 1344, 1538
renamed global flag
@Malicos
Merge branch 'master' into Nazjatar-terrain
6660766
@Malicos
Merge branch 'master' into Nazjatar-terrain
282696a
@Malicos
Update wc_nazjatar_events.txt
c58e916 
added barren atol de_jure_liege effect
@Malicos
Nazjatar event groundwork
3957ce1 
Added Tidestone of Golganneth
Added Zin-Azshari dynasty
Added Herald of Azshara nickname
Added has_tidestone_of_golganneth trigger
Added tidestone_of_golganneth artifact flag
Added events and decision for Nazjatar crisis (note: work in progress)
@Malicos
Update wc_crisis_events.txt
293e58d
@sejtalk
Merge branch 'master' into Nazjatar-terrain
e9bf92b
@sejtalk sejtalk removed the 🚧 WIP 🚧 This pull request or issue's solution is a work in progress label Dec 4, 2020
@sejtalk sejtalk requested a review from a team December 4, 2020 10:50
@sejtalk sejtalk added ❗ priority high Issue or addition is high priority and removed ❗ priority critical Issue or addition is of upmost importance labels Dec 4, 2020
@Malicos Malicos modified the milestones: 1.10, 1.11 Dec 23, 2020
LordSilvermane
LordSilvermane reviewed Feb 17, 2021
View reviewed changes
Copy link
Contributor

@LordSilvermane LordSilvermane left a comment

Choose a reason for hiding this comment

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

Works fine. The islands appear fine, the only playable one is Barren Atol, and upon entering the game, all of the islands briefly appear as independent counties (with dead rulers) before becoming wastelands within the day. The only exception to this is Barren Atol, which I presume is intended, given that it is a Blackbeard easter egg. Looks good to me.

One very minor quibble. The word atol should be spelled atoll. I think I caught (and suggested fixes for) all the cases within the files, but I don't seem to be able to suggest fixes for file names, so I'll leave that to you (I'm also not sure if they are necessary, so...).

Rerequest review once the spelling changes are done and I'll happily approve it. That's the only thing that needs to be fixed.

common/landed_titles/01_landed_titles.txt Outdated Show resolved Hide resolved
common/landed_titles/01_landed_titles.txt Outdated Show resolved Hide resolved
common/landed_titles/01_landed_titles.txt Outdated Show resolved Hide resolved
common/landed_titles/01_landed_titles.txt Outdated Show resolved Hide resolved
common/landed_titles/01_landed_titles.txt Outdated Show resolved Hide resolved
localisation/00_Province_names.csv Outdated Show resolved Hide resolved
map/definition.csv Outdated Show resolved Hide resolved
map/definition.csv Outdated Show resolved Hide resolved
map/positions.txt Show resolved Hide resolved
map/positions.txt Show resolved Hide resolved
LordSilvermane
LordSilvermane approved these changes Feb 21, 2021
View reviewed changes
Copy link
Contributor

@LordSilvermane LordSilvermane left a comment •
edited
Loading

Choose a reason for hiding this comment

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

There are a couple of cases in the files where atol has not been corrected to atoll, but I assume that they aren't necessary fixes. All in all, LGTM!

@sejtalk sejtalk requested a review from MemeWizard September 16, 2021 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LordSilvermane LordSilvermane LordSilvermane approved these changes

@zumbak04 zumbak04 Awaiting requested review from zumbak04

@MemeWizard MemeWizard Awaiting requested review from MemeWizard

Assignees

@sejtalk sejtalk

@Malicos Malicos

Labels
🎨 2D graphics 🖌️ Adding 2D graphic files or adjustments ❗ priority high Issue or addition is high priority 📄 localisation 🖊️ Adding or adjusting localisations ⭐ new feature 🆕 Adding a new feature to the mod
Projects
None yet
Milestone
Patch 1.11
Development

Successfully merging this pull request may close these issues.
5 participants
@Malicos @LordSilvermane @sejtalk @TitusVonDoom @zumbak04