LWJGL 3.4.0 and Remove OpenVR #2581
Conversation
@stephengold notified me that LWJGL 3.4 has been released and has also done some testing in their non jME projects with this new version already. (big thanks for keeping a tab on this area) I still have most of my own jME projects running lwjgl2 ('ve been procrastinating upgrading to LWJGL3 for quite a while now, long overdue), so I will finally do so in my own projects so I can help test as well. OpenVR is also no longer supported in LWJGL 3.4.0 so it appears it needed removed, but (unless I'm mistaken) this should not be an issue since the Tamarin VR library is using the newer openXR, and openVR is considered outdated and deprecated. Any jME apps still relying on openVR can still use v3.9 or earlier without issue (@richardTingle correct me if I'm incorrect on any of this)
|
🖼️ Screenshot tests have failed. The purpose of these tests is to ensure that changes introduced in this PR don't break visual features. They are visual unit tests. 📄 Where to find the report:
✅ If you did mean to change things: ✨ If you are creating entirely new tests: Note; it is very important that the committed reference images are created on the build pipeline, locally created images are not reliable. Similarly tests will fail locally but you can look at the report to check they are "visually similar". See https://github.com/jMonkeyEngine/jmonkeyengine/blob/master/jme3-screenshot-tests/README.md for more information Contact @richardTingle (aka richtea) for guidance if required |
|
Screenshot tests and IOS/andriod builds failed so i think i may be incorrect that OpenVR needed removed... |
|
It's the jme3-vr module that is using openvr. jme3-vr also has some duplicated jme internal classes inside, and it is always a bit of a pain to backport the changes. |
|
Fixed the jme3-vr module as you mentioned, and the IOs/Android builds are good now. Although It looks like we may also need to remove some tests/examples related to openVR too, considering the screenshot tests failed. Edit: I also overlooked some classes relying on OpenVR that need resolved for desktop builds |
|
I think we need to remove the jme3-vr module entirely, since it won't work without openvr anyway |
|
Good news: I finally resolved everything and got the android, IOS, and desktop builds to succeed. There were some references to the (now deleted) jme3-vr module in some unexpected places that needed deleted. And there was a small change with the new LWJGL 3.4.0 version where a method now returns a long instead of int. Which also required changing a switch statement to an if statement because (unbeknownst to me prior) switch statements don't work with longs prior to java 25. I personally hate switch statements and never use them anyway, so this is yet another reason reinforcing my personal bias against using switch statements instead of ifs 😆 Bad news: It looks like the majority of our screenshot tests now fail due to this upgrade, and it sounds like this may simply be due to some very minor pixel level differences with the new version of LWJGL 3.4.0 so (unless I'm mistaken on this or made a mistake that I have yet to notice) we will also have to update most of our screenshot tests that failed to match the new correct screenshot. I will hold off on this for now since I still want to have others review my changes to make sure everything in this PR indeed looks good first. The newly generated screenshots in the "changed-images" directory do look correct: 
So this likely confirms that everything is working and we just need to update the new images for these failing tests. |
|
jme3-vr: (nice job!) |
|
Nothing was changed, but it looks like tests are passing now... |
|
For completeness getting rid of the old VR stuff was discussed previously at https://hub.jmonkeyengine.org/t/should-osvr-very-old-vr-standard-be-removed-from-jme/48234. I agree that it is time to get rid of it. It is very outdated and we agreed previously to support VR as user supported libraries. |
| physicsDebugRootNode.setCullHint(Spatial.CullHint.Never); | ||
|
|
||
| if (isVr()) { | ||
| /* This is a less good solution than the non-vr version (as the debug shapes can be obscured by the regular |
There was a problem hiding this comment.
I think I wrote this and I'm so glad to see it go. Nasty hack that didn't really achieve what the debug state is supposed to do (because jme3-vr never supported view overlays)
Yes, this is all correct. Tamarin 2 onwards completely divorced itself from jme3-vr. I'll need to do a Tamarin update to also bring it up to LWJGL 4.0.0 but that's no big deal |
@stephengold notified me that LWJGL 3.4 has been released and has also done some testing in their non jME projects with this new version already. (big thanks for keeping a tab on this area)
I still have most of my own jME projects running lwjgl2 ('ve been procrastinating upgrading to LWJGL3 for quite a while now, long overdue), so I will finally do so in my own projects so I can help test as well.
OpenVR is also no longer supported in LWJGL 3.4.0 so it appears it needed removed, but (unless I'm mistaken) this should not be an issue since the Tamarin VR library is using the newer openXR, and openVR is considered outdated and deprecated. Any jME apps still relying on openVR can still use v3.9 or earlier without issue (@richardTingle correct me if I'm incorrect on any of this)