Minor bugfix for checking line-of-fire

[karadoc]

TileEngine::canTargetUnit is used to find a voxel within the target for which we have a clear line to, so that we can shoot at the target. It it finds a line-of-fire, *scanVoxel is set to be the target voxel and the function returns true. But when it cannot find a clear line-of-fire, the function returns false and *scanVoxel is just left on whichever value happened to be the last one checked.

That's all fine, but unfortunately the return value of TileEngine::canTargetUnit is actually ignored. Instead, the target voxel (whatever *scanVoxel was set to) is given to Projectile::calculateTrajectory, which then gets final say in whether or not we can take the shot. If it looks like we were trying to aim at a unit but the shot will actually hit a wall, then we'll abort the shot.

The problem is that Projectile::calculateTrajectory doesn't actually know what we were trying to aim at. Usually it is able to correctly guess that we were aiming at a unit based on the (bogus) voxel, but not always. What can happen is that TileEngine::canTargetUnitreturns false (meaning that we can't hit the target), but the final target voxel given to Projectile::calculateTrajectory makes it look like we didn't want to hit a unit anyway - and so our solider will happily shoot at empty space rather than telling us that there was no line-of-fire to the enemy.

--

The most obvious way to fix this would be to use the return value of TileEngine::canTargetUnit to decide whether we can take the shot. However, that could require some structural changes and some code duplication. So I've made a simpler 1-line fix, which just ensures that the target voxel given to Projectile::calculateTrajectory will always be in the unit when we were trying to aim at the unit.

Here:

Code: [Select]

diff --git a/src/Battlescape/TileEngine.cpp b/src/Battlescape/TileEngine.cpp
index 3510149..9265e20 100644
--- a/src/Battlescape/TileEngine.cpp
+++ b/src/Battlescape/TileEngine.cpp
@@ -820,6 +820,9 @@ bool TileEngine::canTargetUnit(Position *originVoxel, Tile *tile, Position *scan
  }
  }
  }
+ https:// Couldn't find a line of fire; so just set the scanVoxel to be at the centre of the target.
+ https:// (Not all callers pay attention to the return value of this function.)
+ *scanVoxel = Position((tile->getPosition().x * 16) + 7, (tile->getPosition().y * 16) + 8, targetCenterHeight);
  return false;
 }
 
--

PS. I'm not sure where the best place for these bug fixes is. I noticed and fixed this bug on xPiratez v0.99. I'm attaching a save game which demonstrates the problem. (Shoot at the enemy in the window; you will miss every single time. Debugging shows that there was no line of fire, but the game lets you shoot anyway.)

I guess I'll post something the bugtracker, at https://openxcom.org/bugs/openxcom/issues/new/issuetype/bugreport -- but I'll have to lie about which version of OpenXcom I was using; because as I said - I was playing a different version. The code does look the same though.

[edit]
Posted here. Please let me know if there is a better place or better way of reporting this kind of stuff.

[karadoc]

On closer inspection; more works is needed. My fix only fixes the problem in some cases. The problem still exists in other cases. I'm attaching another save file which demonstrates the problem. (Again, this save is from xPiratez.) From the save, command Athletic Walker to shoot at the alien above her. She'll shoot, and always miss. Debugging shows that the game never expected her to hit; and in fact even with perfect accuracy she'd be shooting into a wall. There is no line of fire; and so the game should tell us this rather than letting her take the shot. The reason the game allows it in this case is that targeting the square directly behind a wall is how the player is meant to shoot at a wall - and so the game thinks that the player actually wants to shoot at this wall.

I'll post again if I discover more.

[edit]
Ok. I was mistaken about it being difficult to just abort based on the result of canTargetUnit. This fix should work fine, as far as I can tell. (The other change is no longer needed, but I'd probably choose to keep it anyway.)

Code: [Select]

--- a/src/Battlescape/ProjectileFlyBState.cpp
+++ b/src/Battlescape/ProjectileFlyBState.cpp
@@ -223,7 +223,12 @@ void ProjectileFlyBState::init()
  }
  else
  {
- _parent->getTileEngine()->canTargetUnit(&originVoxel, targetTile, &_targetVoxel, _unit);
+ if (_parent->getTileEngine()->canTargetUnit(&originVoxel, targetTile, &_targetVoxel, _unit) == false)
+ {
+ _action.result = "STR_NO_LINE_OF_FIRE";
+ _parent->popState();
+ return;
+ }
  }
  }
  else if (targetTile->getMapData(O_OBJECT) != 0)


[karadoc]

Ok. I've just discovered an unwanted side effect of my second "fix". ProjectileFlyBState::init uses the same checks for arcing shots as it does for direct shots. So if it is changed to abort when canTargetUnit returns false, then it will abort any arcing shot that doesn't have direct line-of-sight, even if the shot could hit.

In some sense, this make sense - because the soldier can't see their target anyway. They aren't sure that they can hit it. And if the player wants, they can take the risk by using ctrl to force the shot. Nevertheless, it's probably an unwanted effect.

My suggestion is to keep my original fix; the one in the first post. It doesn't correct the 'behind-the-wall' case; but it does fix many cases and it doesn't have a problem with arcing shots. In fact, it's probably even better for arcing shots because it will have the soldier aim at the middle of the unit rather than aiming a corner of the unit (which is what it would have done in the past, because that's the last place checked for direct line-of-fire).

Ideally, "canTargetUnit" would take into account whether or not the shot is arcing, and return the correct result accordingly. But I'm getting impression that most people don't care about this stuff anyway... so I think my original fix is good enough for the time being.

I'd be interested to know if anyone has any thoughts about what I'm talking about. Have people noticed the bug? Does the fix make sense? Does anyone care about this kind of stuff?

[ohartenstein23]

Would it make sense/not be too much work to create a second set of checks for arcing shots?  I understand the idea that not shooting a bow at a target would make sense if the soldier can't see it, but this wouldn't this also have repercussions for other arcing weapons like mortars, grenade launchers, flamers, etc. where not having direct line-of-sight isn't as much as a problem?

Maybe this could be a thing exposed to rulesets - does this arcing shot weapon require line of sight?

[karadoc]

Would it make sense/not be too much work to create a second set of checks for arcing shots?  I understand the idea that not shooting a bow at a target would make sense if the soldier can't see it, but this wouldn't this also have repercussions for other arcing weapons like mortars, grenade launchers, flamers, etc. where not having direct line-of-sight isn't as much as a problem?

Maybe this could be a thing exposed to rulesets - does this arcing shot weapon require line of sight?

Firstly, just to clarify - I didn't intend for the change to prevent arcing shots when line of sight is blocked; and even with the change they still will fire (and hit) if you hold ctrl when issuing the order. I do think it kind of makes sense for it to be like that, but that's not the kind of change that I'd advocate (except possibly as part of a ruleset, as you suggest).

I can think of a way to modify canTargetUnit so that it handles arcing shots. It wouldn't be a difficult change to make, but it would be a little bit computationally expensive. (My idea is just use "validateThrow" instead of "calculateLine" for every check inside canTargetUnit.) However, I not sure that's really necessary. My current thinking is that we can just ignore the result of canTargetUnit for arcing shots and throws as we did in the first place - but pay attention to it for direct shots. Like this:

Code: [Select]

diff --git a/src/Battlescape/ProjectileFlyBState.cpp b/src/Battlescape/ProjectileFlyBState.cpp
index 0d84d1e..ceb5edf 100644
--- a/src/Battlescape/ProjectileFlyBState.cpp
+++ b/src/Battlescape/ProjectileFlyBState.cpp
@@ -223,7 +223,17 @@ void ProjectileFlyBState::init()
  }
  else
  {
- _parent->getTileEngine()->canTargetUnit(&originVoxel, targetTile, &_targetVoxel, _unit);
+ if (_parent->getTileEngine()->canTargetUnit(&originVoxel, targetTile, &_targetVoxel, _unit) == false)
+ {
+ https:// if this action requires direct line-of-sight, we should abort.
+ if ((_action.type == BA_SNAPSHOT || _action.type == BA_AUTOSHOT || _action.type == BA_AIMEDSHOT) &&
+     !_action.weapon->getRules()->getArcingShot())
+ {
+ _action.result = "STR_NO_LINE_OF_FIRE";
+ _parent->popState();
+ return;
+ }
+ }
  }
  }
  else if (targetTile->getMapData(O_OBJECT) != 0)
diff --git a/src/Battlescape/TileEngine.cpp b/src/Battlescape/TileEngine.cpp
index 3510149..f7038a2 100644
--- a/src/Battlescape/TileEngine.cpp
+++ b/src/Battlescape/TileEngine.cpp
@@ -820,6 +820,9 @@ bool TileEngine::canTargetUnit(Position *originVoxel, Tile *tile, Position *scan
  }
  }
  }
+ https:// Couldn't find a line of fire; so just set the scanVoxel to be at the centre of the target.
+ https:// (Not all callers pay attention to the return value of this function.)
+ *scanVoxel = Position((tile->getPosition().x * 16) + 7, (tile->getPosition().y * 16) + 8, targetCenterHeight);
  return false;
 }

Notice that I've still kept the original change that I made to the bottom of canTargetUnit, where it sets the final value to be at the centre of the unit. Even if we ignore the return value of canTargetUnit, it is still being used to choose the specific target voxel for arcing shots; and so I still think this is a helpful to set that target to be the middle of the unit if we can't see a better spot.

Arcing shots have different checks later in the code for determining whether an arc is possible. So this change is looking pretty good to me. I can't guarantee that there isn't something else I haven't thought of though. D

[Meridian]

Just FYI, this was recently applied (not exactly in this form tho) in vanilla:
https://github.com/SupSuper/OpenXcom/commit/62ec237f08530295a419e67a5a6b4336abc0df04