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

Cycle through all Armament.LocalOffsets if weapon has Burst: 1 #14579

Merged
merged 4 commits into from Dec 29, 2017

Conversation

Projects
None yet
6 participants
@reaperrr
Contributor

reaperrr commented Dec 26, 2017

While the approach in #14578 might be good enough in some cases, the fact that modders are forced to use BurstDelay < ReloadDelay and therefore couldn't balance this 100% perfectly just highlights that this needs a code-level solution.

Also, TS was still using values from a time when BurstDelay == ReloadDelay was supposed to be supported (but dropped due to other technical issues that would have caused).

Fixes #14464.
Fixes #14577.

@@ -232,7 +236,10 @@ public virtual Barrel CheckFire(Actor self, IFacing facing, Target target)
ticksSinceLastShot = 0;
var barrel = Barrels[Burst % Barrels.Length];
// If Weapon.Burst == 1, cycle through all LocalOffsets, otherwise use the offset corresponding to current Burst

This comment has been minimized.

@pchote

pchote Dec 26, 2017

Member

Can you please document this in the LocalOffset desc too?

@pchote

pchote Dec 26, 2017

Member

Can you please document this in the LocalOffset desc too?

This comment has been minimized.

@reaperrr

reaperrr Dec 26, 2017

Contributor

Done.

@reaperrr

reaperrr Dec 26, 2017

Contributor

Done.

@fruestueck

Tested TD with APC and worked fine. Can't tell much to the code.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Dec 26, 2017

Contributor

Updated. Fixes #14577 as well now.

Contributor

reaperrr commented Dec 26, 2017

Updated. Fixes #14577 as well now.

@@ -39,7 +39,8 @@ public class ArmamentInfo : PausableConditionalTraitInfo, Requires<AttackBaseInf
[Desc("Time (in frames) until the weapon can fire again.")]
public readonly int FireDelay = 0;
[Desc("Muzzle position relative to turret or body. (forward, right, up) triples")]
[Desc("Muzzle position relative to turret or body, (forward, right, up) triples.",
"If weapon Burst = 1, it cycles through all listed offsets, otherwise the offset corresponding to current burst is used.")]

This comment has been minimized.

@chrisforbes

chrisforbes Dec 26, 2017

Member

current shot in the burst, right?

@chrisforbes

chrisforbes Dec 26, 2017

Member

current shot in the burst, right?

This comment has been minimized.

@reaperrr

reaperrr Dec 26, 2017

Contributor

According to what I found online, 'burst' stands for a shot in a salvo.

Edit: Googling for "burst salvo" brought up this (quote from F4 Phantom manual):

The number of bursts within the salvo and the interval between the bursts is determined by the settings of the burst selector knob

@reaperrr

reaperrr Dec 26, 2017

Contributor

According to what I found online, 'burst' stands for a shot in a salvo.

Edit: Googling for "burst salvo" brought up this (quote from F4 Phantom manual):

The number of bursts within the salvo and the interval between the bursts is determined by the settings of the burst selector knob

This comment has been minimized.

@pchote

pchote Dec 27, 2017

Member

See #13090 in relation to this ongoing confusion.

@pchote

pchote Dec 27, 2017

Member

See #13090 in relation to this ongoing confusion.

@reaperrr reaperrr added this to the Next release milestone Dec 26, 2017

reaperrr added some commits Dec 26, 2017

Remove dysfunctional TS yaml hacks
In addition to now being redundant due to Armament-side cycling through LocalOffsets, these didn't work because Burst is reset once ReloadDelay is reached, so that equal BurstDelay never really had the intended effect.
Make RA heli weapons alternate between offsets
Instead of using one for AG-only and one for AA-only.
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Dec 27, 2017

Contributor

Updated.

Contributor

reaperrr commented Dec 27, 2017

Updated.

@AoAGeneral

This comment has been minimized.

Show comment
Hide comment
@AoAGeneral

AoAGeneral Dec 28, 2017

Contributor

With this change in mind does this affect Apache and Orcas as well? When this also gets implemented in does this create extra damage for the APC?

Contributor

AoAGeneral commented Dec 28, 2017

With this change in mind does this affect Apache and Orcas as well? When this also gets implemented in does this create extra damage for the APC?

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Dec 28, 2017

Contributor

This change is plainly visual.

Contributor

GraionDilach commented Dec 28, 2017

This change is plainly visual.

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Dec 28, 2017

Contributor

Could this logic be expanded to apply to all cases where the amount of the firing offsets is a direct multiplication of burst? (say, Burst: 2 with 4 offsets).

Contributor

GraionDilach commented Dec 28, 2017

Could this logic be expanded to apply to all cases where the amount of the firing offsets is a direct multiplication of burst? (say, Burst: 2 with 4 offsets).

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Dec 28, 2017

Contributor

With this change in mind does this affect Apache and Orcas as well?

Yes.

When this also gets implemented in does this create extra damage for the APC?

No.
This change is 100% cosmetical, it has zero impact on range, targeting, dps or anything.
Basically, in all RA/TD cases this will work just as

ReloadDelay: 20
Burst: 2
BurstDelays: 20

would, if BurstDelays were allowed to be equal to ReloadDelay (which they're not for technical reasons).

Contributor

reaperrr commented Dec 28, 2017

With this change in mind does this affect Apache and Orcas as well?

Yes.

When this also gets implemented in does this create extra damage for the APC?

No.
This change is 100% cosmetical, it has zero impact on range, targeting, dps or anything.
Basically, in all RA/TD cases this will work just as

ReloadDelay: 20
Burst: 2
BurstDelays: 20

would, if BurstDelays were allowed to be equal to ReloadDelay (which they're not for technical reasons).

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Dec 28, 2017

Contributor

Could this logic be expanded to apply to all cases where the amount of the firing offsets is a direct multiplication of burst? (say, Burst: 2 with 4 offsets).

I don't feel like coming up with the code and math for that right now, but generally speaking I don't see why not.

Contributor

reaperrr commented Dec 28, 2017

Could this logic be expanded to apply to all cases where the amount of the firing offsets is a direct multiplication of burst? (say, Burst: 2 with 4 offsets).

I don't feel like coming up with the code and math for that right now, but generally speaking I don't see why not.

@pchote

LGTM and confirmed the noted fixes ingame.

However, this doesn't fix the TD Orca, despite that having two firing offsets defined?

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Dec 29, 2017

Member

The Orca uses the same kind of workaround as TS, which should now be removed.

Member

pchote commented Dec 29, 2017

The Orca uses the same kind of workaround as TS, which should now be removed.

Fix TD Orca missiles alternating between offsets
This is now properly fixed by using Burst: 1.
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Dec 29, 2017

Contributor

Fixed.

Contributor

reaperrr commented Dec 29, 2017

Fixed.

@pchote

pchote approved these changes Dec 29, 2017

@pchote pchote merged commit 4ef11f2 into OpenRA:bleed Dec 29, 2017

2 checks passed

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

@reaperrr reaperrr deleted the reaperrr:fix-td-apcburst branch Feb 22, 2018

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