The Contribution Mega-Thread

Yes! Fixing that SDK issue would be a major contribution.

And since pull-request integration is getting attention this week, let me point out that reviewing a PR also counts as a contribution in my book.

The Engine currently has 10 open pull requests: https://github.com/jMonkeyEngine/jmonkeyengine/pulls?q=is%3Aopen+is%3Apr

Only a few active people are authorized to integrate pull requests. But if you can read this thread, it’s likely you can contribute by reviewing a PR.

I’m particularly concerned about PR 1049, which has been awaiting review since March. It addresses an important bug. It’s a bit complicated, but not outrageously so. Would someone who understands collision detection take a close look, please?

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:

  1. 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

  2. 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 multipleCollisions

  3. 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

  4. 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)

2 Likes