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

Add sprite trail support to NukeLaunch/NukePower and use it for D2k Death Hand #16399

Merged
merged 5 commits into from Apr 22, 2019

Conversation

@reaperrr
Copy link
Contributor

commented Apr 11, 2019

And use it for D2k Death Hand.
While working on #16363, I noticed that the Death Hand in original D2k had this.

Also added trail to TS nuke/"Cluster Missile" for some visual polish (and increased FlightVelocity), since who knows when we'll get around to implementing the real voxel projectile.

@reaperrr reaperrr changed the title Add sprite trail support to NukeLaunch/NukePower Add sprite trail support to NukeLaunch/NukePower and use it for D2k Death Hand Apr 11, 2019

@@ -41,6 +41,24 @@ class NukePowerInfo : SupportPowerInfo, IRulesetLoaded, Requires<BodyOrientation
[Desc("Custom palette is a player palette BaseName.")]
public readonly bool IsPlayerPalette = false;

[Desc("Trail animation.")]

This comment has been minimized.

Copy link
@pchote

pchote Apr 14, 2019

Member

How much work do you think it would be to move all of this into a NukeLaunchInfo and then define Projectile: NukeLaunch like other normal weapons.

This comment has been minimized.

Copy link
@reaperrr

reaperrr Apr 19, 2019

Author Contributor

After some working on it locally, to put the tl;dr first:

As much as I feared.

While none of the individual changes are particularly hard (except one, more on that later), the diff on NukePower would be large, the diff on NukeLaunch would be large, the diff on the yamls would be large, and the update rule I'd have to write would be huge, because moving to a proper projectile args means moving half of NukePower's properties.

As icing on the cake, I don't know how I would pass the NukeLaunch.FractionComplete back to NukePower for the beacon timer.
All in all, the PR would very likely go into mid-3-digit removals and low-4-digit additions. Not saying we shouldn't get this over with at some point, but I don't really feel like investing that much effort right now to get... trails, at least not as long as the aircraft/resupply refactors aren't finished (among other things).

Personally, I'd rather swallow the pill of having to write a slightly larger update rule later on when/if we refactor this, than this PR growing stale/getting closed and not revived until maybe a year later.

@pchote
Copy link
Member

left a comment

One request, otherwise LGTM:

pos = WPos.LerpQuadratic(ascendSource, ascendTarget, WAngle.Zero, ticks, turn);
if (!string.IsNullOrEmpty(trailImage) && --trailTicks < 0)

This comment has been minimized.

Copy link
@pchote

pchote Apr 20, 2019

Member

Please reduce duplication by defining delayedPos in these two cases and then moving the rest of this below the if/else.

This comment has been minimized.

Copy link
@reaperrr

reaperrr Apr 21, 2019

Author Contributor

I moved everything related to the trail below the if/else into a single if block, as that allowed to reduce the line count further. I don't think that one duplicate ticks < turn check is worth dragging out a var, so I didn't bother.

reaperrr added 5 commits Apr 11, 2019
Group all Bullet Trail* properties together
Just a small code readability improvement.
Give TS ClusterMissile a smoke trail
And increase visual FlightVelocity.

We don't know when this will be replaced with
TS-style voxel projectile, so a bit of visual polish can't hurt.

@reaperrr reaperrr force-pushed the reaperrr:NukeTrail branch from c5c84eb to 89f50db Apr 21, 2019

@pchote
pchote approved these changes Apr 21, 2019
@matjaeck
Copy link
Contributor

left a comment

LGTM

@reaperrr reaperrr merged commit 36783ef into OpenRA:bleed Apr 22, 2019

2 checks passed

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

This comment has been minimized.

Copy link
Contributor

commented May 4, 2019

This should have had controls on which direction the smoke is spawned - RA2 only spawned smokes when it's nuke was flying upwards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.