Page MenuHomeWildfire Games

Fix sounds inheritance of template_unit_hero_{unit}.xml
ClosedPublic

Authored by Grugnas on Jul 16 2017, 9:09 PM.

Details

Summary

Since the template for healer hero has been implemented assuming that any unit hero has Soldier class, this patch gather all the SoundGroups entries into template_unit_hero.xml ( since the unit won't play a sound it can't execute like gather, build and ranged/melee attacks for melee/ranged units ) and will be eventually overridden in order to replace the corresponding entry in the affected template and its children.

Test Plan

test that units play only the intended sounds.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Grugnas created this revision.Jul 16 2017, 9:09 PM
Owners added a subscriber: Restricted Owners Package.Jul 16 2017, 9:09 PM
elexis updated the Trac tickets for this revision.
Vulcan added a subscriber: Vulcan.Jul 16 2017, 9:56 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1749/ for more details.

Stan requested changes to this revision.Jul 30 2017, 2:50 PM
Stan added a subscriber: Stan.

I'd use alarm_herojoin.xml and alarm_herodead.xml if possible.

Otherwise looks good.

This revision now requires changes to proceed.Jul 30 2017, 2:50 PM
Grugnas planned changes to this revision.Aug 1 2017, 5:48 PM

I will recheck as soon as possible, thanks for the input

fatherbushido resigned from this revision.Aug 2 2017, 9:15 PM
Grugnas updated this revision to Diff 3157.Aug 17 2017, 9:21 PM
Grugnas edited edge metadata.

As suggested, the alarms herojoin.xml and alarm_herodead.xml are now used in order to have special sounds for heroes when they train or die.

Stan accepted this revision.Aug 17 2017, 10:27 PM
This revision is now accepted and ready to land.Aug 17 2017, 10:27 PM
fatherbushido added a subscriber: fatherbushido.EditedAug 18 2017, 9:30 AM

I was close to commit that but quickly tested before:

  • perl validate.pl -> ok
  • else did you really test that thing? I am not an artist nor a singer nor a musician, but the training sound and the death sound don't fit at all. (Btw that's not the one used for other heroes).

@Stan: could you consider again your review?

Stan added a comment.Aug 18 2017, 12:08 PM

@fatherbushido I get what you mean. So about the review the patch is good it passes the test and fix the given bug and one other. So to me that is commitable. Now I'm no sound artist and indeed I feel we should use those sounds. And change them at some point if necessary or remove them if they can't be used. Maybe it feels out of place because we are not used to it I don't know.

To sum up. Since you commit it it's up to you either don't commit that sound change and the patch will be fine to me or commit it as is and it'll be fine either way.

fatherbushido added a comment.EditedAug 18 2017, 12:33 PM
In D747#31642, @Stan wrote:

To sum up. Since you commit it it's up to you either don't commit that sound change and the patch will be fine to me or commit it as is and it'll be fine either way.

So you would commit that?

Stan added a comment.Aug 18 2017, 2:54 PM

Yeah. I would probably update other hero actors to use those sounds afterwards.

If you do not want to do that this will work. Delete those lines.

<trained>interface/alarm/alarm_herojoin.xml</trained>
<death>interface/alarm/alarm_herodead.xml</death>

And change the inheritance

from

<Entity parent="template_unit">

to

<Entity parent="template_unit_hero_infantry">

Which will make it inherit from the death sound variant.

In D747#31675, @Stan wrote:

change the inheritance
from

<Entity parent="template_unit">

to

<Entity parent="template_unit_hero_infantry">

Which will make it inherit from the death sound variant.

I recall proposing that relative template change but as healer and support unit, that hero shouldn't be considered as infantry.

fatherbushido added a comment.EditedAug 19 2017, 4:49 PM

@Grugnas @Stan
So?
(Perhaps Chanakya is a Mech though)

@Grugnas @Stan
So?
(Perhaps Chanakya is a Mech though)

if i can say my personal opinion, that alarm_herojoin.xml doesn't fit the game (but it is only a personal opinion) thus we could copy paste infantry death sound as workaround in order to use alarm_herojoin.xml and alarm_herodeath.xml as placeholder for a future sound implement.
That's not up to me though.

Stan requested changes to this revision.Aug 20 2017, 12:15 AM

@elexis Ping (You are marked as reviewer)

@Grugnas
Let's go to the simple way and keep it coherent.

1 - Make it inherit the default sounds (No need to duplicate code all over) and let's ignore those new sounds for now.

2 - Create another ticket for those two sounds to at least fix the buggy behaviour and at best find a way to properly use them, maybe there is a sound that fits in the game.

This revision now requires changes to proceed.Aug 20 2017, 12:15 AM
Grugnas updated this revision to Diff 3196.Aug 20 2017, 1:26 AM
Grugnas edited edge metadata.
Grugnas retitled this revision from Chanakya hero sounds to Chanakya hero inherit template_unit_hero.
Grugnas edited the summary of this revision. (Show Details)
Grugnas added a parent revision: D806: Allow non Soldier Heroes.
fatherbushido added a comment.EditedSep 16 2017, 5:57 PM

@Grugnas: would you like to finish that (ie: clean the sound inheritance of hero template and add the healer ones?)

(fix also the attack sound for ranged cavalry)

Grugnas added a comment.EditedSep 20 2017, 4:39 PM

this patch should be complete already.
this is the diff new sound:

</Identity>
<Sound>
  <SoundGroups>
    <trained>interface/alarm/alarm_create_infantry.xml</trained>
    <order_heal>voice/{lang}/civ/civ_{gender}_heal.xml</order_heal>
  </SoundGroups>
</Sound>

EDIT: just noticed that i commented but the comment has never be sent..........
Anyway looking at the patch again, seems like the patch has to inherit the new template

EDIT: just noticed that i commented but the comment has never be sent..........
Anyway looking at the patch again, seems like the patch has to inherit the new template

Yes, I asked you if you want to clean the hero sounds inheritence and add the sounds to the healer (and possibly some other fix like ranged attack sound for ranged cavalry hero)

fatherbushido requested changes to this revision.Sep 29 2017, 10:15 PM

any news about that?

  • the title should be renamed to the first one
  • the sound group should be added
This revision now requires changes to proceed.Sep 29 2017, 10:15 PM
Stan added a comment.Dec 12 2017, 5:24 PM

@Grugnas Bump, I'd like to commit this before A23 please.

Grugnas planned changes to this revision.Dec 12 2017, 5:36 PM

I'll update the proper patch asap.

(Make sure that the new template has the same properties as before, i.e. comparing every line of the two parent templates)

elexis resigned from this revision.Dec 12 2017, 8:35 PM
Grugnas updated this revision to Diff 4772.Dec 14 2017, 5:51 PM
Grugnas retitled this revision from Chanakya hero inherit template_unit_hero to Fix sounds inheritance of template_unit_hero_{unit}.txt.
Grugnas edited the summary of this revision. (Show Details)
Grugnas edited the test plan for this revision. (Show Details)
Stan added inline comments.Dec 18 2017, 11:32 PM
binaries/data/mods/public/simulation/templates/template_unit_hero.xml
57 ↗(On Diff #4772)

I don't know why but the beginning of the line is yellow, Whitespace maybe ?

67 ↗(On Diff #4772)

Do we use that everywhere ?

binaries/data/mods/public/simulation/templates/template_unit_hero_elephant_melee.xml
39 ↗(On Diff #4772)

Why did that move up ?

Grugnas added inline comments.Dec 18 2017, 11:42 PM
binaries/data/mods/public/simulation/templates/template_unit_hero.xml
57 ↗(On Diff #4772)

Apparently this was an "unerased" space thus it is brighter as happens with "unerased" code in red.

67 ↗(On Diff #4772)

Yes, cavalry and elephant heroes override it with the proper file.

binaries/data/mods/public/simulation/templates/template_unit_hero_elephant_melee.xml
39 ↗(On Diff #4772)

Moved up because other templates had it on top of the entries ( apparently its parent template has <select> on top though )

elexis added inline comments.Dec 19 2017, 12:34 AM
binaries/data/mods/public/simulation/templates/template_unit_hero.xml
57 ↗(On Diff #4772)

Lines are marked as yellow if they were copied or moved, see tooltip.

Stan added a comment.Dec 19 2017, 12:48 AM

Patch looks good to me. I'll have another check tomorrow to see if I haven't missed anything.
Note to self : Write a python script that does just that.
If I have no more things to say tomorrow I'll commit this.

Thanks @elexis for the comment.

Stan added a subscriber: s0600204.Dec 19 2017, 7:43 PM

Any reason those lines in template_unit_hero don't go directly in template_unit ?
@s0600204 Any input on those changes ?

binaries/data/mods/public/simulation/templates/template_unit_hero.xml
63 ↗(On Diff #4772)

Not sure about that one. That template is not supposed to define behaviour for melee isn't it ?
That being said The parent template does define the template sound

I think not, but I'll check since it seems reasonable to have all sounds inherited from template_unit and then overriden

Stan requested changes to this revision.Dec 20 2017, 3:00 PM
This revision now requires changes to proceed.Dec 20 2017, 3:00 PM

@Stan You're asking me?

Uh...

  • The change to template_unit_hero_elephant_melee is (imo) unneccesary. Also...
$ cd ./binaries/data/mods/public/simulation/templates
$ ack -A16 select | grep -c trained   # select before trained
20
$ ack -A16 trained | grep -c select   # trained before select
5

...a greater number of templates currently have the trained sound group listed after the select group than before. (Numbers without this patch applied.)

  • I agree that the sounds of the alarm_herojoin and alarm_herodead sound groups don't really fit into the game.
  • Patch doesn't currently apply cleanly via arcanist. This is because this revision has a prerequisite (D806) that arc tries to apply first. (Applying this revision's changes manually works.)
  • The order_gather soundgroup was removed from the ..._hero_infantry and ..._hero_cavalry templates, but has not reappeared in ..._hero. ..._hero_elephant_melee does still have it though.
  • Similarly, the order_repair soundgroup is also gone from ..._hero_infantry and has not reappeared. Yes, I'm aware these are heroes but, y'know, modders. (Incidentally, Aristeian egyptian heroes can build obelisks. Just as an example of Heroes doing something that you might not expect them to do.)

Otherwise, looks good.

Any reason those lines in template_unit_hero don't go directly in template_unit ?

template_unit is inherited by, amongst other things, fauna. Some fauna do currently provide overrides (sheep, goats) but many don't. It would be a little weird to hear footsteps coming from a shark as is swims.

And then we have the soundgroups that use {lang} and/or {gender} placeholders. If the template is unable to provide values, then there are fall-back defaults (see GetGender() in the Identity component) that might not be appropriate; resulting in wild boar that respond in greek when you select them, and deer that die with a very human groan.

Anyway, something to consider in another revision, I think.

Thank you for the interest for the diff (even if implicit 💃 ).
The parent revision explained the arc issue, thus removed.

The change to template_unit_hero_elephant_melee is (imo) unneccesary. Also...

$ cd ./binaries/data/mods/public/simulation/templates
$ ack -A16 select | grep -c trained # select before trained
20
$ ack -A16 trained | grep -c select # trained before select
5
...a greater number of templates currently have the trained sound group listed after the select group than before. (Numbers without this patch applied.)

I thought that having a creation ( train ) and ( death ) would open and close the "life cycle" of the entity thus moved the entries. Since it is unwanted and not consistent, it will be reverted.

The order_gather soundgroup was removed from the ..._hero_infantry and ..._hero_cavalry templates, but has not reappeared in ..._hero. ..._hero_elephant_melee does still have it though.

That's a battle i lost already once. In my humble opinion a hero shouldn't be conceptually a soldier ( that's what D806 was about ) by default but a generic unit with an eventual Soldier class child with specific sound entries ( like attack, order-attack, etc.). Matter of fact the -Soldier class in the template_hero_healer is ugly and not very maintainable.

Similarly, the order_repair soundgroup is also gone from ..._hero_infantry and has not reappeared

They are soldiers, but sure, template_unit_hero should have them. you are right.

Grugnas updated this revision to Diff 4902.Dec 23 2017, 11:59 AM
Grugnas retitled this revision from Fix sounds inheritance of template_unit_hero_{unit}.txt to Fix sounds inheritance of template_unit_hero_{unit}.xml.
Stan edited reviewers, added: s0600204; removed: Stan.Dec 24 2017, 12:15 PM

I'm not confident enough for that change. I will leave it to experts.

s0600204 accepted this revision.Dec 28 2017, 9:47 PM

Looks good.

Unless anyone has anything further to say, I'll commit this on behalf of Grugnas in a day or so. (Unless Stan beats me to it.)

This revision is now accepted and ready to land.Dec 28 2017, 9:47 PM
Stan added a comment.Dec 28 2017, 11:10 PM

Thanks ! Please do. my SVN is broken until I fixed and nuked every duplicate for new horses.

This revision was automatically updated to reflect the committed changes.