From 7014904755aa361192009b720edb0596a9a4b396 Mon Sep 17 00:00:00 2001 From: Andy Ferris Date: Fri, 26 Aug 2016 13:46:40 +1000 Subject: [PATCH 01/10] Changed mouse controls * Double click for select point and recenter * Right click becomes pan. In standard mode it pans in the x-y plane. In trackball mode it pans in the image plane. Might be nice for touchscreen interface? * Seperate codepath for mouse wheel (same UI effect though) --- src/gui/InteractiveCamera.h | 67 +++++++++++++++++++++++++++++++++---- src/render/View3D.cpp | 13 ++++--- src/render/View3D.h | 1 + 3 files changed, 71 insertions(+), 10 deletions(-) diff --git a/src/gui/InteractiveCamera.h b/src/gui/InteractiveCamera.h index b641744c..cea7d0d9 100644 --- a/src/gui/InteractiveCamera.h +++ b/src/gui/InteractiveCamera.h @@ -218,20 +218,75 @@ class InteractiveCamera : public QObject m_trackballInteraction = trackballInteraction; } + /// Zoom the camera using a drag of the mouse + /// + /// The previous and current positions of the mouse during the move are + /// given by prevPos and currPos. The camera position is zoomed in + /// toward the center. + void mouseZoom(QPoint prevPos, QPoint currPos) + { + // exponential zooming gives scale-independent sensitivity + qreal dy = qreal(currPos.y() - prevPos.y())/m_viewport.height(); + const qreal zoomSpeed = 3.0f; + m_dist *= std::exp(zoomSpeed*dy); + emit viewChanged(); + } + + /// Move the camera using a drag of the mouse. /// /// The previous and current positions of the mouse during the move are /// given by prevPos and currPos. By default this rotates the camera - /// around the center, but if zoom is true, the camera position is - /// zoomed in toward the center instead. - void mouseDrag(QPoint prevPos, QPoint currPos, bool zoom = false) + /// around the center, but if pan is true, the camera position is + /// panned in the x-y plane (or image plane in trackball mode) instead. + void mouseDrag(QPoint prevPos, QPoint currPos, bool pan = false) { - if(zoom) + if(pan) { // exponential zooming gives scale-independent sensitivity + //qreal dy = qreal(currPos.y() - prevPos.y())/m_viewport.height(); + //const qreal zoomSpeed = 3.0f; + //m_dist *= std::exp(zoomSpeed*dy); + + qreal dx = qreal(currPos.x() - prevPos.x())/m_viewport.height(); qreal dy = qreal(currPos.y() - prevPos.y())/m_viewport.height(); - const qreal zoomSpeed = 3.0f; - m_dist *= std::exp(zoomSpeed*dy); + + if (m_trackballInteraction) + { + // Trackball mode is straightforard: just rotate a + // translation in the image plane to the global coordinates + QVector3D dr(-dx, dy, 0); + dr = m_rot.inverted() * dr; // Rotate it + + m_center.x += m_dist * dr.x(); + m_center.y += m_dist * dr.y(); + m_center.z += m_dist * dr.z(); + } + else + { + // In this mode we move in the x-y plane. Need to project + // translation seperately for x and y to avoid the vertical + // drag being scaled by cos(angle from vertical). + + // vertical drag + QVector3D dr_x(0, dy, 0); + dr_x = m_rot.inverted() * dr_x; // Rotate it + qreal len = dr_x.length(); + qreal len_xy = sqrt(dr_x.x()*dr_x.x() + dr_x.y()*dr_x.y()); + if (len_xy > 0) + { + qreal scale = len / len_xy; + m_center.x += m_dist * dr_x.x() * scale; + m_center.y += m_dist * dr_x.y() * scale; + } + + // horizontal drag + QVector3D dr_y(-dx, 0, 0); + dr_y = m_rot.inverted() * dr_y; // Rotate it + + m_center.x += m_dist * dr_y.x(); + m_center.y += m_dist * dr_y.y(); + } } else { diff --git a/src/render/View3D.cpp b/src/render/View3D.cpp index bf317230..1d4335e7 100644 --- a/src/render/View3D.cpp +++ b/src/render/View3D.cpp @@ -534,6 +534,11 @@ void View3D::mousePressEvent(QMouseEvent* event) } } +void View3D::mouseDoubleClickEvent(QMouseEvent* event) +{ + snapToPoint(guessClickPosition(event->pos())); +} + void View3D::snapToPoint(const Imath::V3d & pos) { @@ -561,16 +566,16 @@ void View3D::mouseMoveEvent(QMouseEvent* event) { if (m_mouseButton == Qt::MidButton) return; - bool zooming = m_mouseButton == Qt::RightButton; + bool panning = (m_mouseButton == Qt::RightButton); if(event->modifiers() & Qt::ControlModifier) { m_cursorPos = m_camera.mouseMovePoint(m_cursorPos, event->pos() - m_prevMousePos, - zooming); + panning); restartRender(); } else - m_camera.mouseDrag(m_prevMousePos, event->pos(), zooming); + m_camera.mouseDrag(m_prevMousePos, event->pos(), panning); m_prevMousePos = event->pos(); } @@ -579,7 +584,7 @@ void View3D::mouseMoveEvent(QMouseEvent* event) void View3D::wheelEvent(QWheelEvent* event) { // Translate mouse wheel events into vertical dragging for simplicity. - m_camera.mouseDrag(QPoint(0,0), QPoint(0, -event->delta()/2), true); + m_camera.mouseZoom(QPoint(0,0), QPoint(0, -event->delta()/2)); } diff --git a/src/render/View3D.h b/src/render/View3D.h index 02817da2..df0b7f12 100644 --- a/src/render/View3D.h +++ b/src/render/View3D.h @@ -73,6 +73,7 @@ class View3D : public QGLWidget // Qt event callbacks void mousePressEvent(QMouseEvent* event); + void mouseDoubleClickEvent(QMouseEvent* event); void mouseMoveEvent(QMouseEvent* event); void wheelEvent(QWheelEvent* event); void keyPressEvent(QKeyEvent* event); From 34ad3923c13578b1cf38191692089934e39966a2 Mon Sep 17 00:00:00 2001 From: Chris Foster Date: Mon, 29 Aug 2016 15:24:20 +1000 Subject: [PATCH 02/10] Refactor logging to use tfm::vformat This allows us to put the early bailout logic in one place, which is a nice cleanup. --- src/gui/QtLogger.h | 10 +++++----- src/logger.cpp | 12 +++++++++++- src/logger.h | 35 ++++++++++++++++------------------- 3 files changed, 32 insertions(+), 25 deletions(-) diff --git a/src/gui/QtLogger.h b/src/gui/QtLogger.h index d8b0a98c..fe55e3c2 100644 --- a/src/gui/QtLogger.h +++ b/src/gui/QtLogger.h @@ -18,11 +18,6 @@ class QtLogger : public QObject, public Logger public: QtLogger(QObject* parent = 0) : QObject(parent) {} - virtual void log(LogLevel level, const std::string& msg) - { - emit logMessage(level, QString::fromUtf8(msg.c_str())); - } - signals: /// Signal emitted every time a log message comes in void logMessage(int logLevel, QString msg); @@ -31,6 +26,11 @@ class QtLogger : public QObject, public Logger void progressPercent(int percent); protected: + virtual void logImpl(LogLevel level, const std::string& msg) + { + emit logMessage(level, QString::fromUtf8(msg.c_str())); + } + virtual void progressImpl(double progressFraction) { emit progressPercent(int(100*progressFraction)); diff --git a/src/logger.cpp b/src/logger.cpp index 2a478a5b..0ea7d401 100644 --- a/src/logger.cpp +++ b/src/logger.cpp @@ -5,6 +5,16 @@ #include +//------------------------------------------------------------------------------ +void Logger::log(LogLevel level, const char* fmt, tfm::FormatListRef flist) +{ + if (level > m_logLevel) + return; + std::ostringstream ss; + tfm::vformat(ss, fmt, flist); + logImpl(level, ss.str()); +} + //------------------------------------------------------------------------------ StreamLogger::StreamLogger(std::ostream& outStream) @@ -19,7 +29,7 @@ StreamLogger::~StreamLogger() m_out << "\n"; } -void StreamLogger::log(LogLevel level, const std::string& msg) +void StreamLogger::logImpl(LogLevel level, const std::string& msg) { if (m_prevPrintWasProgress) tfm::format(m_out, "\n"); diff --git a/src/logger.h b/src/logger.h index 2ee48644..8281f824 100644 --- a/src/logger.h +++ b/src/logger.h @@ -32,50 +32,45 @@ class Logger template \ void progress(const char* fmt, TINYFORMAT_VARARGS(n)) \ { \ - if (m_logProgress) \ - log(Progress, tfm::format(fmt, TINYFORMAT_PASSARGS(n))); \ + log(Progress, fmt, tfm::makeFormatList(TINYFORMAT_PASSARGS(n)));\ } \ \ template \ void debug(const char* fmt, TINYFORMAT_VARARGS(n)) \ { \ - if (m_logLevel >= Debug) \ - log(Debug, tfm::format(fmt, TINYFORMAT_PASSARGS(n))); \ + log(Debug, fmt, tfm::makeFormatList(TINYFORMAT_PASSARGS(n))); \ } \ \ template \ void info(const char* fmt, TINYFORMAT_VARARGS(n)) \ { \ - if (m_logLevel >= Info) \ - log(Info, tfm::format(fmt, TINYFORMAT_PASSARGS(n))); \ + log(Info, fmt, tfm::makeFormatList(TINYFORMAT_PASSARGS(n))); \ } \ \ template \ void warning(const char* fmt, TINYFORMAT_VARARGS(n)) \ { \ - if (m_logLevel >= Warning) \ - log(Warning, tfm::format(fmt, TINYFORMAT_PASSARGS(n))); \ + log(Warning, fmt, tfm::makeFormatList(TINYFORMAT_PASSARGS(n)));\ } \ \ template \ void error(const char* fmt, TINYFORMAT_VARARGS(n)) \ { \ - if (m_logLevel >= Error) \ - log(Error, tfm::format(fmt, TINYFORMAT_PASSARGS(n))); \ + log(Error, fmt, tfm::makeFormatList(TINYFORMAT_PASSARGS(n))); \ } TINYFORMAT_FOREACH_ARGNUM(DISPLAZ_MAKE_LOG_FUNCS) # undef DISPLAZ_MAKE_LOG_FUNCS // 0-arg versions - void progress (const char* fmt) { log(Progress, tfm::format(fmt)); } - void debug (const char* fmt) { log(Debug, tfm::format(fmt)); } - void info (const char* fmt) { log(Info, tfm::format(fmt)); } - void warning (const char* fmt) { log(Warning, tfm::format(fmt)); } - void error (const char* fmt) { log(Error, tfm::format(fmt)); } + void progress (const char* fmt) { log(Progress, fmt, tfm::makeFormatList()); } + void debug (const char* fmt) { log(Debug, fmt, tfm::makeFormatList()); } + void info (const char* fmt) { log(Info, fmt, tfm::makeFormatList()); } + void warning (const char* fmt) { log(Warning, fmt, tfm::makeFormatList()); } + void error (const char* fmt) { log(Error, fmt, tfm::makeFormatList()); } - /// Log message at the given log level - virtual void log(LogLevel level, const std::string& msg) = 0; + /// Log format list at the given log level + virtual void log(LogLevel level, const char* fmt, tfm::FormatListRef flist); /// Report progress of some processing step void progress(double progressFraction) @@ -85,6 +80,8 @@ class Logger } protected: + virtual void logImpl(LogLevel level, const std::string& msg) = 0; + virtual void progressImpl(double progressFraction) = 0; private: @@ -100,9 +97,9 @@ class StreamLogger : public Logger StreamLogger(std::ostream& outStream); ~StreamLogger(); - virtual void log(LogLevel level, const std::string& msg); - protected: + virtual void logImpl(LogLevel level, const std::string& msg); + virtual void progressImpl(double progressFraction); private: From 3a4a920555bd93868813297b3e8a111512a96af8 Mon Sep 17 00:00:00 2001 From: Chris Foster Date: Mon, 29 Aug 2016 17:44:11 +1000 Subject: [PATCH 03/10] Limit number of "duplicate final vertex" warnings * System to limit total number of warnings with a given format expression and log level. * Use this for warnings generated when parsing polygons --- src/PolygonBuilder.cpp | 2 +- src/logger.cpp | 9 ++++++++- src/logger.h | 31 +++++++++++++++++++++++++++---- 3 files changed, 36 insertions(+), 6 deletions(-) diff --git a/src/PolygonBuilder.cpp b/src/PolygonBuilder.cpp index c04f6a79..3f99260f 100644 --- a/src/PolygonBuilder.cpp +++ b/src/PolygonBuilder.cpp @@ -29,7 +29,7 @@ static void initTPPLPoly(TPPLPoly& poly, verts[3*inds[0]+1] == verts[3*inds[size-1]+1] && verts[3*inds[0]+2] == verts[3*inds[size-1]+2])) { - g_logger.warning("Ignoring duplicate final vertex in explicitly closed polygon"); + g_logger.warning_limited("Ignoring duplicate final vertex in explicitly closed polygon"); size -= 1; } // Copy into polypartition data structure diff --git a/src/logger.cpp b/src/logger.cpp index 0ea7d401..cb58c5e1 100644 --- a/src/logger.cpp +++ b/src/logger.cpp @@ -6,10 +6,17 @@ #include //------------------------------------------------------------------------------ -void Logger::log(LogLevel level, const char* fmt, tfm::FormatListRef flist) +void Logger::log(LogLevel level, const char* fmt, tfm::FormatListRef flist, int maxMsgs) { if (level > m_logLevel) return; + if (maxMsgs > 0) + { + int& count = m_logCountLimit[LogCountKey(fmt, level)]; + if (count >= maxMsgs) + return; + ++count; + } std::ostringstream ss; tfm::vformat(ss, fmt, flist); logImpl(level, ss.str()); diff --git a/src/logger.h b/src/logger.h index 8281f824..dc1724f5 100644 --- a/src/logger.h +++ b/src/logger.h @@ -4,8 +4,11 @@ #ifndef LOGGER_H_INCLUDED #define LOGGER_H_INCLUDED +#include + #include "tinyformat.h" + //------------------------------------------------------------------------------ /// Logger class for log message formatting using printf-style strings class Logger @@ -20,8 +23,12 @@ class Logger Progress }; - Logger(LogLevel logLevel = Info, bool logProgress = true) - : m_logLevel(logLevel), m_logProgress(logProgress) { } + Logger(LogLevel logLevel = Info, bool logProgress = true, + int logMessageLimit = 1000) + : m_logLevel(logLevel), + m_logProgress(logProgress), + m_logMessageLimit(logMessageLimit) + { } void setLogLevel(LogLevel logLevel) { m_logLevel = logLevel; } void setLogProgress(bool logProgress) { m_logProgress = logProgress; } @@ -57,6 +64,13 @@ class Logger void error(const char* fmt, TINYFORMAT_VARARGS(n)) \ { \ log(Error, fmt, tfm::makeFormatList(TINYFORMAT_PASSARGS(n))); \ + } \ + \ + /* Versions of above which limit total number of messages to m_logMessageLimit */ \ + template \ + void warning_limited(const char* fmt, TINYFORMAT_VARARGS(n)) \ + { \ + log(Warning, fmt, tfm::makeFormatList(TINYFORMAT_PASSARGS(n)), m_logMessageLimit); \ } TINYFORMAT_FOREACH_ARGNUM(DISPLAZ_MAKE_LOG_FUNCS) @@ -68,9 +82,13 @@ class Logger void info (const char* fmt) { log(Info, fmt, tfm::makeFormatList()); } void warning (const char* fmt) { log(Warning, fmt, tfm::makeFormatList()); } void error (const char* fmt) { log(Error, fmt, tfm::makeFormatList()); } + void warning_limited (const char* fmt) { log(Warning, fmt, tfm::makeFormatList(), m_logMessageLimit); } - /// Log format list at the given log level - virtual void log(LogLevel level, const char* fmt, tfm::FormatListRef flist); + /// Log result of `tfm::vformat(fmt,flist)` at the given log level. + /// + /// If `maxMsgs` is positive, messages with the same `(level,fmt)` key + /// will be logged at most `maxMsgs` times. + virtual void log(LogLevel level, const char* fmt, tfm::FormatListRef flist, int maxMsgs = 0); /// Report progress of some processing step void progress(double progressFraction) @@ -85,8 +103,13 @@ class Logger virtual void progressImpl(double progressFraction) = 0; private: + typedef std::pair LogCountKey; + LogLevel m_logLevel; bool m_logProgress; + int m_logMessageLimit; + + std::map m_logCountLimit; }; From e2a00e387e86d894ee20e690475e7ec0dff878b7 Mon Sep 17 00:00:00 2001 From: Chris Foster Date: Mon, 29 Aug 2016 22:34:29 +1000 Subject: [PATCH 04/10] Add displaz-gui -server option to ease debugging Normally the GUI will inherit an instance lock from the command line tool, but this makes debugging quite hard; add a -server option to allow the GUI to be started manually. --- src/gui/guimain.cpp | 20 +++++++++++++++++++- src/main.cpp | 23 ++++------------------- src/util.cpp | 12 ++++++++++++ src/util.h | 8 ++++++++ 4 files changed, 43 insertions(+), 20 deletions(-) diff --git a/src/gui/guimain.cpp b/src/gui/guimain.cpp index b1f94336..17ec75c5 100644 --- a/src/gui/guimain.cpp +++ b/src/gui/guimain.cpp @@ -11,6 +11,7 @@ #include "argparse.h" #include "config.h" #include "InterProcessLock.h" +#include "util.h" class Geometry; @@ -43,12 +44,14 @@ int main(int argc, char* argv[]) std::string lockName; std::string lockId; std::string socketName; + std::string serverName; ArgParse::ArgParse ap; ap.options( "displaz-gui - don't use this directly, use the displaz commandline helper instead)", "-instancelock %s %s", &lockName, &lockId, "Single instance lock name and ID to reacquire", - "-socketname %s", &socketName, "Local socket name for IPC", + "-socketname %s", &socketName, "Local socket name for IPC", + "-server %s", &serverName, "DEBUG: Compute lock file and socket name; do not inherit lock", NULL ); @@ -80,9 +83,24 @@ int main(int argc, char* argv[]) QGLFormat::setDefaultFormat(f); PointViewerMainWindow window(f); + + // Inherit instance lock (or debug: acquire it) + if (!serverName.empty()) + getDisplazIpcNames(socketName, lockName, serverName); InterProcessLock instanceLock(lockName); if (!lockId.empty()) + { instanceLock.inherit(lockId); + } + else if (!serverName.empty()) + { + if (!instanceLock.tryLock()) + { + std::cerr << "ERROR: Another displaz instance has the lock\n"; + return EXIT_FAILURE; + } + } + if (!socketName.empty()) window.startIpcServer(QString::fromStdString(socketName)); window.show(); diff --git a/src/main.cpp b/src/main.cpp index 6f9f7dcc..dd0c4080 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -16,19 +16,6 @@ //------------------------------------------------------------------------------ -/// Get resource name for displaz IPC -/// -/// This is a combination of the program name and user name to avoid any name -/// clashes, along with a user-defined suffix. -static std::string displazIpcName(const std::string& suffix = "") -{ - std::string name = "displaz-ipc-" + currentUserUid(); - if (!suffix.empty()) - name += "-" + suffix; - return name; -} - - /// Callback and globals for positional argument parsing struct PositionalArg { @@ -167,10 +154,8 @@ int main(int argc, char *argv[]) serverName = QUuid::createUuid().toByteArray().constData(); } - std::string ipcResourceName = displazIpcName(serverName); - QString socketName = QString::fromStdString(ipcResourceName); - - std::string lockName = ipcResourceName + ".lock"; + std::string socketName, lockName; + getDisplazIpcNames(socketName, lockName, serverName); InterProcessLock instanceLock(lockName); bool startedGui = false; qint64 guiPid = -1; @@ -191,7 +176,7 @@ int main(int argc, char *argv[]) QStringList args; args << "-instancelock" << QString::fromStdString(lockName) << QString::fromStdString(instanceLock.makeLockId()) - << "-socketname" << socketName; + << "-socketname" << QString::fromStdString(socketName); QString guiExe = QDir(QCoreApplication::applicationDirPath()) .absoluteFilePath("displaz-gui"); if (!QProcess::startDetached(guiExe, args, @@ -206,7 +191,7 @@ int main(int argc, char *argv[]) // Remote displaz instance should now be running (either it existed // already, or we started it above). Communicate with it via the socket // interface to set any requested parameters, load additional files etc. - std::unique_ptr channel = IpcChannel::connectToServer(socketName); + std::unique_ptr channel = IpcChannel::connectToServer(QString::fromStdString(socketName)); if (!channel) { std::cerr << "ERROR: Could not open IPC channel to remote instance - exiting\n"; diff --git a/src/util.cpp b/src/util.cpp index 64e4f680..3d376653 100644 --- a/src/util.cpp +++ b/src/util.cpp @@ -202,6 +202,18 @@ SigIntTransferHandler::~SigIntTransferHandler() { } +//------------------------------------------------------------------------------ +void getDisplazIpcNames(std::string& socketName, std::string& lockFileName, + const std::string& serverName) +{ + std::string id = "displaz-ipc-" + currentUserUid(); + if (!serverName.empty()) + id += "-" + serverName; + socketName = id; + lockFileName = id + ".lock"; +} + + //------------------------------------------------------------------------------ bool iequals(const std::string& a, const std::string& b) { diff --git a/src/util.h b/src/util.h index 2ce74d72..f3b487c5 100644 --- a/src/util.h +++ b/src/util.h @@ -236,6 +236,14 @@ class SigIntTransferHandler }; +/// Get socket and lock file names for displaz IPC +/// +/// This is a combination of the program name and user name to avoid any name +/// clashes, along with a user-defined serverName. +void getDisplazIpcNames(std::string& socketName, std::string& lockFileName, + const std::string& serverName); + + //------------------------------------------------------------------------------ // String utils From 109aff7859542431aa121380481607773b9e9271 Mon Sep 17 00:00:00 2001 From: Chris Foster Date: Thu, 1 Sep 2016 00:33:03 +1000 Subject: [PATCH 05/10] Fix framebuffer allocation and deallocation Ensure that the framebuffer resource management is watertight. glDeleteFramebuffers() was being called on uninitalized memory - looks like something left over from debugging. --- src/render/View3D.cpp | 16 ++----- src/render/View3D.h | 2 +- src/render/glutil.h | 105 +++++++++++++++++++++++++++++++++++------- 3 files changed, 94 insertions(+), 29 deletions(-) diff --git a/src/render/View3D.cpp b/src/render/View3D.cpp index 1d4335e7..d63357b0 100644 --- a/src/render/View3D.cpp +++ b/src/render/View3D.cpp @@ -43,7 +43,7 @@ View3D::View3D(GeometryCollection* geometries, const QGLFormat& format, QWidget m_selectionModel(0), m_shaderParamsUI(0), m_incrementalFrameTimer(0), - m_incrementalFramebuffer(0), + m_incrementalFramebuffer(), m_incrementalDraw(false), m_drawAxesBackground(QImage(":/resource/axes.png")), m_drawAxesLabelX(QImage(":/resource/x.png")), @@ -287,10 +287,7 @@ void View3D::initializeGL() int w = width() * dPR; int h = height() * dPR; - m_incrementalFramebuffer = allocIncrementalFramebuffer(w, h); - - glFrameBufferStatus(m_incrementalFramebuffer); - glCheckError(); + m_incrementalFramebuffer.init(w, h); initializeGLGeometry(0, m_geometries->get().size()); @@ -321,7 +318,7 @@ void View3D::resizeGL(int w, int h) m_camera.setViewport(QRect(0,0,double(w)/dPR,double(h)/dPR)); - m_incrementalFramebuffer = allocIncrementalFramebuffer(w,h); + m_incrementalFramebuffer.init(w,h); glCheckError(); } @@ -388,7 +385,7 @@ void View3D::paintGL() resizeGL(w, h); } - glBindFramebuffer(GL_DRAW_FRAMEBUFFER, m_incrementalFramebuffer); + glBindFramebuffer(GL_DRAW_FRAMEBUFFER, m_incrementalFramebuffer.id()); //-------------------------------------------------- // Draw main scene @@ -444,7 +441,7 @@ void View3D::paintGL() // TODO: this should really render a texture onto a quad and not use glBlitFramebuffer glBindFramebuffer(GL_DRAW_FRAMEBUFFER, 0); - glBindFramebuffer(GL_READ_FRAMEBUFFER, m_incrementalFramebuffer); + glBindFramebuffer(GL_READ_FRAMEBUFFER, m_incrementalFramebuffer.id()); glBlitFramebuffer(0,0,w,h, 0,0,w,h, GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT, GL_NEAREST); // has to be GL_NEAREST to work with DEPTH @@ -487,9 +484,6 @@ void View3D::paintGL() m_incrementalFrameTimer->start(10); m_incrementalDraw = true; - - glFrameBufferStatus(m_incrementalFramebuffer); - glCheckError(); } void View3D::drawMeshes(const TransformState& transState, diff --git a/src/render/View3D.h b/src/render/View3D.h index df0b7f12..e96e725a 100644 --- a/src/render/View3D.h +++ b/src/render/View3D.h @@ -146,7 +146,7 @@ class View3D : public QGLWidget QWidget* m_shaderParamsUI; /// Timer for next incremental frame QTimer* m_incrementalFrameTimer; - unsigned int m_incrementalFramebuffer; + Framebuffer m_incrementalFramebuffer; bool m_incrementalDraw; /// Controller for amount of geometry to draw DrawCostModel m_drawCostModel; diff --git a/src/render/glutil.h b/src/render/glutil.h index d018475e..676b4e6d 100644 --- a/src/render/glutil.h +++ b/src/render/glutil.h @@ -24,6 +24,29 @@ #include "util.h" #include "typespec.h" + +//------------------------------------------------------------------------------- +// OpenGL Debug tools +void _glError(const char *file, int line); +void _glFrameBufferStatus(GLenum target, const char *file, int line); + + +// #define GL_CHECK +#ifdef GL_CHECK + + #define glCheckError() _glError(__FILE__,__LINE__) + #define glFrameBufferStatus(TARGET) _glFrameBufferStatus(TARGET, __FILE__,__LINE__) + +#else + + #define glCheckError() + #define glFrameBufferStatus(TARGET) + +#endif //GL_CHECK + + + +//------------------------------------------------------------------------------- struct TransformState { Imath::V2i viewSize; @@ -185,6 +208,71 @@ class GlBuffer }; +//------------------------------------------------------------------------------ +/// Framebuffer resource wrapper with color and depth attachements for the +/// particular framebuffer settings needed in the incremental framebuffer in +/// View3D.cpp +class Framebuffer +{ + public: + /// Generate an uninitialized framebuffer. + Framebuffer() + : m_fbo(0), m_colorBuf(0), m_zBuf(0) + { } + + ~Framebuffer() + { + destroy(); + } + + /// Initialize framebuffer with given width and height. + void init(int width, int height) + { + destroy(); + + glGenFramebuffers(1, &m_fbo); + glBindFramebuffer(GL_DRAW_FRAMEBUFFER, m_fbo); + + glGenRenderbuffers(1, &m_colorBuf); + glBindRenderbuffer(GL_RENDERBUFFER, m_colorBuf); + glRenderbufferStorage(GL_RENDERBUFFER, GL_RGBA8, width, height); + glFramebufferRenderbuffer(GL_DRAW_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_RENDERBUFFER, m_colorBuf); + + glGenRenderbuffers(1, &m_zBuf); + glBindRenderbuffer(GL_RENDERBUFFER, m_zBuf); + glRenderbufferStorage(GL_RENDERBUFFER, GL_DEPTH_COMPONENT24, width, height); + glFramebufferRenderbuffer(GL_DRAW_FRAMEBUFFER, GL_DEPTH_ATTACHMENT, GL_RENDERBUFFER, m_zBuf); + + glFrameBufferStatus(m_incrementalFramebuffer.id()); + glCheckError(); + } + + void destroy() + { + if (m_fbo != 0) + glDeleteFramebuffers(1, &m_fbo); + if (m_colorBuf != 0) + glDeleteRenderbuffers(1, &m_colorBuf); + if (m_zBuf != 0) + glDeleteRenderbuffers(1, &m_zBuf); + m_fbo = 0; + m_colorBuf = 0; + m_zBuf = 0; + } + + /// Get OpenGL identifier for the buffer + GLuint id() const + { + assert(m_fbo != 0); + return m_fbo; + } + + private: + GLuint m_fbo; + GLuint m_colorBuf; + GLuint m_zBuf; +}; + //------------------------------------------------------------------------------ // Texture utility @@ -281,21 +369,4 @@ const ShaderAttribute* findAttr(const std::string& name, const std::vector& attrs); -void _glError(const char *file, int line); -void _glFrameBufferStatus(GLenum target, const char *file, int line); - - -// #define GL_CHECK -#ifdef GL_CHECK - - #define glCheckError() _glError(__FILE__,__LINE__) - #define glFrameBufferStatus(TARGET) _glFrameBufferStatus(TARGET, __FILE__,__LINE__) - -#else - - #define glCheckError() - #define glFrameBufferStatus(TARGET) - -#endif //GL_CHECK - #endif // GLUTIL_H_INCLUDED From 705804a7988ee2815a3612b9562e2ab34075d112 Mon Sep 17 00:00:00 2001 From: Chris Foster Date: Thu, 1 Sep 2016 17:09:15 +1000 Subject: [PATCH 06/10] Code cleanup in PointArray::drawPoints() No functionality should change here, just a code cleanup - renaming, adding comments, etc. --- src/render/PointArray.cpp | 87 ++++++++++++++++++++------------------- 1 file changed, 45 insertions(+), 42 deletions(-) diff --git a/src/render/PointArray.cpp b/src/render/PointArray.cpp index 546726a9..f64655d1 100644 --- a/src/render/PointArray.cpp +++ b/src/render/PointArray.cpp @@ -609,6 +609,17 @@ DrawCount PointArray::drawPoints(QGLShaderProgram& prog, const TransformState& t prog.enableAttributeArray(attributes[i]->location); } + // Compute number of bytes required to store all attributes of a vertex, in + // bytes. + size_t perVertexBytes = 0; + for (size_t i = 0; i < m_fields.size(); ++i) + { + const GeomField &field = m_fields[i]; + unsigned int arraySize = field.spec.arraySize(); + unsigned int vecSize = field.spec.vectorSize(); + perVertexBytes += arraySize * vecSize * field.spec.elsize; //sizeof(glBaseType(field.spec)); + } + DrawCount drawCount; ClipBox clipBox(relativeTrans); @@ -634,85 +645,77 @@ DrawCount PointArray::drawPoints(QGLShaderProgram& prog, const TransformState& t } continue; } - size_t idx = node->beginIndex; if (!incrementalDraw) node->nextBeginIndex = node->beginIndex; DrawCount nodeDrawCount = node->drawCount(relCamera, quality, incrementalDraw); drawCount += nodeDrawCount; - idx = node->nextBeginIndex; if (nodeDrawCount.numVertices == 0) continue; if (m_fields.size() < 1) continue; - long bufferSize = 0; - for (size_t i = 0; i < m_fields.size(); ++i) - { - const GeomField &field = m_fields[i]; - unsigned int arraySize = field.spec.arraySize(); - unsigned int vecSize = field.spec.vectorSize(); + // Create a new uninitialized buffer for the current node, reserving + // enough space for the entire set of vertex attributes which will be + // passed to the shader. + // + // (This new memory area will be bound to the "point_buffer" VBO until + // the memory is orphaned by calling glBufferData() next time through + // the loop. The orphaned memory should be cleaned up by the driver, + // and this may actually be quite efficient, see + // http://stackoverflow.com/questions/25111565/how-to-deallocate-glbufferdata-memory + // http://hacksoflife.blogspot.com.au/2015/06/glmapbuffer-no-longer-cool.html ) + GLsizeiptr nodeBufferSize = perVertexBytes * nodeDrawCount.numVertices; + glBufferData(GL_ARRAY_BUFFER, nodeBufferSize, NULL, GL_STREAM_DRAW); - // tfm::printfln("FIELD-NAME: %s", field.name); - // tfm::printfln("AS: %i, VS: %i, FSS: %i, FSES: %i, GLBTFSS: %i", arraySize, vecSize, field.spec.size(), field.spec.elsize, sizeof(glBaseType(field.spec))); - - bufferSize += arraySize * vecSize * field.spec.elsize; //sizeof(glBaseType(field.spec)); - } - - bufferSize = bufferSize * (GLsizei)nodeDrawCount.numVertices; - - // TODO: might be able to do something more efficient here, for example use glBufferSubData to avoid re-allocation of memory by glBufferData - // INITIALIZE THE BUFFER TO FULL SIZE - // tfm::printfln("INIT BUFFER: %i, BS: %i", vbo, bufferSize); - - glBufferData(GL_ARRAY_BUFFER, (GLsizeiptr)bufferSize, NULL, GL_STREAM_DRAW); - /// ======================================================================== - /// ======================================================================== GLintptr bufferOffset = 0; - for (size_t i = 0, k = 0; i < m_fields.size(); k+=m_fields[i].spec.arraySize(), ++i) { const GeomField& field = m_fields[i]; int arraySize = field.spec.arraySize(); int vecSize = field.spec.vectorSize(); - // TODO: should use a single data-array that isn't split into vertex / normal / color / etc. sections, but has interleaved data ? - // OpenGL has a stride value in glVertexAttribPointer for exactly this purpose, which should be used for better efficiency - // here we write only the current attribute data into this the buffer (e.g. all positions, then all colors) - bufferSize = arraySize * vecSize * field.spec.elsize * (GLsizei)nodeDrawCount.numVertices; //sizeof(glBaseType(field.spec)) - - char* bufferData = field.data.get() + idx*field.spec.size(); - glBufferSubData(GL_ARRAY_BUFFER, bufferOffset, bufferSize, bufferData); - - // tfm::printfln("UPDATE BUFFER: %i, BS: %i", vbo, bufferSize); - + // TODO?: Could use a single data-array that isn't split into + // vertex / normal / color / etc. sections, but has interleaved + // data ? OpenGL has a stride value in glVertexAttribPointer for + // exactly this purpose, which should be used for better efficiency + // here we write only the current attribute data into this the + // buffer (e.g. all positions, then all colors) + GLsizeiptr fieldBufferSize = arraySize * vecSize * field.spec.elsize * nodeDrawCount.numVertices; + + // Upload raw data for `field` to the appropriate part of the buffer. + char* bufferData = field.data.get() + node->nextBeginIndex*field.spec.size(); + glBufferSubData(GL_ARRAY_BUFFER, bufferOffset, fieldBufferSize, bufferData); + + // Tell OpenGL how to interpret the buffer of raw data which was + // just uploaded. This should be a single call, but OpenGL spec + // insanity says we need `arraySize` calls (though arraySize=1 + // for most usage.) for (int j = 0; j < arraySize; ++j) { const ShaderAttribute* attr = attributes[k+j]; if (!attr) continue; - // we have to create an intermediate buffer offsets for glVertexAttribPointer, but we can still upload the whole data array earlier !? - GLintptr intermediate_bufferOffset = bufferOffset + j*field.spec.elsize; + GLintptr arrayElementOffset = bufferOffset + j*field.spec.elsize; if (attr->baseType == TypeSpec::Int || attr->baseType == TypeSpec::Uint) { - glVertexAttribIPointer(attr->location, vecSize, - glBaseType(field.spec), 0, (const GLvoid *)intermediate_bufferOffset); + glVertexAttribIPointer(attr->location, vecSize, glBaseType(field.spec), + 0, (const GLvoid *)arrayElementOffset); } else { - glVertexAttribPointer(attr->location, vecSize, - glBaseType(field.spec), - field.spec.fixedPoint, 0, (const GLvoid *)intermediate_bufferOffset); + glVertexAttribPointer(attr->location, vecSize, glBaseType(field.spec), + field.spec.fixedPoint, 0, (const GLvoid *)arrayElementOffset); } glEnableVertexAttribArray(attr->location); } - bufferOffset += bufferSize; + bufferOffset += fieldBufferSize; } glDrawArrays(GL_POINTS, 0, (GLsizei)nodeDrawCount.numVertices); From f05c64269fd030bb09bb4f3c3a0fd6f152adece8 Mon Sep 17 00:00:00 2001 From: Chris Foster Date: Fri, 2 Sep 2016 15:32:47 +1000 Subject: [PATCH 07/10] Minor cleanups: use OpenGL calls directly for attrib arrays This gives less of the nasty mixture of Qt wrapped vs raw OpenGL calls. --- src/render/PointArray.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/render/PointArray.cpp b/src/render/PointArray.cpp index f64655d1..e93c99a3 100644 --- a/src/render/PointArray.cpp +++ b/src/render/PointArray.cpp @@ -606,7 +606,7 @@ DrawCount PointArray::drawPoints(QGLShaderProgram& prog, const TransformState& t for (size_t i = 0; i < attributes.size(); ++i) { if (attributes[i]) - prog.enableAttributeArray(attributes[i]->location); + glEnableVertexAttribArray(attributes[i]->location); } // Compute number of bytes required to store all attributes of a vertex, in @@ -711,8 +711,6 @@ DrawCount PointArray::drawPoints(QGLShaderProgram& prog, const TransformState& t glVertexAttribPointer(attr->location, vecSize, glBaseType(field.spec), field.spec.fixedPoint, 0, (const GLvoid *)arrayElementOffset); } - - glEnableVertexAttribArray(attr->location); } bufferOffset += fieldBufferSize; @@ -728,7 +726,7 @@ DrawCount PointArray::drawPoints(QGLShaderProgram& prog, const TransformState& t for (size_t i = 0; i < attributes.size(); ++i) { if (attributes[i]) - prog.disableAttributeArray(attributes[i]->location); + glDisableVertexAttribArray(attributes[i]->location); } glBindBuffer(GL_ARRAY_BUFFER, 0); From f8fded2af22242e40e00d2d27fba50ed761a327f Mon Sep 17 00:00:00 2001 From: Chris Foster Date: Mon, 5 Sep 2016 07:00:17 +1000 Subject: [PATCH 08/10] Remove old framebuffer allocation code --- src/render/View3D.cpp | 42 ------------------------------------------ src/render/View3D.h | 1 - 2 files changed, 43 deletions(-) diff --git a/src/render/View3D.cpp b/src/render/View3D.cpp index d63357b0..a08344d1 100644 --- a/src/render/View3D.cpp +++ b/src/render/View3D.cpp @@ -319,52 +319,10 @@ void View3D::resizeGL(int w, int h) m_camera.setViewport(QRect(0,0,double(w)/dPR,double(h)/dPR)); m_incrementalFramebuffer.init(w,h); - glCheckError(); } -unsigned int View3D::allocIncrementalFramebuffer(int w, int h) const -{ - if (w < 1 || h < 1) - return 0; - - //should we really delete this every time? - GLuint fb; - glDeleteFramebuffers(1, &fb); - - const QGLFormat fmt = context()->format(); - - glGenFramebuffers(1, &fb); - glBindFramebuffer(GL_DRAW_FRAMEBUFFER, fb); - - // TODO: - // * Should we use multisampling 1 to avoid binding to a texture? - - // Intel HD 3000 driver doesn't like the multisampling mode that Qt 4.8 uses - // for samples==1, so work around it by forcing 0, if possible - - // requires OpenGL 4 - //glFramebufferParameteri(GL_DRAW_FRAMEBUFFER, GL_FRAMEBUFFER_DEFAULT_WIDTH, w); - //glFramebufferParameteri(GL_DRAW_FRAMEBUFFER, GL_FRAMEBUFFER_DEFAULT_HEIGHT, h); - //glFramebufferParameteri(GL_DRAW_FRAMEBUFFER, GL_FRAMEBUFFER_DEFAULT_SAMPLES, fmt.samples() > 1 ? fmt.samples() : 0); - - GLuint c_buff; - glGenRenderbuffers(1, &c_buff); - glBindRenderbuffer(GL_RENDERBUFFER, c_buff); - glRenderbufferStorage(GL_RENDERBUFFER, GL_RGBA8, w, h); - glFramebufferRenderbuffer(GL_DRAW_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_RENDERBUFFER, c_buff); - - GLuint z_buff; - glGenRenderbuffers(1, &z_buff); - glBindRenderbuffer(GL_RENDERBUFFER, z_buff); - glRenderbufferStorage(GL_RENDERBUFFER, GL_DEPTH_COMPONENT24, w, h); - glFramebufferRenderbuffer(GL_DRAW_FRAMEBUFFER, GL_DEPTH_ATTACHMENT, GL_RENDERBUFFER, z_buff); - - return fb; -} - - void View3D::paintGL() { if (m_badOpenGL) diff --git a/src/render/View3D.h b/src/render/View3D.h index e96e725a..de7e23e3 100644 --- a/src/render/View3D.h +++ b/src/render/View3D.h @@ -88,7 +88,6 @@ class View3D : public QGLWidget private: double getDevicePixelRatio(); - unsigned int allocIncrementalFramebuffer(int w, int h) const; void initializeGLGeometry(int begin, int end); void initCursor(float cursorRadius, float centerPointRadius); From b0aaf1224bf62c65be80eb79401ccaceb0350830 Mon Sep 17 00:00:00 2001 From: Chris Foster Date: Mon, 5 Sep 2016 07:12:38 +1000 Subject: [PATCH 09/10] Update framebuffer debug tools Do not pass an argument to glCheckFramebufferStatus(), because this is just asking for bugs - it always applies to the currently bound framebuffer. Remove associated bug! --- src/render/glutil.cpp | 16 ++++++++++------ src/render/glutil.h | 6 +++--- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/render/glutil.cpp b/src/render/glutil.cpp index 90c738d9..a46f7261 100644 --- a/src/render/glutil.cpp +++ b/src/render/glutil.cpp @@ -307,16 +307,20 @@ void _glError(const char *file, int line) { } } -void _glFrameBufferStatus(GLenum target, const char *file, int line) { +void _glFrameBufferStatus(GLenum target, const char *file, int line) +{ + GLenum fbStatus = glCheckFramebufferStatus(target); - GLenum fb_status(glCheckFramebufferStatus(target)); + if (fbStatus == GL_FRAMEBUFFER_COMPLETE) + return; std::string status; - switch(fb_status) { + switch(fbStatus) { + case GL_INVALID_ENUM: status="?? (bad target)"; break; case GL_FRAMEBUFFER_COMPLETE: status="COMPLETE"; break; case GL_FRAMEBUFFER_UNDEFINED: status="UNDEFINED"; break; - case GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT: status="INCOMPLETE_ATTACHMENT"; break; + case GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT: status="INCOMPLETE_ATTACHMENT"; break; //case GL_FRAMEBUFFER_INCOMPLETE_MISSING_ATTACHMENT: status="INCOMPLETE_MISSING_ATTACHMENT"; break; case GL_FRAMEBUFFER_INCOMPLETE_DRAW_BUFFER: status="INCOMPLETE_DRAW_BUFFER"; break; case GL_FRAMEBUFFER_INCOMPLETE_READ_BUFFER: status="INCOMPLETE_READ_BUFFER"; break; @@ -325,9 +329,9 @@ void _glFrameBufferStatus(GLenum target, const char *file, int line) { //case GL_FRAMEBUFFER_INCOMPLETE_DIMENSIONS: status="INCOMPLETE_DIMENSIONS"; break; case GL_FRAMEBUFFER_INCOMPLETE_MISSING_ATTACHMENT: status="INCOMPLETE_MISSING_ATTACHMENT"; break; case GL_FRAMEBUFFER_UNSUPPORTED: status="UNSUPPORTED"; break; - default: status="INCOMPLETE"; break; + default: status="???"; break; } - tfm::printfln("GL_FRAMEBUFFER_%s - %s:%i", status, file, line); + tfm::printfln("GL_FRAMEBUFFER_%s (%d) - %s:%i", status, fbStatus, file, line); } diff --git a/src/render/glutil.h b/src/render/glutil.h index 676b4e6d..dcf1d2ae 100644 --- a/src/render/glutil.h +++ b/src/render/glutil.h @@ -35,12 +35,12 @@ void _glFrameBufferStatus(GLenum target, const char *file, int line); #ifdef GL_CHECK #define glCheckError() _glError(__FILE__,__LINE__) - #define glFrameBufferStatus(TARGET) _glFrameBufferStatus(TARGET, __FILE__,__LINE__) + #define glCheckFrameBufferStatus() _glFrameBufferStatus(GL_FRAMEBUFFER, __FILE__,__LINE__) #else #define glCheckError() - #define glFrameBufferStatus(TARGET) + #define glCheckFrameBufferStatus() #endif //GL_CHECK @@ -243,7 +243,7 @@ class Framebuffer glRenderbufferStorage(GL_RENDERBUFFER, GL_DEPTH_COMPONENT24, width, height); glFramebufferRenderbuffer(GL_DRAW_FRAMEBUFFER, GL_DEPTH_ATTACHMENT, GL_RENDERBUFFER, m_zBuf); - glFrameBufferStatus(m_incrementalFramebuffer.id()); + glCheckFrameBufferStatus(); glCheckError(); } From b40fe763e4de7f0fdf025ca870b4ae21938388ac Mon Sep 17 00:00:00 2001 From: Chris Foster Date: Sat, 10 Sep 2016 13:50:49 +1000 Subject: [PATCH 10/10] Fix horrible bug: glDeleteBuffers used instead of glDeleteVertexArrays OMSDLFKJG this was hard to find, thank goodness for apitrace. The bug It leads to some VBOs (particularly that used by PointArray) being randomly deleted before we're done with them, intermittently leading to segfaults inside the driver. Driver- and OS-dependent madness! Another effect of this bug was that vertex array handles continually leaked. This is probably the cause of some lower powered cards eventually locking up under resource starvation. --- src/render/Geometry.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/render/Geometry.cpp b/src/render/Geometry.cpp index 15a90fa1..68e47620 100644 --- a/src/render/Geometry.cpp +++ b/src/render/Geometry.cpp @@ -87,7 +87,7 @@ void Geometry::destroyBuffers() for (auto& it: m_VAO) { GLuint vao = it.second; - glDeleteBuffers(1, &vao); + glDeleteVertexArrays(1, &vao); } m_VAO.clear();