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:

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. Here (for instance), a collision is added to results, but the caller 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, there no multi collisions are possible, but fixes the aspect of 1 (only add if within distance), and you can also see multipleCollisions

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


Requesting a quick review of PR 1181.

This looks to be an easy fix: issue 1197

1 Like

Continuous integration at TravisCI appears to be broken again. Would someone please take a look?

As we discussed in pm, i took this opportunity to merge the pr that moved the CI to github actions. It should build successfully now.