Skip to content

Commit

Permalink
Attempt to fix leaking OpenGL state drawing bounding boxes
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@c42f
c42f committed Sep 6, 2016
1 parent 6cfb4fd commit 3a33d74
Show file tree
Hide file tree
Showing 5 changed files with 76 additions and 110 deletions.
55 changes: 0 additions & 55 deletions src/render/Geometry.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,6 @@ std::shared_ptr<Geometry> Geometry::create(QString fileName)
void Geometry::initializeGL()
{
destroyBuffers();

initializeBboxGL(shaderId("boundingbox"));
}

void Geometry::destroyBuffers()
Expand All @@ -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
Expand Down
1 change: 0 additions & 1 deletion src/render/Geometry.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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; }
Expand Down
20 changes: 7 additions & 13 deletions src/render/View3D.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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");

Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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());
}
}
}
Expand All @@ -442,6 +431,7 @@ void View3D::paintGL()
m_incrementalFrameTimer->start(10);

m_incrementalDraw = true;
glCheckError();
}

void View3D::drawMeshes(const TransformState& transState,
Expand Down Expand Up @@ -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();
}


Expand Down Expand Up @@ -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;
}

Expand All @@ -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
Expand Down
94 changes: 59 additions & 35 deletions src/render/glutil.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}


Expand Down
16 changes: 10 additions & 6 deletions src/render/glutil.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
///
Expand Down

0 comments on commit 3a33d74

Please sign in to comment.