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

[Tutorial] Small fixes #3963

Merged
merged 6 commits into from
Mar 4, 2019
Merged

[Tutorial] Small fixes #3963

merged 6 commits into from
Mar 4, 2019

Conversation

nemaara
Copy link
Contributor

@nemaara nemaara commented Mar 2, 2019

For #3390, see my comment there (#3390 (comment)).

@nemaara nemaara added this to the 1.14.7 milestone Mar 2, 2019
@CelticMinstrel
Copy link
Member

You should really merge 6f23fc1 and 3777569 into a single commit, since the former references the unit added by the latter. Could also merge ecb0f1a in with it, but that's not so important (even though it makes no sense to commit it before actually adding the new unit, at least it's not directly referencing anything in the other commits).

That aside, this looks good!

@CelticMinstrel
Copy link
Member

(Oh right, also... shouldn't that changelog entry be in 1.14.7+dev, not 1.14.6+dev?)

@nemaara
Copy link
Contributor Author

nemaara commented Mar 3, 2019

Maybe I'm not understanding what the headings mean, but for example when I did the TSG revision, the changelog entry was under 1.14.5+dev (but the TSG revision went in 1.14.6). Since this is going in 1.14.7, I thought that the changelog entry should be in 1.14.6+dev similarly?

@CelticMinstrel
Copy link
Member

Ah, sorry, you're right. I got confused there about the changelog headers. (By the way, not related, but why is 1.14.5+dev not changed to 1.14.6 yet?)

@jostephd
Copy link
Member

jostephd commented Mar 3, 2019

Ah, sorry, you're right. I got confused there about the changelog headers. (By the way, not related, but why is 1.14.5+dev not changed to 1.14.6 yet?)

It is on the 1.14 branch but not on master.

@@ -125,6 +125,11 @@
* S17c: Modified Burin's description of trolls
* The South Guard:
* Fixed various typos
* Tutorial:
* Remove swamp from map for part 2
Copy link
Member

Choose a reason for hiding this comment

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

Why remove the swamp hexes? Couldn't we just add an event for them? The tutorial is basically an interactive trailer, I think it is good to have it show off a bit of terrain variety.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are literally just a couple swamp hexes laying around that look pretty ugly anyway. If we wanted to show off terrain variety, it would involve redrawing the map, not having those hexes.

@CelticMinstrel
Copy link
Member

For the record, I don't really care one way or another about the swamp hexes.

@nemaara nemaara merged commit 4a6bd38 into wesnoth:master Mar 4, 2019
@nemaara nemaara deleted the tutorial branch March 6, 2019 16:47
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

3 participants