Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some fixes in jme-vr #1163

Merged
merged 3 commits into from
Aug 29, 2019
Merged

Some fixes in jme-vr #1163

merged 3 commits into from
Aug 29, 2019

Conversation

grizeldi
Copy link
Member

This PR:

  • Adds Valve Index to the list of recognized HMDs
  • Brings the overall lwjgl version of the jme-vr module up to speed with the rest of the engine. This fixes OpenVR lwjgl backend.
  • Removes Linux from the list of blacklisted OSes when lwjgl backend is used. In my testing, I got a picture inside the HMD on Linux, but it's cropped to be only the top half of the rendered view for some reason. Debugging this issue led me to believe that the version of OpenVR lwjgl 3.2.1 ships with is bugged on Linux, so this is unlikely to be fixed until we update lwjgl. On Windows it works fine.

Also, is it just me or does git really think I deleted the whole files and then created them again?

@Ali-RS
Copy link
Member

Ali-RS commented Aug 27, 2019

Also, is it just me or does git really think I deleted the whole files and then created them again?

Yes, it displays you removed −1,209 and added +1,223. It makes the code reviewing hard. Any possibility to open another one with only the changed lines?

@grizeldi
Copy link
Member Author

Yeah, thought so. Go home git, you're drunk.

Not really sure what to change to not get this result as all I did was regular commits, push to a new branch on github and open a PR. No idea how this happened. Maybe I can retry.

@MeFisto94
Copy link
Member

Just click on the settings wheel and then select "hide whitespace changes" and it looks good.
I guess grizeldi did some reformatting changing tabs to spaces or something

Copy link
Member

@MeFisto94 MeFisto94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there problems when the lwjgl versio doesn't match the one in core?
Also the OS detection seems a bit strange to me, like there won't be any OS called Bindows, Kindows or whatever, which will be like Windows, so I don't understand why not have

OS.startsWith("windows") || OS.startsWith("linux")

But obviously that's how it has been implemented so far.

@grizeldi
Copy link
Member Author

Are there problems when the lwjgl versio doesn't match the one in core?

Yes, I got crashes about some abstract void not returning the correct type.

Exception in thread "main" java.lang.AbstractMethodError: org.lwjgl.system.StructBuffer.getElementFactory()Lorg/lwjgl/system/Struct;
    at org.lwjgl.system.StructBuffer.get(StructBuffer.java:90)
    at com.jme3.input.vr.lwjgl_openvr.LWJGLOpenVR.initialize(LWJGLOpenVR.java:180)
    at com.jme3.app.VREnvironment.initialize(VREnvironment.java:477)
    at com.grizeldi.vrtests.Main.main(Main.java:29)

@stephengold stephengold merged commit 484d192 into jMonkeyEngine:master Aug 29, 2019
@jseinturier
Copy link
Contributor

jseinturier commented Aug 29, 2019

@MeFisto94 Yes the OS.startsWith("indows") || OS.startsWith("nux") is from the very first implementation and could be updated. The initial concern was to process "Windows" and "windows" but as now we use lower case OS, this is no more revelant.

Regarding this PR, my concern is that the files seem to be completely rewritten and i cannot see what are the changes provided. I'm not agree with validating this PR without seeing the real changes.

@MeFisto94
Copy link
Member

MeFisto94 commented Aug 29, 2019

@jseinturier take a look at what I described above, there you can see the changes. Essentially it adds a constant to the enum for valve index, bumps lwjgl version and "supports linux".

Yes, I got crashes about some abstract void not returning the correct type.

So we need to change jme3-lwjgl3 as well? So this should/could've been in this pr as well?

@grizeldi
Copy link
Member Author

So we need to change jme3-lwjgl3 as well? So this should/could've been in this pr as well?

No, since jme-lwjgl3 is already on lwjgl version 3.2.1. Only jme-vr was on 3.2.0. Fixing the inconsistency fixed the crash, since I assume it was native related.

@jseinturier
Copy link
Contributor

Be also careful if you re using both jme3-lwjgl and jme3-lwjgl3 within your application. This can lead to unstability.

@grizeldi
Copy link
Member Author

As far as I'm aware lwjgl 2 doesn't have OpenVR bindings, so you can't really use it with jme-vr.

@jseinturier
Copy link
Contributor

jseinturier commented Aug 29, 2019

Of course but if you're integrating a jme3-vr based module within large JME application with other jme3-lwjgl modules that can lead to inconsistencies between lwjgl2 and lwjgl3 are some classes have same names but different implementations. A solution is to ensure that lwjgl3 stuff is loaded before lwjgl2 stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants