-
Notifications
You must be signed in to change notification settings - Fork 293
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
Weapon attack cleanup #5922
base: master
Are you sure you want to change the base?
Weapon attack cleanup #5922
Changes from 5 commits
175a3f1
2db8795
be1cec7
29643a5
34ecf28
80404a0
d3c98a9
76b4441
ca0bbd2
c2827f9
d061789
564eb35
c59a753
720a88e
bdfb73b
d16dce5
891a17d
b21fe79
12ed947
014cb8f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -555,11 +555,12 @@ | |
|| (atype.getAmmoType() == AmmoType.T_NLRM) | ||
|| (atype.getAmmoType() == AmmoType.T_MEK_MORTAR)) | ||
&& (munition.contains(AmmoType.Munitions.M_SEMIGUIDED))) { | ||
for (TagInfo ti : game.getTagInfo()) { | ||
if (target.getId() == ti.target.getId()) { | ||
spotter = game.getEntity(ti.attackerId); | ||
} | ||
} | ||
final Targetable currentTarget = target; // Required for concurrency reasons | ||
spotter = game.getTagInfo().stream() | ||
.filter(ti -> currentTarget.getId() == ti.target.getId()) | ||
.findAny() | ||
.map(ti -> game.getEntity(ti.attackerId)) | ||
.orElse(null); | ||
} | ||
} | ||
|
||
|
@@ -954,11 +955,8 @@ | |
toHit = new ToHitData(); | ||
} | ||
|
||
Entity te = null; | ||
if (ttype == Targetable.TYPE_ENTITY) { | ||
//Some weapons only target valid entities | ||
te = (Entity) target; | ||
} | ||
//Some weapons only target valid entities | ||
final Entity te = ttype == Targetable.TYPE_ENTITY ? (Entity) target : null; | ||
|
||
// If the attacker and target are in the same building & hex, they can | ||
// always attack each other, TW pg 175. | ||
|
@@ -972,7 +970,7 @@ | |
} | ||
|
||
// are we bracing a location that's not where the weapon is located? | ||
if (ae.isBracing() && (ae.braceLocation() != weapon.getLocation())) { | ||
Check warning Code scanning / CodeQL Dereferenced variable may be null Warning
Variable
weapon Error loading related location Loading this Error loading related location Loading Variable weapon Error loading related location Loading this Error loading related location Loading Variable weapon Error loading related location Loading this Error loading related location Loading Variable weapon Error loading related location Loading this Error loading related location Loading Variable weapon Error loading related location Loading this Error loading related location Loading Variable weapon Error loading related location Loading this Error loading related location Loading Variable weapon Error loading related location Loading this Error loading related location Loading Variable weapon Error loading related location Loading this Error loading related location Loading Variable weapon Error loading related location Loading this Error loading related location Loading Variable weapon Error loading related location Loading this Error loading related location Loading Variable weapon Error loading related location Loading this Error loading related location Loading Variable weapon Error loading related location Loading this Error loading related location Loading Variable weapon Error loading related location Loading this Error loading related location Loading |
||
return String.format(Messages.getString("WeaponAttackAction.BracingOtherLocation"), | ||
ae.getLocationName(ae.braceLocation()), ae.getLocationName(weapon.getLocation())); | ||
} | ||
|
@@ -1275,13 +1273,9 @@ | |
&& ae.isSpaceborne()) { | ||
boolean networkFiringSolution = false; | ||
//Check to see if the attacker has a firing solution. Naval C3 networks share targeting data | ||
if (ae.hasNavalC3()) { | ||
for (Entity en : game.getC3NetworkMembers(ae)) { | ||
if (te != null && en.hasFiringSolutionFor(te.getId())) { | ||
networkFiringSolution = true; | ||
break; | ||
} | ||
} | ||
if (ae.hasNavalC3() && te != null | ||
Saklad5 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
&& game.getC3NetworkMembers(ae).stream().anyMatch(en -> en.hasFiringSolutionFor(te.getId()))) { | ||
networkFiringSolution = true; | ||
} | ||
if (!networkFiringSolution) { | ||
//If we don't check for target type here, we can't fire screens and missiles at hexes... | ||
|
@@ -1303,15 +1297,14 @@ | |
&& (te != null) && te.hasSeenEntity(ae.getOwner())) | ||
&& !isArtilleryIndirect && !isIndirect && !isBearingsOnlyMissile) { | ||
boolean networkSee = false; | ||
if (ae.hasC3() || ae.hasC3i() || ae.hasActiveNovaCEWS()) { | ||
if (ae.hasC3() || ae.hasC3i() || ae.hasActiveNovaCEWS() | ||
&& game.getEntitiesVector().stream().anyMatch(en -> | ||
!en.isEnemyOf(ae) | ||
&& en.onSameC3NetworkAs(ae) | ||
&& Compute.canSee(game, en, target))) { | ||
Comment on lines
+1299
to
+1303
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please don't do this inside of a conditional block. If you're going to do a stream, please do so either inside or outside of it. |
||
// c3 units can fire if any other unit in their network is in | ||
// visual or sensor range | ||
for (Entity en : game.getEntitiesVector()) { | ||
if (!en.isEnemyOf(ae) && en.onSameC3NetworkAs(ae) && Compute.canSee(game, en, target)) { | ||
networkSee = true; | ||
break; | ||
} | ||
} | ||
networkSee = true; | ||
} | ||
if (!networkSee) { | ||
if (!Compute.inSensorRange(game, ae, target, null)) { | ||
|
@@ -1410,27 +1403,17 @@ | |
|
||
// limit large craft to zero net heat and to heat by arc | ||
final int heatCapacity = ae.getHeatCapacity(); | ||
if (ae.usesWeaponBays() && (weapon != null) && !weapon.getBayWeapons().isEmpty()) { | ||
if (ae.usesWeaponBays() && (weapon != null)) { | ||
int totalHeat = 0; | ||
|
||
// first check to see if there are any usable weapons | ||
boolean usable = false; | ||
for (WeaponMounted m : weapon.getBayWeapons()) { | ||
WeaponType bayWType = m.getType(); | ||
boolean bayWUsesAmmo = (bayWType.getAmmoType() != AmmoType.T_NA); | ||
if (m.canFire()) { | ||
if (bayWUsesAmmo) { | ||
if ((m.getLinked() != null) && (m.getLinked().getUsableShotsLeft() > 0)) { | ||
usable = true; | ||
break; | ||
} | ||
} else { | ||
usable = true; | ||
break; | ||
} | ||
} | ||
} | ||
if (!usable) { | ||
if (!weapon.streamBayWeapons() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Request to keep streams out of if statements. I'd rather have an extra line and boolean assignment and a variable name that says what this test is about. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see how having a distinct boolean would add clarity beyond what the existing comment provides. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you want me to change the comment to be an initialized boolean anyway? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @SJuliez. It makes the code harder to read. |
||
.filter(WeaponMounted::canFire) | ||
.anyMatch(m -> | ||
m.getType().getAmmoType() == AmmoType.T_NA | ||
|| Optional.ofNullable(m.getLinked()).map(a -> a.getUsableShotsLeft() > 0).orElse(false) | ||
) | ||
) { | ||
return Messages.getString("WeaponAttackAction.BayNotReady"); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For readability and code length, this does not feel like an improvement to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
target
really should be final, but the current implementation for Swarm LRMs prevents that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this more readable because it makes it much easier to tell that it is simply getting any entity that successfully applied TAG to the target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is an issue, I could just drop this commit. I'd rather not hold up further review of this pull request due to a few controversial changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the code and may have other impacts as the old code picked the last target that matched where as the new code picks a random one.
Depending upon the rules, this may not be a valid change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will literally never make a difference. The source of the TAG does not matter.