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

Add quick effect support on stem #13123

Merged
merged 4 commits into from
Aug 25, 2024

Conversation

acolombier
Copy link
Member

@acolombier acolombier commented Apr 17, 2024

Kooha-2024-04-17-19-45-50.mp4

Depends on #13086

@acolombier acolombier changed the title Add quick efect support on stem Add quick effect support on stem Apr 17, 2024
@acolombier acolombier mentioned this pull request Apr 17, 2024
8 tasks
@acolombier acolombier force-pushed the feat/add-quick-fx-on-stem branch 2 times, most recently from 463cea7 to fc42319 Compare April 23, 2024 16:40
@JoergAtGithub JoergAtGithub added this to the 2.6-beta milestone May 8, 2024
@acolombier acolombier force-pushed the feat/add-quick-fx-on-stem branch 2 times, most recently from a19d91e to 0aab61d Compare May 15, 2024 15:47
@acolombier acolombier force-pushed the feat/add-quick-fx-on-stem branch 2 times, most recently from db6b8ea to abc909c Compare May 22, 2024 17:30
@JoergAtGithub
Copy link
Member

Two minor remarks regarding the floating widgets:

  • It's sometimes difficult to distinguish the colored widget from the waveform which has the same colors. A border in a different color might help.
    grafik
  • The effect toggle button is very small an difficult to hit with the mouse

@acolombier
Copy link
Member Author

Yeah, that widget definitely would need some refactoring, but I think the deal was to remove it before we merge that feature, and is only here to facilitate testing, thus I didn't want to spend too much time with it.

@JoergAtGithub
Copy link
Member

I suggested to move the floating widgets in an own PR, to not block merging this PR and the PRs before.
I'm not against these widgets, I just expect a longer review discussion than for the other code.

@acolombier
Copy link
Member Author

Ah I had misunderstood the plan for that. Sounds good then, I try and see if I can use the effect state indicator used in the effect bar instead of the mixer one.

@acolombier acolombier force-pushed the feat/add-quick-fx-on-stem branch from e05f296 to 36312a8 Compare May 31, 2024 19:53
@acolombier acolombier force-pushed the feat/add-quick-fx-on-stem branch 2 times, most recently from b2c3d25 to b49e163 Compare June 3, 2024 13:18
@acolombier acolombier marked this pull request as ready for review June 9, 2024 17:17
@Eve00000
Copy link
Contributor

Eve00000 commented Aug 8, 2024

Remark after Mixxxing a lot (and having lots of fun)
As I feel (and the way I DJ) it would prefer to have the fx standard disabled when a track is loaded, when I switch a lot loading tracks, I forget an effect is enabled and pushing/turning one stemcontrol activates all the others.
So analogous to the [ChannelXStemY],mute (that is disabled by default) ([QuickEffectRackX_[ChannelXStemY]],enabled) also on 0.
Or is there a certain idea to put these on enabled buh default?

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

The code is probably good. But it is not instantly understadable. I have left some comments.

src/engine/channels/enginedeck.cpp Outdated Show resolved Hide resolved
src/util/sample.h Outdated Show resolved Hide resolved
This allows the effect manager to apply quick effect on each stem.
Effects are applied post stem fader, but pre deck fader
@acolombier acolombier force-pushed the feat/add-quick-fx-on-stem branch from 1a40dd5 to d62e1b3 Compare August 22, 2024 15:58
@acolombier
Copy link
Member Author

I have removed the UI widget, so this PR should be ready for merge. If you wish to test it with the UI widget, you can use the branch feat/add-quick-fx-on-stem-with-ui

@acolombier
Copy link
Member Author

fx standard disabled when a track is loaded

I will look into this. There is already a setting for mute/volume, so it makes sense to extend it to FX IMO.

@JoergAtGithub
Copy link
Member

fx standard disabled when a track is loaded

I will look into this. There is already a setting for mute/volume, so it makes sense to extend it to FX IMO.

Before you implement this, have a look on the discussion in PR #11198

@daschuer
Copy link
Member

How is the state of the QML GUI inegration? I see the controls, but they are reset immoderately.
What is the best option to test it without?

@acolombier
Copy link
Member Author

QML GUI integration should be functional and I intend to have it fully usable there, so if it's not, it's a bug that I should fix!

@daschuer
Copy link
Member

Maybe this relates to my old Qt version 6.2.
Is there a minimum requirement that we need to met?

@daschuer
Copy link
Member

daschuer commented Aug 23, 2024

@JoergAtGithub
How does the testing succeeds on your side?
Since we don't officially ship QML anyway and my Qt is outdated it shall not block the merge.

Which Qt versions are known working?

@acolombier
Copy link
Member Author

@daschuer I am also using Qt 6.2 - it turns out that something was broken on QML. The control should now work correctly. Also turned out that I had applied the iterator change and committed by accident.

Before you implement this, have a look on the discussion in PR #11198

Ah I have a pending review on that PR - I guess we could add the feature suggested by @Eve00000 as part of that change or once this PR is merged so we can use the same setting for it?

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Works now like a charm. Thank you.

@JoergAtGithub
Copy link
Member

@JoergAtGithub How does the testing succeeds on your side? Since we don't officially ship QML anyway and my Qt is outdated it shall not block the merge.

My last tests before removing the non-QML GUI code was flawless. But I can't start QML anymore, neither with main nor this PR (#13600). As this is unrelated, and Daniel tested the QML changes successful on his system, this shouldn't block the merge of this PR.

@JoergAtGithub
Copy link
Member

Thank you!

@JoergAtGithub JoergAtGithub merged commit 2a48746 into mixxxdj:main Aug 25, 2024
13 checks passed
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.

4 participants