Page MenuHomeWildfire Games

Allow multiple attack sounds for units
ClosedPublic

Authored by bb on Jul 28 2017, 2:27 PM.

Details

Summary

Parallel to rP13060 change the attack related sounds so multiple attacks are allowed.

Also adding some missing sounds in templates f.e. siege tower. And impact sound for all archers (instead only infantry) and bolt shooters.

On the way too that re-order the sounds, so they are logically defined in the templates.

Fixing slaughter sounds, since currently a ranged unit makes another sound when attacking a sheep than a melee unit, using the same dagger. Both will use the sword sound after the patch.

The mauryan siege elephant (and some other units) still miss attack sounds, due to missing event tags in the actors. But that falls out of scope for this patch (see https://github.com/bb-bb/0ad/tree/attackSounds for commits fixing that and other related issues and cleanup)

Test Plan

Make sure buildings and units make sounds when attacking. Notice f.e. the siege tower makes sounds now.

grep for <attack> and <attack_impact> that all template entries are changed.
Make sure all places where the sound is played, are using the new names.

Notice the slaughter sound is correct now.

Notice there wasn't a capture sound and there isn't one now.

Change some template of melee, capture, splash and deathdamage ents and hear they can have impact sounds now.

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

bb created this revision.Jul 28 2017, 2:27 PM
Owners added a subscriber: Restricted Owners Package.Jul 28 2017, 2:27 PM
Vulcan added a subscriber: Vulcan.Jul 28 2017, 3:15 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/1767/ for more details.

bb edited the summary of this revision. (Show Details)Jul 29 2017, 4:28 PM
Stan added a reviewer: Stan.Aug 31 2017, 7:25 PM
Stan added a subscriber: Stan.

Did you make sure that there is no sound that overrides your changes to the top templates ? Or that it doesn't conflict somehow ?

The code is correct. I'm going to try it soon.

binaries/data/mods/public/simulation/components/BuildingAI.js
341 ↗(On Diff #2951)

Why to lower ?

binaries/data/mods/public/simulation/components/Damage.js
240 ↗(On Diff #2951)

Why to lower ?

binaries/data/mods/public/simulation/templates/template_structure.xml
124 ↗(On Diff #2951)

Does it make sense to have anything else that ranged that do impact ?

bb added a comment.Sep 7 2017, 10:53 AM
In D757#33565, @Stan wrote:

Did you make sure that there is no sound that overrides your changes to the top templates ? Or that it doesn't conflict somehow ?

Well iirc i changed all occurrences but one gotta check that indeed...

binaries/data/mods/public/simulation/components/BuildingAI.js
341 ↗(On Diff #2951)

This way it is similar to the animations: the attackType uses a cap ("Melee") and the animation and sound don't, so keeping consistency with that

binaries/data/mods/public/simulation/templates/template_structure.xml
124 ↗(On Diff #2951)

Well nope i guess, but in my planned changes for #252 it is allowed to have N attackTypes with arbitrary names. Thus one could have a jav attack AND a bow and arrow and both could make a different sound (f.e. a WW2 mod having explosives and just bullets crashing on the enemy).

bb updated this revision to Diff 3791.Sep 29 2017, 8:26 PM

rebase

fatherbushido added a subscriber: fatherbushido.EditedSep 29 2017, 8:47 PM

Looks ok at first reading.
It fixes current bug, allow to do thing with the current stuff (we can add sound capture, right?), and will allow to do thing with future stuff.
Imo, you can currently move all the attack and attack_impact sound (when it's possible) to parents (infantry, cavalry, champion, hero). It will in the same move fix the missing ones (for cavalry ranged hero, there were missing things iirc).
I'll review but not today.

ps: sorry for the 'things' and the 'stuff'.

edit: I didn't read the test plan about 'Notice there wasn't a capture sound and there isn't one now'. Ask perhaps @Pureon one?

binaries/data/mods/public/simulation/components/UnitAI.js
1955 ↗(On Diff #3791)

(making a variable soundName is indeed useless)

Build is green

Updating workspaces...
Updating bundled third-party dependencies...

FCollada/FCollada.cpp
FCollada/FColladaPlugin.cpp
FCollada/FCDocument/FCDAnimated.cpp
FCollada/FCDocument/FCDAnimationChannel.cpp
FCollada/FCDocument/FCDAnimationClip.cpp
FCollada/FCDocument/FCDAnimationClipTools.cpp
FCollada/FCDocument/FCDAnimation.cpp
FCollada/FCDocument/FCDAnimationCurve.cpp
FCollada/FCDocument/FCDAnimationCurveTools.cpp
FCollada/FCDocument/FCDAnimationKey.cpp
FCollada/FCDocument/FCDAnimationMultiCurve.cpp
FCollada/FCDocument/FCDAsset.cpp
FCollada/FCDocument/FCDCamera.cpp
FCollada/FCDocument/FCDController.cpp
FCollada/FCDocument/FCDControllerInstance.cpp
FCollada/FCDocument/FCDControllerTools.cpp
FCollada/FCDocument/FCDEffectCode.cpp
FCollada/FCDocument/FCDEffect.cpp
FCollada/FCDocument/FCDEffectParameter.cpp
FCollada/FCDocument/FCDEffectParameterFactory.cpp
FCollada/FCDocument/FCDEffectParameterSampler.cpp
FCollada/FCDocument/FCDEffectParameterSurface.cpp
FCollada/FCDocument/FCDEffectPass.cpp
FCollada/FCDocument/FCDEffectPassShader.cpp
FCollada/FCDocument/FCDEffectPassState.cpp
FCollada/FCDocument/FCDEffectProfile.cpp
FCollada/FCDocument/FCDEffectProfileFX.cpp
FCollada/FCDocument/FCDEffectStandard.cpp
FCollada/FCDocument/FCDEffectTechnique.cpp
FCollada/FCDocument/FCDEffectTools.cpp
FCollada/FCDocument/FCDEmitter.cpp
FCollada/FCDocument/FCDEmitterInstance.cpp
FCollada/FCDocument/FCDEmitterObject.cpp
FCollada/FCDocument/FCDEmitterParticle.cpp
FCollada/FCDocument/FCDEntity.cpp
FCollada/FCDocument/FCDEntityInstance.cpp
FCollada/FCDocument/FCDEntityReference.cpp
FCollada/FCDocument/FCDExternalReferenceManager.cpp
FCollada/FCDocument/FCDExtra.cpp
FCollada/FCDocument/FCDForceDeflector.cpp
FCollada/FCDocument/FCDForceDrag.cpp
FCollada/FCDocument/FCDForceField.cpp
FCollada/FCDocument/FCDForceGravity.cpp
FCollada/FCDocument/FCDForcePBomb.cpp
FCollada/FCDocument/FCDForceWind.cpp
FCollada/FCDocument/FCDGeometry.cpp
FCollada/FCDocument/FCDGeometryInstance.cpp
FCollada/FCDocument/FCDGeometryMesh.cpp
FCollada/FCDocument/FCDGeometryNURBSSurface.cpp
FCollada/FCDocument/FCDGeometryPolygons.cpp
FCollada/FCDocument/FCDGeometryPolygonsInput.cpp
FCollada/FCDocument/FCDGeometryPolygonsTools.cpp
FCollada/FCDocument/FCDGeometrySource.cpp
FCollada/FCDocument/FCDGeometrySpline.cpp
FCollada/FCDocument/FCDImage.cpp
FCollada/FCDocument/FCDLibrary.cpp
FCollada/FCDocument/FCDLight.cpp
FCollada/FCDocument/FCDLightTools.cpp
FCollada/FCDocument/FCDMaterial.cpp
FCollada/FCDocument/FCDMaterialInstance.cpp
FCollada/FCDocument/FCDMorphController.cpp
FCollada/FCDocument/FCDObject.cpp
FCollada/FCDocument/FCDObjectWithId.cpp
FCollada/FCDocument/FCDocument.cpp
FCollada/FCDocument/FCDocumentTools.cpp
FCollada/FCDocument/FCDParameterAnimatable.cpp
FCollada/FCDocument/FCDParticleModifier.cpp
FCollada/FCDocument/FCDPhysicsAnalyticalGeometry.cpp
FCollada/FCDocument/FCDPhysicsForceFieldInstance.cpp
FCollada/FCDocument/FCDPhysicsMaterial.cpp
FCollada/FCDocument/FCDPhysicsModel.cpp
FCollada/FCDocument/FCDPhysicsModelInstance.cpp
FCollada/FCDocument/FCDPhysicsRigidBody.cpp
FCollada/FCDocument/FCDPhysicsRigidBodyInstance.cpp
FCollada/FCDocument/FCDPhysicsRigidBodyParameters.cpp
FCollada/FCDocument/FCDPhysicsRigidConstraint.cpp
FCollada/FCDocument/FCDPhysicsRigidConstraintInstance.cpp
FCollada/FCDocument/FCDPhysicsScene.cpp
FCollada/FCDocument/FCDPhysicsShape.cpp
FCollada/FCDocument/FCDPlaceHolder.cpp
FCollada/FCDocument/FCDSceneNode.cpp
FCollada/FCDocument/FCDSceneNodeIterator.cpp
FCollada/FCDocument/FCDSceneNodeTools.cpp
FCollada/FCDocument/FCDSkinController.cpp
FCollada/FCDocument/FCDTargetedEntity.cpp
FCollada/FCDocument/FCDTexture.cpp
FCollada/FCDocument/FCDTransform.cpp
FCollada/FCDocument/FCDVersion.cpp
FCollada/FMath/FMAllocator.cpp
FCollada/FMath/FMAngleAxis.cpp
FCollada/FMath/FMColor.cpp
FCollada/FMath/FMInterpolation.cpp
FCollada/FMath/FMLookAt.cpp
FCollada/FMath/FMMatrix33.cpp
FCollada/FMath/FMMatrix44.cpp
FCollada/FMath/FMQuaternion.cpp
FCollada/FMath/FMRandom.cpp
FCollada/FMath/FMSkew.cpp
FCollada/FMath/FMVector3.cpp
FCollada/FMath/FMVolume.cpp
FCollada/FUtils/FUAssert.cp

http://jenkins-master:8080/job/phabricator/2079/ for more details.

Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/components/BuildingAI.js
| 125| »   if·(enemies.length·&&·enemies[0]·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/simulation/components/BuildingAI.js
| 295| »   if·(this.currentRound·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 196| »   let·wantedTypesReal·=·wantedTypes.filter(wtype·=>·wtype.indexOf("!")·!=·0);
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 236| »   ···(!wantedTypes·||·!wantedTypes.filter(wType·=>·wType.indexOf("!")·!=·0).length))
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 526| »   »   cmpTimer.SetTimeout(SYSTEM_ENTITY,·IID_Damage,·"MissileHit",·timeToTarget·*·1000·+·+this.template.Ranged.Delay,·data);
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.

binaries/data/mods/public/simulation/components/Attack.js
| 535| »   »   if·(!cmpHealth·||·cmpHealth.GetHitpoints()·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 587| »   if·(c·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.
|    | [NORMAL] ESLintBear (no-else-return):
|    | Unnecessary 'else' after 'return'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
| 911| 911| 					this.FinishOrder();
| 912| 912| 					return;
| 913| 913| 				}
| 914|    |-				else
| 915|    |-				{
|    | 914|+				
| 916| 915| 					// Out of range; move there in formation
| 917| 916| 					if (this.MoveToGarrisonRange(msg.data.target))
| 918| 917| 					{
| 919| 918| 						this.SetNextState("GARRISON.APPROACHING");
| 920| 919| 						return;
| 921| 920| 					}
| 922|    |-				}
|    | 921|+				
| 923| 922| 			}
| 924| 923| 
| 925| 924| 			this.SetNextState("GARRISON.GARRISONING");
|    | [NORMAL] ESLintBear (no-unneeded-ternary):
|    | Unnecessary use of boolean literals in conditional expression.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|1958|1958| 					// TODO: we should probably only bother syncing projectile attacks, not melee
|1959|1959| 
|1960|1960| 					// If using a non-default prepare time, re-sync the animation when the timer runs.
|1961|    |-					this.resyncAnimation = (prepare != this.attackTimers.prepare) ? true : false;
|    |1961|+					this.resyncAnimation = (prepare != this.attackTimers.prepare);
|1962|1962| 
|1963|1963| 					this.FaceTowardsTarget(this.order.data.target);
|1964|1964| 
|    | [NORMAL] ESLintBear (no-else-return):
|    | Unnecessary 'else' after 'return'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|2184|2184| 							this.PerformGather(nearby, false, false);
|2185|2185| 							return true;
|2186|2186| 						}
|2187|    |-						else
|2188|    |-						{
|    |2187|+						
|2189|2188| 							// It's probably better in this case, to avoid units getting stuck around a dropsite
|2190|2189| 							// in a "Target is far away, full, nearby are no good resources, return to dropsite" loop
|2191|2190| 							// to order it to GatherNear the resource position.
|2206|2205| 									return true;
|2207|2206| 								}
|2208|2207| 							}
|2209|    |-						}
|    |2208|+

http://jenkins-master:8080/job/phabricator_lint/555/ for more details.

bb updated this revision to Diff 3829.Oct 1 2017, 5:31 PM

move some impacts to parents

Vulcan added a comment.Oct 1 2017, 6:08 PM

Build is green

Updating workspaces...
Updating bundled third-party dependencies...


SpiderMonkey is already up to date


make: Entering directory '/mnt/data/jenkins-phabricator/workspace/phabricator/build/premake/premake4/build/gmake.unix'
==== Building Premake4 (release) ====
make: Leaving directory '/mnt/data/jenkins-phabricator/workspace/phabricator/build/premake/premake4/build/gmake.unix'

Premake args:  --with-system-nvtt --without-miniupnpc --collada --atlas
Building configurations...
Running action 'gmake'...
Generating ../workspaces/gcc/Makefile...
Generating ../workspaces/gcc/pyrogenesis.make...
Generating ../workspaces/gcc/network.make...
Generating ../workspaces/gcc/tinygettext.make...
Generating ../workspaces/gcc/lobby.make...
Generating ../workspaces/gcc/glooxwrapper.make...
Generating ../workspaces/gcc/simulation2.make...
Generating ../workspaces/gcc/scriptinterface.make...
Generating ../workspaces/gcc/engine.make...
Generating ../workspaces/gcc/graphics.make...
Generating ../workspaces/gcc/atlas.make...
Generating ../workspaces/gcc/gui.make...
Generating ../workspaces/gcc/lowlevel.make...
Generating ../workspaces/gcc/mongoose.make...
Generating ../workspaces/gcc/mocks_real.make...
Generating ../workspaces/gcc/mocks_test.make...
Generating ../workspaces/gcc/AtlasObject.make...
Generating ../workspaces/gcc/AtlasUI.make...
Generating ../workspaces/gcc/ActorEditor.make...
Generating ../workspaces/gcc/Collada.make...
Generating ../workspaces/gcc/test.make...
Done.
Building configurations...
Running action 'codeblocks'...
Generating ../workspaces/codeblocks/pyrogenesis.workspace...
Generating ../workspaces/codeblocks/pyrogenesis.cbp...
Generating ../workspaces/codeblocks/network.cbp...
Generating ../workspaces/codeblocks/tinygettext.cbp...
Generating ../workspaces/codeblocks/lobby.cbp...
Generating ../workspaces/codeblocks/glooxwrapper.cbp...
Generating ../workspaces/codeblocks/simulation2.cbp...
Generating ../workspaces/codeblocks/scriptinterface.cbp...
Generating ../workspaces/codeblocks/engine.cbp...
Generating ../workspaces/codeblocks/graphics.cbp...
Generating ../workspaces/codeblocks/atlas.cbp...
Generating ../workspaces/codeblocks/gui.cbp...
Generating ../workspaces/codeblocks/lowlevel.cbp...
Generating ../workspaces/codeblocks/mongoose.cbp...
Generating ../workspaces/codeblocks/mocks_real.cbp...
Generating ../workspaces/codeblocks/mocks_test.cbp...
Generating ../workspaces/codeblocks/AtlasObject.cbp...
Generating ../workspaces/codeblocks/AtlasUI.cbp...
Generating ../workspaces/codeblocks/ActorEditor.cbp...
Generating ../workspaces/codeblocks/Collada.cbp...
Generating ../workspaces/codeblocks/test.cbp...
Done.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/2093/ for more details.

Vulcan added a comment.Oct 1 2017, 6:09 PM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/components/BuildingAI.js
| 125| »   if·(enemies.length·&&·enemies[0]·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/simulation/components/BuildingAI.js
| 295| »   if·(this.currentRound·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 196| »   let·wantedTypesReal·=·wantedTypes.filter(wtype·=>·wtype.indexOf("!")·!=·0);
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 236| »   ···(!wantedTypes·||·!wantedTypes.filter(wType·=>·wType.indexOf("!")·!=·0).length))
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 526| »   »   cmpTimer.SetTimeout(SYSTEM_ENTITY,·IID_Damage,·"MissileHit",·timeToTarget·*·1000·+·+this.template.Ranged.Delay,·data);
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.

binaries/data/mods/public/simulation/components/Attack.js
| 535| »   »   if·(!cmpHealth·||·cmpHealth.GetHitpoints()·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 587| »   if·(c·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.
|    | [NORMAL] ESLintBear (no-else-return):
|    | Unnecessary 'else' after 'return'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
| 911| 911| 					this.FinishOrder();
| 912| 912| 					return;
| 913| 913| 				}
| 914|    |-				else
| 915|    |-				{
|    | 914|+				
| 916| 915| 					// Out of range; move there in formation
| 917| 916| 					if (this.MoveToGarrisonRange(msg.data.target))
| 918| 917| 					{
| 919| 918| 						this.SetNextState("GARRISON.APPROACHING");
| 920| 919| 						return;
| 921| 920| 					}
| 922|    |-				}
|    | 921|+				
| 923| 922| 			}
| 924| 923| 
| 925| 924| 			this.SetNextState("GARRISON.GARRISONING");
|    | [NORMAL] ESLintBear (no-unneeded-ternary):
|    | Unnecessary use of boolean literals in conditional expression.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|1958|1958| 					// TODO: we should probably only bother syncing projectile attacks, not melee
|1959|1959| 
|1960|1960| 					// If using a non-default prepare time, re-sync the animation when the timer runs.
|1961|    |-					this.resyncAnimation = (prepare != this.attackTimers.prepare) ? true : false;
|    |1961|+					this.resyncAnimation = (prepare != this.attackTimers.prepare);
|1962|1962| 
|1963|1963| 					this.FaceTowardsTarget(this.order.data.target);
|1964|1964| 
|    | [NORMAL] ESLintBear (no-else-return):
|    | Unnecessary 'else' after 'return'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/UnitAI.js
|2184|2184| 							this.PerformGather(nearby, false, false);
|2185|2185| 							return true;
|2186|2186| 						}
|2187|    |-						else
|2188|    |-						{
|    |2187|+						
|2189|2188| 							// It's probably better in this case, to avoid units getting stuck around a dropsite
|2190|2189| 							// in a "Target is far away, full, nearby are no good resources, return to dropsite" loop
|2191|2190| 							// to order it to GatherNear the resource position.
|2206|2205| 									return true;
|2207|2206| 								}
|2208|2207| 							}
|2209|    |-						}
|    |2208|+

http://jenkins-master:8080/job/phabricator_lint/566/ for more details.

I wonder if attack_impact make senses for all the kind of attack?

binaries/data/mods/public/simulation/components/Attack.js
555 ↗(On Diff #3829)

If we want attack_impact melee sound, add it here

Stan added a comment.Oct 4 2017, 8:36 AM

Mêlée makes sense for slashing sounds ranged too. Capture could make sense if it's some kind of cheering and breaking glass sounds.

In D757#37001, @Stan wrote:

Mêlée makes sense for slashing sounds ranged too. Capture could make sense if it's some kind of cheering and breaking glass sounds.

(I was speaking of attack_impact_foo sounds and not of attack_foo sounds.)

Stan added a comment.Oct 4 2017, 2:56 PM

In this case maybe it would work for hybrid attacks, like AOE attacks.

fatherbushido added a comment.EditedOct 4 2017, 3:01 PM
In D757#37005, @Stan wrote:

In this case maybe it would work for hybrid attacks, like AOE attacks.

What are you talking about?

  • @bb did a good patch to allow attack_type sounds.
  • he also changed the templates for attack_impact_type. The current attack_impact sounds is only emit for range attack (one of my previous commit). I asked if attack_impact_type is relevant or is only a ranged sound. (In the first case, if we want it for melee, I pointed out where we can add it in the code).

Edit:
I see the kind of thing you mean.

bb added a comment.Oct 4 2017, 8:34 PM

The idea in my implementation for the secondary attack is that attack types can have arbitrary names (so having a jav and a bow is allowed and stuff), and those imo can have different attack sound. Making that code general, leaves me allowing the impact sounds settable for melee only units. However making them actually heard when set, we need to change a bit in damage.js. Since I planned those changes already they are included now. For the capture I just hacked them into attack.js, that will probably change when doing the secondary attacks.

bb updated this revision to Diff 3842.Oct 4 2017, 8:36 PM
bb edited the test plan for this revision. (Show Details)

Add support for melee and 'capture' attacks, by moving some logic out of the ranged only function to the general function to avoid duplication.

Vulcan added a comment.Oct 4 2017, 9:25 PM

Build is green

Updating workspaces...
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimation]’:
FCollada/FCDocument/FCDLibrary.cpp:149:30:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
  const T* cptr = ((const FCDLibrary<T>*)l1)->GetEntity(0);
           ^
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimationClip]’:
FCollada/FCDocument/FCDLibrary.cpp:150:34:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDCamera]’:
FCollada/FCDocument/FCDLibrary.cpp:151:27:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDController]’:
FCollada/FCDocument/FCDLibrary.cpp:152:31:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEffect]’:
FCollada/FCDocument/FCDLibrary.cpp:153:27:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEmitter]’:
FCollada/FCDocument/FCDLibrary.cpp:154:28:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDForceField]’:
FCollada/FCDocument/FCDLibrary.cpp:155:31:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDGeometry]’:
FCollada/FCDocument/FCDLibrary.cpp:156:29:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDImage]’:
FCollada/FCDocument/FCDLibrary.cpp:157:26:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDLight]’:
FCollada/FCDocument/FCDLibrary.cpp:158:26:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDMaterial]’:
FCollada/FCDocument/FCDLibrary.cpp:159:29:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDSceneNode]’:
FCollada/FCDocument/FCDLibrary.cpp:160:30:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsModel]’:
FCollada/FCDocument/FCDLibrary.cpp:161:33:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsMaterial]’:
FCollada/FCDocument/FCDLibrary.cpp:162:36:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ s

http://jenkins-master:8080/job/phabricator/2102/ for more details.

Vulcan added a comment.Oct 4 2017, 9:27 PM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/components/BuildingAI.js
| 125| »   if·(enemies.length·&&·enemies[0]·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/simulation/components/BuildingAI.js
| 295| »   if·(this.currentRound·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 196| »   let·wantedTypesReal·=·wantedTypes.filter(wtype·=>·wtype.indexOf("!")·!=·0);
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 236| »   ···(!wantedTypes·||·!wantedTypes.filter(wType·=>·wType.indexOf("!")·!=·0).length))
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 527| »   »   cmpTimer.SetTimeout(SYSTEM_ENTITY,·IID_Damage,·"MissileHit",·timeToTarget·*·1000·+·+this.template.Ranged.Delay,·data);
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.

binaries/data/mods/public/simulation/components/Attack.js
| 536| »   »   if·(!cmpHealth·||·cmpHealth.GetHitpoints()·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 594| »   if·(c·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/simulation/components/UnitAI.js
| 363| »   »   ····&&·(this.lastShorelinePosition.z·==·cmpPosition.GetPosition().z))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|1951| »   »   »   »   »   »   var·cmpFormation·=·Engine.QueryInterface(this.formationController,·IID_Formation);
|    | [NORMAL] JSHintBear:
|    | 'cmpFormation' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|2177| »   »   »   »   »   »   »   »   ·&&·((type.generic·==·"treasure"·&&·oldType.generic·==·"treasure")
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|2178| »   »   »   »   »   »   »   »   ·||·(type.specific·==·oldType.specific
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|2179| »   »   »   »   »   »   »   »   ·&&·(type.specific·!=·"meat"·||·oldTemplate·==·template)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|2202| »   »   »   »   »   »   »   »   var·nearby·=·this.FindNearestDropsite(oldType.generic);
|    | [NORMAL] JSHintBear:
|    | 'nearby' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|2242| »   »   »   »   »   »   »   »   &&·((type.generic·==·"treasure"·&&·oldType.generic·==·"treasure")
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|2243| »   »   »   »   »   »   »   »   ||·(type.specific·==·oldType.specific
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|2244| »   »   »   »   »   »   »   »   &&·(type.specific·!=·"meat"·||·oldTemplate·==·template)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|2288| »   »   »   »   »   »   »

http://jenkins-master:8080/job/phabricator_lint/575/ for more details.

fatherbushido added a comment.EditedOct 4 2017, 10:02 PM

@bb : I didn't request that at all, my pov was that attack_impact sound are a bit meaningless for other attacks so you don't add to add that code :) For example for capture, you would hear the capture attack sound (currently there are none I think) then you will immediatly hear on each tick a capture attack impact sound, that 'sounds' really weird. I have a similar doubt for the melee attack. That was just something I was thinking about (should we change attack_impact to attack_impact_type?) But your patch, your choice ;-)
So: I will review/'accept' version (n-1) or that one or version (n-1) withtout attack_impact_type (my own pref is 3).

edit: I missed one comment (the bow and jav thing). So, some things above are wrong.

bb added a comment.Oct 4 2017, 10:35 PM

agree for capture and melee the sounds being useless, but I do want to allow different sounds for different ranged attacks like a jav makes another sound than an archer's arrow. But for that we do need the type in the soundgroup, ofc we need some more code, and I plan on doing that. Regarding those planned changes it doesn't really matter if this one gets committed or n-1 (with the _impact_type), so probably n-1 is a better choice.

Ok for n-1 (with attack_impact_type).
Unrelated to this patch, for your bow and javelin thing, you mean you would have Ranged_Bow and Ranged_Javelin and something like a start with "Ranged" in the PerformAttack code?

bb added a comment.Oct 6 2017, 12:42 PM

Not exactly just "Javelin" or "Archer" would be enough (but putting "Ranged" before is allowed, but it is only a name). What I have in patch is merging the current 4 attack types into 1, which have all the features we have now and even some more, f.e. we can have projectiles with capture "damage" (as in @Stan suggestion for throwing rocks to break windows in capture). Probably its a bit vague what I mean here, but I will come up with a patch in a bit (mostly need to rebase).

In D757#37037, @bb wrote:

Not exactly just "Javelin" or "Archer" would be enough (but putting "Ranged" before is allowed, but it is only a name). What I have in patch is merging the current 4 attack types into 1, which have all the features we have now and even some more, f.e. we can have projectiles with capture "damage" (as in @Stan suggestion for throwing rocks to break windows in capture). Probably its a bit vague what I mean here, but I will come up with a patch in a bit (mostly need to rebase).

In a fancy unit:
Javelin -> Ranged -> stuff
Bow -> Ranged -> stuff
Punch -> Melee -> stuff

That's it?

bb added a comment.Oct 6 2017, 5:57 PM

sorta, yes

bb added a comment.Oct 24 2017, 7:20 PM

@fatherbushido mind to click accept for the n-1 state? (when its reviewed ofc)

In D757#38226, @bb wrote:

@fatherbushido mind to click accept for the n-1 state? (when its reviewed ofc)

sure, I was waiting (could I accept a previous diff?)

bb updated this revision to Diff 3957.Oct 24 2017, 10:46 PM

n-1+=2

bb added a comment.Oct 24 2017, 10:46 PM

Well lets port that question to the "Using phab" forum for now I upload the old diff again.

Build is green

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

http://jenkins-master:8080/job/phabricator/2158/ for more details.

Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/components/BuildingAI.js
| 125| »   if·(enemies.length·&&·enemies[0]·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/simulation/components/BuildingAI.js
| 295| »   if·(this.currentRound·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 196| »   let·wantedTypesReal·=·wantedTypes.filter(wtype·=>·wtype.indexOf("!")·!=·0);
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 236| »   ···(!wantedTypes·||·!wantedTypes.filter(wType·=>·wType.indexOf("!")·!=·0).length))
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 526| »   »   cmpTimer.SetTimeout(SYSTEM_ENTITY,·IID_Damage,·"MissileHit",·timeToTarget·*·1000·+·+this.template.Ranged.Delay,·data);
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.

binaries/data/mods/public/simulation/components/Attack.js
| 535| »   »   if·(!cmpHealth·||·cmpHealth.GetHitpoints()·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/simulation/components/Attack.js
| 587| »   if·(c·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/simulation/components/UnitAI.js
| 363| »   »   ····&&·(this.lastShorelinePosition.z·==·cmpPosition.GetPosition().z))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|1923| »   »   »   »   »   »   var·cmpFormation·=·Engine.QueryInterface(this.formationController,·IID_Formation);
|    | [NORMAL] JSHintBear:
|    | 'cmpFormation' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|2149| »   »   »   »   »   »   »   »   ·&&·((type.generic·==·"treasure"·&&·oldType.generic·==·"treasure")
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|2150| »   »   »   »   »   »   »   »   ·||·(type.specific·==·oldType.specific
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|2151| »   »   »   »   »   »   »   »   ·&&·(type.specific·!=·"meat"·||·oldTemplate·==·template)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|2174| »   »   »   »   »   »   »   »   var·nearby·=·this.FindNearestDropsite(oldType.generic);
|    | [NORMAL] JSHintBear:
|    | 'nearby' is already defined.

binaries/data/mods/public/simulation/components/UnitAI.js
|2214| »   »   »   »   »   »   »   »   &&·((type.generic·==·"treasure"·&&·oldType.generic·==·"treasure")
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|2215| »   »   »   »   »   »   »   »   ||·(type.specific·==·oldType.specific
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|2216| »   »   »   »   »   »   »   »   &&·(type.specific·!=·"meat"·||·oldTemplate·==·template)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/UnitAI.js
|2260| »   »   »   »   »   »   »

http://jenkins-master:8080/job/phabricator_lint/626/ for more details.

fatherbushido accepted this revision.Oct 27 2017, 9:36 AM

I didn't checked again all templates (unrelated: if we want to look deeper, there is perhaps a lot of polishing work for sounds)

This revision is now accepted and ready to land.Oct 27 2017, 9:36 AM
This revision was automatically updated to reflect the committed changes.