Re: [Plib-devel] A Second Look Into SSG

[Wolfram K. <w_...@rz...>]

>Trivial:
>
>(1) More tabs that should be blank spaces in ssgSimpleState.cxx:545;
>ssgStateSelector.cxx:44-46, 83; ssgVtxTable.cxx:31-56, 66-126, 319-322,
>363-383, 418, 447-452, 796-798, 810-819

I untabified ssgStateSelector.cxx. I have local changes in the other
files, so it would be too much hassle to get the "Clean version",
untabify and commit.=20

>(2) In "ssgEntity.cxx" on lines 117 and 131, the words "gimbles,"
>"seperate," and "seperated" should be "gimbals," "separate," and
>"separated."

=46ixed

>(3) In "ssgLeaf.cxx" we may want to check the indentation of lines 36, =
51,
>and 53 (two spaces instead of the usual four).

Hopefully fixed, please check it.

>(4) In "ssgVtxTable.cxx" on lines 46-47 the same test is repeated.

=46ixed.

>(5) In "ssgVtxTable.cxx" on line 314, should we account for the =
GL_POINTS,
>GL_LINES, GL_LINE_LOOP, and GL_LINE_STRIP cases inside the "switch"
>statement?

Well, if you find that more elegant, ok. Changed.

>Less Trivial:
>
>(6) In "ssgEntity.cxx" on lines 80ff, the local variable "spherebotch" =
is
>set to a pseudorandom (but repeatable) value from zero to nine and
>incremented through a "switch" statement.  This is strange; each entity =
will
>get its own value from zero to nine, but the same entity will always get=
 the
>same value.  Further, "spherebotch" is incremented on line 82 and reset =
to
>zero on line 92.  It will be reinitialized each time through the =
function.
>It should be added to the private variables of the "ssgEntity" class.

That sounds good and the current implementation is obviously not what
the author wanted. Still, before a change we might want to hear what
the author wanted to achieve.

>(7) In "ssgEntity.cxx" on line 432, we set the variable =
"bsphere_is_invalid"
>to true.  Do we want to call "dirtyBSphere" instead, so that we set the
>parent entities' b-spheres dirty as well?

Yes. Fixed.

>(8) In "ssgList.cxx" on line 102 I think there is a bug.  The "if ( next=
 >=3D
>n )" should be "if ( next > n )".  In the extreme case, if both "next" =
and
>"n" are zero, we will wind up pointing to "next" equal to -1.  In =
general,
>if "next" is equal to "n", we wind up pointing at the element we just
>processed.

Sounds good to me. I have not changed it yet though.

>(9) In "ssgSimpleList.cxx" on line 29, do we want to test for "if ( this=
 =3D=3D
>src ) return ;"?  The test is in "ssgBase.cxx" on line 41, also in the
>"copy_from" function.

Could you check filename/line number please?


>(15) In "ssgVtxTable.cxx" on line 704ff, I can do this test with six
>multiplies.  Is there enough of a demand for it that somebody is willing=
 to
>put it into CVS for me?

I would guess the FGFS people would appreciate it!

>I have some more comments to "ssgVtxTable.cxx" and the geometry =
functions,
>specifically in terms of making them more efficient; does anybody want =
to
>hear them?

Certainly, I can not promise any time though.

Thank you for your work!

Bye bye,
Wolfram.