avoid overlapping marks #12273

m0dB · 2023-11-08T09:33:53Z

draw overlapping marks at increasing/decreasing y position in the waveform viewer

ronso0 · 2023-11-08T12:26:17Z

TODO: i haven't figured out yet how to get signalled when the loop mark changes.

What about WaveformMarkSet::connectSamplePositionChanged?

Swiftb0y · 2023-11-08T12:32:42Z

The ideas in this PR are pretty good (I like the priority concept) but I feel like just stacking marks with the exact same sampleposition on top of each other is not the best solution. For example, cues set carelessly and without quantize might have slightly different samplepositions, but are still essentially on top of each other. Also, the labels of different cues still fight with each other as well (as seen in your screenshot). If instead comparing cues by sampleposition, we just avoided the collision of the boxes in general, the special cases where markers are on top of each other or very near would be solved automatically. How feasible do you think this is?

m0dB · 2023-11-08T22:28:28Z

Certainly feasible, definitely more complicated to implement, and potentially resulting in undesired situations with markers jumping up and down when changing the zoom level.

In any case, I would only consider doing that after a more thorough refactoring of the related code. With the current code, these are minor changes that bring a very noticable improvement. My proposal is to start here and implement further improvements like you are suggestion down the road.

Swiftb0y · 2023-11-09T06:13:28Z

Sounds good as well, let me know when this PR is ready.

ywwg · 2023-11-15T17:58:28Z

I agree this is a good first start, and that in the longer term a generalized box-avoidance solution is preferable. I think it's ok if things move around as the zoom changes, that's like a web page reflowing when the size changes.

m0dB · 2023-11-16T01:33:08Z

TODO: i haven't figured out yet how to get signalled when the loop mark changes.

What about WaveformMarkSet::connectSamplePositionChanged?

Yes, I am using this, and this works for all markers excepts for the loop markers.

EDIT: No, I am not, sorry for the confusion.

…aveform renderers. reduced code duplications, fix setting the initial size of the waveform renderers

m0dB · 2023-11-18T11:35:57Z

TODO: i haven't figured out yet how to get signalled when the loop mark changes.

What about WaveformMarkSet::connectSamplePositionChanged?

Works like a charm!

I have reduced code duplication between the legacy and allshader waveformmarkrenderer.cpp (with a shared intermediate baseclass) and done cleanup. Everything is working as expected, so this is now ready for review.

ronso0 · 2023-11-19T20:32:28Z

When I build this I get

/src/waveform/renderers/allshader/waveformrendermark.cpp: In member function ‘void allshader::WaveformRenderMark::drawMark(const QRectF&, QColor)’:
/src/waveform/renderers/allshader/waveformrendermark.cpp:118:5: error: ‘getRgbF’ was not declared in this scope
  118 |     getRgbF(color, &r, &g, &b, &a);
      |     ^~~~~~~

I reconfigured to make sure precompiled headers are off, though same reasult.

JoergAtGithub · 2023-11-20T07:00:24Z

On Windows it builds - and works as intended!

ronso0 · 2023-11-20T10:00:41Z

src/waveform/renderers/allshader/waveformrendermark.cpp

 #include <QOpenGLTexture>
 #include <QPainterPath>

-#include "track/track.h"
-#include "util/color/color.h"
-#include "util/colorcomponents.h"


It builds when I restore this include

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?

ronso0 · 2023-11-20T12:34:13Z

It works as desired, thanks.

Though there's a small UX issue with the hover threshold:

create two cues in one place (1 +5)
hover 1: is highlighted
move slightly to the left: while the pointer is technically still above [1] the mark line is hovered and highlights [5]
move slightly to the right: now the pointer is definitely above [1] but [5] remains highlighted

I doub't anyone would do this while the waveform is moving, so there is no real (live) performance problem, also the cues are ultimately in the same place, but with a hotcue and a loop cue this might result in some confusion.
Anyhow, with 2.4 this situation would much more unclear, so it's not a real issue.

m0dB · 2023-11-26T10:37:38Z

@ronso0 I added the missing include and I slightly improved the hovering detection. It is still not perfect, but certainly better, and good enough for now. I think that with these last changes this PR is ready for merge.

ywwg · 2023-11-27T00:32:10Z

I feel like just stacking marks with the exact same sampleposition on top of each other is not the best solution.

It think even if this is not the best solution, it's an improvement and does not go down a road that would be hard to reverse. I am in favor of merging this for 2.4

ronso0 · 2023-11-28T02:02:09Z

@ronso0 I added the missing include and I slightly improved the hovering detection. It is still not perfect, but certainly better, and good enough for now. I think that with these last changes this PR is ready for merge.

Okay, great! I'll test this soonish, but I'll the code review to others ;) I'm still no good at fully understanding / judging class inheritance, ownership, basic oop rules ..

daschuer

Thank you for the PR.
Here some code and comment improvement comments.

daschuer · 2023-11-28T06:55:26Z

src/waveform/renderers/waveformmark.cpp

        const WaveformSignalColors& signalColors,
        int hotCue)
-        : m_linePosition{}, m_breadth{}, m_iHotCue{hotCue} {
+        : m_linePosition{}, m_breadth{}, m_level{}, m_iPriority(priority), m_iHotCue(hotCue) {


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.

daschuer · 2023-11-28T06:58:45Z

src/waveform/renderers/waveformmark.cpp

    }
-    bool lineHovered = m_linePosition >= point.x() - lineHoverPadding &&
+    return m_linePosition >= point.x() - lineHoverPadding &&


lineHoverPadding and the comment can be moved to the anonymous namespace.

daschuer · 2023-11-28T07:01:09Z

src/waveform/renderers/waveformmark.cpp

-    return m_label.area().contains(point) || lineHovered;
+bool WaveformMark::contains(QPoint point, Qt::Orientation orientation) const {
+    if (orientation == Qt::Vertical) {
+        point = QPoint(point.y(), static_cast<int>(m_breadth) - point.x());


Can you add a comment here? Maybe we can also rename or document m_breath. Can it be width?

We don't have width here, but I will add a comment explaining that in the case of Qt::Vertical orientation, breadth is width.

daschuer · 2023-11-28T20:03:21Z

src/waveform/renderers/waveformmark.cpp

@@ -292,7 +306,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);


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

daschuer · 2023-11-28T20:31:17Z

src/waveform/renderers/waveformmark.h

@@ -109,7 +148,8 @@ class WaveformMark {
    QString m_iconPath;

    float m_linePosition;
-    int m_breadth;
+    float m_breadth;
+    int m_level;


m_level can make use of a comment.

daschuer · 2023-11-28T20:32:57Z

src/waveform/renderers/waveformmark.h

@@ -19,16 +19,16 @@ class WaveformMark {
  public:
    class Graphics {
      public:
-        Graphics()
-                : m_obsolete{false} {
+        Graphics() {


I think the ctor can be removed now.

daschuer · 2023-11-28T20:40:32Z

src/waveform/renderers/allshader/waveformrendermark.cpp

-            generateMarkImage(pMark);
-        }
+    // Will create textures so requires OpenGL context
+    updateMarkImages();


Is m_obsolete still in use?

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

daschuer · 2023-11-28T20:42:35Z

src/waveform/renderers/waveformrendermarkbase.cpp

+}
+
+void WaveformRenderMarkBase::updateMarkImages() {
+    for (auto& pMark : m_marks) {


Suggested change

for (auto& pMark : m_marks) {

for (const auto& pMark : qAsConst(m_marks)) {

daschuer · 2023-11-28T20:47:02Z

src/waveform/renderers/waveformmarkset.cpp

+    double prevSamplePosition = Cue::kNoPosition;
+    std::map<Qt::Alignment, int> levels;
+    for (auto& pMark : m_marksToRender) {
+        if (pMark->getSamplePosition() != prevSamplePosition) {


do we need to consider a certain distance here?

Which values of https://doc.qt.io/qt-6/qt.html#AlignmentFlag-enumf are considered?

do we need to consider a certain distance here?

Ideally yes, but that goes beyond this PR. This deals only with marks set at quantised positions, which IMO covers the primary use case.

Which values of https://doc.qt.io/qt-6/qt.html#AlignmentFlag-enumf are considered?

I'll leave a comment

daschuer · 2023-11-28T20:56:39Z

src/waveform/renderers/waveformrendermarkbase.cpp

+#include "waveform/renderers/waveformwidgetrenderer.h"
+
+WaveformRenderMarkBase::WaveformRenderMarkBase(
+        WaveformWidgetRenderer* waveformWidgetRenderer,


the p prefix is missing.

ronso0 · 2023-11-30T00:47:49Z

@ronso0 I added the missing include and I slightly improved the hovering detection. It is still not perfect, but certainly better, and good enough for now.

Indeed its much better now! Thanks!

m0dB · 2023-12-08T22:03:05Z

Sorry for the delay, @daschuer , this fell of my radar (I was convinced it was already merged). I addressed all your comments.

JoergAtGithub · 2023-12-18T17:56:51Z

Ok, let's merge this! Thank you!

fwcd · 2024-01-02T18:39:16Z

Hm, after playing with this I've encountered a number of cases where it makes the waveform harder to read.

There are a lot of tracks where the main cue overlaps with the intro marker and thus obscures the waveform (at least when viewed at a small-ish height):

Any ideas? Perhaps the calculation could somehow incorporate the amplitude of the waveform to "avoid" stacking markers if there isn't enough space?

m0dB · 2024-01-02T18:53:11Z

Yeah, that is really very small, I never do that, as my eyesight is quite bad 😅

Do you really mean "the amplitude of the waveform", or do you mean the height of the waveform viewer? I think the first is a bad idea, the second might work and should be easy to implement. Like limiting the total space occupied by the markers from either top or bottom to a third of the total height. In your example that would mean (almost) fully overlapping I guess. Alternatively we could scale the markers to a smaller size. I guess that if you are okay with staring at small waveforms you are also okay with staring at small markers ;-)

JoergAtGithub · 2024-01-02T19:01:55Z

Why not scale the markers below a certain height limit - e.g. no marker must be higher than 20% of the waveform viewer height.
Another approach would be transparenc, but I fear, that can impact readability.

BTW: The feature itself helped me removing saved loop from Serato, where I wasn't ware of before, because hotcues/loops >8 have no pads.

fwcd · 2024-01-02T19:02:36Z

Yeah, that is really very small, I never do that

On laptops where screen real estate is limited, it's quite useful to have small waveforms IMO, especially when browsing the library and even more when playing with 4 decks.

Do you really mean "the amplitude of the waveform", or do you mean the height of the waveform viewer?

I was thinking of the actual waveform, though admittedly that might be too complex to implement/might have surprising semantics e.g. when adjusting gain etc.

Like limiting the total space occupied by the markers from either top or bottom to a third of the total height.

I like that idea!

In your example that would mean (almost) fully overlapping I guess

Yeah, I guess that would be hard to avoid, though I think it would be fine if they overlap like in previous versions in that case. Ideally perhaps even in a deterministic order (i.e. always rendering the CUE marker above the intro/outro markers)

Alternatively we could scale the markers to a smaller size. I guess that if you are okay with staring at small waveforms you are also okay with staring at small markers

Interesting idea, though I'd probably still consider overlapping markers the best tradeoff for readability.

fwcd · 2024-01-02T19:08:55Z

Why not scale the markers below a certain height limit - e.g. no marker must be higher than 20% of the waveform viewer height.

I feel like scaling the markers non-uniformly could potentially look a bit off. But perhaps we could just make the marker scale configurable?

In any case, we should make sure that the default works well.

m0dB · 2024-01-02T23:17:57Z

How about this? (and let's move the conversation there) #12501

draw overlapping marks at increasing level, both in legacy and glsl w…

0277e3c

…aveform renderers. reduced code duplications, fix setting the initial size of the waveform renderers

m0dB force-pushed the fix-overlapping-marks branch from 7e2568e to 0277e3c Compare November 18, 2023 04:10

github-actions bot added build ui labels Nov 18, 2023

m0dB added 2 commits November 18, 2023 12:24

fix gcc compilation

e3c585b

formatting

d16dcc8

m0dB marked this pull request as ready for review November 18, 2023 11:36

m0dB requested review from Swiftb0y and ronso0 November 18, 2023 11:37

ronso0 reviewed Nov 20, 2023

View reviewed changes

m0dB added 2 commits November 26, 2023 11:32

missing include

ed37eda

slightly improved hovering detection

887dea6

m0dB requested a review from ronso0 November 26, 2023 21:53

daschuer requested changes Nov 28, 2023

View reviewed changes

added comments and minor changed based on review

f7e7cd5

m0dB requested a review from daschuer December 8, 2023 22:03

daschuer approved these changes Dec 18, 2023

View reviewed changes

JoergAtGithub merged commit 69662da into mixxxdj:2.4 Dec 18, 2023
12 checks passed

daschuer added this to the 2.4.0 milestone Dec 18, 2023

m0dB mentioned this pull request Jan 2, 2024

gradually "compact" the markers if the waveform height is reduced #12501

Merged

fwcd added the waveform label Jan 2, 2024

	for (auto& pMark : m_marks) {
	for (const auto& pMark : qAsConst(m_marks)) {

avoid overlapping marks #12273

avoid overlapping marks #12273

Conversation

m0dB commented Nov 8, 2023 • edited

ronso0 commented Nov 8, 2023

Swiftb0y commented Nov 8, 2023

m0dB commented Nov 8, 2023

Swiftb0y commented Nov 9, 2023

ywwg commented Nov 15, 2023

m0dB commented Nov 16, 2023 • edited

m0dB commented Nov 18, 2023 • edited

ronso0 commented Nov 19, 2023

JoergAtGithub commented Nov 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronso0 commented Nov 20, 2023

m0dB commented Nov 26, 2023 • edited

ywwg commented Nov 27, 2023

ronso0 commented Nov 28, 2023

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m0dB Dec 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronso0 commented Nov 30, 2023

m0dB commented Dec 8, 2023

JoergAtGithub commented Dec 18, 2023

fwcd commented Jan 2, 2024 • edited

m0dB commented Jan 2, 2024

JoergAtGithub commented Jan 2, 2024

fwcd commented Jan 2, 2024

fwcd commented Jan 2, 2024

m0dB commented Jan 2, 2024

m0dB commented Nov 8, 2023 •

edited

m0dB commented Nov 16, 2023 •

edited

m0dB commented Nov 18, 2023 •

edited

m0dB commented Nov 26, 2023 •

edited

m0dB Dec 8, 2023 •

edited

fwcd commented Jan 2, 2024 •

edited