Code review for quadrics work

[Chris Foster-5]

Here's a code review for Trevor's quadrics work (partly building on
the work Tobi
did on this a while ago):

Overall I think it's a solid improvement over the existing code, and on the
capsules.rib test scene, it now seems to decrease the render time by about 5%,
as expected!

Following is the detailed review of mostly minor things which I think should be
improved before the code goes into the trunk.

> diff --git a/libs/core/geometry/quadrics.cpp b/libs/core/geometry/quadrics.cpp
> index ab4e035..e0f580d 100644
> --- a/libs/core/geometry/quadrics.cpp
> +++ b/libs/core/geometry/quadrics.cpp
> @@ -184,6 +180,32 @@ TqInt CqQuadric::DiceAll( CqMicroPolyGrid* pGrid )
>   bool flipNormals = pAttributes()->
>   GetIntegerAttribute("System", "Orientation")[0] != 0;
>
> + pGrid->pVar(EnvVars_P)->GetPointPtr(pointGrid);
> + pGrid->pVar(EnvVars_N)->GetNormalPtr(normalGrid);
> +
> + // check pVars to see if we need to calc points, normals
> + bool usePoints=false;
> + bool useNormals=false;
> +
> + if( USES( lUses, EnvVars_P ) && NULL != pGrid->pVar(EnvVars_P) )
> + {
> + usePoints=true;

You can assume that USES(lUses, EnvVars_P) is true - I can't imagine what
possible situation there would be where this is not the case.  That makes the
following code a bit simpler since there's no need for the usePoints variable.

> + if (USES( lUses, EnvVars_Ng ) && NULL != pGrid->pVar(EnvVars_Ng))
> + {
> + useNormals = true;
> + DicePoints(pointGrid, normalGrid);
> + // Indicate that P and Ng will be filled in by the loops below.
> + DONE( lDone, EnvVars_P );
> + DONE( lDone, EnvVars_Ng );
> + }
> + else
> + {
> + DicePoints(pointGrid, NULL);
> + // Indicate that P (only) will be filled in by the loops below
> + DONE( lDone, EnvVars_P );
> + }
> + }
> +
>   TqFloat du = 1.0 / uDiceSize();
>   TqFloat dv = 1.0 / vDiceSize();
>   for ( v = 0; v <= vDiceSize(); v++ )
> @@ -192,20 +214,24 @@ TqInt CqQuadric::DiceAll( CqMicroPolyGrid* pGrid )
>   for ( u = 0; u <= uDiceSize(); u++ )
>   {
>   TqFloat uf = u * du;
> - TqInt igrid = ( v * ( uDiceSize() + 1 ) ) + u;
> - if( USES( lUses, EnvVars_P ) && NULL != pGrid->pVar(EnvVars_P) )
> + TqInt igrid = ( v * ( uDiceSize() + 1 ) ) + u; // offset for grid point/normal buffer
> +
> + if(usePoints)
>   {
> - if( USES( lUses, EnvVars_Ng ) && NULL != pGrid->pVar(EnvVars_Ng) )
> + if(useNormals)
>   {
> - P = DicePoint( u, v, N );
> + P = pointGrid[igrid];
> + N = normalGrid[igrid];
> +
>   if(flipNormals)
> - N = -N;
> +    N = -N;
> +
>   pGrid->pVar(EnvVars_P)->SetPoint( m_matTx * P, igrid );
>   pGrid->pVar(EnvVars_Ng)->SetNormal( m_matITTx * N, igrid );

I know this SetPoint stuff was in the original code, but it would be more
efficient to replace it with

pointGrid[igrid] = m_matTx*pointGrid[igrid];

after all, pointGrid is already the raw array associated with
pGrid->pVar(EnvVars_P).

> @@ -459,38 +463,45 @@ TqInt CqSphere::PreSubdivide( std::vector<boost::shared_ptr<CqSurface> >& aSplit

[...]

> +/** Calculate all points on the surface indexed by the surface paramters passed.
> +  * \param pointGrid CqVector3D surface points.
> +  * \param normalGrid CqVector3D surface normals.
> +  */
> +void CqSphere::DicePoints( CqVector3D* pointGrid, CqVector3D* normalGrid )

[...]

> + for (TqInt v = 0; v <= m_vDiceSize; v++ )
> + {
> + for (TqInt u = 0; u <= m_uDiceSize; u++ )
> + {
> + TqFloat cosTheta = cosThetaTab[u];
> + TqFloat sinTheta = sinThetaTab[u];
> + TqFloat cosPhi = cosPhiTab[v];
> + TqFloat sinPhi = sinPhiTab[v];
> +
> + // calc surface point
> + TqInt gridOffset = ( v * ( m_uDiceSize + 1 ) ) + u;
> + pointGrid[gridOffset] = CqVector3D( ( m_Radius * cosTheta * cosPhi ), ( m_Radius * sinTheta * cosPhi ), ( m_Radius * sinPhi ) );
> +
> + // calc normal vector
> + if (normalGrid != NULL) {
> + CqVector3D N_new = pointGrid[gridOffset];
> + N_new.Unit();
> + normalGrid[gridOffset] = N_new;
> + }

You can avoid the N_new.Unit() here if you first construct the vector

unitP = CqVector3D(cosTheta*cosPhi, sinTheta*cosPhi, sinPhi)

which is normalized by definition.

We then have

P = m_Radius * unitP;

and

N = unitP

> @@ -595,58 +606,50 @@ TqInt CqCone::PreSubdivide( std::vector<boost::shared_ptr<CqSurface> >& aSplits,

[...]

> +/** Calculate all points on the surface indexed by the surface paramters passed.
> +  * \param pointGrid CqVector3D surface points.
> +  * \param normalGrid CqVector3D surface normals.
> +  */
> +void CqCone::DicePoints( CqVector3D* pointGrid, CqVector3D* normalGrid )

[...]

> + for (TqInt v = 0; v <= m_vDiceSize; v++ )
> + {
> + for (TqInt u = 0; u <= m_uDiceSize; u++ )
> + {
> + // calc surface point
> + TqFloat zmin = m_vMin * m_Height;
> + TqFloat zmax = m_vMax * m_Height;
> + TqFloat z = zmin + ( ( TqFloat ) v * ( zmax - zmin ) ) / m_vDiceSize;
> + TqFloat vv = m_vMin + ( ( TqFloat ) v * ( m_vMax - m_vMin ) ) / m_vDiceSize;
> + TqFloat r = m_Radius * ( 1.0 - vv );
> + TqFloat cosTheta = cosThetaTab[u];
> + TqFloat sinTheta = sinThetaTab[u];
> +
> + TqInt gridOffset = ( v * ( m_uDiceSize + 1 ) ) + u;
> + pointGrid[gridOffset] = CqVector3D( r * cosTheta, r * sinTheta, z );
> +
> + // calc normal
> + if (normalGrid != NULL) {
> + TqFloat coneLength = sqrt( m_Height * m_Height + m_Radius * m_Radius );

Calculate stuff like coneLength outside the loops (sqrt is somewhat
expensive)!  In fact, I'd calculate both xN and m_Radius/coneLength outside
the loop.

> + TqFloat xN = m_Height / coneLength;
> + CqVector3D Normal;
> + Normal.x( xN * cosTheta );
> + Normal.y( xN * sinTheta );
> + Normal.z( m_Radius / coneLength );
> +
> + normalGrid[gridOffset] = Normal;
> + }
> + }
> + }
>  }

[...]

> +/** Calculate all points on the surface indexed by the surface paramters passed.
> +  * \param pointGrid CqVector3D surface points.
> +  * \param normalGrid CqVector3D surface normals.
> +  */
> +void CqCylinder::DicePoints( CqVector3D* pointGrid, CqVector3D* normalGrid )

[...]

> + for (TqInt v = 0; v <= m_vDiceSize; v++ )
> + {
> + for (TqInt u = 0; u <= m_uDiceSize; u++ )
> + {
> + // calc surface point
> + TqFloat sinTheta = sinThetaTab[u];
> + TqFloat cosTheta = cosThetaTab[u];
> + TqFloat vz = m_ZMin + ( ( TqFloat ) v * ( m_ZMax - m_ZMin ) ) / m_vDiceSize;
> + CqVector3D point =  CqVector3D( m_Radius * cosTheta, m_Radius * sinTheta, vz );
> + TqInt gridOffset = ( v * ( m_uDiceSize + 1 ) ) + u;
> + pointGrid[gridOffset] = point;
> +
> + // calc normal
> + if (normalGrid != NULL) {
> + CqVector3D normal = point;
> + normal.z( 0 );
> + normal.Unit();

Again, here you can avoid the call to normal.Unit(), since

N = CqVector3D(cosTheta, sinTheta, 0)

is normalized by definition.

> +
> + normalGrid[gridOffset] = normal;
> + }
> + }
> + }
>  }

[...]

> +/** Calculate all points on the surface indexed by the surface paramters passed.
> +  * \param pointGrid CqVector3D surface points.
> +  * \param normalGrid CqVector3D surface normals.
> +  */
> +void CqHyperboloid::DicePoints( CqVector3D* pointGrid, CqVector3D* normalGrid )
>  {

[...]

> + for (TqInt v = 0; v <= m_vDiceSize; v++ )
> + {
> + for (TqInt u = 0; u <= m_uDiceSize; u++ )
> + {
> + TqFloat cosTheta = cosThetaTab[u];
> + TqFloat sinTheta = sinThetaTab[u];
> +
> + // calc surface point
> + CqVector3D p;
> + TqFloat vv = static_cast<TqFloat>( v ) / m_vDiceSize;
> + p = m_Point1 * ( 1.0 - vv ) + m_Point2 * vv;
> +
> + TqInt gridOffset = ( v * ( m_uDiceSize + 1 ) ) + u;
> + pointGrid[gridOffset] = CqVector3D( p.x() * cosTheta - p.y() * sinTheta, p.x() * sinTheta + p.y() * cosTheta, p.z() );
> +
> + // Calculate the normal vector - this is a bit tortuous, and uses the general
> + // formula for the normal to a surface that is specified by two parametric
> + // parameters.
> + if (normalGrid != NULL) {
> + // Calculate a vector, a, of derivatives of coordinates w.r.t. u
> + TqFloat dxdu = -p.x() * m_ThetaMax * sinTheta - p.y() * m_ThetaMax * cosTheta;
> + TqFloat dydu =  p.x() * m_ThetaMax * cosTheta - p.y() * m_ThetaMax * sinTheta;
> + TqFloat dzdu = 0.0;
> + CqVector3D a(dxdu, dydu, dzdu);
> +
> + // Calculate a vector, b, of derivatives of coordinates w.r.t. v
> + CqVector3D p2p1 = m_Point2 - m_Point1;
> + TqFloat dxdv = p2p1.x() * cosTheta  -  p2p1.y() * sinTheta;
> + TqFloat dydv = p2p1.x() * sinTheta  +  p2p1.y() * cosTheta;
> + TqFloat dzdv = p2p1.z();
> + CqVector3D b(dxdv, dydv, dzdv);
> +
> + // The normal vector points in the direction of: a x b
> + CqVector3D Normal = a % b;
> + Normal.Unit();

A thought strikes me here: you actually shouldn't ever bother to normalize the
normals inside DicePoints, since they're going to be transformed via the
m_matITTx matrix anyway, and this will potentially cause the normals to be
scaled, in which case the normalization will be pointless anyway.

> + normalGrid[gridOffset] = Normal;
> + }
> + }
> + }
>  }

[...]

> -CqVector3D CqParaboloid::DicePoint( TqInt u, TqInt v, CqVector3D& Normal )
> +/** Calculate all points on the surface indexed by the surface paramters passed.
> +  * \param pointGrid CqVector3D surface points.
> +  * \param normalGrid CqVector3D surface normals.
> +  */
> +void CqParaboloid::DicePoints( CqVector3D* pointGrid, CqVector3D* normalGrid )
>  {
> - TqFloat theta = degToRad( m_ThetaMin + ( ( m_ThetaMax - m_ThetaMin ) * ( TqFloat ) u ) / m_uDiceSize );
> - TqFloat sin_theta = sin( theta );
> - TqFloat cos_theta = cos( theta );
> + TqInt thetaRes = m_uDiceSize + 1;
> + boost::scoped_array<TqFloat> sinThetaTab(new TqFloat[thetaRes]);
> + boost::scoped_array<TqFloat> cosThetaTab(new TqFloat[thetaRes]);
>
> - TqFloat z = m_ZMin + ( ( TqFloat ) v * ( m_ZMax - m_ZMin ) ) / m_vDiceSize;
> - TqFloat r = m_RMax * sqrt( z / m_ZMax );
> + //build look-up tables for sin(theta), cos(theta)
> + sinCosGrid(degToRad(m_ThetaMin), degToRad(m_ThetaMax), thetaRes, sinThetaTab.get(), cosThetaTab.get());
>
> - TqFloat dzdr = r * 2.0 * m_ZMax / ( m_RMax * m_RMax );
> - TqFloat normalAngle = M_PI_2 - atan( dzdr );
> - Normal.x( cos_theta * cos( normalAngle ) );
> - Normal.y( sin_theta * cos( normalAngle ) );
> - Normal.z( -sin( normalAngle ) );
> -
> - return ( CqVector3D( r * cos_theta, r * sin_theta, z ) );
> + for (TqInt v = 0; v <= m_vDiceSize; v++ )
> + {
> + for (TqInt u = 0; u <= m_uDiceSize; u++ )
> + {
> + TqFloat cosTheta = cosThetaTab[u];
> + TqFloat sinTheta = sinThetaTab[u];
> +
> + // calc surface point
> + TqFloat z = m_ZMin + ( ( TqFloat ) v * ( m_ZMax - m_ZMin ) ) / m_vDiceSize;
> + TqFloat r = m_RMax * sqrt( z / m_ZMax );
> +
> + TqInt gridOffset = ( v * ( m_uDiceSize + 1 ) ) + u;
> + pointGrid[gridOffset] = CqVector3D( r * cosTheta, r * sinTheta, z );
> +
> + // calc normal
> + if (normalGrid != NULL) {
> + // TODO: could put this calculation in look-up-tables as well. -trev

Yes, please do this; the current method with atan is awful ;-)

> + TqFloat dzdr = r * 2.0 * m_ZMax / ( m_RMax * m_RMax );
> + TqFloat normalAngle = M_PI_2 - atan( dzdr );
> + CqVector3D Normal;
> +
> + Normal.x( cosTheta * cos( normalAngle ) );
> + Normal.y( sinTheta * cos( normalAngle ) );
> + Normal.z( -sin( normalAngle ) );
> + normalGrid[gridOffset] = Normal;

Here's roughly how I reckon it should go:

Draw a picture of a paraboloid; the normal direction in the xy plane is
(cosTheta, sinTheta).  If you look in the RISpec, you'll see the parametric
equations for the cone, which specify that the radius of the surface of
revolution goes as

r = r_max * sqrt(z / z_max)

Looking at the graph of r vs z, I think the z-component of the normal should be
equal to -dr/dz, so after a short calculation, I think you should use

(cosTheta, sinTheta, -0.5*r_max*r_max/z_max / r)

Notes:
 - Careful of when r == 0!
 - this isn't normalized, but as I noted above, I don't think that matters.


[...]

> @@ -1523,9 +1525,36 @@ CqBound CqQuadric::RevolveForBound( const std::vector<CqVector3D>& profile, cons
>  }
>
>
> +/** \brief Compute a grid of sin(t) and cos(t) at regularly spaced values.
> + *
> + * Computes a regular grid of numSteps sin() and cos() values spaced equally
> + * between t0 and t1 inclusive:
> + *
> + * sint[i] = sin(t0 + dt*i);
> + * cost[i] = cos(t0 + dt*i);
> + *
> + * Where
> + *
> + * dt = (t1-t0)/(numSteps-1);
> + */
> +void sinCosGrid(TqFloat t0, TqFloat t1, TqInt numSteps,
> +        TqFloat* sint, TqFloat* cost)
> +{
> +    cost[0] = cos(t0);
> +    sint[0] = sin(t0);
> +    TqFloat dt = (t1 - t0)/(numSteps-1);
> +    TqFloat cosDt = cos(dt);
> +    TqFloat sinDt = sin(dt);

As we discussed another time (I think), it's probably better to change these
variables into doubles for numerical robustness.

> +    for(TqInt i = 1; i < numSteps; ++i)
> +    {
> +        cost[i] = cosDt * cost[i-1] - sinDt * sint[i-1];
> +        sint[i] = sinDt * cost[i-1] + cosDt * sint[i-1];

obviously you'll need to keep the previous angles as doubles too instead of
using cost[i-1] and sint[i-1] here.


That's it!
~Chris.

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

[Trevor Lovett]

Thanks for the review Chris.  I just pushed quadrics.cpp with the changes you requested.

I will be checking my email in the morning tomorrow (Norwegian time) in case there are last-minute things popping up.

Have you had a chance to look at curves?  I don't think we will be finished with that by the hard-deadline (as I mentioned before we do have that bug that pops up when the curves are forced into a turn of radius < their width and this needs to be figured out).  I'd like to keep going with that project after the GSoC deadline because I think it might be difficult for someone else to pick up the code and finish it.

Trevor

On Sat, Aug 22, 2009 at 2:29 PM, Chris Foster <chris42f@...> wrote:

Here's a code review for Trevor's quadrics work (partly building on
the work Tobi
did on this a while ago):

Overall I think it's a solid improvement over the existing code, and on the
capsules.rib test scene, it now seems to decrease the render time by about 5%,
as expected!

Following is the detailed review of mostly minor things which I think should be
improved before the code goes into the trunk.


> diff --git a/libs/core/geometry/quadrics.cpp b/libs/core/geometry/quadrics.cpp
> index ab4e035..e0f580d 100644
> --- a/libs/core/geometry/quadrics.cpp
> +++ b/libs/core/geometry/quadrics.cpp
> @@ -184,6 +180,32 @@ TqInt CqQuadric::DiceAll( CqMicroPolyGrid* pGrid )
>       bool flipNormals = pAttributes()->
>               GetIntegerAttribute("System", "Orientation")[0] != 0;
>
> +     pGrid->pVar(EnvVars_P)->GetPointPtr(pointGrid);
> +     pGrid->pVar(EnvVars_N)->GetNormalPtr(normalGrid);
> +
> +     // check pVars to see if we need to calc points, normals
> +     bool usePoints=false;
> +     bool useNormals=false;
> +
> +     if( USES( lUses, EnvVars_P ) && NULL != pGrid->pVar(EnvVars_P) )
> +     {
> +             usePoints=true;

You can assume that USES(lUses, EnvVars_P) is true - I can't imagine what
possible situation there would be where this is not the case.  That makes the
following code a bit simpler since there's no need for the usePoints variable.

> +             if (USES( lUses, EnvVars_Ng ) && NULL != pGrid->pVar(EnvVars_Ng))
> +             {
> +                     useNormals = true;
> +                     DicePoints(pointGrid, normalGrid);
> +                     // Indicate that P and Ng will be filled in by the loops below.
> +                     DONE( lDone, EnvVars_P );
> +                     DONE( lDone, EnvVars_Ng );
> +             }
> +             else
> +             {
> +                     DicePoints(pointGrid, NULL);
> +                     // Indicate that P (only) will be filled in by the loops below
> +                     DONE( lDone, EnvVars_P );
> +             }
> +     }
> +
>       TqFloat du = 1.0 / uDiceSize();
>       TqFloat dv = 1.0 / vDiceSize();
>       for ( v = 0; v <= vDiceSize(); v++ )
> @@ -192,20 +214,24 @@ TqInt CqQuadric::DiceAll( CqMicroPolyGrid* pGrid )
>               for ( u = 0; u <= uDiceSize(); u++ )
>               {
>                       TqFloat uf = u * du;
> -                     TqInt igrid = ( v * ( uDiceSize() + 1 ) ) + u;
> -                     if( USES( lUses, EnvVars_P ) && NULL != pGrid->pVar(EnvVars_P) )
> +                     TqInt igrid = ( v * ( uDiceSize() + 1 ) ) + u; // offset for grid point/normal buffer
> +
> +                     if(usePoints)
>                       {
> -                             if( USES( lUses, EnvVars_Ng ) && NULL != pGrid->pVar(EnvVars_Ng) )
> +                             if(useNormals)
>                               {
> -                                     P = DicePoint( u, v, N );
> +                                     P = pointGrid[igrid];
> +                                     N = normalGrid[igrid];
> +
>                                       if(flipNormals)
> -                                             N = -N;
> +                                         N = -N;
> +
>                                       pGrid->pVar(EnvVars_P)->SetPoint( m_matTx * P, igrid );
>                                       pGrid->pVar(EnvVars_Ng)->SetNormal( m_matITTx * N, igrid );

I know this SetPoint stuff was in the original code, but it would be more
efficient to replace it with

pointGrid[igrid] = m_matTx*pointGrid[igrid];

after all, pointGrid is already the raw array associated with
pGrid->pVar(EnvVars_P).

> @@ -459,38 +463,45 @@ TqInt CqSphere::PreSubdivide( std::vector<boost::shared_ptr<CqSurface> >& aSplit

[...]

> +/** Calculate all points on the surface indexed by the surface paramters passed.
> +  * \param pointGrid CqVector3D surface points.
> +  * \param normalGrid CqVector3D surface normals.
> +  */
> +void CqSphere::DicePoints( CqVector3D* pointGrid, CqVector3D* normalGrid )

[...]

> +     for (TqInt v = 0; v <= m_vDiceSize; v++ )
> +     {
> +             for (TqInt u = 0; u <= m_uDiceSize; u++ )
> +             {
> +                     TqFloat cosTheta = cosThetaTab[u];
> +                     TqFloat sinTheta = sinThetaTab[u];
> +                     TqFloat cosPhi = cosPhiTab[v];
> +                     TqFloat sinPhi = sinPhiTab[v];
> +
> +                     // calc surface point
> +                     TqInt gridOffset = ( v * ( m_uDiceSize + 1 ) ) + u;
> +                     pointGrid[gridOffset] = CqVector3D( ( m_Radius * cosTheta * cosPhi ), ( m_Radius * sinTheta * cosPhi ), ( m_Radius * sinPhi ) );
> +
> +                     // calc normal vector
> +                     if (normalGrid != NULL) {
> +                             CqVector3D N_new = pointGrid[gridOffset];
> +                             N_new.Unit();
> +                             normalGrid[gridOffset] = N_new;
> +                     }

You can avoid the N_new.Unit() here if you first construct the vector

unitP = CqVector3D(cosTheta*cosPhi, sinTheta*cosPhi, sinPhi)

which is normalized by definition.

We then have

P = m_Radius * unitP;

and

N = unitP

> @@ -595,58 +606,50 @@ TqInt CqCone::PreSubdivide( std::vector<boost::shared_ptr<CqSurface> >& aSplits,

[...]

> +/** Calculate all points on the surface indexed by the surface paramters passed.
> +  * \param pointGrid CqVector3D surface points.
> +  * \param normalGrid CqVector3D surface normals.
> +  */
> +void CqCone::DicePoints( CqVector3D* pointGrid, CqVector3D* normalGrid )

[...]

> +     for (TqInt v = 0; v <= m_vDiceSize; v++ )
> +     {
> +             for (TqInt u = 0; u <= m_uDiceSize; u++ )
> +             {
> +                     // calc surface point
> +                     TqFloat zmin = m_vMin * m_Height;
> +                     TqFloat zmax = m_vMax * m_Height;
> +                     TqFloat z = zmin + ( ( TqFloat ) v * ( zmax - zmin ) ) / m_vDiceSize;
> +                     TqFloat vv = m_vMin + ( ( TqFloat ) v * ( m_vMax - m_vMin ) ) / m_vDiceSize;
> +                     TqFloat r = m_Radius * ( 1.0 - vv );
> +                     TqFloat cosTheta = cosThetaTab[u];
> +                     TqFloat sinTheta = sinThetaTab[u];
> +
> +                     TqInt gridOffset = ( v * ( m_uDiceSize + 1 ) ) + u;
> +                     pointGrid[gridOffset] = CqVector3D( r * cosTheta, r * sinTheta, z );
> +
> +                     // calc normal
> +                     if (normalGrid != NULL) {
> +                             TqFloat coneLength = sqrt( m_Height * m_Height + m_Radius * m_Radius );

Calculate stuff like coneLength outside the loops (sqrt is somewhat
expensive)!  In fact, I'd calculate both xN and m_Radius/coneLength outside
the loop.

> +                             TqFloat xN = m_Height / coneLength;
> +                             CqVector3D Normal;
> +                             Normal.x( xN * cosTheta );
> +                             Normal.y( xN * sinTheta );
> +                             Normal.z( m_Radius / coneLength );
> +
> +                             normalGrid[gridOffset] = Normal;
> +                     }
> +             }
> +     }
>  }

[...]

> +/** Calculate all points on the surface indexed by the surface paramters passed.
> +  * \param pointGrid CqVector3D surface points.
> +  * \param normalGrid CqVector3D surface normals.
> +  */
> +void CqCylinder::DicePoints( CqVector3D* pointGrid, CqVector3D* normalGrid )

[...]

> +     for (TqInt v = 0; v <= m_vDiceSize; v++ )
> +     {
> +             for (TqInt u = 0; u <= m_uDiceSize; u++ )
> +             {
> +                     // calc surface point
> +                     TqFloat sinTheta = sinThetaTab[u];
> +                     TqFloat cosTheta = cosThetaTab[u];
> +                     TqFloat vz = m_ZMin + ( ( TqFloat ) v * ( m_ZMax - m_ZMin ) ) / m_vDiceSize;
> +                     CqVector3D point =  CqVector3D( m_Radius * cosTheta, m_Radius * sinTheta, vz );
> +                     TqInt gridOffset = ( v * ( m_uDiceSize + 1 ) ) + u;
> +                     pointGrid[gridOffset] = point;
> +
> +                     // calc normal
> +                     if (normalGrid != NULL) {
> +                             CqVector3D normal = point;
> +                             normal.z( 0 );
> +                             normal.Unit();

Again, here you can avoid the call to normal.Unit(), since

N = CqVector3D(cosTheta, sinTheta, 0)

is normalized by definition.

> +
> +                             normalGrid[gridOffset] = normal;
> +                     }
> +             }
> +     }
>  }

[...]

> +/** Calculate all points on the surface indexed by the surface paramters passed.
> +  * \param pointGrid CqVector3D surface points.
> +  * \param normalGrid CqVector3D surface normals.
> +  */
> +void CqHyperboloid::DicePoints( CqVector3D* pointGrid, CqVector3D* normalGrid )
>  {

[...]

> +     for (TqInt v = 0; v <= m_vDiceSize; v++ )
> +     {
> +             for (TqInt u = 0; u <= m_uDiceSize; u++ )
> +             {
> +                     TqFloat cosTheta = cosThetaTab[u];
> +                     TqFloat sinTheta = sinThetaTab[u];
> +
> +                     // calc surface point
> +                     CqVector3D p;
> +                     TqFloat vv = static_cast<TqFloat>( v ) / m_vDiceSize;
> +                     p = m_Point1 * ( 1.0 - vv ) + m_Point2 * vv;
> +
> +                     TqInt gridOffset = ( v * ( m_uDiceSize + 1 ) ) + u;
> +                     pointGrid[gridOffset] = CqVector3D( p.x() * cosTheta - p.y() * sinTheta, p.x() * sinTheta + p.y() * cosTheta, p.z() );
> +
> +                     // Calculate the normal vector - this is a bit tortuous, and uses the general
> +                     // formula for the normal to a surface that is specified by two parametric
> +                     // parameters.
> +                     if (normalGrid != NULL) {
> +                             // Calculate a vector, a, of derivatives of coordinates w.r.t. u
> +                             TqFloat dxdu = -p.x() * m_ThetaMax * sinTheta - p.y() * m_ThetaMax * cosTheta;
> +                             TqFloat dydu =  p.x() * m_ThetaMax * cosTheta - p.y() * m_ThetaMax * sinTheta;
> +                             TqFloat dzdu = 0.0;
> +                             CqVector3D a(dxdu, dydu, dzdu);
> +
> +                             // Calculate a vector, b, of derivatives of coordinates w.r.t. v
> +                             CqVector3D p2p1 = m_Point2 - m_Point1;
> +                             TqFloat dxdv = p2p1.x() * cosTheta  -  p2p1.y() * sinTheta;
> +                             TqFloat dydv = p2p1.x() * sinTheta  +  p2p1.y() * cosTheta;
> +                             TqFloat dzdv = p2p1.z();
> +                             CqVector3D b(dxdv, dydv, dzdv);
> +
> +                             // The normal vector points in the direction of: a x b
> +                             CqVector3D Normal = a % b;
> +                             Normal.Unit();

A thought strikes me here: you actually shouldn't ever bother to normalize the
normals inside DicePoints, since they're going to be transformed via the
m_matITTx matrix anyway, and this will potentially cause the normals to be
scaled, in which case the normalization will be pointless anyway.

> +                             normalGrid[gridOffset] = Normal;
> +                     }
> +             }
> +     }
>  }

[...]

> -CqVector3D CqParaboloid::DicePoint( TqInt u, TqInt v, CqVector3D& Normal )
> +/** Calculate all points on the surface indexed by the surface paramters passed.
> +  * \param pointGrid CqVector3D surface points.
> +  * \param normalGrid CqVector3D surface normals.
> +  */
> +void CqParaboloid::DicePoints( CqVector3D* pointGrid, CqVector3D* normalGrid )
>  {
> -     TqFloat theta = degToRad( m_ThetaMin + ( ( m_ThetaMax - m_ThetaMin ) * ( TqFloat ) u ) / m_uDiceSize );
> -     TqFloat sin_theta = sin( theta );
> -     TqFloat cos_theta = cos( theta );
> +     TqInt thetaRes = m_uDiceSize + 1;
> +     boost::scoped_array<TqFloat> sinThetaTab(new TqFloat[thetaRes]);
> +     boost::scoped_array<TqFloat> cosThetaTab(new TqFloat[thetaRes]);
>
> -     TqFloat z = m_ZMin + ( ( TqFloat ) v * ( m_ZMax - m_ZMin ) ) / m_vDiceSize;
> -     TqFloat r = m_RMax * sqrt( z / m_ZMax );
> +     //build look-up tables for sin(theta), cos(theta)
> +     sinCosGrid(degToRad(m_ThetaMin), degToRad(m_ThetaMax), thetaRes, sinThetaTab.get(), cosThetaTab.get());
>
> -     TqFloat dzdr = r * 2.0 * m_ZMax / ( m_RMax * m_RMax );
> -     TqFloat normalAngle = M_PI_2 - atan( dzdr );
> -     Normal.x( cos_theta * cos( normalAngle ) );
> -     Normal.y( sin_theta * cos( normalAngle ) );
> -     Normal.z( -sin( normalAngle ) );
> -
> -     return ( CqVector3D( r * cos_theta, r * sin_theta, z ) );
> +     for (TqInt v = 0; v <= m_vDiceSize; v++ )
> +     {
> +             for (TqInt u = 0; u <= m_uDiceSize; u++ )
> +             {
> +                     TqFloat cosTheta = cosThetaTab[u];
> +                     TqFloat sinTheta = sinThetaTab[u];
> +
> +                     // calc surface point
> +                     TqFloat z = m_ZMin + ( ( TqFloat ) v * ( m_ZMax - m_ZMin ) ) / m_vDiceSize;
> +                     TqFloat r = m_RMax * sqrt( z / m_ZMax );
> +
> +                     TqInt gridOffset = ( v * ( m_uDiceSize + 1 ) ) + u;
> +                     pointGrid[gridOffset] = CqVector3D( r * cosTheta, r * sinTheta, z );
> +
> +                     // calc normal
> +                     if (normalGrid != NULL) {
> +                             // TODO: could put this calculation in look-up-tables as well. -trev

Yes, please do this; the current method with atan is awful ;-)

> +                             TqFloat dzdr = r * 2.0 * m_ZMax / ( m_RMax * m_RMax );
> +                             TqFloat normalAngle = M_PI_2 - atan( dzdr );
> +                             CqVector3D Normal;
> +
> +                             Normal.x( cosTheta * cos( normalAngle ) );
> +                             Normal.y( sinTheta * cos( normalAngle ) );
> +                             Normal.z( -sin( normalAngle ) );
> +                             normalGrid[gridOffset] = Normal;

Here's roughly how I reckon it should go:

Draw a picture of a paraboloid; the normal direction in the xy plane is
(cosTheta, sinTheta).  If you look in the RISpec, you'll see the parametric
equations for the cone, which specify that the radius of the surface of
revolution goes as

r = r_max * sqrt(z / z_max)

Looking at the graph of r vs z, I think the z-component of the normal should be
equal to -dr/dz, so after a short calculation, I think you should use

(cosTheta, sinTheta, -0.5*r_max*r_max/z_max / r)

Notes:
 - Careful of when r == 0!
 - this isn't normalized, but as I noted above, I don't think that matters.


[...]

> @@ -1523,9 +1525,36 @@ CqBound CqQuadric::RevolveForBound( const std::vector<CqVector3D>& profile, cons
>  }
>
>
> +/** \brief Compute a grid of sin(t) and cos(t) at regularly spaced values.
> + *
> + * Computes a regular grid of numSteps sin() and cos() values spaced equally
> + * between t0 and t1 inclusive:
> + *
> + * sint[i] = sin(t0 + dt*i);
> + * cost[i] = cos(t0 + dt*i);
> + *
> + * Where
> + *
> + * dt = (t1-t0)/(numSteps-1);
> + */
> +void sinCosGrid(TqFloat t0, TqFloat t1, TqInt numSteps,
> +        TqFloat* sint, TqFloat* cost)
> +{
> +    cost[0] = cos(t0);
> +    sint[0] = sin(t0);
> +    TqFloat dt = (t1 - t0)/(numSteps-1);
> +    TqFloat cosDt = cos(dt);
> +    TqFloat sinDt = sin(dt);

As we discussed another time (I think), it's probably better to change these
variables into doubles for numerical robustness.

> +    for(TqInt i = 1; i < numSteps; ++i)
> +    {
> +        cost[i] = cosDt * cost[i-1] - sinDt * sint[i-1];
> +        sint[i] = sinDt * cost[i-1] + cosDt * sint[i-1];

obviously you'll need to keep the previous angles as doubles too instead of
using cost[i-1] and sint[i-1] here.


That's it!
~Chris.

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

[Chris Foster-5]

On Sun, Aug 23, 2009 at 10:45 PM, Trevor Lovett<trevlovett@...> wrote:
> Thanks for the review Chris.  I just pushed quadrics.cpp with the changes
> you requested.

Generally speaking these look pretty good I think, thanks.  I do have one
final comment, and that is that the double changes in sinCosGrid still aren't
quite right:

> void sinCosGrid(TqFloat t0, TqFloat t1, TqInt numSteps,
>         TqFloat* sint, TqFloat* cost)
> {
>     TqDouble prevCos = cos(t0);
>     TqDouble prevSin = sin(t0);
>     TqDouble dt = (t1 - t0)/(numSteps-1);
>     TqDouble cosDt = cos(dt);
>     TqDouble sinDt = sin(dt);
>
>     cost[0] = prevCos;
>     sint[0] = prevSin;
>
>     for(TqInt i = 1; i < numSteps; ++i)
>     {
>         cost[i] = cosDt * prevCos - sinDt * prevSin;
>         sint[i] = sinDt * prevCos + cosDt * prevSin;
>
>         prevCos = cost[i];
>         prevSin = sint[i];
>     }
> }

The problem is that cost[i] and sint[i] are only float precision, so when you
compute

cost[i] = cosDt * prevCos - sinDt * prevSin;

the precision is truncated to 32 bits.  You then assign

prevCos = cost[i];

but you've already truncated to 32 bits so you loose the precision in prevCos.

I'll sort this issue out since it's pretty small, and try to get this stuff
into the trunk tonight if possible.


> Have you had a chance to look at curves?  I don't think we will be finished
> with that by the hard-deadline (as I mentioned before we do have that bug
> that pops up when the curves are forced into a turn of radius < their width
> and this needs to be figured out).  I'd like to keep going with that project
> after the GSoC deadline because I think it might be difficult for someone
> else to pick up the code and finish it.

I haven't had the time to look at this yet I'm afraid, but by all means,
you're more than welcome to keep working on this after the deadline.
Naturally I'll make sure that I revew the progress.


Generally speaking, we're in reasonable shape for the final deadline now.  In
terms of goals, I would have liked to see some more performance testing: it's
always good to know exactly how much difference a set of changes have made for
various scenes.  Obviously we're out of time now though!

Cheers,
~Chris.

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

[Trevor Lovett]

I fixed that oversight in sinCosGrid and you should see it pushed.  Also, I'm attaching a spreadsheet showing the relative speedup with the quadrics optimizations in place.  The numbers were taken from this phrase in the regression report:

Frame took 298.46 milli secs

I used the quadrics .rib's from the regression suite and got an average speedup of 3.2%-- a little less than predicted.  As for my system specs: a macbook pro w/ 2.4 ghz intel core 2 duo, 2gb ram.  Should I add this to the project beeker wiki? 

I'm around today if you need anything else before the bell rings.

Trevor

On Mon, Aug 24, 2009 at 1:17 AM, Chris Foster <chris42f@...> wrote:

On Sun, Aug 23, 2009 at 10:45 PM, Trevor Lovett<trevlovett@...> wrote:
> Thanks for the review Chris.  I just pushed quadrics.cpp with the changes
> you requested.

Generally speaking these look pretty good I think, thanks.  I do have one
final comment, and that is that the double changes in sinCosGrid still aren't
quite right:

> void sinCosGrid(TqFloat t0, TqFloat t1, TqInt numSteps,
>         TqFloat* sint, TqFloat* cost)

> {
>     TqDouble prevCos = cos(t0);
>     TqDouble prevSin = sin(t0);
>     TqDouble dt = (t1 - t0)/(numSteps-1);
>     TqDouble cosDt = cos(dt);
>     TqDouble sinDt = sin(dt);
>
>     cost[0] = prevCos;
>     sint[0] = prevSin;

>
>     for(TqInt i = 1; i < numSteps; ++i)
>     {

>         cost[i] = cosDt * prevCos - sinDt * prevSin;
>         sint[i] = sinDt * prevCos + cosDt * prevSin;
>
>         prevCos = cost[i];
>         prevSin = sint[i];
>     }
> }

The problem is that cost[i] and sint[i] are only float precision, so when you
compute

cost[i] = cosDt * prevCos - sinDt * prevSin;

the precision is truncated to 32 bits.  You then assign

prevCos = cost[i];

but you've already truncated to 32 bits so you loose the precision in prevCos.

I'll sort this issue out since it's pretty small, and try to get this stuff
into the trunk tonight if possible.

> Have you had a chance to look at curves?  I don't think we will be finished
> with that by the hard-deadline (as I mentioned before we do have that bug
> that pops up when the curves are forced into a turn of radius < their width
> and this needs to be figured out).  I'd like to keep going with that project
> after the GSoC deadline because I think it might be difficult for someone
> else to pick up the code and finish it.

I haven't had the time to look at this yet I'm afraid, but by all means,
you're more than welcome to keep working on this after the deadline.
Naturally I'll make sure that I revew the progress.


Generally speaking, we're in reasonable shape for the final deadline now.  In
terms of goals, I would have liked to see some more performance testing: it's
always good to know exactly how much difference a set of changes have made for
various scenes.  Obviously we're out of time now though!

Cheers,

~Chris.

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

[Chris Foster-5]

On Mon, Aug 24, 2009 at 7:48 PM, Trevor Lovett<trevlovett@...> wrote:
> I fixed that oversight in sinCosGrid and you should see it pushed.

Odd, I don't see this in the repository.  Are you sure you pushed it
to the soc_optimization_cleanup branch?

> Also,
> I'm attaching a spreadsheet showing the relative speedup with the quadrics
> optimizations in place.  The numbers were taken from this phrase in the
> regression report:
>
> Frame took 298.46 milli secs
>
> I used the quadrics .rib's from the regression suite and got an average
> speedup of 3.2%-- a little less than predicted.  As for my system specs: a
> macbook pro w/ 2.4 ghz intel core 2 duo, 2gb ram.  Should I add this to the
> project beeker wiki?

Yeah, sure.  Thanks for the timings.

The 5% was a rough guess from what I was seeing inside the valgrind
profile, so it was never going to be super-accurate.  I am seeing
roughly 5% on my machine, but these things can vary widely anyway.

~Chris.

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

[Trevor Lovett]

Just pushed again--I could have accidentally pushed to the wrong repo. -t

On Wed, Aug 26, 2009 at 2:40 PM, Chris Foster <chris42f@...> wrote:

On Mon, Aug 24, 2009 at 7:48 PM, Trevor Lovett<trevlovett@...> wrote:
> I fixed that oversight in sinCosGrid and you should see it pushed.

Odd, I don't see this in the repository.  Are you sure you pushed it
to the soc_optimization_cleanup branch?

> Also,
> I'm attaching a spreadsheet showing the relative speedup with the quadrics
> optimizations in place.  The numbers were taken from this phrase in the
> regression report:
>
> Frame took 298.46 milli secs
>
> I used the quadrics .rib's from the regression suite and got an average
> speedup of 3.2%-- a little less than predicted.  As for my system specs: a
> macbook pro w/ 2.4 ghz intel core 2 duo, 2gb ram.  Should I add this to the
> project beeker wiki?

Yeah, sure.  Thanks for the timings.

The 5% was a rough guess from what I was seeing inside the valgrind
profile, so it was never going to be super-accurate.  I am seeing
roughly 5% on my machine, but these things can vary widely anyway.

~Chris.

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development

[Chris Foster-5]

On Wed, Aug 26, 2009 at 10:47 PM, Trevor Lovett<trevlovett@...> wrote:
> Just pushed again--I could have accidentally pushed to the wrong repo. -t

Ok, I've got it this time.  I ran the RTS quickly and things looked
good, so I've committed it to the trunk now :-)

BTW, you can check the state of your local repository to see what
things are committed using git log or even the graphical tool gitk.
Maybe you added the files to the index but forgot to commit?

~Chris.

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
Aqsis-development mailing list
Aqsis-development@...
https://lists.sourceforge.net/lists/listinfo/aqsis-development