improve alignment of markers, avoid images that are half empty

src/waveform/renderers/allshader/waveformrendermark.cpp

@@ -46,7 +46,7 @@ class WaveformMarkNode : public rendergraph::GeometryNode {
     void update(float x, float y, float devicePixelRatio) {
         TexturedVertexUpdater vertexUpdater{
                 geometry().vertexDataAs<Geometry::TexturedPoint2D>()};
-        vertexUpdater.addRectangle({std::round(x), std::round(y)},
+        vertexUpdater.addRectangle({x, y},
                 {x + m_textureWidth / devicePixelRatio,
                         y + m_textureHeight / devicePixelRatio},
                 {0.f, 0.f},
@@ -100,21 +100,10 @@ class WaveformMarkNodeGraphics : public WaveformMark::Graphics {
 
     std::unique_ptr<rendergraph::BaseNode> m_pNode;
 };
-} // namespace
 
-// Both allshader::WaveformRenderMark and the non-GL ::WaveformRenderMark derive
-// from WaveformRenderMarkBase. The base-class takes care of updating the marks
-// when needed and flagging them when their image needs to be updated (resizing,
-// cue changes, position changes)
-//
-// While in the case of ::WaveformRenderMark those images can be updated immediately,
-// in the case of allshader::WaveformRenderMark we need to do that when we have an
-// openGL context, as we create new textures.
-//
-// The boolean argument for the WaveformRenderMarkBase constructor indicates
-// that updateMarkImages should not be called immediately.
+constexpr float kPlayPosWidth{11.f};
+constexpr float kPlayPosOffset{-(kPlayPosWidth - 1.f) / 2.f};
 
-namespace {
 QString timeSecToString(double timeSec) {
     int hundredths = std::lround(timeSec * 100.0);
     int seconds = hundredths / 100;
@@ -125,8 +114,31 @@ QString timeSecToString(double timeSec) {
     return QString::asprintf("%d:%02d.%02d", minutes, seconds, hundredths);
 }
 
+struct RoundToPixel {
+    const float m_devicePixelRatio;
+    RoundToPixel(float devicePixelRatio)
+            : m_devicePixelRatio(devicePixelRatio) {
+    }
+    // round to nearest pixel, taking into account the devicePixelRatio
+    float operator()(float pos) const {
+        return std::round(pos * m_devicePixelRatio) / m_devicePixelRatio;
+    }
+};
+
 } // namespace
 
+// Both allshader::WaveformRenderMark and the non-GL ::WaveformRenderMark derive
+// from WaveformRenderMarkBase. The base-class takes care of updating the marks
+// when needed and flagging them when their image needs to be updated (resizing,
+// cue changes, position changes)
+//
+// While in the case of ::WaveformRenderMark those images can be updated immediately,
+// in the case of allshader::WaveformRenderMark we need to do that when we have an
+// openGL context, as we create new textures.
+//
+// The boolean argument for the WaveformRenderMarkBase constructor indicates
+// that updateMarkImages should not be called immediately.
+
 allshader::WaveformRenderMark::WaveformRenderMark(
         WaveformWidgetRenderer* waveformWidget,
         ::WaveformRendererAbstract::PositionSource type)
@@ -235,6 +247,8 @@ void allshader::WaveformRenderMark::update() {
     const float devicePixelRatio = m_waveformRenderer->getDevicePixelRatio();
     QList<WaveformWidgetRenderer::WaveformMarkOnScreen> marksOnScreen;
 
+    RoundToPixel roundToPixel{devicePixelRatio};
+
     for (const auto& pMark : std::as_const(m_marks)) {
         pMark->setBreadth(slipActive ? m_waveformRenderer->getBreadth() / 2
                                      : m_waveformRenderer->getBreadth());
@@ -268,36 +282,27 @@ void allshader::WaveformRenderMark::update() {
         if (!pMarkGraphics) // is this even possible?
             continue;
 
-        const float currentMarkPoint =
-                std::round(
-                        static_cast<float>(
-                                m_waveformRenderer
-                                        ->transformSamplePositionInRendererWorld(
-                                                samplePosition, positionType)) *
-                        devicePixelRatio) /
-                devicePixelRatio;
+        const float currentMarkPos = static_cast<float>(
+                m_waveformRenderer->transformSamplePositionInRendererWorld(
+                        samplePosition, positionType));
         if (pMark->isShowUntilNext() &&
                 samplePosition >= playPosition + 1.0 &&
                 samplePosition < nextMarkPosition) {
             nextMarkPosition = samplePosition;
         }
         const double sampleEndPosition = pMark->getSampleEndPosition();
 
-        // Pixmaps are expected to have the mark stroke at the center,
-        // and preferably have an odd width in order to have the stroke
-        // exactly at the sample position.
-        const float markHalfWidth = pMarkNodeGraphics->textureWidth() / devicePixelRatio / 2.f;
-        const float drawOffset = currentMarkPoint - markHalfWidth;
+        const float markWidth = pMarkNodeGraphics->textureWidth() / devicePixelRatio;
+        const float drawOffset = currentMarkPos + pMark->getOffset();
 
         bool visible = false;
         // Check if the current point needs to be displayed.
-        if (drawOffset > -markHalfWidth &&
-                drawOffset < m_waveformRenderer->getLength() +
-                                markHalfWidth) {
+        if (drawOffset > -markWidth &&
+                drawOffset < m_waveformRenderer->getLength()) {
             pMarkNodeGraphics->update(
-                    drawOffset,
+                    roundToPixel(drawOffset),
                     !m_isSlipRenderer && slipActive
-                            ? m_waveformRenderer->getBreadth() / 2
+                            ? roundToPixel(m_waveformRenderer->getBreadth() / 2.f)
                             : 0,
                     devicePixelRatio);
 
@@ -310,13 +315,10 @@ void allshader::WaveformRenderMark::update() {
         // Check if the range needs to be displayed.
         if (samplePosition != sampleEndPosition && sampleEndPosition != Cue::kNoPosition) {
             DEBUG_ASSERT(samplePosition < sampleEndPosition);
-            const float currentMarkEndPoint = static_cast<
-                    float>(
-                    m_waveformRenderer
-                            ->transformSamplePositionInRendererWorld(
-                                    sampleEndPosition, positionType));
-
-            if (visible || currentMarkEndPoint > 0) {
+            const float currentMarkEndPos = static_cast<float>(
+                    m_waveformRenderer->transformSamplePositionInRendererWorld(
+                            sampleEndPosition, positionType));
+            if (visible || currentMarkEndPos > 0.f) {
                 QColor color = pMark->fillColor();
                 color.setAlphaF(0.4f);
 
@@ -329,9 +331,9 @@ void allshader::WaveformRenderMark::update() {
                 }
 
                 updateRangeNode(pRangeChild,
-                        QRectF(QPointF(currentMarkPoint, 0),
-                                QPointF(currentMarkEndPoint,
-                                        m_waveformRenderer->getBreadth())),
+                        QRectF(QPointF(roundToPixel(currentMarkPos), 0.f),
+                                QPointF(roundToPixel(currentMarkEndPos),
+                                        roundToPixel(m_waveformRenderer->getBreadth()))),
                         color);
 
                 visible = true;
@@ -354,30 +356,23 @@ void allshader::WaveformRenderMark::update() {
 
     m_waveformRenderer->setMarkPositions(marksOnScreen);
 
-    const float currentMarkPoint =
-            std::round(static_cast<float>(
-                               m_waveformRenderer->getPlayMarkerPosition() *
-                               m_waveformRenderer->getLength()) *
-                    devicePixelRatio) /
-            devicePixelRatio;
-
+    const float playMarkerPos = m_waveformRenderer->getPlayMarkerPosition() *
+            m_waveformRenderer->getLength();
     {
-        const float markHalfWidth = 11.f / 2.f;
-        const float drawOffset = currentMarkPoint - markHalfWidth;
-
+        const float drawOffset = roundToPixel(playMarkerPos + kPlayPosOffset);
         TexturedVertexUpdater vertexUpdater{
                 m_pPlayPosNode->geometry()
                         .vertexDataAs<Geometry::TexturedPoint2D>()};
         vertexUpdater.addRectangle({drawOffset, 0.f},
-                {drawOffset + 11.f, static_cast<float>(m_waveformRenderer->getBreadth())},
+                {drawOffset + kPlayPosWidth, static_cast<float>(m_waveformRenderer->getBreadth())},
                 {0.f, 0.f},
                 {1.f, 1.f});
     }
 
     if (WaveformWidgetFactory::instance()->getUntilMarkShowBeats() ||
             WaveformWidgetFactory::instance()->getUntilMarkShowTime()) {
         updateUntilMark(playPosition, nextMarkPosition);
-        drawUntilMark(currentMarkPoint + 20);
+        drawUntilMark(roundToPixel(playMarkerPos + 20.f));
     }
 }
 
@@ -443,7 +438,7 @@ void allshader::WaveformRenderMark::updatePlayPosMarkTexture(rendergraph::Contex
 
     const float lineX = 5.5f;
 
-    imgwidth = 11.f;
+    imgwidth = kPlayPosWidth;
     imgheight = height;
 
     QImage image(static_cast<int>(imgwidth * devicePixelRatio),

src/waveform/renderers/waveformmark.cpp

@@ -100,6 +100,7 @@ WaveformMark::WaveformMark(const QString& group,
         const WaveformSignalColors& signalColors,
         int hotCue)
         : m_linePosition{},
+          m_offset{},
           m_breadth{},
           m_level{},
           m_iPriority(priority),
@@ -278,6 +279,15 @@ struct MarkerGeometry {
         const Qt::Alignment alignH = align & Qt::AlignHorizontal_Mask;
         const Qt::Alignment alignV = align & Qt::AlignVertical_Mask;
         const bool alignHCenter{alignH == Qt::AlignHCenter};
+
+        // The image width is the label rect width + 1, so that the label rect
+        // left and right positions can be at an integer + 0.5. This is so that
+        // the label rect is drawn at an exact pixel positions.
+        //
+        // Likewise, the line position also has to fall on an integer + 0.5.
+        // When center aligning, the image width has to be odd, so that the
+        // center is an integer + 0.5. For the image width to be odd, to
+        // label rect width has to be even.
         const qreal widthRounding{alignHCenter ? 2.f : 1.f};
 
         m_labelRect = QRectF{0.f,
@@ -286,17 +296,9 @@ struct MarkerGeometry {
                         widthRounding,
                 std::ceil(capHeight + 2.f * margin)};
 
-        m_imageSize = QSizeF{alignHCenter ? m_labelRect.width() + 1.f
-                                          : 2.f * m_labelRect.width() + 1.f,
-                breadth};
+        m_imageSize = QSizeF{m_labelRect.width() + 1.f, breadth};
 
-        if (alignH == Qt::AlignHCenter) {
-            m_labelRect.moveLeft((m_imageSize.width() - m_labelRect.width()) / 2.f);
-        } else if (alignH == Qt::AlignRight) {
-            m_labelRect.moveRight(m_imageSize.width() - 0.5f);
-        } else {
-            m_labelRect.moveLeft(0.5f);
-        }
+        m_labelRect.moveLeft(0.5f);
 
         const float increment = overlappingMarkerIncrement(
                 static_cast<float>(m_labelRect.height()), breadth);
@@ -373,22 +375,39 @@ QImage WaveformMark::generateImage(float devicePixelRatio) {
 
     painter.setWorldMatrixEnabled(false);
 
-    // Draw marker lines
-    const auto hcenter = markerGeometry.m_imageSize.width() / 2.f;
-    m_linePosition = static_cast<float>(hcenter);
+    const Qt::Alignment alignH = m_align & Qt::AlignHorizontal_Mask;
+    switch (alignH) {
+    case Qt::AlignHCenter:
+        m_linePosition = markerGeometry.m_imageSize.width() / 2.f;
+        m_offset = -(markerGeometry.m_imageSize.width() - 1.f) / 2.f;
+        DEBUG_ASSERT(linePos - std::roundf(linePos) < 0.000001f);
+        break;
+    case Qt::AlignLeft:
+        m_linePosition = markerGeometry.m_imageSize.width() - 1.5f;
+        m_offset = -markerGeometry.m_imageSize.width() + 2.f;
+        break;
+    case Qt::AlignRight:
+    default:
+        m_linePosition = 1.5f;
+        m_offset = -1.f;
+        break;
+    }
+
+    // Note: linePos has to be at integer + 0.5 to draw correctly
+    const float linePos = m_linePosition;
 
     // Draw the center line
     painter.setPen(fillColor());
-    painter.drawLine(QLineF(hcenter, 0.f, hcenter, markerGeometry.m_imageSize.height()));
+    painter.drawLine(QLineF(linePos, 0.f, linePos, markerGeometry.m_imageSize.height()));
 
     painter.setPen(borderColor());
-    painter.drawLine(QLineF(hcenter - 1.f,
+    painter.drawLine(QLineF(linePos - 1.f,
             0.f,
-            hcenter - 1.f,
+            linePos - 1.f,
             markerGeometry.m_imageSize.height()));
-    painter.drawLine(QLineF(hcenter + 1.f,
+    painter.drawLine(QLineF(linePos + 1.f,
             0.f,
-            hcenter + 1.f,
+            linePos + 1.f,
             markerGeometry.m_imageSize.height()));
 
     if (useIcon || label.length() != 0) {

src/waveform/renderers/waveformmark.h

@@ -37,6 +37,10 @@ class WaveformMark {
     WaveformMark(const WaveformMark&) = delete;
     WaveformMark& operator=(const WaveformMark&) = delete;
 
+    float getOffset() const {
+        return m_offset;
+    }
+
     int getHotCue() const {
         return m_iHotCue;
     };
@@ -158,6 +162,7 @@ class WaveformMark {
     QString m_iconPath;
 
     float m_linePosition;
+    float m_offset;
     float m_breadth;
 
     // When there are overlapping marks, level is increased for each overlapping mark,

src/waveform/renderers/waveformrendermark.cpp

@@ -37,17 +37,13 @@ void WaveformRenderMark::draw(QPainter* painter, QPaintEvent* /*event*/) {
                     m_waveformRenderer->transformSamplePositionInRendererWorld(samplePosition);
             const double sampleEndPosition = pMark->getSampleEndPosition();
             if (m_waveformRenderer->getOrientation() == Qt::Horizontal) {
-                // Pixmaps are expected to have the mark stroke at the center,
-                // and preferably have an odd width in order to have the stroke
-                // exactly at the sample position.
-                const int markHalfWidth =
-                        static_cast<int>(image.width() / 2.0 /
-                                m_waveformRenderer->getDevicePixelRatio());
-                const int drawOffset = static_cast<int>(currentMarkPoint) - markHalfWidth;
+                const int markWidth = std::lround(image.width() /
+                        m_waveformRenderer->getDevicePixelRatio());
+                const int drawOffset = std::lround(currentMarkPoint + pMark->getOffset());
 
                 bool visible = false;
                 // Check if the current point needs to be displayed.
-                if (currentMarkPoint > -markHalfWidth && currentMarkPoint < m_waveformRenderer->getWidth() + markHalfWidth) {
+                if (drawOffset > -markWidth && drawOffset < m_waveformRenderer->getWidth()) {
                     painter->drawImage(drawOffset, 0, image);
                     visible = true;
                 }
@@ -84,16 +80,13 @@ void WaveformRenderMark::draw(QPainter* painter, QPaintEvent* /*event*/) {
                                     pMark, drawOffset});
                 }
             } else {
-                const int markHalfHeight =
-                        static_cast<int>(image.height() / 2.0 /
-                                m_waveformRenderer->getDevicePixelRatio());
-                const int drawOffset = static_cast<int>(currentMarkPoint) - markHalfHeight;
+                const int markHeight = std::lroundf(image.height() /
+                        m_waveformRenderer->getDevicePixelRatio());
+                const int drawOffset = std::lroundf(currentMarkPoint + pMark->getOffset());
 
                 bool visible = false;
                 // Check if the current point needs to be displayed.
-                if (currentMarkPoint > -markHalfHeight &&
-                        currentMarkPoint < m_waveformRenderer->getHeight() +
-                                        markHalfHeight) {
+                if (drawOffset > -markHeight && drawOffset < m_waveformRenderer->getHeight()) {
                     painter->drawImage(0, drawOffset, image);
                     visible = true;
                 }

src/waveform/renderers/waveformwidgetrenderer.cpp

@@ -256,8 +256,8 @@ void WaveformWidgetRenderer::draw(QPainter* painter, QPaintEvent* event) {
 }
 
 void WaveformWidgetRenderer::drawPlayPosmarker(QPainter* painter) {
-    const int lineX = static_cast<int>(m_width * m_playMarkerPosition);
-    const int lineY = static_cast<int>(m_height * m_playMarkerPosition);
+    const int lineX = std::lround(m_width * m_playMarkerPosition);
+    const int lineY = std::lround(m_height * m_playMarkerPosition);
 
     // draw dim outlines to increase playpos/waveform contrast
     painter->setOpacity(0.5);