Repository: FreeCAD/FreeCAD
File: src/Gui/NaviCube.cpp (C++)

Code before:
    glPopAttrib();
}

bool NaviCubeImplementation::isFramebufferValid() const
{
    return m_PickingFramebuffer && m_PickingFramebuffer->isValid();
}


void NaviCubeImplementation::ensureFramebufferValid()
{
    if (!isFramebufferValid()) {
        if (m_PickingFramebuffer) {
            delete m_PickingFramebuffer;
            m_PickingFramebuffer = nullptr;
        }

        qreal physicalCubeWidgetSize = getPhysicalCubeWidgetSize();
        m_PickingFramebuffer =
            new QOpenGLFramebufferObject(2 * physicalCubeWidgetSize,
                                         2 * physicalCubeWidgetSize,
                                         QOpenGLFramebufferObject::CombinedDepthStencil);
    }
}

NaviCubeImplementation::PickId NaviCubeImplementation::pickFace(short x, short y) {
    qreal physicalCubeWidgetSize = getPhysicalCubeWidgetSize();
    GLubyte pixels[4] = {0};
    ensureFramebufferValid();
    if (m_PickingFramebuffer && std::abs(x) <= physicalCubeWidgetSize / 2 &&
        std::abs(y) <= physicalCubeWidgetSize / 2) {
        static_cast<QOpenGLWidget*>(m_View3DInventorViewer->viewport())->makeCurrent();
        m_PickingFramebuffer->bind();

        glViewport(0, 0, physicalCubeWidgetSize * 2, physicalCubeWidgetSize * 2);

        drawNaviCube(true, 1.f);

        glFinish();
        glReadPixels(2 * x + physicalCubeWidgetSize, 2 * y + physicalCubeWidgetSize, 1, 1,
                     GL_RGBA, GL_UNSIGNED_BYTE, &pixels);
        m_PickingFramebuffer->release();
        static_cast<QOpenGLWidget*>(m_View3DInventorViewer->viewport())->doneCurrent();
    }
    return pixels[3] == 255 ? static_cast<PickId>(pixels[0]) : PickId::None;
}

bool NaviCubeImplementation::mousePressed(short x, short y) {
    m_MouseDown = true;
    m_MightDrag = inDragZone(x, y);
    PickId pick = pickFace(x, y);

Review comment (bug):
New framebuffer is allocated, even in case original pointer is still valid. So the test for `m_PickingFramebuffer` being not null could lead to memory leak. And while this is all theory not verified yet, it would be nice to put some debugging message informing user it would normally crash, but this change fixed problem.

Code after:
    glPopAttrib();
}

bool NaviCubeImplementation::isFramebufferValid() const
{
    return m_PickingFramebuffer && m_PickingFramebuffer->isValid();
}


void NaviCubeImplementation::ensureFramebufferValid()
{
    if (!isFramebufferValid()) {
        if (m_PickingFramebuffer) {

            if (!m_PickingFramebuffer->isValid()) {
                Base::Console().developerWarning("NaviCube", "The frame buffer has become invalid, a new frame buffer will be created\n");
            }
            
            delete m_PickingFramebuffer;
            m_PickingFramebuffer = nullptr;
        }

        qreal physicalCubeWidgetSize = getPhysicalCubeWidgetSize();
        m_PickingFramebuffer =
            new QOpenGLFramebufferObject(2 * physicalCubeWidgetSize,
                                         2 * physicalCubeWidgetSize,
                                         QOpenGLFramebufferObject::CombinedDepthStencil);
    }
}

NaviCubeImplementation::PickId NaviCubeImplementation::pickFace(short x, short y) {
    qreal physicalCubeWidgetSize = getPhysicalCubeWidgetSize();
    GLubyte pixels[4] = {0};
    ensureFramebufferValid();
    if (m_PickingFramebuffer && std::abs(x) <= physicalCubeWidgetSize / 2 &&
        std::abs(y) <= physicalCubeWidgetSize / 2) {
        static_cast<QOpenGLWidget*>(m_View3DInventorViewer->viewport())->makeCurrent();
        m_PickingFramebuffer->bind();

        glViewport(0, 0, physicalCubeWidgetSize * 2, physicalCubeWidgetSize * 2);

        drawNaviCube(true, 1.f);

        glFinish();
        glReadPixels(2 * x + physicalCubeWidgetSize, 2 * y + physicalCubeWidgetSize, 1, 1,
                     GL_RGBA, GL_UNSIGNED_BYTE, &pixels);
        m_PickingFramebuffer->release();
        static_cast<QOpenGLWidget*>(m_View3DInventorViewer->viewport())->doneCurrent();
    }
    return pixels[3] == 255 ? static_cast<PickId>(pixels[0]) : PickId::None;
}