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

Revision for The Hammer of Thursagan Scenario 12 "The Underlevels" #3126

Merged
merged 15 commits into from Dec 15, 2018

Conversation

@nemaara
Copy link
Contributor

nemaara commented May 17, 2018

Reworked the last scenario of THoT to be more similar in difficulty and scope with the rest of the campaign.

@@ -1,9 +1,9 @@
#textdomain wesnoth-thot
#textdomain wesnoth-thotr

This comment has been minimized.

@CelticMinstrel

CelticMinstrel May 18, 2018

Member

What's with the random textdomain change?

@stevecotton

This comment has been minimized.

Copy link
Contributor

stevecotton commented May 20, 2018

I think this is a good improvement on the middle part of the old map, but I think the entryway from the old map (with the alternating flat and cave terrain) is preferable to the new "we're in safety behind a gate" start. I'm undecided about the final area, although it's good not to have a long fight before it.

It would be good to either be able to see through the iron gates (no I don't know how to tell the engine that), or to have some dialogue about why the dwarves will only break open a few of the gates.

There's a mix of tabs and spaces in the file.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

CelticMinstrel commented May 20, 2018

It would be good to either be able to see through the iron gates (no I don't know how to tell the engine that), or to have some dialogue about why the dwarves will only break open a few of the gates.

You want [vision_costs] on the player's units. Probably added as an [object] in a unit_create event or something similar (I think [effect]apply_to=vision works).

(Or if we want gates to be see-through always we could add this to the movetype definitions in data/core/units.cfg.)

@nemaara

This comment has been minimized.

Copy link
Contributor

nemaara commented May 21, 2018

Side 1 should now be able to see past gates. The tabs should also be removed now. I could draw an entryway with more regular patterns to mimic the old one, but the old one inserted verbatim would be far too large compared to the current map. Opinions?


[object]
silent=yes
take_only_once=no

This comment has been minimized.

@sevu

sevu May 28, 2018

Member

I think this key serves no purpose.
But you can add duration=scenario

This comment has been minimized.

@CelticMinstrel

CelticMinstrel May 28, 2018

Member

It does do something but I think it's basically redundant in a one shot event.

[/object]
[/event]
[event]
name=recall

This comment has been minimized.

@sevu

sevu May 28, 2018

Member

event names can be a combination of muliple ones. E.g.

name=prerecruit, recall
first_time_only=yes

would fire only once, for whatever of the two may happens first.

Here the relatively new unit placed event would be a good fit.

[/object]
[/do]
[/for]
{CLEAR_VARIABLE units}

This comment has been minimized.

@sevu

sevu May 28, 2018

Member

If already applying this at recruited and recalled units, it is eventually not needed here. In and case, a unit placed event covers all.

@nemaara

This comment has been minimized.

Copy link
Contributor

nemaara commented May 29, 2018

I've replaced the previous code for the gate vision with a single "unit placed" event. A quick test seems to suggest that it works.

@sevu sevu added the Campaign label Jun 15, 2018

@sevu sevu added this to the 1.14.6 milestone Sep 9, 2018

@shikadiqueen shikadiqueen force-pushed the wesnoth:master branch from 71a6daf to 60fd504 Oct 14, 2018

@nemaara nemaara changed the base branch from master to 1.14 Oct 15, 2018

@nemaara nemaara changed the base branch from 1.14 to master Oct 15, 2018

@nemaara nemaara changed the base branch from master to old_master_20181015 Oct 15, 2018

@nemaara nemaara force-pushed the nemaara:master branch from d67be2b to a18679e Oct 15, 2018

@jostephd jostephd force-pushed the nemaara:master branch from a18679e to 27cc3b9 Oct 15, 2018

@nemaara nemaara changed the base branch from old_master_20181015 to master Oct 15, 2018

@GregoryLundberg

This comment has been minimized.

Copy link
Contributor

GregoryLundberg commented Oct 17, 2018

This has been wanted for some time, now. Assigning to Zookeeper for approval. On a read-through I like what I'm seeing, though.

@GregoryLundberg
Copy link
Contributor

GregoryLundberg left a comment

This is on the 1.14.6 milestone but merges against master.

Rebase this against 1.14 and we can cherry-pick to master once this merges.

Also, this really needs a note in the player's change notes.

@nemaara

This comment has been minimized.

Copy link
Contributor

nemaara commented Oct 18, 2018

Can someone point me to the players' changelog? Also, I honestly have no idea how to rebase this against 1.14, so perhaps I could just make a separate PR against the 1.14 branch?

@jostephd

This comment has been minimized.

Copy link
Member

jostephd commented Oct 18, 2018

@nemaara I assume @GregoryLundberg meant changelog.md (there used to be players_changelog.md but it's been removed).

I don't think we should ask @nemaara to rebase. This patch will need to go to master anyway so we might as well merge it to master and then cherry-pick to 1.14. (I'm a bit surprised that we'd include this change in a patch release, actually, but okay.)

@GregoryLundberg

This comment has been minimized.

Copy link
Contributor

GregoryLundberg commented Oct 18, 2018

I'll handle cherry-picking to 1.14 when I merge this. All I'm waiting for is the changelog update. (Yes, changelog.md and, yes, if we still had players_changelog.md, there too.)

Oh, and I'd really like zookeeper to take a gander at this but if it languishes too long before he does, I'll simply merge and cherry-pick it.

@nemaara

This comment has been minimized.

Copy link
Contributor

nemaara commented Oct 19, 2018

I am currently working through josteph's post on the forum, where he got stuck at the entrance for a little bit longer than I would've liked. After playtesting the scenario and trying the style he tried, I've decided to try to rebalance it so it feels a little less "grindy" at the start, so to speak (right now you can get stuck behind quite a few waves of enemies and it takes a long time to fight through them). I should be able to come up with something within a couple days (hopefully).

nemaara added some commits Oct 21, 2018

@nemaara

This comment has been minimized.

Copy link
Contributor

nemaara commented Oct 21, 2018

I've rebalanced it and quickly playtested the scenario. It should be easier to get through the AI lines (less grindy) now, although ideally someone else would playtest it as well (may or may not really be necessary). I've also added the requested changelog update.

@nemaara nemaara force-pushed the nemaara:master branch from 0d17569 to 6256341 Oct 23, 2018

@GregoryLundberg

This comment has been minimized.

Copy link
Contributor

GregoryLundberg commented Oct 23, 2018

Leave this be for a while. The merge conflict is simple: accept both changes. There's a couple/four lines with trailing whitespace. I just did a test run rebasing, squashing and fixing and it's no problem.

I want to give this a week or so more for play-testing and review.

@nemaara

This comment has been minimized.

Copy link
Contributor

nemaara commented Dec 8, 2018

@GregoryLundberg can this be merged now, or would you like to wait longer?

@Pentarctagon

This comment has been minimized.

Copy link
Member

Pentarctagon commented Dec 13, 2018

It's been quite a bit longer than a week.

@GregoryLundberg

This comment has been minimized.

Copy link
Contributor

GregoryLundberg commented Dec 13, 2018

Oh my. I've been pulled off for another project and never got around to this. Someone else, please, fix the conflict, merge this, and backport to 1.14.

@Pentarctagon

This comment has been minimized.

Copy link
Member

Pentarctagon commented Dec 14, 2018

I'll do this at the same time as #3648 then.

@Pentarctagon Pentarctagon merged commit 6256341 into wesnoth:master Dec 15, 2018

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor was unable to build non-mergeable pull request
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment