The Contribution Mega-Thread

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

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?

https://travis-ci.org/jMonkeyEngine/jmonkeyengine/builds/595261829

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.

2 Likes

Here’s another easy issue to address: https://github.com/jMonkeyEngine/jmonkeyengine/issues/1210

1 Like

I have a holiday next week, I’ll try to squeeze in some work on the GitHub issues.

Maybe a bit off topic, but it might be related to improvements/work on the engine… I notice that since some time now, the asset pipeline is pointing towards gltf. This importer is maintained and is the best option to load/import models into jme. The importer however is using by default the PBRLighting material. The fragment shader used by this material PBRLighting.frag is however incompatible with GLSL 1.10 which is the default of OpenGL2.0 which is also the default of jme…

(Or at least it is incompatible on my machine, bumping the version of OpenGL to 3.2 removes this issue)

I don’t know what you guys think of it, but I think we should either bump the default version of jme to OpenGL3.2 or change the behaviour of the gltf importer to use the Lighting material by default instead of the PBR one. As of now a new user will extend SimpleApplication load a model and will be presented with an error.

Stacktrace:

Uncaught exception thrown in Thread[jME3 Main,5,main]
RendererException: compile error in: ShaderSource[name=Common/MatDefs/Light/PBRLighting.frag, defines, type=Fragment, language=GLSL110]
ERROR: 0:193: GLSL 110 does not allow sub- or super-matrix constructors
ERROR: 0:194: Use of undeclared identifier 'wToLocalRot'
ERROR: 0:194: Use of undeclared identifier 'wToLocalRot'
ERROR: 0:207: Use of undeclared identifier 'wToLocalRot'
ERROR: 0:208: Use of undeclared identifier 'rayLs'
ERROR: 0:211: Use of undeclared identifier 'wToLocalRot'
ERROR: 0:215: Use of undeclared identifier 'rayLs'
ERROR: 0:216: Use of undeclared identifier 'rayLs'
ERROR: 0:217: Use of undeclared identifier 'firstPlaneIntersect'
ERROR: 0:217: Use of undeclared identifier 'secondPlaneIntersect'
ERROR: 0:218: Use of undeclared identifier 'furthestPlane'
ERROR: 0:218: Use of undeclared identifier 'furthestPlane'
ERROR: 0:218: Use of undeclared identifier 'furthestPlane'
ERROR: 0:220: Attempt to use 'distance' as a variable
ERROR: 0:221: Use of undeclared identifier 'intersectPositionWs'

edit: or leave everything as is and fix the fragment shader. I didn’t look at it yet, but if I remember correctly newer versions of GLSL extract the upper-left part of matrices. This way you can create a mat3 from a mat4 for example. I’m not saying this is the error in this particular case, I just remembered that I made this mistake before :slight_smile:

1 Like

The engine uses by default the compatibility profile, it is sometimes not be in pair with the core profile supported by the same driver, and a source of many issues. My personal opinion is that the engine should use by default a more recent core profile, 3.2 would be probably the most logical choice, since it is old enough to be supported by pretty much every driver (2009) and contains also all the most important features.

1 Like

Important discussion, but definitely off-topic.

2 Likes

Also note that this particular issue mostly only happens with Mac OS X and thus most people don’t see this issue with PBR

Just putting out there that for those who are interested in contributing, there is a project on github that gives you a guage of what you may be able to achieve; especially if it’s your first time getting your feet wet.

5 Likes

Here’s an interesting and important issue involving filters and MSAA, just opened yesterday:
https://github.com/jMonkeyEngine/jmonkeyengine/issues/1246

GitHub is hopping. Several recent PRs are ready for review:

3 Likes

Also ready for review: PR 1268

Looking for someone who understands LWJGL3.

2 Likes

Recent cleanup efforts uncovered a couple easy first fixes:

1 Like

Code review requested for PR 1308.

1 Like

Here’s a new “Easy First Fix” for someone to self-assign:

I think @plassma wants to fix this, but I am not 100% certain

I just opened another “Easy first fix” issue:
https://github.com/jMonkeyEngine/jmonkeyengine/issues/1347

3 Likes

The advantage of gltf is pbr coloring. If you change to phong lighting, I feel that gltf is not needed. It is better to use the obj model. The mainstream is pbr coloring. I suggest that gltf should be pbr coloring by default. This is the advantage of gltf. If you really want to change to phong coloring, why not provide user settings, don’t write it in the code. Otherwise, it is painful to replace the materials one by one, especially when the scene is complicated. :worried: