Merge pull request #182 from johnhaddon/faceNormals

OpenGL polygon shading fix

Author: andrewkaufman

SConstruct

@@ -1173,6 +1173,7 @@ if env["PLATFORM"] == "posix" :
 # get the python link flags
 if pythonEnv["PYTHON_LINK_FLAGS"]=="" :
 	pythonEnv["PYTHON_LINK_FLAGS"] = getPythonConfig( pythonEnv, "--ldflags" )
+	pythonEnv["PYTHON_LINK_FLAGS"] = pythonEnv["PYTHON_LINK_FLAGS"].replace( "Python.framework/Versions/" + pythonEnv["PYTHON_VERSION"] + "/Python", "" )
 
 pythonEnv.Append( SHLINKFLAGS = pythonEnv["PYTHON_LINK_FLAGS"].split() )
 

include/IECore/MeshNormalsOp.h

@@ -36,6 +36,7 @@
 #define IECORE_MESHNORMALSOP_H
 
 #include "IECore/TypedPrimitiveOp.h"
+#include "IECore/NumericParameter.h"
 
 namespace IECore
 {
@@ -57,6 +58,8 @@ class MeshNormalsOp : public MeshPrimitiveOp
 		StringParameter * nPrimVarNameParameter();
 		const StringParameter * nPrimVarNameParameter() const;
 
+		IntParameter *interpolationParameter();
+		const IntParameter *interpolationParameter() const;
 
 	protected:
 

include/IECore/PolygonAlgo.inl

@@ -58,21 +58,17 @@ typename std::iterator_traits<Iterator>::value_type polygonNormal( Iterator firs
 
 	Vec n( 0 );
 
-	CircularIt v0It( first, last, first );
-	CircularIt v1It( v0It ); v1It++;
+	CircularIt vIt( first, last, first );
 	do
 	{
-		const Vec &v0 = *v0It;
-		const Vec &v1 = *v1It;
+		const Vec &v0 = *vIt; ++vIt;
+		const Vec &v1 = *vIt;
 
 		n.x += (v0.y - v1.y) * (v0.z + v1.z);
 		n.y += (v0.z - v1.z) * (v0.x + v1.x);
 		n.z += (v0.x - v1.x) * (v0.y + v1.y);
-
-		v0It++;
-		v1It++;
 	}
-	while( v0It != first );
+	while( vIt != first );
 
 	return normalized ? n.normalized() : n;
 }
@@ -89,19 +85,15 @@ Winding polygonWinding( Iterator first, Iterator last )
 
 	Real z( 0 );
 
-	CircularIt v0It( first, last, first );
-	CircularIt v1It( v0It ); v1It++;
+	CircularIt vIt( first, last, first );
 	do
 	{
-		const Vec &v0 = *v0It;
-		const Vec &v1 = *v1It;
+		const Vec &v0 = *vIt; ++vIt;
+		const Vec &v1 = *vIt;
 
 		z += (v0.x - v1.x) * (v0.y + v1.y);
-
-		v0It++;
-		v1It++;
 	}
-	while( v0It != first );
+	while( vIt != first );
 
 	return z < Real( 0 ) ? ClockwiseWinding : CounterClockwiseWinding;
 }

include/IECoreGL/MeshPrimitive.h

@@ -51,6 +51,14 @@ class MeshPrimitive : public Primitive
 		IE_CORE_DECLARERUNTIMETYPEDEXTENSION( IECoreGL::MeshPrimitive, MeshPrimitiveTypeId, Primitive );
 
 		/// Copies of all data are taken.
+		/// \deprecated. This constructor was being used to allow the MeshPrimitive to support
+		/// Vertex and Varying primitive variables in addPrimitiveVariable(), but it lacks the
+		/// information necessary to support Uniform primitive variables. In the future this
+		/// constructor will be removed - for forwards compatibility, use a ToGLMeshConverter
+		/// to create MeshPrimitives.
+		/// \todo Replace this with a simple MeshPrimitive( numTriangles ) constructor, remove all
+		/// the conversions from addPrimitiveVariable, and just rely on the work the ToGLMeshConverter
+		/// already does.
 		MeshPrimitive( IECore::ConstIntVectorDataPtr vertIds );
 		virtual ~MeshPrimitive();
 

include/IECoreGL/bindings/MeshPrimitiveBinding.h

@@ -0,0 +1,45 @@
+//////////////////////////////////////////////////////////////////////////
+//
+//  Copyright (c) 2013, Image Engine Design Inc. All rights reserved.
+//
+//  Redistribution and use in source and binary forms, with or without
+//  modification, are permitted provided that the following conditions are
+//  met:
+//
+//     * Redistributions of source code must retain the above copyright
+//       notice, this list of conditions and the following disclaimer.
+//
+//     * Redistributions in binary form must reproduce the above copyright
+//       notice, this list of conditions and the following disclaimer in the
+//       documentation and/or other materials provided with the distribution.
+//
+//     * Neither the name of Image Engine Design nor the names of any
+//       other contributors to this software may be used to endorse or
+//       promote products derived from this software without specific prior
+//       written permission.
+//
+//  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS
+//  IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+//  THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+//  PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+//  CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
+//  EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+//  PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
+//  PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+//  LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
+//  NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+//  SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+//
+//////////////////////////////////////////////////////////////////////////
+
+#ifndef IECOREGL_MESHPRIMITIVEBINDING_H
+#define IECOREGL_MESHPRIMITIVEBINDING_H
+
+namespace IECoreGL
+{
+
+void bindMeshPrimitive();
+
+}
+
+#endif // IECOREGL_MESHPRIMITIVEBINDING_H

src/IECore/MeshNormalsOp.cpp

@@ -32,12 +32,12 @@
 //
 //////////////////////////////////////////////////////////////////////////
 
+#include "boost/format.hpp"
+
 #include "IECore/MeshNormalsOp.h"
 #include "IECore/DespatchTypedData.h"
 #include "IECore/CompoundParameter.h"
 
-#include "boost/format.hpp"
-
 using namespace IECore;
 using namespace std;
 
@@ -59,8 +59,19 @@ MeshNormalsOp::MeshNormalsOp() : MeshPrimitiveOp( "Calculates vertex normals for
 		"N"
 	);
 
+	IntParameter::PresetsContainer interpolationPresets;
+	interpolationPresets.push_back( IntParameter::Preset( "Vertex", PrimitiveVariable::Vertex ) );
+	interpolationPresets.push_back( IntParameter::Preset( "Uniform", PrimitiveVariable::Uniform ) );
+	IntParameterPtr interpolationParameter = new IntParameter(
+		"interpolation",
+		"The primitive variable interpolation type for the calculated normals.",
+		PrimitiveVariable::Vertex,
+		interpolationPresets
+	);
+
 	parameters()->addParameter( pPrimVarNameParameter );
 	parameters()->addParameter( nPrimVarNameParameter );
+	parameters()->addParameter( interpolationParameter );
 }
 
 MeshNormalsOp::~MeshNormalsOp()
@@ -87,12 +98,22 @@ const StringParameter * MeshNormalsOp::nPrimVarNameParameter() const
 	return parameters()->parameter<StringParameter>( "nPrimVarName" );
 }
 
+IntParameter * MeshNormalsOp::interpolationParameter()
+{
+	return parameters()->parameter<IntParameter>( "interpolation" );
+}
+
+const IntParameter * MeshNormalsOp::interpolationParameter() const
+{
+	return parameters()->parameter<IntParameter>( "interpolation" );
+}
+
 struct MeshNormalsOp::CalculateNormals
 {
 	typedef DataPtr ReturnType;
 
-	CalculateNormals( const IntVectorData * vertsPerFace, const IntVectorData * vertIds )
-		:	m_vertsPerFace( vertsPerFace ), m_vertIds( vertIds )
+	CalculateNormals( const IntVectorData *vertsPerFace, const IntVectorData *vertIds, PrimitiveVariable::Interpolation interpolation )
+		:	m_vertsPerFace( vertsPerFace ), m_vertIds( vertIds ), m_interpolation( interpolation )
 	{
 	}
 
@@ -109,39 +130,63 @@ struct MeshNormalsOp::CalculateNormals
 		typename T::Ptr normalsData = new T;
 		normalsData->setInterpretation( GeometricData::Normal );
 		VecContainer &normals = normalsData->writable();
-		normals.resize( points.size(), Vec( 0 ) );
+		if( m_interpolation == PrimitiveVariable::Uniform )
+		{
+			normals.reserve( vertsPerFace.size() );
+		}
+		else
+		{
+			normals.resize( points.size(), Vec( 0 ) );
+		}
 
-		// for each face, calculate its normal, and accumulate that normal onto
-		// the normal for each of its vertices.
+		// loop over the faces
 		const int *vertId = &(vertIds[0]);
 		for( vector<int>::const_iterator it = vertsPerFace.begin(); it!=vertsPerFace.end(); it++ )
 		{
+			// calculate the face normal. note that this method is very naive, and doesn't
+			// cope with colinear vertices or concave faces - we could use polygonNormal() from
+			// PolygonAlgo.h to deal with that, but currently we'd prefer to avoid the overhead.
 			const Vec &p0 = points[*vertId];
 			const Vec &p1 = points[*(vertId+1)];
 			const Vec &p2 = points[*(vertId+2)];
 
 			Vec normal = (p2-p1).cross(p0-p1);
 			normal.normalize();
-			for( int i=0; i<*it; i++ )
+
+			if( m_interpolation == PrimitiveVariable::Uniform )
 			{
-				normals[*vertId] += normal;
-				vertId++;
+				normals.push_back( normal );
+				vertId += *it;
+			}
+			else
+			{
+				// accumulate the face normal onto each of the vertices
+				// for this face.
+				for( int i=0; i<*it; ++i )
+				{
+					normals[*vertId] += normal;
+					++vertId;
+				}
 			}
 		}
 
 		// normalize each of the vertex normals
-		for( typename VecContainer::iterator it=normals.begin(); it!=normals.end(); it++ )
+		if( m_interpolation == PrimitiveVariable::Vertex )
 		{
-			it->normalize();
+			for( typename VecContainer::iterator it=normals.begin(), eIt=normals.end(); it != eIt; ++it )
+			{
+				it->normalize();
+			}
 		}
-
+		
 		return normalsData;
 	}
 
 	private :
 
 		ConstIntVectorDataPtr m_vertsPerFace;
 		ConstIntVectorDataPtr m_vertIds;
+		PrimitiveVariable::Interpolation m_interpolation;
 
 };
 
@@ -171,8 +216,10 @@ void MeshNormalsOp::modifyTypedPrimitive( MeshPrimitive * mesh, const CompoundOb
 		throw InvalidArgumentException( e );
 	}
 
-	CalculateNormals f( mesh->verticesPerFace(), mesh->vertexIds() );
+	const PrimitiveVariable::Interpolation interpolation = static_cast<PrimitiveVariable::Interpolation>( operands->member<IntData>( "interpolation" )->readable() );
+	
+	CalculateNormals f( mesh->verticesPerFace(), mesh->vertexIds(), interpolation );
 	DataPtr n = despatchTypedData<CalculateNormals, TypeTraits::IsVec3VectorTypedData, HandleErrors>( pvIt->second.data, f );
 
-	mesh->variables[ nPrimVarNameParameter()->getTypedValue() ] = PrimitiveVariable( PrimitiveVariable::Vertex, n );
+	mesh->variables[ nPrimVarNameParameter()->getTypedValue() ] = PrimitiveVariable( interpolation, n );
 }

src/IECoreGL/MeshPrimitive.cpp

@@ -60,8 +60,12 @@ struct MeshPrimitive::MemberData : public IECore::RefCounted
 	IECore::ConstIntVectorDataPtr vertIds;
 	Imath::Box3f bound;
 
-	/// \todo This could be removed and the ToGLMeshConverter could use FaceVaryingPromotionOp
-	/// to convert everything to FaceVarying before being added.
+	/// \todo This could be removed now the ToGLMeshConverter uses FaceVaryingPromotionOp
+	/// to convert everything to FaceVarying before being added. The only reason we're even
+	/// doing this still is in case client code is creating MeshPrimitives directly rather
+	/// than using the converter. We should actually be able to remove the code here, and
+	/// instead of accept vertIds in the MeshPrimitive constructor, just accept the number
+	/// of triangles instead.
 	class ToFaceVaryingConverter
 	{
 		public:
@@ -118,23 +122,23 @@ IECore::ConstIntVectorDataPtr MeshPrimitive::vertexIds() const
 
 void MeshPrimitive::addPrimitiveVariable( const std::string &name, const IECore::PrimitiveVariable &primVar )
 {
-	if ( primVar.interpolation==IECore::PrimitiveVariable::Vertex || primVar.interpolation==IECore::PrimitiveVariable::Varying )
+	if( name == "P" )
 	{
-		if ( name == "P" )
+		// update the bounding box.
+		m_memberData->bound.makeEmpty();
+		IECore::ConstV3fVectorDataPtr points = IECore::runTimeCast< IECore::V3fVectorData >( primVar.data );
+		if( points )
 		{
-			// update the bounding box.
-			m_memberData->bound.makeEmpty();
-			IECore::ConstV3fVectorDataPtr points = IECore::runTimeCast< IECore::V3fVectorData >( primVar.data );
-			if ( points )
+			const std::vector<Imath::V3f> &p = points->readable();
+			for( unsigned int i=0; i<p.size(); i++ )
 			{
-				const std::vector<Imath::V3f> &p = points->readable();
-				for( unsigned int i=0; i<p.size(); i++ )
-				{
-					m_memberData->bound.extendBy( p[i] );
-				}
+				m_memberData->bound.extendBy( p[i] );
 			}
 		}
-
+	}
+	
+	if ( primVar.interpolation==IECore::PrimitiveVariable::Vertex || primVar.interpolation==IECore::PrimitiveVariable::Varying )
+	{
 		MemberData::ToFaceVaryingConverter primVarConverter( m_memberData->vertIds );
 		// convert to facevarying
 		IECore::DataPtr newData = IECore::despatchTypedData< MemberData::ToFaceVaryingConverter, IECore::TypeTraits::IsVectorTypedData >( primVar.data, primVarConverter );