Re: [brlcad-devel] [brlcad-commits] SF.net SVN: brlcad:[41463] brlcad/trunk/src/libbn/plane.c

[Christopher S. M. <br...@ma...>]

I'm very worried about this change.

While I don't doubt that the new tolerance values may have made a  
particular conversion case better, I'd be surprised if the  
sensitivity across random inputs is improved by setting those "less  
strict" tolerances.  The magic numbers are now merely tuned to a  
specific set of input geometry.  It could very well make conversion  
worse for other geometry.

I'll see if the new conversion script shows an improvement, but a  
more general solution is needed.  If there is a bn_tol struct, that  
is an alternative source of tolerance values less strict than  
VUNITIZE_TOL and SMALL_FASTF.

If having a specific magic number other than one of the well-defined  
defaults (like VUNITIZE_TOL and SMALL_FASTF), then it should also be  
documented in detail so someone else isn't left guessing why such  
specific values are in the code.

Cheers!
Sean




On Nov 24, 2010, at 9:07 PM, r_...@us... wrote:

> Revision: 41463
>           http://brlcad.svn.sourceforge.net/brlcad/?rev=41463&view=rev
> Author:   r_weiss
> Date:     2010-11-25 02:07:28 +0000 (Thu, 25 Nov 2010)
>
> Log Message:
> -----------
> Made changes to functions bn_isect_line3_line3 and bn_coplanar.  
> Updated most of the tolerances in these functions using values  
> determined by capturing values during test runs and determining  
> where the values converge to 0, 1, -1 etc. These tolerances are not  
> perfect and I believe these values should be computed since they  
> can vary. These tolerance changes appear to improve the results of  
> the mged 'ev' command and 'facetize' command and fewer error  
> messages are reported.
>
> Modified Paths:
> --------------
>     brlcad/trunk/src/libbn/plane.c
>
> Modified: brlcad/trunk/src/libbn/plane.c
> ===================================================================
> --- brlcad/trunk/src/libbn/plane.c	2010-11-24 20:37:44 UTC (rev 41462)
> +++ brlcad/trunk/src/libbn/plane.c	2010-11-25 02:07:28 UTC (rev 41463)
> @@ -1235,7 +1235,6 @@
>      int parallel = 0;
>      int colinear = 0;
>
> -
>      BN_CK_TOL(tol);
>
>      if (NEAR_ZERO(MAGSQ(c), VUNITIZE_TOL) || NEAR_ZERO(MAGSQ(d),  
> VUNITIZE_TOL)) {
> @@ -1254,11 +1253,12 @@
>
>      VCROSS(n, d, c);
>      det = VDOT(n, p) - VDOT(n, a);
> -    if (!NEAR_ZERO(det, SMALL_FASTF)) {
> +
> +    if (!NEAR_ZERO(det, 1.0e-10)) {
>  	return -1; /* no intersection, lines not in same plane */
>      }
>
> -    if (NEAR_ZERO(MAGSQ(n), VUNITIZE_TOL)) {
> +    if (NEAR_ZERO(MAGSQ(n), 1.0e-19)) {
>  	/* lines are parallel, must find another way to get normal vector */
>  	vect_t a_to_p;
>
> @@ -1266,7 +1266,7 @@
>  	VSUB2(a_to_p, p, a);
>  	VCROSS(n, a_to_p, d);
>
> -	if (NEAR_ZERO(MAGSQ(n), VUNITIZE_TOL)) {
> +	if (NEAR_ZERO(MAGSQ(n), 1.0e-19)) {
>  	    /* lines are parallel and colinear */
>
>              colinear = 1;
> @@ -1274,7 +1274,7 @@
>  	}
>      }
>
> -    if (NEAR_ZERO(MAGSQ(n), VUNITIZE_TOL)) {
> +    if (NEAR_ZERO(MAGSQ(n), 1.0e-19)) {
>  	bu_bomb("bn_isect_line3_line3(): ortho vector zero magnitude\n");
>      }
>
> @@ -1370,11 +1370,9 @@
>       */
>      VSUB2(h, a, p);
>      det = c[q] * d[r] - d[q] * c[r];
> -    det1 = (c[q] * h[r] - h[q] * c[r]);		/* see below */
> -    /* XXX This should be no smaller than 1e-16.  See
> -     * bn_isect_line2_line2 for details.
> -     */
> -    if (NEAR_ZERO(det, DETERMINANT_TOL)) {
> +    det1 = (c[q] * h[r] - h[q] * c[r]);
> +
> +    if (NEAR_ZERO(det, 1.0e-10)) {
>  	/* Lines are parallel */
>  	if (!colinear || !NEAR_ZERO(det1, DETERMINANT_TOL)) {
>  	    return -2;	/* parallel, not colinear, no intersection */
> @@ -1420,15 +1418,9 @@
>
>      /* Check that these values of t and u satisfy the 3rd equation as
>       * well!
> -     *
> -     * XXX It isn't clear that "det" is exactly a model-space
> -     * distance.
>       */
>      det = *t * d[s] - *u * c[s] - h[s];
> -    if (!NEAR_ZERO(det, SMALL_FASTF)) {
> -	/* XXX This tolerance needs to be much less loose than
> -	 * SQRT_SMALL_FASTF.  What about DETERMINANT_TOL?
> -	 */
> +    if (!NEAR_ZERO(det, 1.0e-23)) {
>  	/* Inconsistent solution, lines miss each other */
>  	return -1;
>      }
> @@ -2244,15 +2236,16 @@
>      }
>
>      dot = VDOT(a, b);
> +
>      VSCALE(pt_a, a, a[3]);
>      VSCALE(pt_b, b, b[3]);
>
> -    if (NEAR_ZERO(dot, SMALL_FASTF)) {
> +    if (NEAR_ZERO(dot, 1.0e-8)) {
>  	return 0; /* planes are perpendicular */
>      }
>
> -    /* parallel is when dot is within SMALL_FASTF of either -1 or  
> 1 */
> -    if ((dot <= -SMALL_FASTF) ? (NEAR_ZERO(dot + 1.0,  
> SMALL_FASTF)) : (NEAR_ZERO(dot - 1.0, SMALL_FASTF))) {
> +    /* parallel is when dot is within 4.5e-16 of either -1 or 1 */
> +    if ((dot <= -SMALL_FASTF) ? (NEAR_ZERO(dot + 1.0, 4.5e-16)) :  
> (NEAR_ZERO(dot - 1.0, 4.5e-16))) {
>  	if (bn_pt3_pt3_equal(pt_a, pt_b, tol)) {
>  	    /* test for coplanar */
>  	    if (dot >= SMALL_FASTF) {
>
>
> This was sent by the SourceForge.net collaborative development  
> platform, the world's largest Open Source development site.
>
> ---------------------------------------------------------------------- 
> --------
> Increase Visibility of Your 3D Game App & Earn a Chance To Win $500!
> Tap into the largest installed PC base & get more eyes on your game by
> optimizing for Intel(R) Graphics Technology. Get started today with  
> the
> Intel(R) Software Partner Program. Five $500 cash prizes are up for  
> grabs.
> http://p.sf.net/sfu/intelisp-dev2dev
> _______________________________________________
> BRL-CAD Source Commits mailing list
> brl...@li...
> https://lists.sourceforge.net/lists/listinfo/brlcad-commits