Skip to content

Add option to disable EQ gain influence on waveforms#14998

Open
artiniest wants to merge 4 commits into
mixxxdj:mainfrom
artiniest:eq-waveform
Open

Add option to disable EQ gain influence on waveforms#14998
artiniest wants to merge 4 commits into
mixxxdj:mainfrom
artiniest:eq-waveform

Conversation

@artiniest
Copy link
Copy Markdown

image

Adds a new option in Preferences > Waveforms to disable the visible effect of EQ gain knobs on waveform rendering. Visual gain remains applied independently.

Feature request here

artiniest and others added 2 commits June 24, 2025 12:40
Adds a checkbox to preferences to toggle if EQ knob gain affects
waveform rendering. Allows displaying waveform amplitude regardless
of EQ knob positions.
@Swiftb0y
Copy link
Copy Markdown
Member

Welcome and thanks for improving this! Please sign the Mixxx Contributor Agreement and comment here when you have done so. It gives us permission to distribute your contribution under the GPL v2 or later license and the Apple Mac App Store. It is also helpful for us to have contact information for contributors in case we may need it in the future.

@artiniest
Copy link
Copy Markdown
Author

I've signed the agreement.

Copy link
Copy Markdown
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

this looks very decent already. I have a couple requests:

Comment thread src/preferences/dialog/dlgprefwaveformdlg.ui
Comment thread src/waveform/renderers/waveformrenderersignalbase.cpp Outdated
Flipped the setting logic to "visualize EQ waveform".
Fixed a logic issue where dead code was not skipped.
@artiniest
Copy link
Copy Markdown
Author

@Swiftb0y Addressed the points you brought up! Let me know if you notice anything else that I should fix.

Copy link
Copy Markdown
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

Thanks. Last nit I think.

Comment thread src/preferences/dialog/dlgprefwaveform.cpp Outdated
@Swiftb0y
Copy link
Copy Markdown
Member

I mean there is the general issue that this setting only applies to some waveform types (RGB, Stacked) and not others, but all of those settings are shown regardless of whether they have an effect on the current type or not. Addressing this is not a requirement for this PR though (you're welcome to look into it in a follow up PR if you'd like to).

Copy link
Copy Markdown
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

thoughts @acolombier?

@acolombier
Copy link
Copy Markdown
Member

acolombier commented Jun 24, 2025

I think it would be best to rely on Waveform option rather than adding a context-less setting. This will allows a better integration with the cleanup work we've made with @m0dB , prevent growing more config key and ease porting on QML/Mixxx 3.0 as not building on the WaveformFactory/dedicated config key.

This should be straight forward to convert tho, as you can pretty much keep all the UI as is, just need to migrate the backend to use options!

@Swiftb0y
Copy link
Copy Markdown
Member

I initially thought the same, but there is slight issue that the waveform options are only a feature of the allshader code but the waveform scaling is backend-agnostic. So the implementation is non-trivial afaict.

@acolombier
Copy link
Copy Markdown
Member

Indeed - I'm wondering if there is a middle ground we could reach, just to make sure we keep things clean. Let me see if I can move the option outside allshader

@acolombier
Copy link
Copy Markdown
Member

Hi @artiniest, waveform options should now be available outside of allshader waveform!

@kruitbosdotdev
Copy link
Copy Markdown

kruitbosdotdev commented Aug 17, 2025

@artiniest do you have any plans (and time) to finish this feature? If not, I'll be willing to improve upon it based on the comments in this thread.

@artiniest
Copy link
Copy Markdown
Author

@kruitbosdotdev I'm pretty busy with other projects at the moment so if you have the time to take this over the finishing line, that'd be great!

@ronso0
Copy link
Copy Markdown
Member

ronso0 commented Aug 18, 2025

For anyone continuing here, a small UI request:
let's move the new waveforms option to a new row. This page is already packed, and we try have pages as compact as possible in order to avoid horizontal scrollbars if possible.
(labels may look short in english, but keep in mind that Mixxx is translated to eg. french ; )

@ronso0 ronso0 linked an issue Aug 27, 2025 that may be closed by this pull request
@ronso0
Copy link
Copy Markdown
Member

ronso0 commented Oct 10, 2025

Unfotunately there are merge conflicts now. Looks easy to fix though.
@artiniest / @kruitbosdotdev who's motivated to finish this?

@DidierMalenfant
Copy link
Copy Markdown
Contributor

Unfotunately there are merge conflicts now. Looks easy to fix though. @artiniest / @kruitbosdotdev who's motivated to finish this?

I've pushed a PR which merges this with the latest main branch and fixes a bug in how the default value was set. Seems to work ok now but @artiniest will have to merge my PR first.

If we don't hear back I can open a new PR for this too.

@DidierMalenfant
Copy link
Copy Markdown
Contributor

Seems to work ok

I take that back. It messes up the regular gain influence rendering when enabled.

Investigating now...

@daschuer
Copy link
Copy Markdown
Member

daschuer commented Jan 6, 2026

Please also note the feedback for RGB types is broken. Here is the fix:
#15808

@DidierMalenfant
Copy link
Copy Markdown
Contributor

Please also note the feedback for RGB types is broken. Here is the fix: #15808

Ah yes... that explains it then. I merged that PR on top of mine and everything is looking good now.

One thing I noticed is that the original PR for this retains a test that keeps getting removed and added back with various merges in main. In waveformrenderersignalbase.cpp:

if (m_pEQEnabled && m_pEQEnabled->get() > 0.0) {

The test for m_pEQEnabled was removed in 4cc1307b but then added back in a merge from 2.6 at d628d80f. I just want to make sure this can stay in with this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Option to Disable Waveform Color Changes When Adjusting EQ

7 participants