The problem is not the fix. The problem is that the collision point is not in the same tile as the target... it might legitimately be a problem with the hill itself.
Inside TileEngineValidate throw, this condition is what is failing.
if (test != V_OUTOFBOUNDS && (trajectory.at(0) / Position(16, 16, 24)) == (targetVoxel / Position(16, 16, 24)))
The collection point (`trajectory.at(0)`) is in layer 0, whereas the target point is in layer 1. It certainly looks as though we're targeting layer 1, but the ground of the hill is apparently barely still in layer 0.
It "works" in the old version because the old version didn't use the actual collision point, and so it essentially just flukes the right final tile; due to the same bug that we were fixing in the first place. (ie. it doesn't really hit the tile we were aiming out; but the bug says that it does.)
However! The fact that it is possible for the old version to fluke a value that's higher up than the contact point suggests there is another bug somewhere. I think there is a problem in the calculateLine function. It's returning a contact point that is not between the initial and final voxels. I'm looking into it right now.
[edit]
Ok. I've found and corrected the bug in TileEngine::calculateLine.
The problem is when the line is at a 45 degree angle with an odd number of steps, the "drift" step happens before the x step. That doesn't matter much, except that the x step is what is used to check that we've reached the destination; and so if the drift is happening first, then we can overstep the target voxel in the drift direction. eg. If you start at (0, 0), and target (1, 1), the game will check voxels in the following order:
start: (0,0)
drift: (0, 1)
x step: (1, 1)
drift: (1, 2)
(x step is at the target, and so the search is over. But we've already checked (1,2), which was outside of our range.)
Here's a patch to fix the bug.
diff --git a/src/Battlescape/TileEngine.cpp b/src/Battlescape/TileEngine.cpp
index cc4fd65..5fa6cb8 100644
--- a/src/Battlescape/TileEngine.cpp
+++ b/src/Battlescape/TileEngine.cpp
@@ -2652,8 +2652,9 @@ int TileEngine::calculateLine(const Position& origin, const Position& target, bo
https://drift controls when to step in 'shallow' planes
https://starting value keeps Line centred
- drift_xy = (delta_x / 2);
- drift_xz = (delta_x / 2);
+ https:// (round up, to prevent overflow drift at the end)
+ drift_xy = (delta_x+1) / 2;
+ drift_xz = (delta_x+1) / 2;
https://direction of line
step_x = 1; if (x0 > x1) { step_x = -1; }
Here's what I put in my commit log.
commit 1b132b82be31a341bacce7dfa6b281d7a43d25a8
Author: karadoc
Date: Sun Sep 4 10:43:47 2016 +1000
Fixed TileEngine::calculateLine to avoid checking outside the range.
On 45 degree angles, the starting value of drift needs to be rounded up
to avoid drifting further than the primary axis on the final step.
---
Unfortunately, this bug fix doesn't fix the issue you asked about. In fact, it makes it so that the same issue appears without the original arc collision fix too.
I know that you've reverted my original fix in your code as a result of this issue, but please think carefully about what's going on. The patches I've posted here (both the original fix and the fix in this post) are correcting real bugs, and the patches are simple enough that you can confirm how they work even without thorough testing.
So although the issue you have asked about only manifests after my fixes are applied, the fixes are not what is causing the issue. The issue is caused by some other problem which should be dealt with separately - certainly not by reverting the arc fix.
At some point I might look again to see how the target tile is decided, and see if that can be improved to correct the issue. But in the mean time, I'd suggest that the best way to fix the issue is to fix the hill. The actual collision points on the hill are in layer zero, whereas the hill appears to be in layer 1. That's the root of the issue.