For 1049 I can recommend reviewing file-by-file, because except of
BresenhamTerrainPicker, everyone should be able to review the files. Most stuff is Unit Test related (BaseTest/BaseAWTTest, build.gradle/TerrainTestCollision), and TerrainQuad is only adding a setter and moving some logic into the picker.
For Bresenham there were two issues and unfortunately I did not do multiple PRs or at least commits, but:
https://github.com/jMonkeyEngine/jmonkeyengine/pull/1049/files#diff-d941a2fa3d691e342c2a72202deedacaL108 Here (for instance), a collision is added to results, but the caller https://github.com/jMonkeyEngine/jmonkeyengine/pull/1049/files#diff-d7bd87d72acc390f85a5bd834b05dbbeL280 doesn’t remove the collision, if it was out of range. It simply sets the result of collideWith to 0. This is “legal”, but doesn’t work, because no one will be able to remove the collision from the results again, because:
When colliding against a whole scene, jme performs collideWith recursively and passes the CollisionResults around, which means the results could already contain some other (legit) collisions
The old code assumed, that there is only one intersection possible, which is wrong if you think about mountains.
It’s questionable if there is a use for a multi collision raycast there, but the engine also does it for other geometries, so why not give that power to the user.
For performance reasons, you can disable multi-collisions, which improves performance, because unlike regular raycasts, terrain can walk “the grid”, starting from the start point, it will move along each quad and do collisions there. This means the first collision will always be the closest.
See https://github.com/jMonkeyEngine/jmonkeyengine/pull/1049/files#diff-d941a2fa3d691e342c2a72202deedacaL127, there no multi collisions are possible, but https://github.com/jMonkeyEngine/jmonkeyengine/pull/1049/files#diff-d941a2fa3d691e342c2a72202deedacaR119 fixes the aspect of 1 (only add if within distance), and you can also see
See https://github.com/jMonkeyEngine/jmonkeyengine/pull/1049/files#diff-d941a2fa3d691e342c2a72202deedacaR127 The old behavior was to return “0 collisions”, if the first collision was already out of range. I am uncertain about that
All in All it would be good if everyone would try to use that code within their projects and see if behavior changes. That’s also why I updated the TerrainTest, there I didn’t notice anything odd anymore, especially with MultiCollisions (toggle that in the code, I did not bind it to a key)