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

[RA] RevealOnDeath with Aircraft #13878

Merged
merged 1 commit into from Sep 2, 2017
Merged

[RA] RevealOnDeath with Aircraft #13878

merged 1 commit into from Sep 2, 2017

Conversation

SoScared
Copy link
Member

@SoScared SoScared commented Aug 20, 2017

Taking advantage of #12999 this PR would add a short duration of vision with aircraft crashes.

Closes #13864

One issue out of my reach is a small time-gap between the disappearance of the dying unit and its added RevealOnDeath vision. This makes the vision "flicker" when the trait is activated. This happen with any unit the trait is assigned to.

Also if there is a way of deactivating the trait on landed aircrafts that could eliminate the effect when the units are on ground. At least within the content of this PR it would be more consistent.

*edit: set 8c0 vision range to Transport.Husk as with the Transport unit itself. Overlooked by the previous balance PR

@pchote
Copy link
Member

pchote commented Aug 20, 2017

Also if there is a way of deactivating the trait on landed aircrafts that could eliminate the effect when the units are on ground. At least within the content of this PR it would be more consistent.

This can be done by adding RequiresCondition: airborne to the trait.

@SoScared
Copy link
Member Author

Adding RequiresCondition: Airborne to ^PlaneHusk and ^HelicopterHusk negates the effect altogether with all aircraft. Perhaps because e.g. YAK.Husk: is somehow defined as being on ground?

@GraionDilach
Copy link
Contributor

Aircraft husks doesn't have the Aircraft trait and aren't considered as aircrafts. They are defined above ground, it's ust that no trait provides the condition. Reasons like that (+ handling targettype switching unison with optional paradrop + my planned eventual magnetron lift) are why I added a GrantConditionAboveAltitude trait into AS.

@SoScared
Copy link
Member Author

SoScared commented Aug 21, 2017

Ah ok, thanks.

So there's currently no way of removing the trait on grounded air units.

For the attacking aircraft this has little implications as they will always die grounded on a structure already providing vision. For the Chinook the trait might come off as awkward as the unit can land and be killed off anywhere. If this trait was part of a broader change adding RevealOnDeath to all units in-game there would be no problem but with this PR as-is, it is sort of an anomaly.

@@ -95,11 +95,11 @@ TRAN.Husk:
Offset: 597,0,213
Sequence: rotor2
RevealsShroud:
Range: 10c0
Range: 8c0
Copy link
Member Author

@SoScared SoScared Aug 21, 2017

Choose a reason for hiding this comment

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

8c0 vision range (and the associated RevealsShroud@GAPGEN) to Transport.Husk as with the Transport unit itself. Overlooked by the previous balance PR

@@ -733,6 +736,9 @@ QTNK:
RevealGeneratedShroud: False
RevealsShroud@GAPGEN:
Range: 4c0
RevealOnDeath:
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of having this universally enabled - if someone kills a QTNK when it's driving around it should behave the same way as any other killed unit. A simple fix would be to enable RevealOnDeath for everything.

Copy link
Member Author

@SoScared SoScared Aug 22, 2017

Choose a reason for hiding this comment

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

Oh yeah. No that's not indented at all.

I'd rather not overreach with this PR tho. Seem like a few things need to be sorted out to extend the feature over to a wider range of units as well. Perhaps just introduce this feature to a few exclusive units - aircrafts (excluding Chinook) and the demo truck? With those units the trait is specifically useful and doesn't have any noticeable trade-offs.

Copy link
Member

@pchote pchote Aug 22, 2017

Choose a reason for hiding this comment

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

We could modify the MadTank trait (and also AttackSuicides for the demo truck) to grant a condition immediately before they die. This could be used to enable the reveal (and possibly other effects?) only when they are deliberately detonated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if that's what you said with the above, but it would be a nice detail if the Nuke Truck provided death vision only when deliberately triggered/deployed.

@SoScared
Copy link
Member Author

SoScared commented Aug 24, 2017

If #13891 goes through, it solves (removes) the aircraft RevealOnDeath vision while grounded.

@SoScared
Copy link
Member Author

SoScared commented Sep 2, 2017

I reverted this PR to solely deal with the aircraft units. The other units require a bit more work, some of which is beyond my scope for the time being, at least with any chance of exposing this to a playtest.

As of now this PR grants RevealOnDeath with all RA aircrafts with a reveal radius of 4c0 in order to see the damage dealt from the crash.

RevealOnDeath has a polish issue with #13896 but beyond that, should perform well with this PR.

@SoScared SoScared changed the title [RA] RevealOnDeath with Aircraft, Demo Truck and Mad Tank. [RA] RevealOnDeath with Aircraft Sep 2, 2017
@@ -135,6 +135,7 @@ BADR.Husk:
MinDamage: Undamaged
RenderSprites:
Image: badr
-RevealOnDeath
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing : at the end!

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@reaperrr reaperrr merged commit 2bfc7cd into OpenRA:bleed Sep 2, 2017
@reaperrr reaperrr added this to the Next Release milestone Sep 2, 2017
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

5 participants