Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

avoid overlapping marks #12273

Merged
merged 6 commits into from
Dec 18, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1405,6 +1405,7 @@ else()
src/waveform/renderers/waveformrendererrgb.cpp
src/waveform/renderers/waveformrenderersignalbase.cpp
src/waveform/renderers/waveformrendermark.cpp
src/waveform/renderers/waveformrendermarkbase.cpp
src/waveform/renderers/waveformrendermarkrange.cpp
src/waveform/renderers/waveformsignalcolors.cpp
src/waveform/renderers/waveformwidgetrenderer.cpp
Expand Down
116 changes: 26 additions & 90 deletions src/waveform/renderers/allshader/waveformrendermark.cpp
Original file line number Diff line number Diff line change
@@ -1,20 +1,13 @@
#include "waveform/renderers/allshader/waveformrendermark.h"

#include <QDomNode>
#include <QOpenGLTexture>
#include <QPainterPath>

#include "track/track.h"
#include "util/color/color.h"
#include "util/colorcomponents.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It builds when I restore this include

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is on Ubuntu 20.04, and it seems this and #12303 slip through because we don't have a 20.04 runner anymore even though we support it?

#include "util/texture.h"
#include "waveform/renderers/allshader/matrixforwidgetgeometry.h"
#include "waveform/renderers/allshader/moc_waveformrendermark.cpp"
#include "waveform/renderers/allshader/rgbadata.h"
#include "waveform/renderers/allshader/vertexdata.h"
#include "waveform/renderers/waveformsignalcolors.h"
#include "waveform/renderers/waveformwidgetrenderer.h"
#include "widget/wimagestore.h"

// On the use of QPainter:
//
Expand All @@ -37,25 +30,30 @@ class TextureGraphics : public WaveformMark::Graphics {
}
};

allshader::WaveformRenderMark::WaveformRenderMark(WaveformWidgetRenderer* waveformWidget)
: WaveformRenderer(waveformWidget),
m_bCuesUpdates(false) {
}
// 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.

void allshader::WaveformRenderMark::setup(const QDomNode& node, const SkinContext& context) {
WaveformSignalColors signalColors = *m_waveformRenderer->getWaveformSignalColors();
m_marks.setup(m_waveformRenderer->getGroup(), node, context, signalColors);
allshader::WaveformRenderMark::WaveformRenderMark(WaveformWidgetRenderer* waveformWidget)
: ::WaveformRenderMarkBase(waveformWidget, false) {
}

void allshader::WaveformRenderMark::initializeGL() {
WaveformRenderer::initializeGL();
allshader::WaveformRendererAbstract::initializeGL();
m_rgbaShader.init();
m_textureShader.init();

for (const auto& pMark : std::as_const(m_marks)) {
generateMarkImage(pMark);
}
generatePlayPosMarkTexture();
// Will create textures so requires OpenGL context
updateMarkImages();
updatePlayPosMarkTexture();
}

void allshader::WaveformRenderMark::drawTexture(float x, float y, QOpenGLTexture* texture) {
Expand Down Expand Up @@ -157,16 +155,13 @@ void allshader::WaveformRenderMark::paintGL() {
const float devicePixelRatio = m_waveformRenderer->getDevicePixelRatio();
QList<WaveformWidgetRenderer::WaveformMarkOnScreen> marksOnScreen;

checkCuesUpdated();

glEnable(GL_BLEND);
glBlendFunc(GL_SRC_ALPHA, GL_ONE_MINUS_SRC_ALPHA);

for (const auto& pMark : std::as_const(m_marks)) {
if (!pMark->m_pGraphics || pMark->m_pGraphics->m_obsolete) {
generateMarkImage(pMark);
}
// Will create textures so requires OpenGL context
updateMarkImages();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is m_obsolete still in use?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, to indicate that the image has to be regenerated. I leave a comment.


for (const auto& pMark : std::as_const(m_marks)) {
QOpenGLTexture* pTexture =
static_cast<TextureGraphics*>(pMark->m_pGraphics.get())
->texture();
Expand Down Expand Up @@ -238,7 +233,7 @@ void allshader::WaveformRenderMark::paintGL() {
// Generate the texture used to draw the play position marker.
// Note that in the legacy waveform widgets this is drawn directly
// in the WaveformWidgetRenderer itself. Doing it here is cleaner.
void allshader::WaveformRenderMark::generatePlayPosMarkTexture() {
void allshader::WaveformRenderMark::updatePlayPosMarkTexture() {
float imgwidth;
float imgheight;

Expand Down Expand Up @@ -317,71 +312,12 @@ void allshader::WaveformRenderMark::drawTriangle(QPainter* painter,
}

void allshader::WaveformRenderMark::resizeGL(int, int) {
for (const auto& pMark : std::as_const(m_marks)) {
generateMarkImage(pMark);
}
generatePlayPosMarkTexture();
}

void allshader::WaveformRenderMark::onSetTrack() {
slotCuesUpdated();

TrackPointer trackInfo = m_waveformRenderer->getTrackInfo();
if (!trackInfo) {
return;
}
connect(trackInfo.get(),
&Track::cuesUpdated,
this,
&allshader::WaveformRenderMark::slotCuesUpdated);
}

void allshader::WaveformRenderMark::slotCuesUpdated() {
m_bCuesUpdates = true;
}

void allshader::WaveformRenderMark::checkCuesUpdated() {
if (!m_bCuesUpdates) {
return;
}
m_bCuesUpdates = false;

TrackPointer trackInfo = m_waveformRenderer->getTrackInfo();
if (!trackInfo) {
return;
}

QList<CuePointer> loadedCues = trackInfo->getCuePoints();
for (const CuePointer& pCue : loadedCues) {
const int hotCue = pCue->getHotCue();
if (hotCue == Cue::kNoHotCue) {
continue;
}

// Here we assume no two cues can have the same hotcue assigned,
// because WaveformMarkSet stores one mark for each hotcue.
WaveformMarkPointer pMark = m_marks.getHotCueMark(hotCue);
if (pMark.isNull()) {
continue;
}

QString newLabel = pCue->getLabel();
QColor newColor = mixxx::RgbColor::toQColor(pCue->getColor());
if (pMark->m_text.isNull() || newLabel != pMark->m_text ||
!pMark->fillColor().isValid() ||
newColor != pMark->fillColor()) {
pMark->m_text = newLabel;
const int dimBrightThreshold = m_waveformRenderer->getDimBrightThreshold();
pMark->setBaseColor(newColor, dimBrightThreshold);
generateMarkImage(pMark);
}
}

m_marks.update();
// Will create textures so requires OpenGL context
updateMarkImages();
updatePlayPosMarkTexture();
}

void allshader::WaveformRenderMark::generateMarkImage(WaveformMarkPointer pMark) {
void allshader::WaveformRenderMark::updateMarkImage(WaveformMarkPointer pMark) {
pMark->m_pGraphics = std::make_unique<TextureGraphics>(
createTexture(pMark->generateImage(m_waveformRenderer->getBreadth(),
m_waveformRenderer->getDevicePixelRatio())));
createTexture(pMark->generateImage(m_waveformRenderer->getDevicePixelRatio())));
}
36 changes: 14 additions & 22 deletions src/waveform/renderers/allshader/waveformrendermark.h
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
#pragma once

#include <QColor>
#include <QObject>

#include "shaders/rgbashader.h"
#include "shaders/textureshader.h"
#include "util/class.h"
#include "waveform/renderers/allshader/waveformrenderer.h"
#include "waveform/renderers/waveformmarkset.h"
#include "waveform/renderers/allshader/waveformrendererabstract.h"
#include "waveform/renderers/waveformrendermarkbase.h"

class QDomNode;
class SkinContext;
Expand All @@ -17,44 +15,38 @@ namespace allshader {
class WaveformRenderMark;
}

class allshader::WaveformRenderMark final : public QObject, public allshader::WaveformRenderer {
Q_OBJECT
class allshader::WaveformRenderMark : public ::WaveformRenderMarkBase,
public allshader::WaveformRendererAbstract {
public:
explicit WaveformRenderMark(WaveformWidgetRenderer* waveformWidget);

void setup(const QDomNode& node, const SkinContext& context) override;
void draw(QPainter* painter, QPaintEvent* event) override {
Q_UNUSED(painter);
Q_UNUSED(event);
}

allshader::WaveformRendererAbstract* allshaderWaveformRenderer() override {
return this;
}

void initializeGL() override;
void paintGL() override;
void resizeGL(int w, int h) override;

// Called when a new track is loaded.
void onSetTrack() override;

public slots:
// Called when the loaded track's cues are added, deleted or modified and
// when a new track is loaded.
// It updates the marks' names and regenerates their image if needed.
// This method is used for hotcues.
void slotCuesUpdated();

private:
void checkCuesUpdated();
void updateMarkImage(WaveformMarkPointer pMark) override;

void generateMarkImage(WaveformMarkPointer pMark);
void generatePlayPosMarkTexture();
void updatePlayPosMarkTexture();

void drawTriangle(QPainter* painter,
const QBrush& fillColor,
QPointF p1,
QPointF p2,
QPointF p3);

WaveformMarkSet m_marks;
mixxx::RGBAShader m_rgbaShader;
mixxx::TextureShader m_textureShader;
std::unique_ptr<QOpenGLTexture> m_pPlayPosMarkTexture;
bool m_bCuesUpdates;

void drawMark(const QRectF& rect, QColor color);
void drawTexture(float x, float y, QOpenGLTexture* texture);
Expand Down
35 changes: 22 additions & 13 deletions src/waveform/renderers/waveformmark.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,10 @@ Qt::Alignment decodeAlignmentFlags(const QString& alignString, Qt::Alignment def
WaveformMark::WaveformMark(const QString& group,
const QDomNode& node,
const SkinContext& context,
int priority,
const WaveformSignalColors& signalColors,
int hotCue)
: m_linePosition{}, m_breadth{}, m_iHotCue{hotCue} {
: m_linePosition{}, m_breadth{}, m_level{}, m_iPriority(priority), m_iHotCue(hotCue) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you stack these. A table like list improves readability IMHO. I though clang-format will do it for us.
We should investigate why it is no longer the case.

QString positionControl;
QString endPositionControl;
if (hotCue != Cue::kNoHotCue) {
Expand Down Expand Up @@ -121,23 +122,26 @@ WaveformMark::WaveformMark(const QString& group,
WaveformMark::~WaveformMark() = default;

void WaveformMark::setBaseColor(QColor baseColor, int dimBrightThreshold) {
if (m_pGraphics) {
m_pGraphics->m_obsolete = true;
if (m_fillColor == baseColor) {
return;
}

m_fillColor = baseColor;
m_borderColor = Color::chooseContrastColor(baseColor, dimBrightThreshold);
m_labelColor = Color::chooseColorByBrightness(baseColor,
QColor(255, 255, 255, 255),
QColor(0, 0, 0, 255),
dimBrightThreshold);
};

setNeedsImageUpdate();
}

bool WaveformMark::contains(QPoint point, Qt::Orientation orientation) const {
// Without some padding, the user would only have a single pixel width that
// would count as hovering over the WaveformMark.
float lineHoverPadding = 5.0;
if (orientation == Qt::Vertical) {
point = QPoint(point.y(), m_breadth - point.x());
point = QPoint(point.y(), static_cast<int>(m_breadth) - point.x());
}
bool lineHovered = m_linePosition >= point.x() - lineHoverPadding &&
m_linePosition <= point.x() + lineHoverPadding;
Expand All @@ -148,13 +152,17 @@ bool WaveformMark::contains(QPoint point, Qt::Orientation orientation) const {
// Helper struct to calculate the geometry and fontsize needed by generateImage
// to draw the label and text
struct MarkerGeometry {
bool m_isSymbol; // it the label normal text or a single symbol (e.g. open circle arrow)
bool m_isSymbol; // is the label normal text or a single symbol (e.g. open circle arrow)
QFont m_font;
QRectF m_contentRect;
QRectF m_labelRect;
QSizeF m_imageSize;

MarkerGeometry(const QString& label, bool useIcon, Qt::Alignment align, float breadth) {
MarkerGeometry(const QString& label,
bool useIcon,
Qt::Alignment align,
float breadth,
int level) {
// If the label is 1 character long, and this character isn't a letter or a number,
// we can assume it's a special symbol
m_isSymbol = !useIcon && label.length() == 1 && !label[0].isLetterOrNumber();
Expand Down Expand Up @@ -240,9 +248,10 @@ struct MarkerGeometry {
if (alignV == Qt::AlignVCenter) {
m_labelRect.moveTop((m_imageSize.height() - m_labelRect.height()) / 2.f);
} else if (alignV == Qt::AlignBottom) {
m_labelRect.moveBottom(m_imageSize.height() - 0.5f);
m_labelRect.moveBottom(m_imageSize.height() - 0.5f -
level * (m_labelRect.height() + 2.f));
} else {
m_labelRect.moveTop(0.5f);
m_labelRect.moveTop(0.5f + level * (m_labelRect.height() + 2.f));
}
}
QSize getImageSize(float devicePixelRatio) const {
Expand All @@ -251,12 +260,12 @@ struct MarkerGeometry {
}
};

QImage WaveformMark::generateImage(float breadth, float devicePixelRatio) {
QImage WaveformMark::generateImage(float devicePixelRatio) {
assert(needsImageUpdate());

// Load the pixmap from file.
// If that succeeds loading the text and stroke is skipped.

m_breadth = static_cast<int>(breadth);

if (!m_pixmapPath.isEmpty()) {
QString path = m_pixmapPath;
// Use devicePixelRatio to properly scale the image
Expand Down Expand Up @@ -292,7 +301,7 @@ QImage WaveformMark::generateImage(float breadth, float devicePixelRatio) {
const bool useIcon = m_iconPath != "";

// Determine drawing geometries
const MarkerGeometry markerGeometry(label, useIcon, m_align, breadth);
const MarkerGeometry markerGeometry(label, useIcon, m_align, m_breadth, m_level);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use here { } initialization for better readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


m_label.setAreaRect(markerGeometry.m_labelRect);

Expand Down