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

turn waveform_zoom into ControlPotmeter #12387

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Dec 2, 2023

c++ alternative to #12373

  • waveform_zoom is now a ControlPotmeter incl. _up, _down, _set_default and some rather useless controls
    step size is 1, i.e. _up/_down act exactly as before
  • added waveform_zoom_selector, triggers _up/_down and thereby simplifies mapping rotary encoders with the SelectKnob MIDI option already possible with the e.g. the Rot64 MIDI option
  • fixed some hard-coded ranges in mappings controllers: fix waveform_zoom ranges #12393

TODO

Note to myself:
pick 55418d0 from #12373

Comment on lines 172 to 174
ControlObject::set(ConfigKey(getGroup(), "waveform_zoom_" + dir), 1);
ControlObject::set(ConfigKey(getGroup(), "waveform_zoom_" + dir), 0);
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not happy with this, but I couldn't make it work with ControlProxies.
I created them right after having created the ControlPotmeter and used them in the lambda, but setting the proxies would do nothing, and I have no idea why. No error or warning was thrown.

@daschuer
Copy link
Member

A conflict has developed. What is the state here? Is still a draft?

@ronso0 ronso0 force-pushed the waveform-zoom-controlpotmeter branch from 2378cb6 to 4dc8d5c Compare February 22, 2024 11:51
@ronso0
Copy link
Member Author

ronso0 commented Feb 22, 2024

Conflict is resolved.

I didn't test this yet with a controller myself.

@ronso0
Copy link
Member Author

ronso0 commented Feb 23, 2024

It zooms pretty fast, so maybe there could be call to increase the number of zoom levels.

Which control did you map? (to what kind of input?)

waveform_zoom is for pots (continuous 0 -127)
waveform_zoom_selector is for encoders (-1, 0, 1 / 63, 64, 54), triggers _up and _down and should work exactly as before.

@mxmilkiib
Copy link
Contributor

I assigned the new one to an encoder.

Previously, I assigned the old one to a pot and it didn't work, like you state here it should, this being my original confusion.

Past of my first message on the forum being; "I’m trying to assign “Waveform Zoom” (not “Waveform Zoom In” or “Waveform Zoom Out”) to a channel dial on a Kork nanoKontroller2, but it does nothing when I turn the dial after."

@mxmilkiib
Copy link
Contributor

mxmilkiib commented Feb 24, 2024

So it's highly likely I got part of an understanding/remembrance turned around once or twice.

I checked again earlier; the original works with a standard pot, the new one with a rotary encoder.

The pot has more granularity to the zoom and you can turn it for longer. The encoder only needs to rotate a rather small amount to go through all the zoom levels.

@ronso0
Copy link
Member Author

ronso0 commented Mar 9, 2024

I checked again earlier; the original works with a standard pot, the new one with a rotary encoder.

Yes, the new "original one" (waveform_zoom) now works right away with a pot (required manual script implementation earlier).

I just tested both this and waveform_zoom_selector with the learning wizard with a pot and a rotary encoder and they work as expected.

@mxmilkiib
Copy link
Contributor

critical [Controller] DEBUG ASSERT: "!"pBehavior == nullptr, abort setValueFromMidi()"" in function void ControlDoublePrivate::setValueFromMidi(MidiOpCode, double) at ./src/control/control.cpp:347
warning [Controller] Cannot get "[Channel1]" , "waveform_zoom" as Midi
critical [Controller] DEBUG ASSERT: "!"pBehavior == nullptr, getMidiParameter() is returning 0"" in function double ControlDoublePrivate::getMidiParameter() const at ./src/control/control.cpp:357
warning [Controller] Cannot set "[Channel1]" , "waveform_zoom" from Midi

@ronso0
Copy link
Member Author

ronso0 commented Apr 7, 2024

critical [Controller] DEBUG ASSERT: "!"pBehavior == nullptr, abort setValueFromMidi()"" in function void ControlDoublePrivate::setValueFromMidi(MidiOpCode, double) at ./src/control/control.cpp:347
warning [Controller] Cannot get "[Channel1]" , "waveform_zoom" as Midi
critical [Controller] DEBUG ASSERT: "!"pBehavior == nullptr, getMidiParameter() is returning 0"" in function double ControlDoublePrivate::getMidiParameter() const at ./src/control/control.cpp:357
warning [Controller] Cannot set "[Channel1]" , "waveform_zoom" from Midi

How did you get these?

Ii just tested again with a clean mapping and all cotnrols work as expected: up, down, selector, pot

Copy link

github-actions bot commented Jul 7, 2024

This PR is marked as stale because it has been open 90 days with no activity.

@github-actions github-actions bot added the stale Stale issues that haven't been updated for a long time. label Jul 7, 2024
@github-actions github-actions bot removed the stale Stale issues that haven't been updated for a long time. label Oct 17, 2024
@mxmilkiib
Copy link
Contributor

This is the same Cannot get "[Channel1]" , "waveform_zoom" as Midi assertion crash as in #10136

@ronso0 ronso0 force-pushed the waveform-zoom-controlpotmeter branch from 4dc8d5c to c2e79cc Compare January 30, 2025 22:12
@ronso0
Copy link
Member Author

ronso0 commented Jan 30, 2025

Well, this branch is more than a year old so it has all the bugs of main back then.
I've just rebased this, please test again.

@ronso0 ronso0 force-pushed the waveform-zoom-controlpotmeter branch from c2e79cc to 33da9cd Compare January 30, 2025 22:19
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.

For my understanding we don't need "waveform_zoom_selector". We have already the mapping options
We have the mapping options of "HercJog"/"HercJogFast"/"Rot64"/"Rot64Fast" for it.

@ronso0
Copy link
Member Author

ronso0 commented Feb 1, 2025

Yup, that's right!
I simplified the actual potmeter commit.

I'd also pick 55418d0 from #12373, wdyt?

@ronso0
Copy link
Member Author

ronso0 commented Feb 1, 2025

And actually this can go to 2.5 now that it's just a backend change of the existing control.
I think being able to map it to pots is an enhancement. Wdyt?

@mxmilkiib
Copy link
Contributor

mxmilkiib commented Feb 1, 2025

the only bump I'd like to mention is #6682, and that, where it does work, the zoom control will not zoom through the entire scale of a patched-to-have-a-larger-zoom-scale build

I don't know how that can potentially be future proofed for given any changes/adaptation here (and trickiness of implementing a scale range change). I understand that that might not be possible, though I'll continue to patch my zoom to allow a better overview of the relationship between song timelines

edit; I forgot that I think why it works on my nanokontrol2 is because of the controller script doing the magick

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.

LGTM, just a compiler warnig. Thank you.
Please amend, an than we can merge.

@@ -559,6 +561,12 @@ void DlgPrefWaveform::slotSetWaveformOverviewType() {

void DlgPrefWaveform::slotSetDefaultZoom(int index) {
WaveformWidgetFactory::instance()->setDefaultZoom(index + 1);
for (int i = 1; i <= PlayerManager::numDecks(); i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for (int i = 1; i <= PlayerManager::numDecks(); i++) {
for (unsigned int i = 1; i <= PlayerManager::numDecks(); i++) {

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, saw that. I've also included samplers update.

@ronso0 ronso0 force-pushed the waveform-zoom-controlpotmeter branch from 4e068f4 to b8c4723 Compare February 4, 2025 01:01
@ronso0
Copy link
Member Author

ronso0 commented Feb 4, 2025

I fixed the build issue, but I also saw that the "zoom_set_default" buttons in Deere did not reset to the desired value.
Couldn't reproduce again, so please do some thorough tests incl. waveform preferences @mxmilkiib

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.

No way to directly (Wizard) map waveform_zoom to a controller pot yet
3 participants