jMonkeyEngine / jmonkeyengine

A complete 3-D game development suite written in Java.
http://jmonkeyengine.org
BSD 3-Clause "New" or "Revised" License
3.81k stars 1.12k forks source link

IllegalStateException when running TestAWTPanels with 3.6.0-beta1and and LWJGL 3 #1945

Closed Ali-RS closed 1 year ago

Ali-RS commented 1 year ago

Testing with JME 3.6.0-beta1 and LWJGL 3 on Linux I am getting this exception in:

> Task :TestAwtPanels.main()
Feb 09, 2023 1:45:24 PM com.jme3.system.JmeDesktopSystem initialize
INFO: Running on jMonkeyEngine 3.6.0-beta1
 * Branch: HEAD
 * Git Hash: ee91fb8
 * Build Date: 2023-01-24
Feb 09, 2023 1:45:25 PM com.jme3.app.LegacyApplication handleError
SEVERE: Failed to create display
java.lang.IllegalStateException
    at com.jme3.system.awt.AwtPanelsContext$AwtPanelsListener.reshape(AwtPanelsContext.java:67)
    at com.jme3.system.lwjgl.LwjglWindow.updateSizes(LwjglWindow.java:393)
    at com.jme3.system.lwjgl.LwjglWindow.createContext(LwjglWindow.java:370)
    at com.jme3.system.lwjgl.LwjglWindow.initInThread(LwjglWindow.java:582)
    at com.jme3.system.lwjgl.LwjglWindow.run(LwjglWindow.java:704)
    at java.lang.Thread.run(Thread.java:750)

https://github.com/jMonkeyEngine/jmonkeyengine/blob/925ff4561b29fff22fe2e47ecc63d129cb3a4000/jme3-desktop/src/main/java/com/jme3/system/awt/AwtPanelsContext.java#L65-L68

I think this has something to do with changes made in PR https://github.com/jMonkeyEngine/jmonkeyengine/pull/1794

This results in some redundant callbacks (multiple callbacks for a single createContext())

Ali-RS commented 1 year ago

Should I modify this method to just log an error instead of throwing an exception?

https://github.com/jMonkeyEngine/jmonkeyengine/blob/925ff4561b29fff22fe2e47ecc63d129cb3a4000/jme3-desktop/src/main/java/com/jme3/system/awt/AwtPanelsContext.java#L65-L68

stephengold commented 1 year ago

Should I modify this method to just log an error instead of throwing an exception?

I think that would be a step in the right direction.

Ali-RS commented 1 year ago

I noticed oldFramebufferWidth and oldFramebufferHeight are never initialized to their default values provided in AppSettings (settings.getWidth(), settings.getHeight()) in LwjglWindow.createContext().

https://github.com/jMonkeyEngine/jmonkeyengine/blob/3d203a16dfba741a7b6e8566f6ee465c2cb3dc89/jme3-lwjgl3/src/main/java/com/jme3/system/lwjgl/LwjglWindow.java#L156-L157

Was this by intent? should we initialize them to the default value in AppSettings? by adding

oldFramebufferWidth = settings.getWidth();
oldFramebufferHeight = settings.getHeight();

in LwjglWindow.createContext() before calling to updateSizes();

https://github.com/jMonkeyEngine/jmonkeyengine/blob/3d203a16dfba741a7b6e8566f6ee465c2cb3dc89/jme3-lwjgl3/src/main/java/com/jme3/system/lwjgl/LwjglWindow.java#L370

this should prevent a redundant call to the reshape method.

@stephengold any thoughts?

tonihele commented 1 year ago

Yeah, I also fixed these here https://github.com/jMonkeyEngine/jmonkeyengine/pull/1868/files#diff-08ccfbcf3d7d07136246c83ffab37eb165bf63cc68a3a52370873710c3d6daee

to get things working..

Ali-RS commented 1 year ago

Also found a small mistake here

https://github.com/jMonkeyEngine/jmonkeyengine/pull/1794/files#r1103590630

Ali-RS commented 1 year ago

Yeah, I also fixed these here

I made it to a separate PR (with also a fix for https://github.com/jMonkeyEngine/jmonkeyengine/issues/1945#issuecomment-1426688843) so that can be directly ported/cherry-picked to v3.6 as well. https://github.com/jMonkeyEngine/jmonkeyengine/pull/1950

Sorry if this will cause "conflicts" on your PR :(

stephengold commented 1 year ago

I noticed oldFramebufferWidth and oldFramebufferHeight are never initialized to their default values provided in AppSettings (settings.getWidth(), settings.getHeight()) in LwjglWindow.createContext().

https://github.com/jMonkeyEngine/jmonkeyengine/blob/3d203a16dfba741a7b6e8566f6ee465c2cb3dc89/jme3-lwjgl3/src/main/java/com/jme3/system/lwjgl/LwjglWindow.java#L156-L157

Was this by intent? should we initialize them to the default value in AppSettings? by adding

oldFramebufferWidth = settings.getWidth();
oldFramebufferHeight = settings.getHeight();

in LwjglWindow.createContext() before calling to updateSizes();

https://github.com/jMonkeyEngine/jmonkeyengine/blob/3d203a16dfba741a7b6e8566f6ee465c2cb3dc89/jme3-lwjgl3/src/main/java/com/jme3/system/lwjgl/LwjglWindow.java#L370

this should prevent a redundant call to the reshape method.

@stephengold any thoughts?

I'm not 100% convinced that initializing oldFrameBufferWidth based on AppSettings is the right thing to do.

There's nothing to prevent the application from storing invalid values in AppSettings. There's also the possibility that values (such as -1) which are valid for AppSettings might not be valid for oldFrameBufferWidth. And as I recall, the values passed to glfwCreateWindow() are hints; some window managers might ignore them.

On the other hand, I can't think of how invalid values in oldFrameBufferWidth could create issues other than an extra callback, which is something we have right now.

Ali-RS commented 1 year ago

I'm not 100% convinced that initializing oldFrameBufferWidth based on AppSettings is the right thing to do.

Please let me know if I need to make changes to https://github.com/jMonkeyEngine/jmonkeyengine/pull/1950