IllegalArgumentException in registerObject()

[stephengold]

This issue was reported at the Discourse hub/forum: https://hub.jmonkeyengine.org/t/java-lang-illegalargumentexception-object-id-must-be-greater-than-zero-on-mac/46559

jmonkeyengine/jme3-core/src/main/java/com/jme3/util/NativeObjectManager.java

Lines 104 to 107 in 44a50de

	public void registerObject(NativeObject obj) {
	if (obj.getId() <= 0) {
	throw new IllegalArgumentException("object id must be greater than zero");
	}

While JME specifically reserves the value -1 for invalid IDs:

jmonkeyengine/jme3-core/src/main/java/com/jme3/util/NativeObject.java

Line 47 in 44a50de

public static final int INVALID_ID = -1;

I believe negative IDs in general don't justify throwing an exception.

The OpenGL documentation doesn't specify range of values for a texture name, only that they are GLuint:

• https://registry.khronos.org/OpenGL-Refpages/gl4/html/glGenBuffers.xhtml
• https://registry.khronos.org/OpenGL-Refpages/gl4/html/glGenTextures.xhtml

According to the OpenGL spec, GLuint is at least 32 bits, so values > 0x7fffffff will produce negative values in a Java int.

I think the tests in lines 105 and 132 should be specifically for == INVALID_ID instead of <= 0.

[stephengold]

The GLRenderer class appears to have similar issues. In some places, it checks against -1; in others, it requires > 0.

• jmonkeyengine/jme3-core/src/main/java/com/jme3/renderer/opengl/GLRenderer.java

  Line 1254 in 44a50de

  assert shader.getId() > 0;

• jmonkeyengine/jme3-core/src/main/java/com/jme3/renderer/opengl/GLRenderer.java

  Line 1364 in 44a50de

  assert shader.getId() > 0;

• jmonkeyengine/jme3-core/src/main/java/com/jme3/renderer/opengl/GLRenderer.java

  Lines 1455 to 1456 in 44a50de

  	int id = source.getId();
  	if (id == -1) {

• jmonkeyengine/jme3-core/src/main/java/com/jme3/renderer/opengl/GLRenderer.java

  Line 1679 in 44a50de

  assert shader.getId() > 0;

• jmonkeyengine/jme3-core/src/main/java/com/jme3/renderer/opengl/GLRenderer.java

  Line 1700 in 44a50de

  if (shader.getId() == -1) {

[pspeed42]

re: "While JME specifically reserves the value -1 for invalid IDs:"

...and that might actually be a valid ID also. Maybe JME should track invalid IDs in a different way.

[stephengold]

We could store IDs as long, which would open up many possibilities.

[Sailsman63]

It appears that INVALID_ID is a semaphore for "Not connected to a native-side object". Not sure if there's a better way to track this. (It feels like something left over from a time when every byte in any object was at a premium.)

[pspeed42]

Or Integer (capital 'i') so that it could be nullable.

[neph1]

I think the tests in lines 105 and 132 should be specifically for == INVALID_ID instead of <= 0.

I will try this locally and see how it works both on linux and try to test it on mac as well. Thanks for looking into this.

[neph1]

Got some more info from the modified build. App now loads but window is black. It seems the assertion was legit in this case:

java.lang.IllegalStateException: Some video driver error or programming error occurred. Framebuffer object status is invalid.
	at com.jme3.renderer.opengl.GLRenderer.checkFrameBufferError(GLRenderer.java:1838)
	at com.jme3.renderer.opengl.GLRenderer.updateFrameBuffer(GLRenderer.java:1996)
	at com.jme3.renderer.opengl.GLRenderer.setFrameBuffer(GLRenderer.java:2127)
	at com.jme3.renderer.RenderManager.renderViewPort(RenderManager.java:1189)
	at com.jme3.renderer.RenderManager.render(RenderManager.java:1287)
	at com.jme3.app.SimpleApplication.update(SimpleApplication.java:278)
	at com.jme3.system.lwjgl.LwjglWindow.runLoop(LwjglWindow.java:580)
	at com.jme3.system.lwjgl.LwjglWindow.run(LwjglWindow.java:669)
	at com.jme3.system.lwjgl.LwjglWindow.create(LwjglWindow.java:493)
	at com.jme3.app.LegacyApplication.start(LegacyApplication.java:490)
	at com.jme3.app.LegacyApplication.start(LegacyApplication.java:442)
	at com.jme3.app.SimpleApplication.start(SimpleApplication.java:126)

And then

SEVERE: Failed to destroy context
java.lang.IllegalArgumentException: The Image[size=1920x1080, format=Depth, id=0] NativeObject is not registered in this NativeObjectManager
	at com.jme3.util.NativeObjectManager.deleteNativeObject(NativeObjectManager.java:138)
	at com.jme3.util.NativeObjectManager.deleteUnused(NativeObjectManager.java:181)
	at com.jme3.util.NativeObjectManager.deleteAllObjects(NativeObjectManager.java:206)
	at com.jme3.renderer.opengl.GLRenderer.cleanup(GLRenderer.java:724)
	at com.jme3.system.lwjgl.LwjglWindow.destroyContext(LwjglWindow.java:442)
	at com.jme3.system.lwjgl.LwjglWindow.deinitInThread(LwjglWindow.java:646)
	at com.jme3.system.lwjgl.LwjglWindow.lambda$initInThread$12(LwjglWindow.java:518)
	at java.base/java.lang.Thread.dispatchUncaughtException(Unknown Source)

But I guess that is due to the first exception.
There's also a
WARNING: JmeDialogsFactory implementation not found.
Both for the first exception and the second. Presumably jme want's to show the exceptions in a dialog and fails.

The graphics card info makes me believe this is maybe related to Radeon cards, and not Macs:

INFO: OpenGL Renderer Information
 * Vendor: ATI Technologies Inc.
 * Renderer: AMD Radeon Pro Vega 20 OpenGL Engine
 * OpenGL Version: 4.1 ATI-4.8.101
 * GLSL Version: 4.10
 * Profile: Core

Would it be worth downgrading lwjgl to 2.9?

[Ali-RS]

There's also a
WARNING: JmeDialogsFactory implementation not found.
Both for the first exception and the second. Presumably jme want's to show the exceptions in a dialog and fails.

This warning can be resolved by adding the jme3-awt-dialogs module but also I believe swing + lwjgl3 is problematic on mac and I guess that is why awt dialogs were moved out in JME 3.6 in the first place.

The graphics card info makes me believe this is maybe related to Radeon cards, and not Macs:

Just in case, I am using an AMD ATI Radeon graphics card on Linux and do not have such issue.

[neph1]

One thing I notice is that
@param waitFor true→wait for the context to be initialized,
is false in the start sequence. Could that be relevant perhaps in combination with the
-XstartOnMainThread
flag that is required for mac? Or just for some reason in general.

[Ali-RS]

I think the waitFor flag has no effect on mac

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

Lines 533 to 555 in a0c2c5e

	public void create(boolean waitFor) {
	if (created.get()) {
	LOGGER.warning("create() called when display is already created!");
	return;
	}
	if (Platform.get() == Platform.MACOSX) {
	// NOTE: this is required for Mac OS X!
	mainThread = Thread.currentThread();
	mainThread.setName("jME3 Main");
	if (waitFor) {
	LOGGER.warning("create(true) is not supported for macOS!");
	}
	run();
	} else {
	mainThread = new Thread(this, "jME3 Main");
	mainThread.start();
	if (waitFor) {
	waitFor(true);
	}
	}
	}

[stephengold]

More evidence that JME requires native-object IDs to be non-negative:

jmonkeyengine/jme3-core/src/main/java/com/jme3/audio/AudioBuffer.java

Lines 133 to 136 in cef0d3b

	@Override
	public long getUniqueId() {
	return ((long) OBJTYPE_AUDIOBUFFER << 32) | ((long) id);
	}

If the ID were negative, sign extension during the conversion to long would obliterate the object-type information.

[andygibson]

Might the answer here be encapsulation? Exposing the id and using it to indicate both the id of the object and the validity of it seems to be the root of the problem. Since -1 (or other negative numbers) can be a valid ID but also a signal for an invalid ID/Object, it is contradictory.

We can encapsulate the id, and provide separate methods to indicate object validity. By removing access to id, we are able to ensure we can capture and fix every case where the id is used or updated. Instead of checking for -1, we can check the isValid method. When we delete the object, we can call an invalidate() method to clear the id and the validity flag.

Behind the scenes, we can either use a separate boolean valid field, store it in the top half of a long id, or even still use -1. How it is implement can change once encapsulated, and tbh, the JVM will probably end up inlining those getter calls to direct variable accesses anyway.

I've put in PR: #2051 as a prototype for what I mean. Perhaps anyone who can reproduce the problem can see if it fixes it.

[riccardobl]

Encapsulation can be a breaking change for older code.
Since the issue is that the id is an uint that overflows when converted to signed int, I have a different proposal:

• store the unsigned id in a long.
• return the long from getId().
• add setId(long).
• deprecate setId(int) and replace its code to convert the overflowed int to an unsigned int .
• warn when setId(-1) is called and ask the developer to use setId(INVALID_ID) instead.

Assuming most code uses setId(INVALID_ID) and not setId(-1), this shouldn't cause any issue, and if it doesn't then there is the warning that gives a chance to figure out that there is something wrong and how to fix it.

Pseudo code:

public static final **long** INVALID_ID=-1;
private **long** id;

public long getId(){
    return id;
}

@Deprecated
public void setId(int id){
    if(id==-1){
       warn("ID was set to -1. Note: this is a valid ID now, please use setId(INVALID_ID) to reset the id to the invalid ID");
    }
    this.id=toUnsignedInt(id);    
}

public void setId(**long** id){
    this.id=id;
}