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

Fix Nuke beacon and reveal times when SkipAscent is true #14286

Merged
merged 1 commit into from Apr 12, 2018

Conversation

Projects
None yet
5 participants
@MustaphaTR
Copy link
Member

MustaphaTR commented Nov 3, 2017

No description provided.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:fix-nuke-beacon-and-reveal-durations branch from c60576d to e85b667 Nov 3, 2017

@@ -126,12 +126,18 @@ public override void Activate(Actor self, Order order, SupportPowerManager manag
var type = info.RevealGeneratedShroud ? Shroud.SourceType.Visibility
: Shroud.SourceType.PassiveVisibility;

var revealDuration = info.SkipAscent ? (info.FlightDelay / 2) - info.CameraSpawnAdvance

This comment has been minimized.

@pchote

pchote Nov 15, 2017

Member

These can be shortened down to info.FlightDelay / (info.SkipAscent ? 2 : 1) - info.CameraSpawnAdvance;

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 15, 2017

This doesn't appear to fix the beacons, which still start half-done.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:fix-nuke-beacon-and-reveal-durations branch from e85b667 to 659b15c Nov 19, 2017

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

MustaphaTR commented Nov 19, 2017

This doesn't appear to fix the beacons, which still start half-done.

This PR wasn't really trying to fix that, but only when beacon is placed/removed, reveal start/end. But i'll try to fix it too now.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:fix-nuke-beacon-and-reveal-durations branch from 659b15c to f5746b7 Nov 19, 2017

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

MustaphaTR commented Nov 19, 2017

Updated, fixed Beacon FractionComplete calculations too.

{
get
{
return skipAscent ? (ticks - turn) * 1f / turn : ticks * 1f / delay;

This comment has been minimized.

@pchote

pchote Feb 23, 2018

Member

The skipAscent case here doesn't look right. I would expect (ticks - turn) * 1f / (delay - turn).

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Feb 23, 2018

The original code halving the delay if skipAscent is true leads to a bunch of not very nice hacks here. It would be much cleaner if you changed it so that delay was the true delay in all cases, and skipAscent then changes the amount of time spent in the up vs down motion.

If you want to keep the current timing behaviour, you can do the above, and then halve delay in the ctor if skipAscent is true.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Mar 30, 2018

@MustaphaTR do you plan to return to this?

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:fix-nuke-beacon-and-reveal-durations branch from f5746b7 to c601d84 Mar 31, 2018

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

MustaphaTR commented Mar 31, 2018

Updated.

I'm not sure about changing that <= to <, but without that change, it gives division by 0 error.

@@ -64,7 +64,7 @@ public class NukeLaunch : IProjectile, ISpatiallyPartitionable
Game.Sound.Play(SoundType.World, weapon.Report.Random(firedBy.World.SharedRandom), pos);

if (skipAscent)
ticks = turn;

This comment has been minimized.

@pchote

pchote Apr 4, 2018

Member

Changing this but not the offset above makes the missiles move visually at half the speed when skipping the ascent.

You can fix and simplify this by removing this if statement and changing the code above to look like:

turn = skipAscent ? 0 : delay / 2;
var offset = new WVec(WDist.Zero, WDist.Zero, velocity * (delay - turn));

and then also

pos = skipAscent ? descendSource : ascendSource;

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:fix-nuke-beacon-and-reveal-durations branch from c601d84 to cdd0f99 Apr 5, 2018

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

MustaphaTR commented Apr 5, 2018

Updated.

@pchote
Copy link
Member

pchote left a comment

The [Desc] for SkipAscent needs to be updated, but 👍 after that is done.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:fix-nuke-beacon-and-reveal-durations branch from cdd0f99 to dd162c5 Apr 11, 2018

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

MustaphaTR commented Apr 11, 2018

Sorry for the delay, updated.

@GraionDilach
Copy link
Contributor

GraionDilach left a comment

👍

@abcdefg30 abcdefg30 merged commit dc71a71 into OpenRA:bleed Apr 12, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Member

abcdefg30 commented Apr 12, 2018

@MustaphaTR MustaphaTR deleted the MustaphaTR:fix-nuke-beacon-and-reveal-durations branch Apr 12, 2018

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