From 67ed09abb4b856327ae6475ad6a7198380bf3d9a Mon Sep 17 00:00:00 2001 From: Daniel Dresser Date: Mon, 23 Mar 2026 14:29:20 -0700 Subject: [PATCH 1/2] IECoreGL::Buffer : Comment on perf hazard --- src/IECoreGL/Buffer.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/IECoreGL/Buffer.cpp b/src/IECoreGL/Buffer.cpp index e04e21dbf9..0f59dd6d3b 100644 --- a/src/IECoreGL/Buffer.cpp +++ b/src/IECoreGL/Buffer.cpp @@ -101,6 +101,11 @@ GLuint Buffer::buffer() const size_t Buffer::size() const { + // \todo : This is a nasty trap to leave lying around in this code - someone looking at the API + // could think that was safe to call while preparing data for rendering a frame, where it will introduce + // a pipeline stall that could likely have a pretty significant impact. If we want to support this, we + // should just store the size we called glBufferData with. + ScopedBinding binding( *this, GL_ARRAY_BUFFER ); int result = 0; glGetBufferParameteriv( GL_ARRAY_BUFFER, GL_BUFFER_SIZE, &result ); From ff3d1852145063001fc7e317cf043e6691e73815 Mon Sep 17 00:00:00 2001 From: Daniel Dresser Date: Mon, 23 Mar 2026 09:25:25 -0700 Subject: [PATCH 2/2] IECoreGL::MeshPrimitive : Use glDrawElements instead of tri'ing all vars --- include/IECoreGL/MeshPrimitive.h | 3 +- src/IECoreGL/MeshPrimitive.cpp | 29 +++++++++++++--- src/IECoreGL/ToGLMeshConverter.cpp | 54 ++++++++++++++++++++++++++++-- 3 files changed, 77 insertions(+), 9 deletions(-) diff --git a/include/IECoreGL/MeshPrimitive.h b/include/IECoreGL/MeshPrimitive.h index cf72faf1b6..421ca933e1 100644 --- a/include/IECoreGL/MeshPrimitive.h +++ b/include/IECoreGL/MeshPrimitive.h @@ -35,6 +35,7 @@ #ifndef IECOREGL_MESHPRIMITIVE_H #define IECOREGL_MESHPRIMITIVE_H +#include "IECoreGL/Buffer.h" #include "IECoreGL/Export.h" #include "IECoreGL/Primitive.h" @@ -51,7 +52,7 @@ class IECOREGL_API MeshPrimitive : public Primitive IE_CORE_DECLARERUNTIMETYPEDEXTENSION( IECoreGL::MeshPrimitive, MeshPrimitiveTypeId, Primitive ); - MeshPrimitive( unsigned numTriangles ); + MeshPrimitive( IECore::ConstIntVectorDataPtr meshIndices ); ~MeshPrimitive() override; Imath::Box3f bound() const override; diff --git a/src/IECoreGL/MeshPrimitive.cpp b/src/IECoreGL/MeshPrimitive.cpp index f08d715f39..88e0cee156 100644 --- a/src/IECoreGL/MeshPrimitive.cpp +++ b/src/IECoreGL/MeshPrimitive.cpp @@ -36,6 +36,7 @@ #include "IECoreGL/GL.h" #include "IECoreGL/State.h" +#include "IECoreGL/CachedConverter.h" #include "IECore/DespatchTypedData.h" @@ -57,11 +58,12 @@ class MeshPrimitive::MemberData : public IECore::RefCounted public : - MemberData( unsigned numTriangles ) : numTriangles( numTriangles ) + MemberData( IECore::ConstIntVectorDataPtr meshIndices ) : meshIndices( meshIndices ) { } - unsigned numTriangles; + IECore::ConstIntVectorDataPtr meshIndices; + ConstBufferPtr meshIndicesGL; Imath::Box3f bound; }; @@ -72,8 +74,8 @@ class MeshPrimitive::MemberData : public IECore::RefCounted IE_CORE_DEFINERUNTIMETYPED( MeshPrimitive ); -MeshPrimitive::MeshPrimitive( unsigned numTriangles ) - : m_memberData( new MemberData( numTriangles ) ) +MeshPrimitive::MeshPrimitive( IECore::ConstIntVectorDataPtr meshIndices ) + : m_memberData( new MemberData( meshIndices ) ) { } @@ -114,7 +116,24 @@ void MeshPrimitive::addPrimitiveVariable( const std::string &name, const IECoreS void MeshPrimitive::renderInstances( size_t numInstances ) const { - glDrawArraysInstancedARB( GL_TRIANGLES, 0, m_memberData->numTriangles * 3, numInstances ); + if( !m_memberData->meshIndicesGL ) + { + // \todo : The use of defaultCachedConverter here is very inefficient - the mesh indices have already + // been expanded from verticesPerFace to an explicit index list before being passed in ... any time + // we find the index list in the cache, the time we previously spent recomputing the expanded list + // was wasted. + // The same problem exists for primvars - we first fully expand the data out to FaceVarying in + // ToGLMeshConverter, and then we look up if it's in the cache and that work has already been done. + // The solution would be passing in more context to this convert call about how it's supposed to be + // converted, so we could just pass in the verticesPerFace, but indicate that they need to be converted + // to expanded indices. I had considered just adding a "Purpose" enum to this call, John suggested + // it should take some sort of function pointer, so this cache could be used for anything. Either + // way, it now seems complicated enough that we're not going to tackle it now. + m_memberData->meshIndicesGL = IECore::runTimeCast( CachedConverter::defaultCachedConverter()->convert( m_memberData->meshIndices.get() ) ); + } + + Buffer::ScopedBinding binding( *m_memberData->meshIndicesGL, GL_ELEMENT_ARRAY_BUFFER ); + glDrawElementsInstancedARB( GL_TRIANGLES, m_memberData->meshIndices->readable().size(), GL_UNSIGNED_INT, 0, numInstances ); } Imath::Box3f MeshPrimitive::bound() const diff --git a/src/IECoreGL/ToGLMeshConverter.cpp b/src/IECoreGL/ToGLMeshConverter.cpp index 2772c5a798..7c5110b034 100644 --- a/src/IECoreGL/ToGLMeshConverter.cpp +++ b/src/IECoreGL/ToGLMeshConverter.cpp @@ -34,6 +34,7 @@ #include "IECoreGL/ToGLMeshConverter.h" +#include "IECoreGL/CachedConverter.h" #include "IECoreGL/MeshPrimitive.h" #include "IECoreScene/FaceVaryingPromotionOp.h" @@ -49,6 +50,49 @@ using namespace IECoreGL; +namespace { + +void triangulateMeshIndices( + const IECoreScene::MeshPrimitive *mesh, + std::vector &newIds +) +{ + const std::vector &verticesPerFace = mesh->verticesPerFace()->readable(); + + newIds.clear(); + + int numTris = 0; + for( auto n : verticesPerFace ) + { + numTris += n - 2; + } + + newIds.reserve( numTris * 3 ); + + int faceVertexIdStart = 0; + for( int faceIdx = 0; faceIdx < (int)verticesPerFace.size(); faceIdx++ ) + { + int numFaceVerts = verticesPerFace[ faceIdx ]; + + const int i0 = faceVertexIdStart + 0; + + for( int i = 1; i < numFaceVerts - 1; i++ ) + { + const int i1 = faceVertexIdStart + i; + const int i2 = faceVertexIdStart + i + 1; + + /// Store the indices required to rebuild the facevarying primvars + newIds.push_back( i0 ); + newIds.push_back( i1 ); + newIds.push_back( i2 ); + } + + faceVertexIdStart += numFaceVerts; + } +} + +} // namespace + IE_CORE_DEFINERUNTIMETYPED( ToGLMeshConverter ); ToGLConverter::ConverterDescription ToGLMeshConverter::g_description; @@ -74,6 +118,9 @@ IECore::RunTimeTypedPtr ToGLMeshConverter::doConversion( IECore::ConstObjectPtr if( mesh->variables.find( "N" )==mesh->variables.end() ) { + // \todo - this is a weird place for this - you don't always need normals in order to render a mesh. + // Pretty wasteful to be running a normalsOp on a mesh if you're using a shader that doesn't need normals. + // the mesh has no normals - we need to explicitly add some. if it's a polygon // mesh (interpolation==linear) then we add per-face normals for a faceted look // and if it's a subdivision mesh we add smooth per-vertex normals. @@ -86,14 +133,15 @@ IECore::RunTimeTypedPtr ToGLMeshConverter::doConversion( IECore::ConstObjectPtr normalOp->operate(); } - mesh = IECoreScene::MeshAlgo::triangulate( mesh.get() ); - IECoreScene::FaceVaryingPromotionOpPtr faceVaryingOp = new IECoreScene::FaceVaryingPromotionOp; faceVaryingOp->inputParameter()->setValue( mesh ); faceVaryingOp->copyParameter()->setTypedValue( false ); faceVaryingOp->operate(); - MeshPrimitivePtr glMesh = new MeshPrimitive( mesh->numFaces() ); + IECore::IntVectorDataPtr meshIndices = new IECore::IntVectorData(); + triangulateMeshIndices( mesh.get(), IECoreScene::PrimitiveVariable::FaceVarying, meshIndices->writable(), nullptr ); + + MeshPrimitivePtr glMesh = new MeshPrimitive( meshIndices ); for ( IECoreScene::PrimitiveVariableMap::iterator pIt = mesh->variables.begin(); pIt != mesh->variables.end(); ++pIt ) {