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

Add Aircraft Takeoff & Landing Sounds (Planes) #15624

Merged
merged 1 commit into from Oct 12, 2018

Conversation

Projects
None yet
5 participants
@Inq8
Copy link
Contributor

Inq8 commented Sep 18, 2018

Added Takeoff & Landing sounds to planes.

Changed Aircraft Trait, TakeoffSound & LandingSound are now arrays & will accept a list of sound files.
It will randomly select one to play.

Changed/Fixed, take off & landing sounds to originate from the aircraft location, rather than play a global sound.

Only affects the offical TS mod, useful for other mods.

@abcdefg30
Copy link
Member

abcdefg30 left a comment

Code looks fine and test case works.

@@ -22,6 +22,7 @@ public class Fly : Activity
readonly Target target;
readonly WDist maxRange;
readonly WDist minRange;
bool playedSound;

This comment has been minimized.

@teinarss

teinarss Sep 22, 2018

Contributor

nitpick: soundPlayed would be more consistent.

This comment has been minimized.

@Inq8

Inq8 Sep 25, 2018

Contributor

playedSound is consistent with the exisiting TakeOff & Landing sounds for helicopers in both, HeliFly.cs & HeliLand.cs.

However, I could edit all four files if required for consistency, if it reads better? (HeliFly, Fly, HeliLand, Land)

@@ -20,6 +20,8 @@ public class Land : Activity
readonly Target target;
readonly Aircraft aircraft;

bool playedSound;

This comment has been minimized.

@teinarss

teinarss Sep 22, 2018

Contributor

same here.

This comment has been minimized.

@Inq8

Inq8 Sep 25, 2018

Contributor

As above.

@pchote
Copy link
Member

pchote left a comment

The main changes here look reasonable, but there is one major caveat from changing the yaml interface:

[Desc("Sound to play when the actor is taking off.")]
public readonly string TakeoffSound = null;
[Desc("Sounds to play when the actor is taking off.")]
public readonly string[] TakeoffSound = { };

This comment has been minimized.

@pchote

pchote Sep 29, 2018

Member

Unfortunately our naming conventions mean that these will need to change to TakeoffSounds and LandingSounds, with an update rule to automate the conversion.

You can use ChangeIntensityToDuration.cs as an example to base this on.

@Inq8

This comment has been minimized.

Copy link
Contributor

Inq8 commented Oct 1, 2018

I've made the requested changes & added an update rule. (I think its right?)

Plus I renamed the existing TakeOffSound & LandingSound parameters in TS Aircraft.yaml

@@ -586,6 +586,7 @@
<Compile Include="Traits\World\WeatherOverlay.cs" />
<Compile Include="Traits\World\ActorSpawnManager.cs" />
<Compile Include="Traits\ActorSpawner.cs" />
<Compile Include="UpdateRules\Rules\20180923\ChangeTakeOffSoundAndLandingSound.cs" />

This comment has been minimized.

@abcdefg30

abcdefg30 Oct 1, 2018

Member

Please revert all other changes to this file and add this change to the last commit. (Otherwise AppVeyor errors as this file is not present in the second commit.)

foreach (var tos in actorNode.ChildrenMatching("Aircraft"))
tos.RenameChildrenMatching("TakeOffSound", "TakeOffSounds");

foreach (var lds in actorNode.ChildrenMatching("Aircraft"))

This comment has been minimized.

@abcdefg30

abcdefg30 Oct 1, 2018

Member

Please combine both those loops into one, like for example

foreach (var aircraft in actorNode.ChildrenMatching("Aircraft"))
{
	aircraft.RenameChildrenMatching("TakeOffSound", "TakeOffSounds");
	aircraft.RenameChildrenMatching("LandingSound", "LandingSounds");
}

This comment has been minimized.

@Inq8

Inq8 Oct 1, 2018

Contributor

I tried doing that and it won't compile because the variable is already used once.
I don't know how else to write it?

This comment has been minimized.

@reaperrr

reaperrr Oct 3, 2018

Contributor

@Inq8: using @abcdefg30's code, I didn't get any compile errors. Please re-check your local code, or just replace it with the above.

Once that works, it would be great if you could squash/fixup all the update commits into the first, I think then the PR would be ready to merge.

Aircraft Takeoff & Landing Sounds (Fixed-Wing)
Added Takeoff & Landing sounds to planes.

Changed Aircraft Trait, TakeoffSounds & LandingSounds are now arrays & accept a list of sound files & it will randomly select one to play.

Changed/fixed take off & landing sounds to originate from the aircraft location, rather than play a global sound.
@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Oct 12, 2018

@Inq8 Looks like something went wrong with your rebase, and there were a few remaining style issues as well.

I've taken the liberty to fix the PR so we can merge it without further delay, hope you don't mind. Thanks for your contribution, planes not supporting these sounds was a really stupid limitation.

@reaperrr reaperrr merged commit fec9fe1 into OpenRA:bleed Oct 12, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Oct 12, 2018

@Inq8

This comment has been minimized.

Copy link
Contributor

Inq8 commented Oct 12, 2018

Thanks for the assistance @reaperrr. I tried to fix this yesterday but I screwed up somewhere... Still learning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment