- Adjusting various comments in EXPERIMENTAL_HIGH_PRECISION_MATH_Q3MAP2_FIXES

code, etc.

- In CreateBrushWindings(), not going into FixWindingAccu() (in 
EXPERIMENTAL_HIGH_PRECISION_MATH_Q3MAP2_FIXES code path) until a winding has
been chopped against all sides.  I don't see any reason to do it the old way
of fixing the winding after each chop, especially since endpoints of the 
winding	may not naturally lie on integer endpoint at that point.

- In q3map2.h, setting normalEpsilon and distanceEpsilon based on
EXPERIMENTAL_SNAP_NORMAL_FIX and EXPERIMENTAL_HIGH_PRECISION_MATH_Q3MAP2_FIXES,
respectively.  The epsilons are adjusted to compensate for the new nature
of the code.

- Removing unused "ourNormalEpsilon" in SnapNormal().

- In FindFloatPlane(), changing '>' to '>=' for comparison of distanceEpsilon
to be consistent with definition of epsilon and with other code.


git-svn-id: svn://svn.icculus.org/gtkradiant/GtkRadiant/branches/Rambetter-math-fix-experiments@414 8a3a26a2-13c4-0310-b231-cf6edde360e5

tools/quake3/common/polylib.c

@@ -668,7 +668,7 @@ void ChopWindingInPlaceAccu(winding_accu_t **inout, vec3_t normal, vec_t dist, v
 	// front, point 4 might be in back, and so on.  So we could end up with a very ugly-
 	// looking chopped winding, and this might be undesirable, and would at least lead to
 	// a possible exhaustion of MAX_POINTS_ON_WINDING.  It's better to assume that points
-	// very very close to the plane ore on the plane, using an infinitesimal epsilon amount.
+	// very very close to the plane are on the plane, using an infinitesimal epsilon amount.
 
 	// Now, the original ChopWindingInPlace() function used a vec_t-based winding_t.
 	// So this minimum epsilon is quite similar to casting the higher resolution numbers to
@@ -707,21 +707,21 @@ void ChopWindingInPlaceAccu(winding_accu_t **inout, vec3_t normal, vec_t dist, v
 	sides[i] = sides[0];
 	dists[i] = dists[0];
 	
+	// I'm wondering if whatever code that handles duplicate planes is robust enough
+	// that we never get a case where two nearly equal planes result in 2 NULL windings
+	// due to the 'if' statement below.  TODO: Investigate this.
 	if (!counts[SIDE_FRONT]) {
 		FreeWindingAccu(in);
 		*inout = NULL;
 		return;
 	}
-	// TODO: I'm wondering what happens if a brush has 2 planes that are the same.  In
-	// other words, nothing is on SIDE_FRONT but everything is on SIDE_ON.  This will cause
-	// both of the duplicate planes to have a NULL winding, as far as I can tell, because if
-	// all points are on SIDE_ON, the above "if" statement will be true and the winding will
-	// be NULL'ed out.  This may actually be the legacy and desired behavior.  Write a
-	// regression test for this.
 	if (!counts[SIDE_BACK]) {
 		return; // Winding is unmodified.
 	}
 
+	// NOTE: The least number of points that a winding can have at this point is 2.
+	// In that case, one point is SIDE_FRONT and the other is SIDE_BACK.
+
 	maxpts = counts[SIDE_FRONT] + 2; // We dynamically expand if this is too small.
 	f = AllocWindingAccu(maxpts);
 

tools/quake3/q3map2/brush.c

@@ -289,7 +289,12 @@ void SnapWeldVectorAccu(vec3_accu_t a, vec3_accu_t b, vec3_accu_t out)
 {
 	// I'm just preserving what I think was the intended logic of the original
 	// SnapWeldVector().  I'm not actually sure where this function should even
-	// be used.
+	// be used.  I'd like to know which kinds of problems this function addresses.
+
+	// TODO: I thought we're snapping all coordinates to nearest 1/8 unit?
+	// So what is natural about snapping to the nearest integer?  Maybe we should
+	// be snapping to the nearest 1/8 unit instead?
+
 	int		i;
 	vec_accu_t	ai, bi, ad, bd;
 	
@@ -414,6 +419,11 @@ qboolean FixWindingAccu(winding_accu_t *w)
 			dist = VectorLengthAccu(vec);
 			if (dist < DEGENERATE_EPSILON)
 			{
+				// TODO: I think the "snap weld vector" was written before
+				// some of the math precision fixes, and its purpose was
+				// probably to address math accuracy issues.  We can think
+				// about changing the logic here.  Maybe once plane distance
+				// gets 64 bits, we can look at it then.
 				SnapWeldVectorAccu(w->p[i], w->p[j], vec);
 				VectorCopyAccu(vec, w->p[i]);
 				for (k = j + 1; k < w->numpoints; k++)
@@ -488,6 +498,17 @@ qboolean CreateBrushWindings( brush_t *brush )
 #endif
 			
 			/* ydnar: fix broken windings that would generate trifans */
+#if EXPERIMENTAL_HIGH_PRECISION_MATH_Q3MAP2_FIXES
+			// I think it's better to FixWindingAccu() once after we chop with all planes
+			// so that error isn't multiplied.  There is nothing natural about welding
+			// the points unless they are the final endpoints.  ChopWindingInPlaceAccu()
+			// is able to handle all kinds of degenerate windings.
+#else
+			FixWinding( w );
+#endif
+		}
+		
+		/* set side winding */
 #if EXPERIMENTAL_HIGH_PRECISION_MATH_Q3MAP2_FIXES
 		if (w != NULL)
 		{
@@ -498,13 +519,6 @@ qboolean CreateBrushWindings( brush_t *brush )
 				w = NULL;
 			}
 		}
-#else
-			FixWinding( w );
-#endif
-		}
-		
-		/* set side winding */
-#if EXPERIMENTAL_HIGH_PRECISION_MATH_Q3MAP2_FIXES
 		side->winding = (w ? CopyWindingAccuToRegular(w) : NULL);
 		if (w) FreeWindingAccu(w);
 #else

tools/quake3/q3map2/map.c

@@ -154,14 +154,14 @@ int CreateNewFloatPlane (vec3_t normal, vec_t dist)
 
 /*
 SnapNormal()
-snaps a near-axial normal vector
+Snaps a near-axial normal vector.
+Returns qtrue if and only if the normal was adjusted.
 */
 
 qboolean SnapNormal( vec3_t normal )
 {
 #if EXPERIMENTAL_SNAP_NORMAL_FIX
 	int		i;
-	vec_t		ourNormalEpsilon;
 	qboolean	adjusted = qfalse;
 
 	// A change from the original SnapNormal() is that we snap each
@@ -219,11 +219,8 @@ qboolean SnapNormal( vec3_t normal )
 	// the epsilon be measured against the vector components close to 0, not 1.0.
 	// I think the logic should be: if 2 of the normal components are within
 	// epsilon of 0, then the vector can be snapped to be perfectly axial.
-	// The epsilon should probably be adjusted to a larger value when we make this
-	// code change.  Really, this code must be incorrect.  Rambetter will address
-	// this issue after the base winding math problems are solved.  In fact I will
-	// create a regression test that will display the effects of this coarse
-	// snapping behavior.
+	// We may consider adjusting the epsilon to a larger value when we make this
+	// code fix.
 
 	for( i = 0; i < 3; i++ )
 	{
@@ -365,9 +362,15 @@ int FindFloatPlane( vec3_t normal, vec_t dist, int numPoints, vec3_t *points )
 			/* ydnar: test supplied points against this plane */
 			for( j = 0; j < numPoints; j++ )
 			{
+				// NOTE: When dist approaches 2^16, the resolution of 32 bit floating
+				// point number is greatly decreased.  The distanceEpsilon cannot be
+				// very small when world coordinates extend to 2^16.  Making the
+				// dot product here in 64 bit land will not really help the situation
+				// because the error will already be carried in dist.
 				d = DotProduct( points[ j ], normal ) - dist;
-				if( fabs( d ) > distanceEpsilon )
-					break;
+				d = fabs(d);
+				if (d != 0.0 && d >= distanceEpsilon)
+					break; // Point is too far from plane.
 			}
 			
 			/* found a matching plane */
@@ -428,6 +431,9 @@ int MapPlaneFromPoints( vec3_t *p )
 	VectorSubtractAccu(paccu[2], paccu[1], t2);
 	CrossProductAccu(t1, t2, normalAccu);
 	VectorNormalizeAccu(normalAccu, normalAccu);
+	// TODO: A 32 bit float for the plane distance isn't enough resolution
+	// if the plane is 2^16 units away from the origin (the "epsilon" approaches
+	// 0.01 in that case).
 	dist = (vec_t) DotProductAccu(paccu[0], normalAccu);
 	VectorCopyAccuToRegular(normalAccu, normal);
 

tools/quake3/q3map2/q3map2.h

@@ -1930,8 +1930,30 @@ Q_EXTERN qboolean			debugSurfaces Q_ASSIGN( qfalse );
 Q_EXTERN qboolean			debugInset Q_ASSIGN( qfalse );
 Q_EXTERN qboolean			debugPortals Q_ASSIGN( qfalse );
 
+#if EXPERIMENTAL_SNAP_NORMAL_FIX
+// Increasing the normalEpsilon to compensate for new logic in SnapNormal(), where
+// this epsilon is now used to compare against 0 components instead of the 1 or -1
+// components.  Unfortunately, normalEpsilon is also used in PlaneEqual().  So changing
+// this will affect anything that calls PlaneEqual() as well (which are, at the time
+// of this writing, FindFloatPlane() and AddBrushBevels()).
+Q_EXTERN double				normalEpsilon Q_ASSIGN(0.00005);
+#else
 Q_EXTERN double				normalEpsilon Q_ASSIGN( 0.00001 );
+#endif
+
+#if EXPERIMENTAL_HIGH_PRECISION_MATH_Q3MAP2_FIXES
+// NOTE: This distanceEpsilon is too small if parts of the map are at maximum world
+// extents (in the range of plus or minus 2^16).  The smallest epsilon at values
+// close to 2^16 is about 0.007, which is greater than distanceEpsilon.  Therefore,
+// maps should be constrained to about 2^15, otherwise slightly undesirable effects
+// may result.  The 0.01 distanceEpsilon used previously is just too coarse in my
+// opinion.  The real fix for this problem is to have 64 bit distances and then make
+// this epsilon even smaller, or to constrain world coordinates to plus minus 2^15
+// (or even 2^14).
+Q_EXTERN double				distanceEpsilon Q_ASSIGN(0.005);
+#else
 Q_EXTERN double				distanceEpsilon Q_ASSIGN( 0.01 );
+#endif
 
 
 /* bsp */