Patch: http://mindwidgets.com/jme/jme2-GameTask-threading-07272008.patch.txt
I went to look at issues with JMEDesktop, but decided it was more important to make the com.jme.util.GameTask* classes thread-safe first. I also fixed a bunch of tests that use the GameTask classes. Here are notes from my first pass:
- GameTaskQueueManager, GameTaskQueue, GameTask are now thread-safe
Notes: if no one should inherit from GameTaskQueueManager the class should be final + private contructor, or it could use the recommended singleton enum pattern (http://en.wikipedia.org/wiki/Singleton_pattern#The_Enum-way)
- Tests I ran/fixed that use GameTaskQueueManager and work for me on my single core desktop:
jmetest.awt.swingui.TestJMEDesktopState
jmetest.effects.RenParticleEditor (fixed AWT access in main method and some compiler warnings, didn't look at anything else)
jmetest.TestStandardGame
jmetest.game.state.TestDebugGameState
jmetest.game.state.TestJMEDesktopState
jmetest.game.state.TestLoadingGameState
jmetest.input.controls.TestSwingControlEditor
jmetest.shape.Test3DFlatText
- Tests that have issues but worked on my single core desktop
jmetest.shape.TestMultiFaceBox (AWT thread issues in here and TextureManager, didn't fix)
jmetest.util.JMESwingTest (AWT thread issues, didn't fix)
- Still lots of issues with JMEDesktop, will look into those soon
Side note: Is it better to put large patches in the code blocks in a post or should I upload it to my server? I didn't know if there were limits or formatting issues with the in-lined code blocks.
What confuses me (maybe it's just the wording) is those tests already work for me on my single core PC and on my dual core Mac…? Are you just saying they still work?
edit: Didn't know about the "Void" class. Nice. 
I've applied the patch, but it's not 1.5 compliant (Override annotation can not be used on interface methods in 1.5). I'll clean it up locally and it should be up in svn later today.
renanse said:
What confuses me (maybe it's just the wording) is those tests already work for me on my single core PC and on my dual core Mac...? Are you just saying they still work?
I should have worded it differently. I didn't test those classes prior to my changes, I just tried to make sure I didn't break anything so I tested every place I saw GameTask* used. After my initial changes, the AWT and OpenGL threading issues really started to show themselves as missing UI components, dead locks, and NPE's usually when creating textures. If you apply all changes except anything in the jmetest packages you'll see what I mean.
renanse said:
edit: Didn't know about the "Void" class. Nice. :)
Kinda seems like a hack to me on the part of the Java spec guys, but its more clear than Object :) I wish it was smart enough so we didn't have to return null.
renanse said:
I've applied the patch, but it's not 1.5 compliant (Override annotation can not be used on interface methods in 1.5). I'll clean it up locally and it should be up in svn later today.
Awe crap, forgot about that. At work, I'm usually the one using java 1.5 complaining about the guys introducing 1.6-only code like that :) String.isEmpty() gets me all the time too.
There is a small issue with this patch in jMEDesktopState.
After the patch, the JMEDesktop Constructor is not anymore called with the help of GameTaskQueueManager in the OpenGL thread, but this is still needed.
There is a TextureState.apply() which gets called in JMEDesktop.setup() which needs to be executed in the Opengl thread.
BUT… its not needed anyway. :) (or is there any reason to call RenderState.apply() when not rendering :?)
I didn't notice any side effect after removing ts.apply() and now it won't throw a NPE if JMEDEsktop is not created in the OpenGl thread.
Index: src/com/jmex/awt/swingui/JMEDesktop.java
===================================================================
--- src/com/jmex/awt/swingui/JMEDesktop.java (revision 3998)
+++ src/com/jmex/awt/swingui/JMEDesktop.java (working copy)
@@ -369,7 +369,6 @@
texture.setScale( new Vector3f( 1, -1, 1 ) );
ts.setTexture( texture );
- ts.apply();
this.setRenderState( ts );
BlendState alpha = DisplaySystem.getDisplaySystem().getRenderer().createBlendState();
edit:
i just stumbled over this thread, where apply() is used as some kind of warm-up.
http://www.jmonkeyengine.com/jmeforum/index.php?topic=3908.0
i'm not sure if this applies to JMEDesktop also, i didn't notice any laggyness, if it turns out to be a problem, maybe a warmUp() method could be added.
The only reason for that apply would be to force the texture to load immediately rather than the first time the object it is applied to is rendered. Sometimes that is good because it reduces the chances of odd pauses when objects appear in view. Probably not a huge issue in this context though.
heh ok, i found the other thread and edited my posting while you answered.
if it really turns out to be a problem, one could add the JMEDesktop.warmUp() method, which does that, so the constructor would still be thread save.