Issue number: 5

Fixed Issue 5 (FlyingGuns crash in native code on Canvas removal).


git-svn-id: https://svn.java.net/svn/j3d-core~svn/trunk@94 ba19aa83-45c5-6ac9-afd3-db810772062c

4 files changed, 134 insertions, 71 deletions

diff --git a/src/classes/share/javax/media/j3d/Canvas3D.java b/src/classes/share/javax/media/j3d/Canvas3D.java
index 55821e0..86f5316 100644
--- a/src/classes/share/javax/media/j3d/Canvas3D.java
+++ b/src/classes/share/javax/media/j3d/Canvas3D.java
@@ -1437,6 +1437,7 @@ public class Canvas3D extends Canvas {
 	evaluateActive();	
 
         VirtualUniverse.mc.freeCanvasBit(canvasBit);
+	canvasBit = 0;
 
 	ra = null;
 	graphicsContext3D = null;
@@ -2423,6 +2424,7 @@ public class Canvas3D extends Canvas {
     native boolean initTexturemapping(long ctx, int texWidth, 
 				      int texHeight, int objectId);
 
+
     /**
      * Sets the position of the manual left eye in image-plate
      * coordinates.  This value determines eye placement when a head
@@ -4092,7 +4094,6 @@ public class Canvas3D extends Canvas {
 	if ((screen != null) && 
 	    (screen.renderer != null) &&
 	    (ctx != 0)) {
-
 	    VirtualUniverse.mc.postRequest(MasterControl.FREE_CONTEXT, 
 					   new Object[]{this, 
 							new Long(screen.display), 
@@ -4299,7 +4300,8 @@ public class Canvas3D extends Canvas {
 	TextureRetained tex;
 	DetailTextureImage detailTex;
 	
-	if (rdr == null) {
+	// Just return if we don't have a valid renderer or context
+	if (rdr == null || ctx == 0) {
 	    return;
 	}
 
diff --git a/src/classes/share/javax/media/j3d/GeometryArrayRetained.java b/src/classes/share/javax/media/j3d/GeometryArrayRetained.java
index 774371d..75ed141 100644
--- a/src/classes/share/javax/media/j3d/GeometryArrayRetained.java
+++ b/src/classes/share/javax/media/j3d/GeometryArrayRetained.java
@@ -16,6 +16,9 @@ import com.sun.j3d.internal.Distance;
 import javax.vecmath.*;
 import java.lang.Math;
 import java.util.ArrayList;
+import java.util.Set;
+import java.util.HashSet;
+import java.util.HashMap;
 import java.util.Vector;
 import java.util.Enumeration;
 import com.sun.j3d.internal.ByteBufferWrapper;
@@ -220,7 +223,6 @@ abstract class GeometryArrayRetained extends GeometryRetained{
     byte[][] mirrorUnsignedByteRefColors= new byte[1][];
     float[][] mirrorInterleavedColorPointer = null;
 
-    
     // This native method builds a native representation of this object, then
     // returns the nativeId.
     native int build(int geoType);
@@ -263,19 +265,21 @@ abstract class GeometryArrayRetained extends GeometryRetained{
     // or a unique canvas otherwise
     int resourceCreationMask = 0x0;
 
+    // Fix for Issue 5
+    //
+    // Replace the per-canvas reference count with a per-RenderBin set
+    // of users.  The per-RenderBin set of users of this display list
+    // is defined as a HashMap where:
+    //
+    //   key   = the RenderBin
+    //   value = a set of RenderAtomListInfo objects using this
+    //           geometry array for display list purposes
+    private HashMap dlistUsers = null;
 
-    // Reference count of renderMolecules per dlist created, this is either
-    // per renderer for useSharedCtx  or per Canvas for non-shared ctx
-    // note since
-    // renderer and canvasindex starts from 1, the first entry of this
-    // array will never be used.
-    int[] renderMolPerDlist = new int[2];
-
-    // timestamp used to create display list; same as above, this is either
+    // timestamp used to create display list. This is either
     // one per renderer for useSharedCtx, or one per Canvas for non-shared
     // ctx
-    long[] timeStampPerDlist = new long[2];
-
+    private long[] timeStampPerDlist = new long[2];
 
     // Unique display list Id, if this geometry is shared
     int dlistId = -1;
@@ -9803,40 +9807,58 @@ abstract class GeometryArrayRetained extends GeometryRetained{
 	}
     }
 
-    void incrDlistRefCount(int bit) {
-	int index = getIndex(bit);
-	int[] newList = null;
-	int i;
-	synchronized(renderMolPerDlist) {
-	    if (index >= renderMolPerDlist.length) {
-		newList = new int[index * 2];
-		for (i = 0; i < renderMolPerDlist.length; i++) {
-		    newList[i] = renderMolPerDlist[i];
-		}
-		newList[index] = 1;
+    // Add the specified render atom as a user of this geometry array
+    // (for the specified render bin)
+    void addDlistUser(RenderBin renderBin, RenderAtomListInfo ra) {
+	if (dlistUsers == null) {
+	    dlistUsers = new HashMap(2, 1.0f);
+	}
 
-		// This must be the last statment for 
-		// sync. to work correctly
-		renderMolPerDlist = newList;
-	    }
-	    else {
-		renderMolPerDlist[index]++;
-	    }
+	Set raSet = (Set)dlistUsers.get(renderBin);
+	if (raSet == null) {
+	    raSet = new HashSet();
+	    dlistUsers.put(renderBin, raSet);
 	}
+	raSet.add(ra);
     }
 
-    int decrDlistRefCount(int rdrBit) {
-	int index = getIndex(rdrBit);
-	synchronized(renderMolPerDlist) {
-	    if (index >= renderMolPerDlist.length) {
-		// In case of sharedDlist it is possible that
-		// decrDlistRefCount is invoke for cleanup
-		// from another canvas
-		return -1;
-	    }
-	    renderMolPerDlist[index]--;
-	    return renderMolPerDlist[index];
+    // Remove the specified render atom from the set of users of this
+    // geometry array (for the specified render bin)
+    void removeDlistUser(RenderBin renderBin, RenderAtomListInfo ra) {
+	if (dlistUsers == null) {
+	    // Nothing to do
+	    return;
+	}
+
+	Set raSet = (Set)dlistUsers.get(renderBin);
+	if (raSet == null) {
+	    // Nothing to do
+	    return;
+	}
+	raSet.remove(ra);
+    }
+
+    // Returns true if the set of render atoms using this geometry
+    // array in the specified render bin is empty.
+    boolean isDlistUserSetEmpty(RenderBin renderBin) {
+	if (dlistUsers == null) {
+	    return true;
+	}
+
+	Set raSet = (Set)dlistUsers.get(renderBin);
+	if (raSet == null) {
+	    return true;
+	}
+	return raSet.isEmpty();
+    }
+
+    // This method is used for debugging only
+    int numDlistUsers(RenderBin renderBin) {
+	if (isDlistUserSetEmpty(renderBin)) {
+	    return 0;
 	}
+	Set raSet = (Set)dlistUsers.get(renderBin);
+	return raSet.size();
     }
 
     void setDlistTimeStamp(int rdrBit, long timeStamp) {
diff --git a/src/classes/share/javax/media/j3d/RenderBin.java b/src/classes/share/javax/media/j3d/RenderBin.java
index ff14754..7f69f0a 100644
--- a/src/classes/share/javax/media/j3d/RenderBin.java
+++ b/src/classes/share/javax/media/j3d/RenderBin.java
@@ -203,7 +203,9 @@ class RenderBin extends J3dStructure  implements ObjectUpdate {
 			      RenderMolecule.RENDER_MOLECULE_LIST, null);
 
     // List of renderAtoms that have a shared dlist (due to geo.refCount > 1)
-    UnorderList sharedDList = new UnorderList(5, RenderAtomListInfo.class);
+    // Fix for Issue 5: change this to a Set rather than a list to
+    // avoid duplicates entried
+    Collection sharedDList = new HashSet();
 
     ArrayList dirtyRenderMoleculeList = new ArrayList(5);
 
@@ -943,12 +945,19 @@ class RenderBin extends J3dStructure  implements ObjectUpdate {
     }
     
     
+    // Shared context case
     void updateDlistRendererResource(Renderer rdr) {
-	int i, value = 0;
+	int i;
 	int size = 0;
 	RenderAtomListInfo arr[];
 	RenderAtomListInfo ra;
 	
+	// TODO: there is a possible problem in the case of multiple
+	// renderers (i.e., multiple screens).  Unless the
+	// MasterControl sends us a separate message for each
+	// renderer, we won't create a new display list for renderers
+	// other than the one passed into this method.
+
 	if (rdr == null) {
 	    return;
 	}
@@ -963,7 +972,7 @@ class RenderBin extends J3dStructure  implements ObjectUpdate {
 		// it is built for no longer matches the context
 		// used in the renderer, create a dlist
 		sharedDList.add(ra);
-		geo.incrDlistRefCount(rdr.rendererBit);
+		geo.addDlistUser(this, ra);
 
 		if (((geo.resourceCreationMask & rdr.rendererBit) == 0) ||
 		    (geo.getDlistTimeStamp(rdr.rendererBit) != 
@@ -981,10 +990,10 @@ class RenderBin extends J3dStructure  implements ObjectUpdate {
 		sharedDList.remove(ra);
 
 		GeometryArrayRetained geo = (GeometryArrayRetained)ra.geometry();
-		value = geo.decrDlistRefCount(rdr.rendererBit);
+		geo.removeDlistUser(this, ra);
 		//		System.out.println("========> geo.refcount = "+geo.refCount);
 		// add this geometry's dlist to be freed
-		if (value == 0) {
+		if (geo.isDlistUserSetEmpty(this)) {
 		    rdr.displayListResourceFreeList.add(geo.dlistObj);
 		    geo.resourceCreationMask &= ~rdr.rendererBit;
 		    // All Dlist on all renderer have been freed, then return dlistID
@@ -1007,8 +1016,9 @@ class RenderBin extends J3dStructure  implements ObjectUpdate {
 	}
     }
     
+    // Non-shared context case
     void updateDlistCanvasResource(Canvas3D[] canvases)  {
-	int i, j, value;
+	int i, j;
 	Canvas3D cv;
 	int size = 0;
 	RenderAtomListInfo arr[];
@@ -1019,6 +1029,10 @@ class RenderBin extends J3dStructure  implements ObjectUpdate {
 	    arr = (RenderAtomListInfo []) addDlist.toArray(false);
 	    for (i = 0; i <size; i++) {
 		sharedDList.add(arr[i]);
+		// Fix for Issue 5: add the render atom to the list of users
+		// of its geometry for this RenderBin
+		GeometryArrayRetained geo = (GeometryArrayRetained) arr[i].geometry();
+		geo.addDlistUser(this, arr[i]);
 	    }
 	}
 
@@ -1027,6 +1041,10 @@ class RenderBin extends J3dStructure  implements ObjectUpdate {
 	    arr = (RenderAtomListInfo []) removeDlist.toArray(false);
 	    for (i = 0; i < size; i++) {
 		sharedDList.remove(arr[i]);
+		// Fix for Issue 5: remove this render atom from the list of users
+		// of its geometry for this RenderBin
+		GeometryArrayRetained geo = (GeometryArrayRetained) arr[i].geometry();
+		geo.removeDlistUser(this, arr[i]);
 	    }
 	}
 		    
@@ -1040,7 +1058,6 @@ class RenderBin extends J3dStructure  implements ObjectUpdate {
 		    ra = arr[i];
 		    GeometryArrayRetained geo = (GeometryArrayRetained) ra.geometry();
 
-		    geo.incrDlistRefCount(cv.canvasBit);
 		    if ((cv.ctx != 0) &&
 			((geo.resourceCreationMask & cv.canvasBit) == 0) || 
 			(geo.getDlistTimeStamp(cv.canvasBit) != 
@@ -1056,9 +1073,8 @@ class RenderBin extends J3dStructure  implements ObjectUpdate {
 		    GeometryArrayRetained geo =
 			(GeometryArrayRetained) arr[i].geometry();
 
-		    value = geo.decrDlistRefCount(canvases[j].canvasBit);
 		    // add this geometry's dlist to be freed
-		    if (value == 0) {
+		    if (geo.isDlistUserSetEmpty(this)) {
 			if (cv.ctx != 0) {
 			    canvases[j].displayListResourceFreeList.add(geo.dlistObj);
 			}
@@ -3124,8 +3140,8 @@ System.out.println("......tb.soleUser= " +
 	if (size > 0) {
 	    RenderAtomListInfo ra;
 	    GeometryArrayRetained geo;
-	    RenderAtomListInfo arr[] =
-		(RenderAtomListInfo []) sharedDList.toArray(false);	
+	    RenderAtomListInfo arr[] = new RenderAtomListInfo[size];
+	    arr = (RenderAtomListInfo []) sharedDList.toArray(arr);	
 	    int bitMask = cv.canvasBit;
 
 	    // We need two passes to avoid extra buildDisplayList
@@ -3173,8 +3189,8 @@ System.out.println("......tb.soleUser= " +
 
 	size =  sharedDList.size();
 	if (size > 0) {
-	    RenderAtomListInfo arr[] =
-		(RenderAtomListInfo []) sharedDList.toArray(false);	
+	    RenderAtomListInfo arr[] = new RenderAtomListInfo[size];
+	    arr = (RenderAtomListInfo []) sharedDList.toArray(arr);	
 	    RenderAtomListInfo ra;
 
 	    if (!setCtx) {
@@ -6702,6 +6718,7 @@ System.out.println("......tb.soleUser= " +
 
 
     void freeAllDisplayListResources(Canvas3D cv) {
+
 	int i;
 	int size = renderMoleculeList.size();
 	Renderer rdr = cv.screen.renderer;
@@ -6717,25 +6734,25 @@ System.out.println("......tb.soleUser= " +
 
 	size =  sharedDList.size();
 	if (size > 0) {
-	    RenderAtomListInfo arr[] =
-		(RenderAtomListInfo []) sharedDList.toArray(false);
-	    
+	    RenderAtomListInfo arr[] = new RenderAtomListInfo[size];
+	    arr = (RenderAtomListInfo []) sharedDList.toArray(arr);	
+
 	    GeometryArrayRetained geo; 
 	    int mask = (cv.useSharedCtx ? rdr.rendererBit : cv.canvasBit);
 
 	    for (i = 0; i < size; i++) {
 		geo = (GeometryArrayRetained)arr[i].geometry();
+		// Fix for Issue 5: free all native display lists and clear the
+		// context creation bits for this canvas, but don't do anything
+		// with the geo's user list.
 		if (geo.dlistId > 0) {
-		    if (!cv.useSharedCtx) {
-			cv.freeDisplayList(cv.ctx, geo.dlistId);
-		    }
-
-		    if (geo.decrDlistRefCount(mask) == 0) {
-			geo.resourceCreationMask &= ~mask;
-			if (cv.useSharedCtx) {
-			    cv.freeDisplayList(cv.ctx, geo.dlistId);
-			}
-		    }
+		    // TODO: for the shared ctx case, we really should
+		    // only free the display lists if this is the last
+		    // Canvas in the renderer.  However, since the
+		    // display lists will be recreated, it doesn't
+		    // really matter.
+		    cv.freeDisplayList(cv.ctx, geo.dlistId);
+		    geo.resourceCreationMask &= ~mask;
 		}
 	    }
 	}
@@ -6758,8 +6775,8 @@ System.out.println("......tb.soleUser= " +
 
 	size =  sharedDList.size();
 	if (size > 0) {
-	    RenderAtomListInfo arr[] =
-		(RenderAtomListInfo []) sharedDList.toArray(false);	
+	    RenderAtomListInfo arr[] = new RenderAtomListInfo[size];
+	    arr = (RenderAtomListInfo []) sharedDList.toArray(arr);	
 	    GeometryArrayRetained geo; 
 
 	    for (i = 0; i < size; i++) {
diff --git a/src/native/ogl/Canvas3D.c b/src/native/ogl/Canvas3D.c
index 4db442f..1899e9c 100644
--- a/src/native/ogl/Canvas3D.c
+++ b/src/native/ogl/Canvas3D.c
@@ -2334,7 +2334,11 @@ void JNICALL Java_javax_media_j3d_Canvas3D_newDisplayList(
     jlong ctxInfo,
     jint id)
 {
-    
+    if (id <= 0) {
+	fprintf(stderr, "JAVA 3D ERROR : glNewList(%d) -- IGNORED\n", id);
+	return;
+    }
+
     glNewList(id, GL_COMPILE);
 }
 
@@ -2391,6 +2395,19 @@ void JNICALL Java_javax_media_j3d_Canvas3D_callDisplayList(
 {
     GraphicsContextPropertiesInfo *ctxProperties = (GraphicsContextPropertiesInfo *)ctxInfo; 
     jlong ctx = ctxProperties->context;
+    static int numInvalidLists = 0;
+
+    if (id <= 0) {
+	if (numInvalidLists < 3) {
+	    fprintf(stderr, "JAVA 3D ERROR : glCallList(%d) -- IGNORED\n", id);
+	    ++numInvalidLists;
+	}
+	else if (numInvalidLists == 3) {
+	    fprintf(stderr, "JAVA 3D : further glCallList error messages discarded\n");
+	    ++numInvalidLists;
+	}
+	return;
+    }
 
     /* resale_normal_ext */
     if (ctxProperties->rescale_normal_ext && isNonUniformScale) {
@@ -2412,6 +2429,11 @@ void JNICALL Java_javax_media_j3d_Canvas3D_freeDisplayList(
     jint id)
 {
     
+    if (id <= 0) {
+	fprintf(stderr, "JAVA 3D ERROR : glDeleteLists(%d,1) -- IGNORED\n", id);
+	return;
+    }
+
     glDeleteLists(id, 1);
 }