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

DW 5 Tirigaz - Changes to orc leader death event #3103

Merged
merged 3 commits into from
Jun 11, 2018
Merged

DW 5 Tirigaz - Changes to orc leader death event #3103

merged 3 commits into from
Jun 11, 2018

Conversation

UnwiseOwl
Copy link
Contributor

@UnwiseOwl UnwiseOwl commented May 13, 2018

Fixes #3092 - Now updates objectives if orc leader is killed first, and gives additional dialogue
Fixes #3093 - Gold event now won't fire if ghosts kill the orc leader

Fixes #3092 - Updates objectives if orc leader is killed first, and gives additional dialogue
Fixed #3903 - Gold event doesn't fire if ghosts kill the orc leader
@Wedge009
Copy link
Member

Just advising that I think spaces are preferred to tabs.

speaker=second_unit
message= _ "It seems that orc was rich! He has a chest here with over 100 pieces of gold!"
speaker=Kai Krellis
message= _ "Now we must defeat the undead."
Copy link
Member

Choose a reason for hiding this comment

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

This is a string change - meaning this can not go into 1.14.2 and must wait for 1.14.3. However, you could exclude this message for now, and add it in a new PR when 1.14.2 is out.
To update this PR, you can use

git commit --amend after your changes
git push -f

That said, I do not know when 1.14.2 will be tagged, and if there is enough time until then.

[gold_carryover]
carryover_percentage=40
[/gold_carryover]
[/objectives]
Copy link
Member

@sevu sevu May 23, 2018

Choose a reason for hiding this comment

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

I would prefer if you could instead of a new [objectives] block edit the first one like it is done here:
https://github.com/wesnoth/wesnoth/blob/1.14/data/campaigns/The_South_Guard/scenarios/07a_Into_the_Depths.cfg#L299-L309
As we have no variable here, the condition could be

[show_if]
    [have_unit]
        side=number of the side
        canrecruit=yes
    [/have_unit]
[/show_if]

This second [objectives] tag could be replaced by
[show_objectives][/show_objectives] then

[/have_unit]

Copy link
Member

Choose a reason for hiding this comment

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

consider using wmlindent - all you need is a working python3 installation

@sevu sevu added Postponed Issues which cannot be worked on at this time. Campaign (any) Deprecated tag, replaced with separate tags for each mainline campaign labels May 23, 2018
@CelticMinstrel
Copy link
Member

It's hard to tell from the diff on mobile, but can you still get the gold if undead kill the orcs?

@UnwiseOwl
Copy link
Contributor Author

UnwiseOwl commented May 29, 2018

Not any more. That was the point of the PR.
There's a new check where that event only fires if the Orcish leader is killed by side 1.

@CelticMinstrel
Copy link
Member

So do the undead get the gold instead? If the undead aren't interested I think it'd make sense to drop the gold on the ground where you can later swoop in and grab it.

@UnwiseOwl
Copy link
Contributor Author

UnwiseOwl commented May 29, 2018

Currently the gold never comes into existence if the ghosts kill the orc, and if that happens then the player never discovers that they could potentially have gotten it. Personally I prefer it this way, as the gold acts as a bonus for killing the orc.

I could make it pickupable, but personally I like that less than the current arrangement.

@CelticMinstrel
Copy link
Member

*shrug* Was just an idea, I'm not especially attached to it or anything.

@UnwiseOwl
Copy link
Contributor Author

This no longer needs to be marked posponed since we're past the string freeze now.

@sevu sevu removed the Postponed Issues which cannot be worked on at this time. label May 30, 2018
@sevu
Copy link
Member

sevu commented Jun 4, 2018

Duh, we're in string freeze again \o/
I see no reason not to merge this (besides the freeze)

@stevecotton
Copy link
Contributor

Just an idea about #2940's tidy up of "mermen" vs "merfolk", I'd like to add a translation tip to the orc's last-breath event. I'm happy to open a separate PR for this.

         [message]
             speaker=Marg-Tonz
+            # po: orc's boss' last breath, deliberately gendered instead of changing to "merfolk"
             message= _ "I hate mermen!"
         [/message]

@stevecotton
Copy link
Contributor

stevecotton commented Jun 10, 2018

Is this meant for 1.14.x? I'm confused because it's targetting master, but it's affected by the 1.14 string freeze.

(I'm in favour of putting it in 1.14.x too)

@sevu sevu merged commit b3c4049 into wesnoth:master Jun 11, 2018
@sevu
Copy link
Member

sevu commented Jun 11, 2018

It could have gone into master before it goes into 1.14, that's true.

I'm in favour of the translation hint.
Btw, I saw that you have commits with the name "stevecotton" and "Steve Cotton", mostly the later. In git you are listed as two persons with that.

@UnwiseOwl UnwiseOwl deleted the tirigazattempt branch June 12, 2018 14:50
jostephd pushed a commit to jostephd/wesnoth that referenced this pull request Oct 6, 2018
DW 5 Tirigaz - Changes to orc leader death event

Fixes wesnoth#3092 - Updates objectives if orc leader is killed first, and gives additional dialogue
Fixes wesnoth#3903 - Gold event doesn't fire if ghosts kill the orc leader
jostephd pushed a commit to jostephd/wesnoth that referenced this pull request Oct 7, 2018
DW 5 Tirigaz - Changes to orc leader death event

Fixes wesnoth#3092 - Updates objectives if orc leader is killed first, and gives additional dialogue
Fixes wesnoth#3903 - Gold event doesn't fire if ghosts kill the orc leader

(cherry-picked from commit b3c4049)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Campaign (any) Deprecated tag, replaced with separate tags for each mainline campaign
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants