improve rendering of waveform marks #12203

m0dB · 2023-10-21T10:32:43Z

This PR improves the rendering of the waveform marks, by auto-scaling and better aligning the symbolic marks (such as the intro/outro triangles and the loop open circle arrow), as discussed here: https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/look.20of.20loop.20out.20marker

Additionally it:

removes code duplication between waveformrendermark.cpp and allshaders/waveformrendermark.cpp by moving the mark image rendering to waveformmark.cpp and doing a general cleanup of that code
doesn't draw a small empty rectangle for marks without a label
fixes drawing of marks in vertical orientation in legacy waveforms
fixes a small bug with hovering over marks (propagate leave event)
uses antialiasing to render the marks and the glsl play position mark image

…educed code duplication, fixed a small bug with leave events not being send to the opengl widgets, use antialiasing for play pos

acolombier · 2023-10-22T12:49:31Z

What happens when labels overlap? (e.g in the screenshot, label of hotcue 4 would be long enough to overlap label of 3)
Do you think it would be possible to add some logic to bring labels on the foreground? For example, the last activated on is always on foreground or if the user hover them on the waveform, or in the deck

ronso0 · 2023-10-22T14:07:08Z

@acolombier I think you're referring to #11745

I suggest to keep this PR compact, then take care of the other issues (#11745 and dark/bright detection / hover effect).

I'll test this asap.

m0dB · 2023-10-22T14:09:40Z

There is currently no such logic. But the case you describe (the previous marker being so long that it overlaps the next marker) could be very easily solved with very simple logic: draw the markers from right to left.

But what annoys me most is when you have two markers at the same position (I actually use that quite a lot, e.g. a normal and a looping hotcue). I want to try placing the second one higher than the first in that case.

In any case, all that should be in a separate PR.

acolombier · 2023-10-22T14:11:57Z

@ronso0 this isn't quite the same now - my question was if instead of OK! as a label, you have Some long text(up to 23 chars + the ... suffix), how will that be interfering with the 3rd hotcue? I assume one will overlap the other, and I'm wondering if we should account for further work (no particularly in that PR) to improve the UX

@m0dB I also use that behavior and suffering from the same limitation and the change you are describing would be very appreciated :)

m0dB · 2023-10-22T14:12:10Z

I think #11745 is more about the inconsistency between the scrolling waveform and the waveform overview.

Swiftb0y · 2023-10-22T14:56:58Z

src/waveform/renderers/waveformmark.h

+    std::unique_ptr<QOpenGLTexture> m_pTexture; // Used by allshader::WaveformRenderMark
+    QImage m_image;                             // Used by WaveformRenderMark


so IIUC, m_pTexture is always nullptr when running with a legacy shader and QImage isNull the other way around, right? Wouldn't it be better instead then if these concepts lived in the shaders then instead? At least that sounds like thats a better separation of concern.

In the renderer you mean? (WaveformMarkRenderer and allshaders::WaveformMarkRenderer). But that would require keeping track of which image or texture goes with which WaveformMark. No, this really needs to be part of WaveformMark. But maybe it can be done by subclassing WaveformMark... I will have a look, I agree this is not ideal.

But that would require keeping track of which image or texture goes with which WaveformMark.

The WaveformMarks in the renderer are just stored in a container, should just be sufficient to store a std::pair<std::unique_ptr<QOpenGLTexture>, WaveformMark> and then implement generateMarkImage in terms of that, right?

No, they are stored in a WaveformMarkSet. But I can derive a WaveformMarkImage and a WaveformMarkTexture from WaveformMark and templatize WaveformMarkSet.

I started doing this, but it adds way too much complexity. I prefer having these two data members, of which either one is used depending on the context, instead of adding all this complexity.

okay. Have you considered putting the members in a variant to highlight the fact that they are mutual exclusive? If that introduces too much complexity again, then ignore it. I'm just trying to come up with solutions to make the code more self-documenting.

yes, that makes sense! will do!

done. hope this is in line with what you had in mind.

Yes, almost. I was rather thinking of std::variant but this also works. The only thing I'm slighly concerned about is the blind static_cast from the parent to the derived type but I don't know any better solution either right now without introducing lots of boilerplate.

Thank you for going through the extra effort.

src/waveform/renderers/waveformmark.h

JoergAtGithub · 2023-10-22T18:59:55Z

On my Win11 system the green loop marker looks different than in your screenshot:

m0dB · 2023-10-23T09:23:05Z

On my Win11 system the green loop marker looks different than in your screenshot:

My screenshot is HDPI so that will make a difference for sure, but indeed this looks worse than it should... Can you try changing, in waveformmark.cpp:

// And we scale the font size accordingly.
m_font.setPointSizeF(50.0 * ratio);

to

m_font.setPixelSize(19);

and see if that improves things? (This will make the intro/outro markers look weird, but that's expected)

JoergAtGithub · 2023-10-23T17:09:32Z

This is how it looks with m_font.setPointSizeF(50.0 * ratio); :

and this is how it looks like with m_font.setPixelSize(19);:

m0dB · 2023-10-23T17:44:00Z

and without this PR ?

JoergAtGithub · 2023-10-23T17:48:39Z

Plain 2.4:

m0dB · 2023-10-23T18:05:29Z

Thanks for testing and following up. Well, that's a relieve! That also looks bad!

What if you paste this character ↻ in a text editor with font Open Sans (bold) at different sizes?

And does it improve things if in waveformmark.cpp instead of painter.drawText(pos, label); you use painter.drawText(static_cast<int>(pos.x()), static_cast<int>(pos.y()), label); ?

JoergAtGithub · 2023-10-23T18:15:09Z

JoergAtGithub · 2023-10-23T19:22:20Z

This PR with painter.drawText(static_cast<int>(pos.x()), static_cast<int>(pos.y()), label);:

m0dB · 2023-10-24T09:19:36Z

My conclusion is that Windows does a pretty bad job at rendering this symbol. Wondering what it looks like on Linux.

Maybe we should use a png instead?

ronso0 · 2023-10-24T09:44:10Z

On Ubuntu Studio 20.04 it looks okay @150%

@250%

JoergAtGithub · 2023-10-24T17:55:15Z

Maybe we should use a png instead?

If possible, we should use svg vector graphics, as in the skins.

ywwg · 2023-10-25T15:40:58Z

+1 to using an SVG instead of a font

m0dB · 2023-10-28T10:26:48Z

I have added the possibility to specify an svg.

This is with an svg for the loop mark.

Does this look okay on non-HDPI, @JoergAtGithub ? Maybe the svg needs a bit of tweaking.

…, depending on the context

JoergAtGithub · 2023-10-28T20:43:16Z

Yes, the loop marker looks now fine on Windows too!

The Intro/Outro markers look not that bad as the loop markers looked before, but also there SVG should be used. Which can be done in another PR.

m0dB · 2023-10-28T22:26:46Z

Yes, the loop marker looks now fine on Windows too!

Great!

The Intro/Outro markers look not that bad as the loop markers looked before, but also there SVG should be used. Which can be done in another PR.

No, I will add it to this PR, it's very little effort.

Edit: Done, please confirm @JoergAtGithub

ronso0 · 2023-10-28T22:46:00Z

No, I will add it to this PR, it's very little effort.

All skins? 😬
Reason for asking: once upon a time we decided to add the Sync Leader control to all skins (even Shade?), and just today I noticed I only managed to add it to LateNight.
It's an entirely different story (in terms of xml integration and design effort), though I wanted to add that note.

m0dB · 2023-10-28T23:13:09Z

That's a good point, @ronso0 . I only added this to Latenight, but the text rendering is still there, so of course other skins can continue using that.

JoergAtGithub · 2023-10-29T06:53:51Z

Now all the markers look fine on Windows11! Thank you!

Swiftb0y

LGTM. thanks a lot. before merging, did someone test this on linux as well?

ywwg · 2023-11-02T01:05:58Z

looks good on linux

ywwg

looks good! Please clean up stray commented-out lines before merging, and I've made some comments. I am not blocking this PR on them though since I am famously bad at getting back to my reviews :)

ywwg · 2023-11-02T01:06:44Z

res/skins/LateNight/waveform.xml

@@ -62,7 +62,8 @@
            </MarkRange>
            <Mark>
              <Control>loop_start_position</Control>
-              <Text>&#8635;</Text>
+              <!--Text>&#8635;</Text-->


remove comments now that we are using the svg files

not sure about this (here). sometimes it's helpful to have the matching unicode char at hand.

Yes, I agree, the Text should be fallback. I will revert the commenting.

ywwg · 2023-11-02T01:07:34Z

src/waveform/renderers/waveformmark.cpp


    return m_label.area().contains(point) || lineHovered;
 }
+
+// Helper class to calculate the geometry and fontsize needed by generateImage


technically this is a struct not a class, so let's call it that

ywwg · 2023-11-02T01:08:02Z

src/waveform/renderers/waveformmark.cpp

+        const qreal margin{3.f};
+
+        qreal capHeight;
+        {


why is this scoping needed?

oh is this copied from the original code? then maybe that's fine 👍🏻

i removed it, as it allows capHeight to be const.

ywwg · 2023-11-02T01:08:54Z

src/waveform/renderers/waveformmark.cpp

+        QString path = m_pixmapPath;
+        // Use devicePixelRatio to properly scale the image
+        QImage image = *WImageStore::getImage(path, devicePixelRatio);
+        // QImage image = QImage(path);


please remove commented-out lines like this throughout

ywwg · 2023-11-02T01:09:16Z

src/waveform/renderers/waveformmark.cpp

+        //  If loading the image didn't fail, then we're done. Otherwise fall
+        //  through and render a label.
+        if (!image.isNull()) {
+            image =


unnecessary newline. if possible, please run the code formatter on changed lines

I am using pre-commit , not sure why this wasn't formatted. Anyway, fixed.

ywwg · 2023-11-02T01:11:26Z

src/waveform/renderers/waveformmark.h

+        Graphics()
+                : m_obsolete{false} {
+        }
+        bool m_obsolete;


I think this is equivalent and simpler, though we might have a style reason not to use this format?

Suggested change

Graphics()

: m_obsolete{false} {

}

bool m_obsolete;

bool m_obsolete = false;

See my comment below on uniform initialization.

ywwg · 2023-11-02T01:12:07Z

src/waveform/renderers/waveformmark.h


-    float m_linePosition;
+    float m_linePosition{};


I'm not sure we use this style of initialization elsewhere, probably clearer to do = 0; instead

I would like to get developer concensus on this. @Swiftb0y (IIRC) recently told me to initialize in the initializer list...

But there are two issues at hand here: 1) using curly braces aka uniform initialization and 2) in-class initialization vs initializer lists.

As for 1, from what I understand, uniform initialization is the prefered way since c++11. The standard https://isocpp.org/wiki/faq/cpp11-language#uniform-init clearly says: "Prefer initializing using {}."

As for 2, I don't think there is such a clear preference. When you have multiple constructors, in-class initialization certainly makes things easier, otherwise I don't think it matters much.

I would like to get developer concensus on this. @Swiftb0y (IIRC) recently told me to initialize in the initializer list...

Let me be clear, I didn't mean to tell you to use initializer lists, I just said other people will complain and I thought I'd mention it so it doesn't seem like I didn't review your PR thoroughly. I also prefer this style and if other devs do so too, then lets use it. If others complain we can settle this in a simple vote. :)

I'm not sure we use this style of initialization elsewhere, probably clearer to do = 0; instead

I also prefer = 0 but only when the type is clearly numerical. On the other hand the meaning of = 0 is already overloaded in the context of pure virtual member functions. So my preference is probably wrong and we should prefer using {} as per the cpp core guidelines.

Ok, let's take this discussion to zulip

removing in-class initialization

m0dB · 2023-11-05T10:39:04Z

All comments have been addressed, so I think this can be merged.

JoergAtGithub · 2023-11-05T11:59:51Z

Just tested this again, and it's still working. LGTM! Thank you!

m0dB added 3 commits October 21, 2023 12:17

auto-scale and improve align of symbol markers, additional changes: r…

8391e9d

…educed code duplication, fixed a small bug with leave events not being send to the opengl widgets, use antialiasing for play pos

fix vertical orientation

7149759

missing headers, missing type cast

32203af

m0dB requested review from ronso0 and Swiftb0y October 22, 2023 00:06

Swiftb0y reviewed Oct 22, 2023

View reviewed changes

optionally use an svg for the mark icon instead of text

50990db

github-actions bot added the skins label Oct 28, 2023

WaveformMark::Graphics can either be ImageGraphics or TextureGraphics…

17f0f19

…, depending on the context

add icons for intro and outro

494c360

Swiftb0y approved these changes Oct 29, 2023

View reviewed changes

ywwg reviewed Nov 2, 2023

View reviewed changes

m0dB added 7 commits November 4, 2023 19:35

plain svg

07b6389

Text as fallback

dd36ac0

corrected a comment

2a03c50

removed unnecessary scoping

7d32e40

removed commented code

e7aeb02

prefer useIcon over symbol

efaf97d

removed in-class initialization

3132017

JoergAtGithub merged commit 9129ace into mixxxdj:2.4 Nov 5, 2023
12 checks passed

daschuer added this to the 2.4.0 milestone Nov 5, 2023

		std::unique_ptr<QOpenGLTexture> m_pTexture; // Used by allshader::WaveformRenderMark
		QImage m_image; // Used by WaveformRenderMark

improve rendering of waveform marks #12203

improve rendering of waveform marks #12203

Conversation

m0dB commented Oct 21, 2023 • edited

acolombier commented Oct 22, 2023

ronso0 commented Oct 22, 2023

m0dB commented Oct 22, 2023

acolombier commented Oct 22, 2023

m0dB commented Oct 22, 2023

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

JoergAtGithub commented Oct 22, 2023

m0dB commented Oct 23, 2023

JoergAtGithub commented Oct 23, 2023

m0dB commented Oct 23, 2023

JoergAtGithub commented Oct 23, 2023

m0dB commented Oct 23, 2023

JoergAtGithub commented Oct 23, 2023 • edited

JoergAtGithub commented Oct 23, 2023

m0dB commented Oct 24, 2023

ronso0 commented Oct 24, 2023

JoergAtGithub commented Oct 24, 2023

ywwg commented Oct 25, 2023

m0dB commented Oct 28, 2023

JoergAtGithub commented Oct 28, 2023 • edited

m0dB commented Oct 28, 2023 • edited

ronso0 commented Oct 28, 2023

m0dB commented Oct 28, 2023

JoergAtGithub commented Oct 29, 2023

Swiftb0y left a comment

Choose a reason for hiding this comment

ywwg commented Nov 2, 2023

ywwg 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

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

Swiftb0y Nov 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m0dB commented Nov 5, 2023

JoergAtGithub commented Nov 5, 2023

m0dB commented Oct 21, 2023 •

edited

JoergAtGithub commented Oct 23, 2023 •

edited

JoergAtGithub commented Oct 28, 2023 •

edited

m0dB commented Oct 28, 2023 •

edited

Swiftb0y Nov 4, 2023 •

edited