Page MenuHomeWildfire Games

Projectile hit sound
ClosedPublic

Authored by Mate-86 on Sep 19 2017, 10:57 PM.

Details

Reviewers
fatherbushido
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Owners Package(Owns No Changed Paths)
Commits
rP20236: Add a new function to emit a sound from a position not attached to an entity.
Trac Tickets
#4779
Summary

New cpp function created to play a sound group at a position without passing the entity. This way the sound is played even if the attacker or target entity died in the meantime.

TODO: proper explosion sound is needed for the siege onager

Test Plan

Testing in game with siege onager (delay=2000 in template)

  • sound is played after the delay
  • sound is played even if the onager is killed before the projectile landed

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

Mate-86 created this revision.Sep 19 2017, 10:57 PM
leper added reviewers: Restricted Owners Package, Restricted Owners Package.Sep 20 2017, 1:18 AM
leper added a subscriber: leper.
leper added inline comments.
binaries/data/mods/public/simulation/components/Damage.js
114

Assuming you also want this TODO comment to be committed add a space before TODO. And probably capitalize create.

source/simulation2/components/CCmpSoundManager.cpp
90

Missing if (g_SoundManager).

Lionkanzen added a subscriber: Lionkanzen.
fatherbushido added inline comments.
binaries/data/mods/public/simulation/components/Damage.js
269–270

We need to use it for that one too.

bb added a subscriber: bb.Sep 20 2017, 12:47 PM

slightly related: D757

Mate-86 marked 2 inline comments as done.EditedSep 20 2017, 10:38 PM

Do you know how to contact @Pureon for new sounds? I cannot tag him here in Differential. Right now there would be good to have an explosion sound for the onager missile hit.
Do we need separate sound for a projectile hitting the ground/target and causing the damage (explode)? The later can be delayed.

binaries/data/mods/public/simulation/components/Damage.js
269–270

I've checked the PlaySound function and it's used as a wrapper for playing the sound of attack, walk, gathering, etc.
Does it mean that all the above sounds should be played using the new PlaySoundGroupAtPosition ?

Mate-86 updated this revision to Diff 3732.Sep 20 2017, 10:42 PM

Do you know how to contact @Pureon for new sounds?

Forum pm is a way to reach him.

Do we need separate sound for a projectile hitting the ground/target and causing the damage (explode)? The later can be delayed.

That question makes sense only for delayed damage I guess. With the current diff, the sound will be emited when causing damage (if delay) and on hit - ground or target (if not delay). I won't design thing but that seems ok like that and it's coherent with the non delayed case.

binaries/data/mods/public/simulation/components/Damage.js
269–270

PlaySound is just an helper to play entity related sound.
In that specific case, the attack_impact sound is attached to the attacker. So if it dies, that sound won't be emited.
There are two ways to fix that:

  • use the SoundGroupManager (a system component) with your new function
  • or perhaps make it related to the target entities: changing PlaySound("attack_impact", data.attacker) to PlaySound("attack_impact", data.target);

I have no preferences. There were perhaps reason to not do the second thing. Need to see with people like Pureon for example.

(those other sounds in your comment don't need that new function)

I've pinged Pureon via Forum PM.

binaries/data/mods/public/simulation/components/Damage.js
269–270

Sorry, I'm a bit lost now (I've mixed up attack and attack_impact). I've realized in the meantime the existing attack_impact and the newly introduced damage sound in L118 serve the same purpose. The only difference that the attack_impact sound played 100 times if 100 units are affected by the splash damage. Does this make sense?

How about removing L271 and using the attack_impact sound in L118?

fatherbushido added inline comments.Sep 21 2017, 7:25 PM
binaries/data/mods/public/simulation/components/Damage.js
269–270

Ah nice.
I guess you was wondering why I always spoke about that!
I agree with your suggestion (removing L271 and using the attack_impact sound in L118) ;-)
(that time we'll have the attack_impact sound even if no unit but only the ground is hit, seems good enough)

Pureon added a subscriber: Pureon.Sep 22 2017, 12:40 AM

I've committed some new siege impact sounds to the svn. Please use audio/attack/impact/siegeprojectilehit.xml

Mate-86 updated this revision to Diff 3756.Sep 22 2017, 11:06 PM

Thanks for the sound Pureon! It fits well in the game. There is an issue with the 3rd file not sure if you have seen this before? "OpenAL: stereo sounds can't be positioned: audio/attack/impact/siegeprojectilehit_03.ogg"

Sounds like this (the warnings on top are just for showing the position used for playing the sound):

In D921#36199, @Mate-86 wrote:

Thanks for the sound Pureon! It fits well in the game. There is an issue with the 3rd file not sure if you have seen this before? "OpenAL: stereo sounds can't be positioned: audio/attack/impact/siegeprojectilehit_03.ogg"

I exported that OGG as stereo by mistake, should be mono channel only. I'll fix now.

Pureon added a comment.EditedSep 23 2017, 12:00 AM

Committed the ogg fix.

Would it also be possible to add debris particles (a short blast of smoke) from the projectile hit location? art/particles/destruction_dust_small_gray.xml should work for testing.

In D921#36217, @Pureon wrote:

Committed the ogg fix.

With the new ogg the warning is not thrown anymore. Thanks!

Would it also be possible to add debris particles (a short blast of smoke) from the projectile hit location? art/particles/destruction_dust_small_gray.xml should work for testing.

#1909 will address this

Glad it worked. Thanks for your work on this @Mate-86

fatherbushido added inline comments.Sep 23 2017, 6:59 PM
binaries/data/mods/public/simulation/components/Attack.js
503

(No idea if it's better to use ' or ")

binaries/data/mods/public/simulation/components/Damage.js
117

I am not certain we need to check the existence of cmpSoundManager
At that point of the game, that system component should exist

118

ok

source/simulation2/components/CCmpSoundManager.cpp
92

It seems that there can perhaps be an issue.
PlayAsGroup (in soundmanager/SoundManager.cpp) use the source entity in that block

	// Failed to load group -> do nothing
	if (group && (ownedSound || !group->TestFlag(eOwnerOnly)))
		group->PlayNext(sourcePos, source);

Then you can go deeper in Playnext in soundmanager/scripting/SoundGroup.cpp
etc...

So you should try to see what exactly happen in that case. (I guess/hope it's ok)

source/simulation2/components/ICmpSoundManager.cpp
26

k

source/simulation2/components/ICmpSoundManager.h
37

Perhaps add a comment like that for consistency
/**

  • Start playing audio defined by a sound group file.
  • @param name VFS path of sound group .xml, relative to audio/
  • @param sourcePos 3d position of the sound emitter
	 */
38

It 'sounds' ok. Should check with cpp guys.

(Else patch is ok)

Mate-86 added inline comments.Sep 23 2017, 7:23 PM
binaries/data/mods/public/simulation/components/Attack.js
503

switched to " to be consistent

binaries/data/mods/public/simulation/components/Damage.js
117

I've added the check to be consistent with other components. ProductionQueue.js:Line 802 and Sound.js:Line 48 doing the same check.

source/simulation2/components/CCmpSoundManager.cpp
92

I went deeper ...
PlayNext calls UploadPropertiesAndPlay which calls CSoundManager::ItemForEntity which does not use the source. It's marked in function header this parameter is unusued:
ISoundItem* CSoundManager::ItemForEntity(entity_id_t UNUSED(source), CSoundData* sndData)
Not sure it's because half-ready or depricated but it works as is right now.

source/simulation2/components/ICmpSoundManager.h
37

adding that

Mate-86 updated this revision to Diff 3760.Sep 23 2017, 7:23 PM
Mate-86 marked 14 inline comments as done.
fatherbushido added inline comments.Sep 23 2017, 7:30 PM
binaries/data/mods/public/simulation/components/Damage.js
117

Well, perhaps keep them so.
Let's grab an input later.

source/simulation2/components/CCmpSoundManager.cpp
92

Ok nice

fatherbushido added inline comments.Sep 23 2017, 7:42 PM
source/simulation2/components/CCmpSoundManager.cpp
92

(In fact it seems that from PlayAsGroup we can nuke the entity_id_t source parameter, but that's unrelated to the patch)

fatherbushido added inline comments.Sep 23 2017, 7:50 PM
source/simulation2/components/CCmpSoundManager.cpp
92

(don't consider my previous comment)

fatherbushido accepted this revision.Sep 23 2017, 7:52 PM
This revision is now accepted and ready to land.Sep 23 2017, 7:52 PM

Need someone in O1: C++ Simulation to look at it.

Need someone in O1: C++ Simulation to look at it.

Nothing out of the ordinary here. One could forward-declare CFixedVector3D in one header, and include the header in two other files, but that doesn't make things a lot cleaner or compile a lot faster so meh.

fatherbushido added inline comments.Sep 25 2017, 2:26 PM
source/simulation2/components/CCmpSoundManager.cpp
92

That function was introduced in rP13209 and the param was tagged as unused in rP13356.
Perhaps open a ticket to know if we can get rid of that.

@leper: is it fine if I commit without O1 acceptation?
@Mate-86: I have always issue with your endline (I need to upload the raw diff and save it again with unix like line ending)

binaries/data/mods/public/simulation/components/Damage.js
107

@param {string} data.attackImpactSound - the name of the sound emited on impact.

(I can do when commiting)

@Mate-86: I have always issue with your endline (I need to upload the raw diff and save it again with unix like line ending)

I thought that a modern VCS can cope with such a challenge :) I'm generating the diff via batch file so that I can add an extra step converting the line endings.

@leper: fine if I commit it without O1 acceptance?

@leper: fine if I commit it without O1 acceptance?

Sure, the groups are there to make it easier for people submitting patches to get someone with some knowledge of the code to look at it. And we do trust team members to do their thing properly, so unless someone isn't sure or wants to require some input (in which case one should set the blocking review option) someone having reviewed it should be enough to commit it.

elexis added a subscriber: elexis.Sep 28 2017, 7:56 PM

(Approach to play some sound at a given position instead of at the position at a given entity sounds correct to me)

source/simulation2/components/CCmpSoundManager.cpp
1

2017

source/simulation2/components/ICmpSoundManager.cpp
1

2017

This revision was automatically updated to reflect the committed changes.

leper: ok!
@Mate-86: thx! I permit myself to not add the todo, instead we'll just do it. So next one?

source/simulation2/components/CCmpSoundManager.cpp
1

thx

(I opened a ticket #4798 about the unused parameter)

leper: ok!
@Mate-86: thx! I permit myself to not add the todo, instead we'll just do it. So next one?

ok, I've started to work on the projectile hit animation: https://trac.wildfiregames.com/ticket/1909