Balance of bows and bullets

[karadoc]

ok. I've found the bug.
Here is a patch.

Code: [Select]

diff --git a/src/Battlescape/TileEngine.cpp b/src/Battlescape/TileEngine.cpp
index 35214de..8d7fb9f 100644
--- a/src/Battlescape/TileEngine.cpp
+++ b/src/Battlescape/TileEngine.cpp
@@ -2755,29 +2754,31 @@ int TileEngine::calculateParabola(const Position& origin, const Position& target
  x = (int)((double)origin.x + (double)i * cos(te) * sin(fi));
  y = (int)((double)origin.y + (double)i * sin(te) * sin(fi));
  z = (int)((double)origin.z + (double)i * cos(fi) - zK * ((double)i - ro / 2.0) * ((double)i - ro / 2.0) + zA);
- if (storeTrajectory && trajectory)
- {
- trajectory->push_back(Position(x, y, z));
- }
  https://passes through this point?
  Position nextPosition = Position(x,y,z);
- int result = calculateLine(lastPosition, nextPosition, false, 0, excludeUnit);
+ std::vector<Position> contactPoint;
+ int result = calculateLine(lastPosition, nextPosition, false, &contactPoint, excludeUnit);
  if (result != V_EMPTY)
  {
  if (lastPosition.z < nextPosition.z)
  {
- result = V_OUTOFBOUNDS;
+ result = V_OUTOFBOUNDS; https:// why??
  }
- if (!storeTrajectory && trajectory != 0)
+ if (trajectory != nullptr)
  { https:// store the position of impact
- trajectory->push_back(nextPosition);
+ assert(contactPoint.size() > 0);
+ trajectory->push_back(contactPoint[0]);
  }
  return result;
  }
+ if (storeTrajectory && trajectory != nullptr)
+ {
+ trajectory->push_back(nextPosition);
+ }
  lastPosition = Position(x,y,z);
  ++i;
  }
- if (!storeTrajectory && trajectory != 0)
+ if (!storeTrajectory && trajectory != nullptr)
  { https:// store the position of impact
  trajectory->push_back(Position(x, y, z));
  }

The problem was a mismatch between the coordinates given to TileEngine::hit, and the coordinates where the actual collision occurred.

To do collection detection on arcs, the game calculates a list of points on the arc then draws straight lines between and tests if those straight lines make contact with anything. The game was correctly detecting the collisions with those straight lines, but then instead of storing the coordinates of the collisions, it just stored the coordinates of the end point of the straight line - which may or may not have been where the collision occurred. And so sometimes the end-point of those lines was inside the target, and sometimes it wasn't. The problem could be reduced by taking more points on the arc (so the lines are shorter); and doing that might still be a good idea to make the arcs more arc-like. But ultimately, we still need to store where the collision takes place - and that's what I've done.


Aside from that fix, here are a couple of other unimportant changes that I've made. Take them or leave them as you see fit.


---
Undefined 'floor' in CraftWeapon.cpp.

Code: [Select]

diff --git a/src/Savegame/CraftWeapon.cpp b/src/Savegame/CraftWeapon.cpp
index 2b952c4..b2e51c0 100644
--- a/src/Savegame/CraftWeapon.cpp
+++ b/src/Savegame/CraftWeapon.cpp
@@ -17,6 +17,7 @@
  * along with OpenXcom.  If not, see <https://www.gnu.org/licenses/>.
  */
 #include <algorithm>
+#include <cmath>
 #include "CraftWeapon.h"
 #include "../Mod/RuleCraftWeapon.h"
 #include "../Mod/Mod.h"
@@ -168,12 +169,12 @@ CraftWeaponProjectile* CraftWeapon::fire() const
  */
 int CraftWeapon::getClipsLoaded(Mod *mod)
 {
- int retVal = (int)floor((double)_ammo / _rules->getRearmRate());
+ int retVal = (int)std::floor((double)_ammo / _rules->getRearmRate());
  RuleItem *clip = mod->getItem(_rules->getClipItem());
 
  if (clip && clip->getClipSize() > 0)
  {
- retVal = (int)floor((double)_ammo / clip->getClipSize());
+ retVal = (int)std::floor((double)_ammo / clip->getClipSize());
  }
 
  return retVal;

---
Marginally reduced code duplication and straightened lines for collision detection. (Unimportant house-keeping changes.)

Code: [Select]

https:// Never mind. I've discovered a minor mistake in this change, and rather than taking the time to fix it and repost it I think it's better to just keep the current code.

----
Named defined value instead of 'magic number'. (This change doesn't do anything at all; but I highly recommend it. There are stacks of 'magic numbers' like this in the code, and they really should be phased out whenever possible to improve clarity and reduce the likelihood that future changes will break something.)

Code: [Select]

diff --git a/src/Battlescape/ProjectileFlyBState.cpp b/src/Battlescape/ProjectileFlyBState.cpp
index 7101e35..0d84d1e 100644
--- a/src/Battlescape/ProjectileFlyBState.cpp
+++ b/src/Battlescape/ProjectileFlyBState.cpp
@@ -564,7 +564,7 @@ void ProjectileFlyBState::think()
  }
  }
 
- if (_projectileImpact == 4)
+ if (_projectileImpact == V_UNIT)
  {
  BattleUnit *victim = _parent->getSave()->getTile(_parent->getMap()->getProjectile()->getPosition(offset) / Position(16,16,24))->getUnit();
  if (victim && !victim->isOut() && victim->getFaction() == FACTION_HOSTILE)


---

[edit]
You probably don't need the "why?" comment in the bugfix...   But I'd consider removing that `if` block. In fact, I'm going to delete it now, and then just keep playing my game. I don't expect any negative side effects; but I do expect it might be marginally easier to fire arrows from the ground into enemies in high-up windows.

[Yankes]

I think proper way of doing bug fix is upload it to github (as your fork) and after that create pull request to supsuper master branch: https://github.com/SupSuper/OpenXcom/

[karadoc]

That's probably the best way; and I do have some pretty nice commit comments to go with my patch... but I kind of feel a bit silly having a whole fork on github just to post a couple of bugfixes. Maybe I'll set up a fork if I decide to work on it some more, but for the time being I think I'll just follow 'option 2' on [urlhttps://openxcom.org/forum/index.php/topic,181.0.html]this page[/url], in which SupSuper suggests posting a patch on the development forum.

--

Incidentally, having played a bit with the parabolic collision fixed - I think I can now return to my original suggestion: I reckon we can afford to reduce the accuracy of bows a bit. Maybe they don't _need_ a nerf for balance; but I just think it's weird that it's far easier to hit things with a bow than with pretty much anything else.

[legionof1]

Perhaps something along the lines of the scaling for snipers? Such a manner would reflect that a poorly skilled archer can't hit squat but a master will manage quite impressive feats. Perhaps with not so extreme a curve given how early bows are presently placed in tech progression.

[karadoc]

That could work. But I think I'd lean towards just having normal accuracy scaling, but with ~100% instead of 130%. (That's a very big drop in accuracy; but its still higher than most guns, and combined with the bug fix we'll probably still get more hits than we're currently getting anyway.)

[Dioxine]

The high accuracy of bows reflects the reality in which starting troops have Throwing in the 40-50 range. Even x150% (hunting bow) this only gives acceptable 75% acc. This, in turn, is to make grenades less-than-100%-sure (their accuracy is multiplied by the fact they target a tile, not an unit). Only with Gym spamming you will easily get Throwing scores in 75-85 range.
'Sniper' scaling is used for throwing knives and ninja stars, so they become deadly only with Throwing exceeding 80-ish. In part this coincides with the plans to add some throwing-dedicated armors.
However, if this bow patch is pulled into the main build, accuracy drop of all bows by circa 1/5th is a good idea.

[Meridian]

However, if this bow patch is pulled into the main build, accuracy drop of all bows by circa 1/5th is a good idea.

I will most probably take this into my fork.
Currently I am in process of merging it with oxce3.0... which will take "forever" since I got a conflict basically on every single file

After it's done, I fully agree with accuracy drop, makes sense.

[Yankes]

I will most probably take this into my fork.
Currently I am in process of merging it with oxce3.0... which will take "forever" since I got a conflict basically on every single file

After it's done, I fully agree with accuracy drop, makes sense.

I have one suggestion that save me couple of days of work:
https://stackoverflow.com/questions/9776527/merging-without-whitespace-conflicts
https://stackoverflow.com/questions/18131515/how-can-i-see-a-three-way-diff-for-a-git-merge-conflict

Without this two I would probably do rage quite and refuse to merge master branch.

[Meridian]

Undefined 'floor' in CraftWeapon.cpp.

It compiles just fine both in vs2015 (windows) and in mingw/mxe (linux) for me.
But I've added that include if it helps.

[karadoc]

It compiles just fine both in vs2015 (windows) and in mingw/mxe (linux) for me.
But I've added that include if it helps.

It does help. Thanks.

I'm surprised that it compiles fine for you without including <cmath>. The functions you are using are definitely in the cmath library, so my best guess is that something else you are including just happens to include cmath in those other systems, but not on my system. I'm compiling it on Windows with mingw (the nuwen distro).

In your recent updates, there have been a couple of other similar cases. For example, in the most recent build I'm getting compiler error:

Code: [Select]

\OpenXcom\src\Savegame\Soldier.cpp:608:15: error: 'ceil' is not a member of 'std'
which again is fixed by including <cmath>. (It also happened with the previous version, but I can't remember which source file has the problem.)

In any case, thanks for including the parabolic collision fix in your version. It's good to see my work actually get used. I was happy to contribute, but it's a bit disheartening for me when I put a bit of work into something, only to have it ignored. There seems to be no interest in the arcing shot fix, or my line-of-fire fix on the development forum, and so I'm not inclined to try to fix anything else in the base game.

I'm pleased that the arcing collision fix is now in xpiratez; and I guess I'll just continue to recompile my own .exe with my line-of-fire fix for each version. (At least that change doesn't affect the balance of the game!)

[legionof1]

Don't feel too discouraged. Your fixes are great. Most people just don't have the engine mechanics knowledge to notice the problems even exist. I mean I don't think even Meridian fully realized the issue till he made the hit log feature.

[Meridian]

Don't feel too discouraged. Your fixes are great. Most people just don't have the engine mechanics knowledge to notice the problems even exist. I mean I don't think even Meridian fully realized the issue till he made the hit log feature.

Hit log was created, because I noticed the issue, not the other way around

In your recent updates, there have been a couple of other similar cases. For example, in the most recent build I'm getting compiler error:

Code: [Select]

\OpenXcom\src\Savegame\Soldier.cpp:608:15: error: 'ceil' is not a member of 'std'
which again is fixed by including <cmath>. (It also happened with the previous version, but I can't remember which source file has the problem.)

OK, just post them here, I'll fix it.

In any case, thanks for including the parabolic collision fix in your version. It's good to see my work actually get used. I was happy to contribute, but it's a bit disheartening for me when I put a bit of work into something, only to have it ignored. There seems to be no interest in the arcing shot fix, or my line-of-fire fix on the development forum, and so I'm not inclined to try to fix anything else in the base game.

I'm pleased that the arcing collision fix is now in xpiratez; and I guess I'll just continue to recompile my own .exe with my line-of-fire fix for each version. (At least that change doesn't affect the balance of the game!)

Last time I checked, the line-of-fire patch was not finished... is it finished and tested now? And if yes, where can I find the latest version? Also, could you add a few words what has been changed and why?

It's good to see my work actually get used.

I know exactly what you mean.

[Dioxine]

or my line-of-fire fix on the development forum, and so I'm not inclined to try to fix anything else in the base game.

You've made that work? Man, why did you keep quiet about this! It's a sorely needed feature!

Also, your contribution was likely doomed as soon as you mentioned X-Piratez Jk; the devs have their own visions about OXC and it's normal that they ignore contributions they don't feel would fit. That they don't communicate their decisions? Well, they don't want to burn the bridges, I guess? Surely your fixes remove some of 'vanilla experience'. My point is, the devs never said they will accept all, or any, contributions. I think this, by itself, is understandable.

[Arthanor]

The bow fix was awesome and I am very much looking forward to a LoF fix too. Nothing as frustrating as seeing a soldier land 20/20 shots in a doorframe when told the shot has 50% chance to hit and being allowed to shoot which implied that there is a line of fire.

Fixing that would definitely improve thinges.  I don't care for "vanilla experience" when it is frustratingly buggy. Don't give up man! We need you people who can make the engine better!

[karadoc]

My line-of-fire fix doesn't "fix" the percentages. You can still miss "100%" shots; and in fact, there are still some 100% shots which miss most of the time. (This happens when there is only a very very narrow line of fire. 100% shots still have a small amount of random drift. So if there is only a few voxels of target visible to hit, then you're still likely to miss.) What it does fix is that shots with no line of fire at all will always be reported as having no line of fire.

Here's the fix I'm currently using. It perfect in the way it handles arcing shots, but to be honest, the way arcing shots work is still a bit haphazard, and so this is about as good as we're going to get without doing a heap of other changes. (And there are no impossible shot problems with arcing shots anyway. It's just that the precise voxel aiming is a bit weird sometimes.)

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..c62687f 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;
 }
 
@@ -2632,7 +2635,7 @@ int TileEngine::calculateLine(const Position& origin, const Position& target, bo
  result = voxelCheck(Position(cx, cy, cz), excludeUnit, false, onlyVisible, excludeAllBut);
  if (result != V_EMPTY)
  {
- if (trajectory)
+ if (trajectory != nullptr)
  { https:// store the position of impact
  trajectory->push_back(Position(cx, cy, cz));
  }
@@ -2682,7 +2685,7 @@ int TileEngine::calculateLine(const Position& origin, const Position& target, bo
  result = voxelCheck(Position(cx, cy, cz), excludeUnit, false, onlyVisible, excludeAllBut);
  if (result != V_EMPTY)
  {
- if (trajectory != 0)
+ if (trajectory != nullptr)
  { https:// store the position of impact
  trajectory->push_back(Position(cx, cy, cz));
  }
Here's the message I typed into my own commit

Code: [Select]

commit a0d17322e4a4344eca0cefc7c5eb811d86038e06
Author: karadoc <karadoc@gmail.com>
Date:   Mon Aug 1 19:40:19 2016 +1000

    Fixed line-of-fire problems
   
    TileEngine::canTargetUnit correctly determines whether or not a
    character has line-of-fire to a target, but this information was not
    being correctly used. The result was that the game often behaved as
    though a character has line-of-fire when they actually didn't.
   
    This is now fixed so that the return value of TileEngine::canTargetUnit
    is the primary way of preventing shots with no line-of-fire.
I think I explained the problem in a bit more detail in the other forum; but the gist of it is that canTargetUnit was only being used to determine precisely where a soldier should aim. If there was no line of fire, then usually the shot where they were aiming would make it obvious that they weren't going to hit what they were aiming for. But problem was that at the point in the code where the game decided whether or not to take the shot, there was no way of knowing what the player was actually trying to hit; and so sometimes the game would allow the player to shoot under the assumption that they had deliberately targeted a wall or an empty tile.

My change makes the game check for line-of-fire when it _does_ know what the player was trying to aim at; and so it can correctly abort impossible shots.

One side point is that the same line-of-fire stuff is used to determine where arcing shots are to be aimed at. Arcing shots don't need a direct line-of-fire, but the line-of-fire is still used to determine which voxel they should target. So the change I made at the end of canTargetUnit is just to make sure that arcing shots will target the centre of a unit if there is no direct line of fire whereas previously they would target somewhere on the unit's soldier or something like that - which is an illogical thing to do; and probably results in unnecessary misses.
To be honest, it might be better if arcing shots just always aimed at the centre. I don't think it is helpful or necessary for them to aim at a voxel with direct line-of-fire. The only reason I didn't change that is that I generally like to fix problems with as few side-effects as possible.

The 'nullptr' changes at the end don't actually have any effect whatsoever. I just like to fix that stuff when I see it, for good code hygiene. There was a time when `if (pointer != 0)` was standard practice; but that time is behind us (since nullptr was introduced in the 2011 version of C++).

OK, just post them here, I'll fix it.

There's the one you fixed already in CraftWeapon.cpp (I also changed `floor` to `std::floor`, but it works either way. I suppose there must be a `using std` somewhere else in the code.). The following two files also need the same treatment. (i.e. #include <cmath>)

Code: [Select]

src/Mod/RuleDamageType.cpp
src/Savegame/Soldier.cpp