From c6613812b36101ade0864fc98cee551ca522fc90 Mon Sep 17 00:00:00 2001 From: Chris Foster Date: Mon, 5 Sep 2016 07:56:27 +1000 Subject: [PATCH] Attempt to fix leaking OpenGL state drawing bounding boxes There was a segfault with Nvidia driver 346.82, inside PointArray::drawPoints() after reloading a pair of files with the keyboard shortcut F5 (see #122). Commenting out the code associated with initializing the bounding box vertex array inside Geometry.cpp works around the segfault. However, it's not clear that this is the root cause and there was actually a bug in this part of the code. For now, refactor things to send the bounding box vertices to the GPU every frame in the hope that this will fix things. I suppose this is a bit inefficient, but the main point cloud data is already sent this way. --- src/render/Geometry.cpp | 55 ------------------------ src/render/Geometry.h | 1 - src/render/View3D.cpp | 20 +++------ src/render/glutil.cpp | 94 ++++++++++++++++++++++++++--------------- src/render/glutil.h | 16 ++++--- 5 files changed, 76 insertions(+), 110 deletions(-) diff --git a/src/render/Geometry.cpp b/src/render/Geometry.cpp index 15a90fa1..64df0b3f 100644 --- a/src/render/Geometry.cpp +++ b/src/render/Geometry.cpp @@ -67,8 +67,6 @@ std::shared_ptr Geometry::create(QString fileName) void Geometry::initializeGL() { destroyBuffers(); - - initializeBboxGL(shaderId("boundingbox")); } void Geometry::destroyBuffers() @@ -94,59 +92,6 @@ void Geometry::destroyBuffers() } -void Geometry::initializeBboxGL(unsigned int bboxShader) -{ - - // tfm::printfln("Geometry :: initializeGL - %i", bboxShader); - - if (!bboxShader) - { - tfm::printfln("Bounding box shader was not defined for geometry."); - return; - } - - // Transform to box min for stability with large offsets - Imath::Box3f box2(V3f(0), V3f(m_bbox.max - m_bbox.min)); - - GLfloat verts[] = { - (GLfloat)box2.min.x, (GLfloat)box2.min.y, (GLfloat)box2.min.z, - (GLfloat)box2.min.x, (GLfloat)box2.max.y, (GLfloat)box2.min.z, - (GLfloat)box2.max.x, (GLfloat)box2.max.y, (GLfloat)box2.min.z, - (GLfloat)box2.max.x, (GLfloat)box2.min.y, (GLfloat)box2.min.z, - (GLfloat)box2.min.x, (GLfloat)box2.min.y, (GLfloat)box2.max.z, - (GLfloat)box2.min.x, (GLfloat)box2.max.y, (GLfloat)box2.max.z, - (GLfloat)box2.max.x, (GLfloat)box2.max.y, (GLfloat)box2.max.z, - (GLfloat)box2.max.x, (GLfloat)box2.min.y, (GLfloat)box2.max.z - }; - unsigned char inds[] = { - // rows: bottom, sides, top - 0,1, 1,2, 2,3, 3,0, - 0,4, 1,5, 2,6, 3,7, - 4,5, 5,6, 6,7, 7,4 - }; - - // create VBA VBO for rendering ... - GLuint bboxVertexArray; - glGenVertexArrays(1, &bboxVertexArray); - glBindVertexArray(bboxVertexArray); - - setVAO("boundingbox", bboxVertexArray); - - GlBuffer positionBuffer; - positionBuffer.bind(GL_ARRAY_BUFFER); - glBufferData(GL_ARRAY_BUFFER, 3*8*sizeof(float), verts, GL_STATIC_DRAW); - - GLuint positionAttribute = glGetAttribLocation(bboxShader, "position"); - glVertexAttribPointer(positionAttribute, 3, GL_FLOAT, GL_FALSE, sizeof(float)*(3), (const GLvoid *)0); - glEnableVertexAttribArray(positionAttribute); - - GlBuffer elementBuffer; - elementBuffer.bind(GL_ELEMENT_ARRAY_BUFFER); - glBufferData(GL_ELEMENT_ARRAY_BUFFER, 2*4*3*sizeof(char), inds, GL_STATIC_DRAW); - - glBindVertexArray(0); -} - const unsigned int Geometry::getVAO(const char * vertexArrayName) const { // always call this from an active OpenGL context diff --git a/src/render/Geometry.h b/src/render/Geometry.h index 43763547..48acfbc9 100644 --- a/src/render/Geometry.h +++ b/src/render/Geometry.h @@ -169,7 +169,6 @@ class Geometry : public QObject void setBoundingBox(const Imath::Box3d& bbox) { m_bbox = bbox; } void destroyBuffers(); - void initializeBboxGL(unsigned int bboxShader); /// Set VAO void setVAO(const char * vertexArrayName, const unsigned int vertArrayId) { m_VAO[std::string(vertexArrayName)] = vertArrayId; } diff --git a/src/render/View3D.cpp b/src/render/View3D.cpp index b5ea4aa8..96de0cf5 100644 --- a/src/render/View3D.cpp +++ b/src/render/View3D.cpp @@ -112,7 +112,6 @@ void View3D::initializeGLGeometry(int begin, int end) if (m_boundingBoxShader->isValid()) { // TODO: build a shader manager for this - geoms[i]->setShaderId("boundingbox", m_boundingBoxShader->shaderProgram().programId()); geoms[i]->setShaderId("meshface", m_meshFaceShader->shaderProgram().programId()); geoms[i]->setShaderId("meshedge", m_meshEdgeShader->shaderProgram().programId()); geoms[i]->initializeGL(); @@ -265,15 +264,10 @@ void View3D::initializeGL() (const char*)glGetString(GL_SHADING_LANGUAGE_VERSION), (const char*)glewGetString(GLEW_VERSION)); - // GL_CHECK has to be defined for this to actually do something - glCheckError(); - initCursor(10, 1); initAxes(); initGrid(2.0f); - glCheckError(); - m_boundingBoxShader.reset(new ShaderProgram()); m_boundingBoxShader->setShaderFromSourceFile("shaders:bounding_box.glsl"); @@ -307,7 +301,6 @@ void View3D::resizeGL(int w, int h) //Note: This function receives "device pixel sizes" for correct OpenGL viewport setup // Under OS X with retina display, this becomes important as it is 2x the width() or height() // of the normal window (there is a devicePixelRatio() function to deal with this). - if (m_badOpenGL) return; @@ -413,15 +406,11 @@ void View3D::paintGL() if (m_boundingBoxShader->isValid()) { QGLShaderProgram &boundingBoxShader = m_boundingBoxShader->shaderProgram(); - // shader boundingBoxShader.bind(); - // matrix stack - transState.setUniforms(boundingBoxShader.programId()); - for (size_t i = 0; i < geoms.size(); ++i) { - drawBoundingBox(transState, geoms[i]->getVAO("boundingbox"), geoms[i]->boundingBox().min, - Imath::C3f(1), geoms[i]->shaderId("boundingbox")); //boundingBoxShader.programId() + drawBox(transState, geoms[i]->boundingBox(), Imath::C3f(1), + boundingBoxShader.programId()); } } } @@ -442,6 +431,7 @@ void View3D::paintGL() m_incrementalFrameTimer->start(10); m_incrementalDraw = true; + glCheckError(); } void View3D::drawMeshes(const TransformState& transState, @@ -471,6 +461,7 @@ void View3D::drawMeshes(const TransformState& transState, for(size_t i = 0; i < geoms.size(); ++i) geoms[i]->drawEdges(meshEdgeShader, transState); } + glCheckError(); } @@ -1001,6 +992,7 @@ DrawCount View3D::drawPoints(const TransformState& transState, totDrawCount += geom.drawPoints(prog, transState, quality, incrementalDraw); } glDisable(GL_VERTEX_PROGRAM_POINT_SIZE); + glCheckError(); return totDrawCount; } @@ -1010,6 +1002,8 @@ DrawCount View3D::drawPoints(const TransformState& transState, /// `clickPos` - 2D position in viewport, as from a mouse event. Imath::V3d View3D::guessClickPosition(const QPoint& clickPos) { + // TODO: Handle device pixel ratio here by scaling clickPos rather than elsewhere? + // Get new point in the projected coordinate system using the click // position x,y and the z of a reference position. Take the reference point // of interest to be between the camera rotation center and the camera diff --git a/src/render/glutil.cpp b/src/render/glutil.cpp index a46f7261..a3aca356 100644 --- a/src/render/glutil.cpp +++ b/src/render/glutil.cpp @@ -70,55 +70,79 @@ void TransformState::setOrthoProjection(double left, double right, double bottom //------------------------------------------------------------------------------ void drawBox(const TransformState& transState, - const Imath::Box3d& bbox, + const Imath::Box3d& box, const Imath::C3f& col, - const GLuint& shaderProgram) + const GLuint shaderProgram) { - // Transform to box min for stability with large offsets - // This function is allowing you to render any box - TransformState trans2 = transState.translate(bbox.min); - Imath::Box3f box2(V3f(0), V3f(bbox.max - bbox.min)); + // Transform to box min in double on CPU side to remove float cancellation + // error on GPU side + TransformState trans2 = transState.translate(box.min); + Imath::Box3f box2(V3f(0), V3f(box.max - box.min)); drawBox(trans2, box2, col, shaderProgram); } + void drawBox(const TransformState& transState, - const Imath::Box3f& bbox, + const Imath::Box3f& box, const Imath::C3f& col, - const GLuint& shaderProgram) -{ - // This function is allowing you to render any box - // TODO: FIX ME - tfm::printfln("drawBox :: has not been implemented, yet."); -} - -void drawBoundingBox(const TransformState& transState, - const GLuint& bboxVertexArray, - const Imath::V3f& offset, - const Imath::C3f& col, - const GLuint& shaderProgram) + const GLuint shaderProgram) { - // Transform to box min for stability with large offsets - TransformState trans2 = transState.translate(offset); + // Set shader uniform parameters. `shaderProgram` should already be bound + transState.setUniforms(shaderProgram); + GLint colorLoc = glGetUniformLocation(shaderProgram, "color"); + if (colorLoc >= 0) + glUniform4f(colorLoc, col.x, col.y, col.z, 1.0); + // Shader varying parameters. + GLint positionLoc = glGetAttribLocation(shaderProgram, "position"); + assert(positionLoc >= 0); + // BBox geometry + GLuint vao; + glGenVertexArrays(1, &vao); + glBindVertexArray(vao); + + // Send vertex data to GPU + GLfloat verts[] = { + box.min.x, box.min.y, box.min.z, + box.min.x, box.max.y, box.min.z, + box.max.x, box.max.y, box.min.z, + box.max.x, box.min.y, box.min.z, + box.min.x, box.min.y, box.max.z, + box.min.x, box.max.y, box.max.z, + box.max.x, box.max.y, box.max.z, + box.max.x, box.min.y, box.max.z + }; + GlBuffer vertexData; + vertexData.bind(GL_ARRAY_BUFFER); + glBufferData(GL_ARRAY_BUFFER, sizeof(verts), verts, GL_STREAM_DRAW); + // Enable position location in VAO & connect to shader location + glEnableVertexAttribArray(positionLoc); + glVertexAttribPointer(positionLoc, 3, GL_FLOAT, GL_FALSE, 0, NULL); + + // Send line connectivity to GPU + GLubyte inds[] = { + // rows: bottom, sides, top + 0,1, 1,2, 2,3, 3,0, + 0,4, 1,5, 2,6, 3,7, + 4,5, 5,6, 6,7, 7,4 + }; + GlBuffer topologyData; + topologyData.bind(GL_ELEMENT_ARRAY_BUFFER); + glBufferData(GL_ELEMENT_ARRAY_BUFFER, sizeof(inds), inds, GL_STREAM_DRAW); + + // Schedule lines for draw. glEnable(GL_LINE_SMOOTH); glEnable(GL_BLEND); glBlendFunc(GL_SRC_ALPHA, GL_ONE_MINUS_SRC_ALPHA); - glLineWidth(1); - - // should already be bound ... - //glUseProgram(shaderProg); - - trans2.setUniforms(shaderProgram); - GLint colorLoc = glGetUniformLocation(shaderProgram, "color"); - //assert(colorLoc >= 0); //this can easily happen, if you don't USE "color" in the shader due to optimization - glUniform4f(colorLoc, col.x, col.y, col.z, 1.0); // , col.a - - glBindVertexArray(bboxVertexArray); - glDrawElements(GL_LINES, 3*8, GL_UNSIGNED_BYTE, 0); - glBindVertexArray(0); - + glDrawElements(GL_LINES, sizeof(inds)/sizeof(inds[0]), GL_UNSIGNED_BYTE, 0); + //glDrawArrays(GL_POINTS, 0, 8); glDisable(GL_BLEND); glDisable(GL_LINE_SMOOTH); + + // Cleanup! + glDisableVertexAttribArray(positionLoc); + glDeleteVertexArrays(1, &vao); + glCheckError(); } diff --git a/src/render/glutil.h b/src/render/glutil.h index dcf1d2ae..c9e0bb94 100644 --- a/src/render/glutil.h +++ b/src/render/glutil.h @@ -90,12 +90,16 @@ struct TransformState //---------------------------------------------------------------------- /// Utilites for drawing simple primitives -void drawBox(const TransformState& transState, - const Imath::Box3d& bbox, const Imath::C3f& col, const GLuint& shaderProgram); -void drawBox(const TransformState& transState, - const Imath::Box3f& bbox, const Imath::C3f& col, const GLuint& shaderProgram); -void drawBoundingBox(const TransformState& transState, const GLuint& bboxVertexArray, - const Imath::V3f& offset, const Imath::C3f& col, const GLuint& shaderProgram); + +/// Draw edges of `box` with the given shader. +/// +/// `shaderProgram` is assumed to already be bound, and to have a "position" +/// input vertex attribute. Color `col` is copied into shader uniform "color" +/// if it exists. +void drawBox(const TransformState& transState, const Imath::Box3d& box, + const Imath::C3f& col, const GLuint shaderProgram); +void drawBox(const TransformState& transState, const Imath::Box3f& box, + const Imath::C3f& col, const GLuint shaderProgram); /// Draw a sphere using the given shader. May be semitransparent. ///