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:
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 Fix and Improve TerrainPicking by MeFisto94 · Pull Request #1049 · jMonkeyEngine/jmonkeyengine · GitHub, there no multi collisions are possible, but Fix and Improve TerrainPicking by MeFisto94 · Pull Request #1049 · jMonkeyEngine/jmonkeyengine · GitHub fixes the aspect of 1 (only add if within distance), and you can also see multipleCollisions
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)
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
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.
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.
