feat: simplify waveform combobox

[acolombier]

This addresses #6428 #13226

This current uses RGB only and the RGB L/R is unused.

By default, it prioritises the GL version if AllShader isn't available, is that the best approach?

Kooha-2024-05-08-18-15-40-10MB.mp4

[JoergAtGithub]

The idea is to merge #13151 and than retire all classic GL and GLSL waveforms.

Since #12994 we have also first waveform type specific sub options, I could imagine, that we use such an aub option for L/R support as well.

[acolombier]

Are we planning to remove MIXXX_USE_QOPENGL in this case?
I guess that doesn't not invalidate this PR, since this is already decoupling the rendering backend complexity away from the waveform type.
When it comes to RGB L/R, is it really something that people use? On LateNight default colour scheme, it just make the waveform even more rainbow looking and hard to follow. We could add a check box when RGB is selected, but since this task is to simplify the UX, I'm wondering if this is really necessary

---

Joerg replied

When it comes to RGB L/R, is it really something that people use?

Yes, we had the discussion already, this is why it got implemented. The old GLSL waveforms had this (but looking different), and to deprecated them requires feature-equality.

[acolombier]

(not sure why you edited my message?)
It doesn't quite make sense as of why there will be two separate waveform renderer tho. Arguably, if this is used, the same option should be done for the HSV waveform.
Anyway, I'm going to add another checkbox.

[acolombier]

I have merged the RGB L/R and RGB in a single renderer, and introduced rendering option, which can be reused with HSV in the future, giving slip mode renderer as a free bonus. Happy to move this commit in a dedicated PR. Would be great if @m0dB could have a quick look on that section!

[acolombier]

Right, I think this is pretty much ready. The only outstanding question I have is, should I hide the Acceleration checkbox is not available/only available? Since I do the following for the L/R option, it might keep it consistent

[JoergAtGithub]

(not sure why you edited my message?)

I did not edit it intentional - sorry - I just intended to reply

[acolombier]

Right, that should be ready for review now.
Once #13151 (and #12641) are in, it should be straight forward to retire the legacy waveforms.

[ywwg]

this is a great quality-of-life fix! thank you

[m0dB]

This is nice.

[acolombier]

I've updated the UI and behaviour according to what #13226 mentioned. @m0dB I have already added the High details option so hopefully will make the integration with #13151 seamless.

Code is a bit dirty in some places, especially in the factory, but hopefully with your help, we can make it good enough.

Here is the demo:

Kooha-2024-05-12-16-58-50.mp4

[acolombier]

@ywwg I have added full coverage on the waveform type upgrade. Please let me know if I have covered your usecase correctly. Also removed the VSync if not in dev mode

[m0dB]

I have created a PR on top of this PR (I hope that works, git wise) to get rid of all the code duplication in the allshader waveformwidget, reducing it to a single one + some cleanup of waveformwidgetfactory.

acolombier#2

[acolombier]

Spec available here

[ywwg]

I hope that works, git wise

that should work, yes

[acolombier]

I have hidden the acceleration checkbox on Mac. Note that it will still be show if running with --developer or if the acceleration backend isn't the recommended one (all-shader, happens if you override the setting in INI)

@m0dB could you confirm it hides properly?

[m0dB]

could you confirm it hides properly?

Not entirely, the combobox is a bit smaller than the other full-width comboboxes.

That said, I created acolombier#3 and checked that the non-accelerated waveform types work correctly on macOS, so we might as well enable them? Not very useful on macOS I guess, but at least we keep the UI and the behaviour identical across platforms.

We don't seem to have stacked and simple software waveform types though. I guess they should be disabled in the combobox when non-accelerated is selected?

[acolombier]

Not entirely, the combobox is a bit smaller than the other full-width comboboxes.

Which backend were you using? All-shader? Otherwise, it is expected it shows it.

we might as well enable them?

As our Mac developer, I'll follow your call the matter! :)
We mentioned in our sync Monday that macs have a fairly unified set of hardware, which all support the all-shader one, so I thought it might make sense to default to all shader and simplify the UI, but I also like the unified UI across platform so happy either way.

We don't seem to have stacked and simple software waveform types though. I guess they should be disabled in the combobox when non-accelerated is selected?

I went the other way around and made the combobox leading the decision with I think is a better UX: if you selected Simple or Stacked it will enable the checkbox (enable hardware) and disable the option. This way, a user can understand no software fallback is available but also don't have to toggle the checkbox first before selecting the new waveform type.
I guess I could add a tooltip too?

[acolombier]

@ Mixxx's Core Team could we look at merging that PR ASAP, and potentially to a thorough retroactive review? I agree to address comments and concerns in one or more follow up PRs, but this would help me and @m0dB to stop that PR to grow out of proportion

[ywwg]

sorry, which PR?

[Swiftb0y]

Thank you, mostly nitpicks to be addressed some other time, only thing that needs work is the upgrade code which currently looks like a bug to me.

[acolombier]

I think we got everything in order now

[Swiftb0y]

LGTM. Any1 else wanting to take a look (just in case I missed any grave errors, not for trivial stuff like I nitpicked in this round)?

[ronso0]

I didn't look into the ui file in detail, just want to share a few thoughts on the UI/UX:

• I think it'd look better if Acceleration, Stereo and High Details are in one row
• the label "This function requires ..." should only be shown if waveforms are enabled and acceleration is disabled
• for consistency, all waveform-related options should be greyed out (end-of-track, play position, ...)
• while working on allow changing the waveform overview type without reloading the skin #13273 I noticed we should move the overview options into their own section, because if we do disable all waveform options, the overview options are in between those disabled options which is a bit confusing IMO
  Maybe move the Overview options to the top so they're more discoverable than below the Waveform option?
  Maybe use QGroupBoxes for waveform and overview?

I'll check if I can provide some demo commits tomorrow.

[acolombier]

Thanks for your input @ronso0
Are you okay with this being improved in a follow up PR?

[ronso0]

Sure.