God-Emperor
Deity
File CvUnit.cpp:
I believe I have found one or two bugs in the CvUnit::rangeStrike and one in CvUnit::canRangeStrikeAt.
The net result is that calling rangeStrike(iX,iY) will never reject an attack on a plot due to range, whether there is terrain or features that should be blocking the attack, or the target plot not currently being visible (or ever revealed to the attacker). The possible second bug in rangeStrike may be related to OOS states that happen from ranged strikes.
(All line numbers given are for the unaltered BtS CvUnit.cpp file.)
1) CvUnit::rangeStrike problems:
1.a) On line 11912 it is using "pPlot" as the first argument in the call to canRangeStrike.
The pPlot is derived from the iX,iY values that are passed to rangeStrike and are being passed to canRangeStrikeAt as the 2nd and 3rd arguments, so it is the same plot. This causes canRangeStrikeAt to determine if the plot at iX, iY can have a ranged strike done on it from a unit in pPlot, which is the same plot. The answer is always "True" as long as there is a valid target in the plot since the check to see if it is in range (line 18970) will always find that the range of 0 from the plot to itself is less than the unit's air range. On line 11890 it will also find out that you can always see a plot from the plot itself.
So it seems clear that it in rangeStrike it should be passing the unit's own position's plot, not the target's plot, as the first argument. Every other unit mission I looked at (recon, airdrop, nuke, etc.) uses "plot()" for the first argument to the corresponding canThingAt() method).
Proposed fix:
In CvUnit::rangeStrike, line 11912, change "canRangeStrikeAt(pPlot, iX, iY)" to "canRangeStrikeAt(plot(), iX, iY)" so that it is checking to see if the unit can do a rangeStrike on plot iX,iY from the attacking unit's current plot instead of from the target plot.
1.b) A potential OOS problem (ranged attacks are know to have such problems).
At the end of the rangeStrike function (line 11958) there is this:
This is inside the "if (pPlot->isActiveVisible(false))" block, thus it will be run on computers where the human player can see the plot where the attack happened. It will not be run on computers where the human player can not see it. In multiplayer games this could be introducing timing differences, leading to state differences. On computers where the human player can not see the plot there is no change to the defending unit's mission timer, but on computers where the human player can see the plot there is a delay. I'm not certain this is a problem, but it certainly can not be a good thing if anything happens during the delay interval.
Proposed fix:
I'm not sure. Either remove the statement entirely, so it happens for nobody, or move it out of the if block so that it happens for everybody. I don't know what the exact effects of doing either of them would be.
2) There is also a bug in canRangeStrikeAt.
It checks the passed pPlot to see if it is visible to the attacking player but it doesn't check to see if the pTargetPlot derrived from the passed iX,iY is visibile.
It seems like around the point it checks pPlot->isVisible (line 11874), it ought to also check pTargetPlot->isVisible. This would not be a bug, as such, if the last pPlot->canSeePlot test ever checked the target plot's basic visibility, but it doesn't. It is only checking to see if it is possible to see the specified plot from the current plot, not if it is actually visible at this time.
Net result: if there is a valid target on the plot at iX,iY and if nothing is blocking the path, like a mountain or asteroid field (in Final Frontier) then canRangeStrikeAt returns true even if the target plot is not currently visible (or has never even been revealed to the player).
Proposed fix:
In CvUnit::canRangeStrikeAt, at line 11878, add another if block like the immediately preceding one but checking the visibility for pTargetPlot rather than pPlot.
Old:
New:
It is possible that instead of adding another visibility check changing the existing check to be for the target plot would be acceptable. If canRangedStrikeAt is never used by the AI to decide if it should move a unit to do the attack, then changing the existing test is probably the way to go.
I believe I have found one or two bugs in the CvUnit::rangeStrike and one in CvUnit::canRangeStrikeAt.
The net result is that calling rangeStrike(iX,iY) will never reject an attack on a plot due to range, whether there is terrain or features that should be blocking the attack, or the target plot not currently being visible (or ever revealed to the attacker). The possible second bug in rangeStrike may be related to OOS states that happen from ranged strikes.
(All line numbers given are for the unaltered BtS CvUnit.cpp file.)
1) CvUnit::rangeStrike problems:
1.a) On line 11912 it is using "pPlot" as the first argument in the call to canRangeStrike.
Code:
if (!canRangeStrikeAt(pPlot, iX, iY))The pPlot is derived from the iX,iY values that are passed to rangeStrike and are being passed to canRangeStrikeAt as the 2nd and 3rd arguments, so it is the same plot. This causes canRangeStrikeAt to determine if the plot at iX, iY can have a ranged strike done on it from a unit in pPlot, which is the same plot. The answer is always "True" as long as there is a valid target in the plot since the check to see if it is in range (line 18970) will always find that the range of 0 from the plot to itself is less than the unit's air range. On line 11890 it will also find out that you can always see a plot from the plot itself.
So it seems clear that it in rangeStrike it should be passing the unit's own position's plot, not the target's plot, as the first argument. Every other unit mission I looked at (recon, airdrop, nuke, etc.) uses "plot()" for the first argument to the corresponding canThingAt() method).
Proposed fix:
In CvUnit::rangeStrike, line 11912, change "canRangeStrikeAt(pPlot, iX, iY)" to "canRangeStrikeAt(plot(), iX, iY)" so that it is checking to see if the unit can do a rangeStrike on plot iX,iY from the attacking unit's current plot instead of from the target plot.
Code:
if (!canRangeStrikeAt(plot(), iX, iY))1.b) A potential OOS problem (ranged attacks are know to have such problems).
At the end of the rangeStrike function (line 11958) there is this:
Code:
//delay death
pDefender->getGroup()->setMissionTimer(GC.getMissionInfo(MISSION_RANGE_ATTACK).getTime());Proposed fix:
I'm not sure. Either remove the statement entirely, so it happens for nobody, or move it out of the if block so that it happens for everybody. I don't know what the exact effects of doing either of them would be.
2) There is also a bug in canRangeStrikeAt.
It checks the passed pPlot to see if it is visible to the attacking player but it doesn't check to see if the pTargetPlot derrived from the passed iX,iY is visibile.
It seems like around the point it checks pPlot->isVisible (line 11874), it ought to also check pTargetPlot->isVisible. This would not be a bug, as such, if the last pPlot->canSeePlot test ever checked the target plot's basic visibility, but it doesn't. It is only checking to see if it is possible to see the specified plot from the current plot, not if it is actually visible at this time.
Net result: if there is a valid target on the plot at iX,iY and if nothing is blocking the path, like a mountain or asteroid field (in Final Frontier) then canRangeStrikeAt returns true even if the target plot is not currently visible (or has never even been revealed to the player).
Proposed fix:
In CvUnit::canRangeStrikeAt, at line 11878, add another if block like the immediately preceding one but checking the visibility for pTargetPlot rather than pPlot.
Old:
Code:
if (!pPlot->isVisible(getTeam(), false))
{
return false;
}
if (plotDistance(pPlot->getX_INLINE(), pPlot->getY_INLINE(), pTargetPlot->getX_INLINE(), pTargetPlot->getY_INLINE()) > airRange())
Code:
if (!pPlot->isVisible(getTeam(), false))
{
return false;
}
if (!pTargetPlot->isVisible(getTeam(), false))
{
return false;
}
if (plotDistance(pPlot->getX_INLINE(), pPlot->getY_INLINE(), pTargetPlot->getX_INLINE(), pTargetPlot->getY_INLINE()) > airRange())It is possible that instead of adding another visibility check changing the existing check to be for the target plot would be acceptable. If canRangedStrikeAt is never used by the AI to decide if it should move a unit to do the attack, then changing the existing test is probably the way to go.
