gnuplot-beta

More fallout from TBOOLEAN change - hidden3d broken

[Ethan M. <merritt@u.washington.edu>]

Is there a tool (or gcc compile option) that will
double check for illegal operations on our TBOOLEANs? 

In stepping through the demos to check 3.8k.3 I found
that something very ugly has happened to hidden3d.
See "surface2.dem".

Reverting to the previous version of src/syscfg.h
made the problem go away, so this was another
bit of fallout from the TBOOLEAN change.

hidden3d.c line 817:
        casenumber = (plist[new_poly].frontfacing != 0)
            + 2 * (plist[old_poly].frontfacing != 0);

frontfacing is a TBOOLEAN.

Even worse:
            TBOOLEAN v1_inside_p, v2_inside_p;
             unsigned int classification[]
-- 
            v1_inside_p = ~ (classification[0]
                             | classification[1]
                             | classification[2]);
--
            v2_inside_p = (v1_inside_p >> 1) & 0x1;
            v1_inside_p = v1_inside_p & 0x1;

I have corrected the syntax involving frontfacing,
and changed v?_inside_p to be (unsigned int).
This fixes surface2.dem, but other problems may lurk.

           

-- 
Ethan A Merritt       merritt@u.washington.edu
Biomolecular Structure Center
Mailstop 357742
University of Washington, Seattle, WA 98195

Re: More fallout from TBOOLEAN change - hidden3d broken

[Daniel J S. <dan...@ie...>]

Ethan Merritt wrote:

>Is there a tool (or gcc compile option) that will
>double check for illegal operations on our TBOOLEANs? 
>
>In stepping through the demos to check 3.8k.3 I found
>that something very ugly has happened to hidden3d.
>See "surface2.dem".
>
>Reverting to the previous version of src/syscfg.h
>made the problem go away, so this was another
>bit of fallout from the TBOOLEAN change.
>
>hidden3d.c line 817:
>        casenumber = (plist[new_poly].frontfacing != 0)
>            + 2 * (plist[old_poly].frontfacing != 0);
>
>frontfacing is a TBOOLEAN.
>
>Even worse:
>            TBOOLEAN v1_inside_p, v2_inside_p;
>             unsigned int classification[]
>
>-- 
>            v1_inside_p = ~ (classification[0]
>                             | classification[1]
>                             | classification[2]);
>--
>            v2_inside_p = (v1_inside_p >> 1) & 0x1;
>            v1_inside_p = v1_inside_p & 0x1;
>
>I have corrected the syntax involving frontfacing,
>and changed v?_inside_p to be (unsigned int).
>This fixes surface2.dem, but other problems may lurk.
>

That fixes things and is fine.  However, v1_inside_p and v2_inside_p do 
have the feel of Boolean variables.  The problem is that v1_inside_p is 
used as a temp variable and from a clean C programming viewpoint 
probably shouldn't be.  Also, doing the bit shift on the variable is 
extraneous.  Could have something like:


#define V1_INSIDE_P_BIT 1
#define V2_INSIDE_P_BIT 2

unsigned_int triangle_classification;

        triangle_classification = ~ (classification[0]
                 | classification[1]
                 | classification[2]);

        v1_inside_p = (traingle_classification & V1_INSIDE_P_BIT);
        v2_inside_p = (triangle_classification & V2_INSIDE_P_BIT);

But, if one goes a step further in the code at that point, you'll notice 
that the logic is

        if ((v1_inside_p) && (v2_inside_p)) {

and "v1_inside_p" and "v2_inside_p" aren't used afterward.  So really, 
is it necessary to break this into two individual tests and recombine 
them?  (Future modification perhaps that I'm missing?)   I mean, in the 
definition of "classify()" there is specific use of "1" and "2" rather 
than definitions such as V1_INSIDE_P_BIT AND V2_INSIDE_P_BIT, so if you 
want to be loose about this subroutine and end up with efficient code 
rather than reader friendly code, one could just do:

        triangle_classification = ~ (classification[0]
                 | classification[1]
                 | classification[2]);

        if (triangle_classification >= 3) {

Dan

-- 

Dan Sebald
email: daniel DOT sebald AT ieee DOT org
URL: http://acer-access DOT com/~dsebald AT acer-access DOT com/

Re: More fallout from TBOOLEAN change - hidden3d broken

[Daniel J S. <dan...@ie...>]

Daniel J Sebald wrote:

>
>
>        triangle_classification = ~ (classification[0]
>                 | classification[1]
>                 | classification[2]);
>
>        if (triangle_classification >= 3) { 


No, that is not exactly right.  Technically it should be

        if ((triangle_classification & 3) == 3) {

to be foolproof in general.  However, in this case the above should work, as well as the slightly simplified:

       triangle_classification = classification[0]
                | classification[1]
                | classification[2];
  
       if (!triangle_classification) {

As I said, not exactly reader friendly.

Dan

Re: More fallout from TBOOLEAN change - hidden3d broken

[Per P. <per...@ma...>]

On Mar 29, 2004, at 20:46, Ethan Merritt wrote:
> In stepping through the demos to check 3.8k.3 I found
> that something very ugly has happened to hidden3d.
> See "surface2.dem".

Yeah, I noticed that too.

>
> Reverting to the previous version of src/syscfg.h
> made the problem go away, so this was another
> bit of fallout from the TBOOLEAN change.

That change to TBOOLEAN was for the benefit of Mac OS X if I understand 
correctly.

Just so everybody knows what options we have, that change is *not* 
necessary anymore,
with the new aquaterm.trm.
Just to test it, I reverted syscfg.h to v1.26 and it compiles and runs 
on Mac OS X 10.3.2

Having said that, I still think making TBOOLEAN a real boolean is the 
Right Thing to do.

/Per