Skip to content
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

Added helicopter takeoff and landing sound #8784

Merged
merged 2 commits into from Aug 4, 2015

Conversation

Mailaender
Copy link
Member

Used for Tiberian Sun dropships.

@@ -35,6 +35,7 @@ public HeliFly(Actor self, Target t, WDist minRange, WDist maxRange)
this.minRange = minRange;
}

static int previousdz;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making this static shares it over all helicopters!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woops!

@pchote
Copy link
Member

pchote commented Jul 25, 2015

Please change this to explicitly check for takeoff and landing. Attaching the logic to any height changes will make the sounds play when the helicopters move over bumpy terrain (after #7904 or a similar pr fixes aircraft height).

@Mailaender
Copy link
Member Author

I tied this together differently now. The main problem is that helicopters never properly fire the TakeOff activity. Not sure why. Also Aircraft does not expose Info and making it public would hide stuff.

@@ -53,6 +54,12 @@ public override Activity Tick(Actor self)
if (IsCanceled || !target.IsValidFor(self))
return NextActivity;

if (!playedSound && helicopter.Info.TakeoffSound != null && helicopter.CenterPosition.Z == helicopter.Info.LandAltitude.Length)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be a lot clearer if it uses LandAltitude.Z instead of LandAltitude.Length, which is the same, because X and Y are supposed to be 0.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LandAltitude is WDist (1D so there is no X, Y or Z)

@penev92
Copy link
Member

penev92 commented Jul 27, 2015

I'd say we're not ready for this (the aircraft needs a lot of work before this can work properly), but maybe we choose the "complete solution to this and related issues" path way too often.
This adds a nice effect (although incomplete) and will serve as a reminder so these don't get forgotten.
I'd also say this should go to Aircraft (you can still either expose its info or cast the helicopter's info), but I don't think we have airplane use-cases for now, so meh.

In summary: I'm not a huge fan of the way this is implemented, but I don't see an alternative without asking you to refactor the entire aircraft code. So I'll go with "better have it and polish later" this time 👍

@Mailaender
Copy link
Member Author

Your suggestions are very vague, but I already tried those (sticking it to Aircraft or Helicopter and other methods). The places I chose in the end seem to be the best in the current code base. Otherwise the sound gets fired too often (or needs boolean guards to suppress it) or it doesn't properly triggered.

@abcdefg30
Copy link
Member

14:27 (abcdefg30) i mean, when you produce an aircraft, should the takeoff sound be played?
14:31 (+antares79) if it takes off, then sure

The takeoff sound is also played when loading units off the orca transport.

@@ -144,12 +146,19 @@ ORCATRAN:
Cost: 1200
Tooltip:
Name: Orca Transport
Buildable:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated to takeoff and landing sounds, so needs to be at least in its own commit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separated.

@abcdefg30
Copy link
Member

The bug with unloading is gone now, but no sound is played when units take off after being produced.

@penev92
Copy link
Member

penev92 commented Aug 1, 2015

OK so I have helicopter.CenterPosition.Z == helicopter.Info.LandAltitude.Length -> 1024 == 0, which is False for obvious reasons, so the sound isn't played.
Perhaps using the newly-introduced (today) IsAtGroundLevel() instead would fix that.

@Mailaender
Copy link
Member Author

Updated to make use of the new function from #8866.

@pchote
Copy link
Member

pchote commented Aug 1, 2015

That may not have been the best idea because aircraft remain completely incompatible with terrain height. I'll be surprised if IsAtGroundLevel works for them.

@penev92
Copy link
Member

penev92 commented Aug 1, 2015

IMO this PR has done all it can with the current aircraft code. The sounds are played when produced (once, ofc) and when landing (every time). This is good enough for me until we have a fix for the aircraft's apparently broken self.CenterPosition.Z.

@abcdefg30
Copy link
Member

Looks good to me. 👍 / ✅

abcdefg30 added a commit that referenced this pull request Aug 4, 2015
Added helicopter takeoff and landing sound
@abcdefg30 abcdefg30 merged commit 582c93d into OpenRA:bleed Aug 4, 2015
@abcdefg30
Copy link
Member

Changelog

@Mailaender Mailaender deleted the heli-drop-sounds branch August 4, 2015 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants