#1097-Fixed inconsistent OpenGL version constants

[louhy]

No description provided.

[riccardobl]

🤔 the lwjgl3 patch seems incomplete. The new constants should also be added here

jmonkeyengine/jme3-lwjgl3/src/main/java/com/jme3/system/lwjgl/LwjglWindow.java

Lines 78 to 111 in f45598b

	static {
	RENDER_CONFIGS.put(AppSettings.LWJGL_OPENGL3, () -> {
	glfwWindowHint(GLFW_CONTEXT_VERSION_MAJOR, 3);
	glfwWindowHint(GLFW_CONTEXT_VERSION_MINOR, 2);
	});
	RENDER_CONFIGS.put(AppSettings.LWJGL_OPENGL33, () -> {
	glfwWindowHint(GLFW_CONTEXT_VERSION_MAJOR, 3);
	glfwWindowHint(GLFW_CONTEXT_VERSION_MINOR, 3);
	});
	RENDER_CONFIGS.put(AppSettings.LWJGL_OPENGL4, () -> {
	glfwWindowHint(GLFW_CONTEXT_VERSION_MAJOR, 4);
	glfwWindowHint(GLFW_CONTEXT_VERSION_MINOR, 0);
	});
	RENDER_CONFIGS.put(AppSettings.LWJGL_OPENGL41, () -> {
	glfwWindowHint(GLFW_CONTEXT_VERSION_MAJOR, 4);
	glfwWindowHint(GLFW_CONTEXT_VERSION_MINOR, 1);
	});
	RENDER_CONFIGS.put(AppSettings.LWJGL_OPENGL42, () -> {
	glfwWindowHint(GLFW_CONTEXT_VERSION_MAJOR, 4);
	glfwWindowHint(GLFW_CONTEXT_VERSION_MINOR, 2);
	});
	RENDER_CONFIGS.put(AppSettings.LWJGL_OPENGL43, () -> {
	glfwWindowHint(GLFW_CONTEXT_VERSION_MAJOR, 4);
	glfwWindowHint(GLFW_CONTEXT_VERSION_MINOR, 3);
	});
	RENDER_CONFIGS.put(AppSettings.LWJGL_OPENGL44, () -> {
	glfwWindowHint(GLFW_CONTEXT_VERSION_MAJOR, 4);
	glfwWindowHint(GLFW_CONTEXT_VERSION_MINOR, 4);
	});
	RENDER_CONFIGS.put(AppSettings.LWJGL_OPENGL45, () -> {
	glfwWindowHint(GLFW_CONTEXT_VERSION_MAJOR, 4);
	glfwWindowHint(GLFW_CONTEXT_VERSION_MINOR, 5);
	});
	}

[louhy]

Yeah. Actually now I'm thinking there's more needed too. Ex. this in LwjglContextVR.java should probably accept also LWJGL_OPENGL30. Will take some more time to go over this carefully.

if (settings.getRenderer().equals(AppSettings.LWJGL_OPENGL2)
                || settings.getRenderer().equals(AppSettings.LWJGL_OPENGL3)) {

[louhy]

IMO, now I think I need to do more here. Those constants are public API so I'm afraid that if someone switches to the non-deprecated constants, any internal code that checks them may only be looking for the deprecated ones. Will look deeper at it this afternoon or tonight.

[louhy]

Wasn't so extensive after all, just vr and desktop. Hope that helps.

[riccardobl]

🤔 I have an idea

   @Deprecated
    public static final String LWJGL_OPENGL3 = "LWJGL-OpenGL3";

    public static final String LWJGL_OPENGL32 = LWJGL_OPENGL3;

If you print it, it will say LWJGL-OpenGL3 instead of *32 ... but i think we can live with that

[louhy]

That should work too, but I believe I've corrected the other references - do you have any concerns with how it is now? It (slightly) bothers me that 3 and 3.2 are conflated like this.

[riccardobl]

Now that you brought up the issue, i see that this might be a problem if a library wants to check the renderer and relies on the deprecated constants.

We can either

1. live with the bad naming where 30 is 3.0 and 3 is actually 3.2...
2. make the new constants refer to the old ones even if the string content is still wrong
3. fix the naming and accept that this change might break some usecases < that's how it is with this pr basically

🤔

[riccardobl]

I think the VR renderer can use this patch a68d8b5 since it seems just a modified version of lwjgl2.
This would both fix the issue and enable the other versions like with the lwjgl2 renderer

[louhy]

I'm not following # 2 listed above, but have you seen this commit? 06c2ab9

Maybe I went down a 4th route? I think it's # 3 but hopefully without breakage, NB isn't finding checks of these constants anywhere else. VR was updated to check for both the deprecated 3 and the new 3.2. I'm not familiar with the VR subproject's requirements, so I didn't want to assume it supported anything other than the versions explicitly specified.

The only thing I can imagine going wrong is if someone hard coded the constant strings somewhere in an external project, but that's just... wrong.

[riccardobl]

The only thing I can imagine going wrong is if someone hard coded the constant strings somewhere in an external project, but that's just... wrong.

That's what im talking about, it breaks libraries that check the renderer with getRenderer().equals(LWJGL_OPENGL3) if the main project is using LWJGL_OPENGL32.

2 is this #1099 (comment)

[louhy]

You're saying VR checked for LWJGL_OPENGL3, so if the main project uses LWJGL_OPENGL32, that's a version change from what the VR library expected?

If I understood that correctly, my reasoning and understanding was that although the library was asking for LWJGL_OPENGL3, JME was always assuming 3.2 capabilities anyway, so VR should support both LWJGL_OPENGL3 (which means 3.2) and LWJGL_OPENGL32 (which means 3.2).

Sorry if I'm being dense and you want to move on. I'm sure you're more familiar with how the renderers work and maybe I'm missing under-the-hood context.

If we just need to make this change I can do that. Was that the only one? (Should LWJGL_OPENGL40 = LWJGL_OPENGL4? LWJGL_OPENGL30 = LWJGL_OPENGL3?)

public static final String LWJGL_OPENGL32 = LWJGL_OPENGL3;

[riccardobl]

Let's say you want to use the new naming in your app, for lwjgl you will set the renderer like this

settings.setRenderer(LWJGL_OPENGL32);

let's say you have an external library that does something like this

if(settings.getRenderer().equals(LWJGL_OPENGL3)){
  //enable something
}

(even though i don't think this is something commonly done in jme...)

you would have to update that aswell.

If we were to change the pr to do this

LWJGL_OPENGL32 = LWJGL_OPENGL3
LWJGL_OPENGL40 = LWJGL_OPENGL4

It would be 100% backward compatible and we won't need the double case for lwjgl2 or both LWJGL_OPENGL32,LWJGL_OPENGL3 and LWJGL_OPENGL40,LWJGL_OPENGL4 in RENDER_CONFIGS for lwjgl3. So this might be a better solution? 🤔

[louhy]

Ah, you're talking about external stuff with multiple references, could be a pain. (In JME itself I didn't find much of that.) Yeah it's a bit cleaner in some ways.

Still going to have
LWJGL_OPENGL30 = "LWJGL-OpenGL30";
plus
LWJGL_OPENGL3 = "LWJGL-OpenGL3";

Which seems a little wild: 3 means 3.2 and 30 means 3.0
You sure?

[louhy]

Ok maybe there isn't a perfect solution, see if that looks reasonable to you.

[riccardobl]

Yeah, i think this is good enough. As you say, not perfect, but at least now the constant names make sense.
Let's leave this open for a while and see if others have opinions on the matter