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.
Details
- Reviewers
s0600204 elexis - Commits
- rP20748: Fix sound inheritance from template_unit_hero.xml
- Trac Tickets
- #4678
test that units play only the intended sounds.
Diff Detail
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Event Timeline
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.
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.
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?
@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.
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.
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.
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.
@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.
@Grugnas: would you like to finish that (ie: clean the sound inheritance of hero template and add the healer ones?)
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)
any news about that?
- the title should be renamed to the first one
- the sound group should be added
(Make sure that the new template has the same properties as before, i.e. comparing every line of the two parent templates)
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 ? |
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 ) |
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. |
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.
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 ? |
I think not, but I'll check since it seems reasonable to have all sounds inherited from template_unit and then overriden
@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.
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.)
Thanks ! Please do. my SVN is broken until I fixed and nuked every duplicate for new horses.