Denon MC7000: Handle autoloop button in script by flosse · Pull Request #4307 · mixxxdj/mixxx

flosse · 2021-09-19T14:09:56Z

I was confused by the Auto-Loop button because I can't toggle the state like expected, so I solved the issue by copying the implementation of Pioneer-DDJ-SX-scripts.js.

Be-ing · 2021-09-19T14:29:17Z

Please rebase this on the 2.3 branch.

Swiftb0y · 2021-09-19T14:46:15Z

@toszlanyi what do you think?

flosse · 2021-09-19T17:24:09Z

Please rebase this on the 2.3 branch.

sure! Just force pushed the branch.

flosse · 2021-09-19T19:35:10Z

BTW:
I'm going to remove the code style issues in a separate commit soon.

Swiftb0y · 2021-09-19T21:56:00Z

We usually squash-merge mapping changes anyways.

flosse · 2021-09-20T01:19:46Z

I don't understand the script linting... first it tells me to replace var with const and now it's mucking around with The keyword 'const' is reserved 👎

Be-ing · 2021-09-20T01:36:55Z

I don't understand the script linting... first it tells me to replace var with const and now it's mucking around with The keyword 'const' is reserved -1

:/ please ignore that one. That's eslint being overzealous. The JavaScript interpreter in 2.3 doesn't support const, though that will be fixed in Mixxx 2.4.

flosse · 2021-09-20T02:19:13Z

:/ please ignore that one.

Ok, I see.. I'm sorry for the massive github action runs but I can't cancel them and the only chance for me was to see what the linter tells me on each iteration.

Swiftb0y · 2021-09-20T08:49:43Z

I don't understand the script linting... first it tells me to replace var with const and now it's mucking around with The keyword 'const' is reserved

I'm sorry, this is a temporary issue. We have upgraded the javascript environment in 2.4 and with that, new linter rules apply as well. So when you base a branch on main (so 2.4) the ES7 linter rules will get applied (suggesting to replace var with const). However, that makes the script incompatible with 2.3. So make sure you are basing your work on the correct branch.

flosse · 2021-09-20T10:48:34Z

I'm sorry, this is a temporary issue.

no problem, i was just confused ;-)

So make sure you are basing your work on the correct branch.

OK 👍 so then I'll try to make it work with 2.3.

Besides of this I just wonder if the new behavior suits for most users or should I also introduce a boolean flag to keep the old behavior?

Swiftb0y · 2021-09-21T10:38:49Z

Besides of this I just wonder if the new behavior suits for most users or should I also introduce a boolean flag to keep the old behavior?

I don't think thats necessary. But I'd like to hear @toszlanyi's opinion on this

flosse · 2021-09-21T13:27:14Z

I don't think thats necessary. But I'd like to hear @toszlanyi's opinion on this

ok... yesterday I had a unexpected behavior with my current (local) script, I don't know if it was me or the implementation logic. Nevertheless I expect to make the final and manually tested PR on Friday, because I need it for a gig on Saturday 😄

toszlanyi · 2021-09-22T09:22:01Z

+// Auto-Loop Button
+MC7000.autoLoop = function(channel, control, value, status, group) {
+    if (engine.getValue(group, "loop_enabled")) {
+        script.toggleControl(group, "reloop_toggle");
+    } else {
+        script.toggleControl(group, "beatloop_activate");
+    }
+};
+


Looks reasonable to me. Just wanna check what happens if a loop is set but inactive. Does it trigger a new loop at play position or reloop the existing one... Can do that by Thursday

I like that little improvement but the first push on Autoloop button activates and deactivates the loop immediately so you have to push a 2nd time. Once a loop is set then it works as expected.

as all var were replaced with let then the script doesnt work in Mixxx 2.3 any longer. I have therefore quickly changed my one and could push this to #4021 if you like. Or we let this one still move.

I couldn't get beatloop_activate to work properly if there was no loop set in the track yet. Anyhow beatloop_4_activate works to set a new loop and activate it. Is this a bug or what else do we miss here?

I found another issue ... Assumed there was a loop inside the track and I reloop it with Shift and Autoloop, go out the loop by pressing Autoloop then the loop position sometimes move to the current play position and activates. This is quite unpredictable - it doesn't happen always but now and then. Expecting the loop to stay where it was but not toggled on to current play position as I did only wanted to exit the Reloop.

I am using a much more complex logic for this purpose:

mixxx/res/controllers/Denon-MC6000MK2-scripts.js

Line 948 in de1853c

DenonMC6000MK2.OldDeck.prototype.onAutoLoopButton = function(isButtonPressed) {

Don't ask me why I came up with this, I don't remember.

We need a domain specific API to unify this behavior for multiple controllers. Manipulating individual control values is too low level. Needing to write a custom function hasLoop() function only to determine if a valid loop range is defined clearly indicates this conceptual gap.

hm...it still does not what I'd expect.
I think I have to read the API docs more accurate to understand the difference behaviors between
loop_*, reloop_* and beatloop_*

So what would be the workflow that you expect?

The method script.toggleControl does not work for me.
At least the last commit (c43ac92) is working at least for 90% of my usecases:
Create and activate a beatloop or disable a loop.

That commit seems to have some significant flaws in it (see eslint output). Also make sure you are using the correct Control Objects...

script.toggleControl(group, "reloop_toggle", 100);

The third parameter (100) doesn't do anything.

Mmhhhh interesting - got this line from the Mixxx Controls page inside the manual. See last example in that linked chapter.

uklotzde · 2021-09-24T07:52:26Z

+            engine.setValue(group, "loop_enabled", false);
+            engine.setValue(group, "reloop_toggle", true);
+        } else {
+            engine.setValue(group, "beatloop_activate", true);


This invocation sequence requires a comment, i.e. why you need a negative edge trigger. Does this even work as intended?

This comment is imprecise and wrong. script.toggleControl() works as expected and inverts a boolean value, i.e. 0/false -> 1/true or 1/true -> 0/false. Setting beatloop_activate to true and then to false is a completely different use case.

How about setting beatloop_activate to true on button down and to false otherwise (button up or when entering the other if arm on button down)?

I am using this pattern a lot in the MC6000MK2 script to avoid getting stuck in inconsistent states, e.g. when a button is pressed shifted and released unshifted or the other way round. All control values that might be affected by a button and all invocation sequences must be considered.

uklotzde · 2021-09-28T09:10:09Z

+            engine.setValue(group, "loop_enabled", false);
+            engine.setValue(group, "reloop_toggle", true);
+        } else {
+            engine.setValue(group, "beatloop_activate", true);


This comment is imprecise and wrong. script.toggleControl() works as expected and inverts a boolean value, i.e. 0/false -> 1/true or 1/true -> 0/false. Setting beatloop_activate to true and then to false is a completely different use case.

How about setting beatloop_activate to true on button down and to false otherwise (button up or when entering the other if arm on button down)?

I am using this pattern a lot in the MC6000MK2 script to avoid getting stuck in inconsistent states, e.g. when a button is pressed shifted and released unshifted or the other way round. All control values that might be affected by a button and all invocation sequences must be considered.

Swiftb0y · 2021-11-28T20:15:36Z

        }
    } else if (MC7000.PADModeCueLoop[deckNumber]) {
-        return;
+        // TODO


Please revert these unrelated changes.

toszlanyi · 2021-11-30T08:03:18Z

Suggest to close this PR as the desired function was implemented globally with #4328

github-actions · 2022-04-16T00:07:27Z

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

flosse · 2022-04-20T11:50:19Z

I use CDJs now, so this PR is no longer relevant to me 😜

github-actions Bot added the controller mappings label Sep 19, 2021

Be-ing changed the base branch from main to 2.3 September 19, 2021 14:27

flosse force-pushed the denon-mc7000-autoloop branch from 8345ec1 to 4af3e95 Compare September 19, 2021 17:23

Swiftb0y requested changes Sep 19, 2021

View reviewed changes

Comment thread res/controllers/Denon-MC7000-scripts.js Outdated

flosse force-pushed the denon-mc7000-autoloop branch from 4af3e95 to faaa21b Compare September 19, 2021 17:39

flosse mentioned this pull request Sep 19, 2021

[WIP]: Denon mc7000 parameter buttons #4310

Closed

flosse changed the title ~~Denon MC7000: Handle autoloop button in script~~ [WIP]: Denon MC7000: Handle autoloop button in script Sep 19, 2021

flosse force-pushed the denon-mc7000-autoloop branch from faaa21b to eadce46 Compare September 20, 2021 01:07

flosse changed the title ~~[WIP]: Denon MC7000: Handle autoloop button in script~~ Denon MC7000: Handle autoloop button in script Sep 20, 2021

flosse force-pushed the denon-mc7000-autoloop branch from eadce46 to a3dc464 Compare September 20, 2021 01:11

flosse force-pushed the denon-mc7000-autoloop branch from a3dc464 to ee2a128 Compare September 20, 2021 01:27

flosse force-pushed the denon-mc7000-autoloop branch 3 times, most recently from 0d5b295 to f593ac1 Compare September 20, 2021 02:09

flosse changed the title ~~Denon MC7000: Handle autoloop button in script~~ [WIP]: Denon MC7000: Handle autoloop button in script Sep 20, 2021

toszlanyi reviewed Sep 22, 2021

View reviewed changes

flosse force-pushed the denon-mc7000-autoloop branch from f593ac1 to c43ac92 Compare September 23, 2021 14:54

flosse requested a review from Swiftb0y September 23, 2021 14:59

flosse requested a review from uklotzde September 23, 2021 14:59

flosse force-pushed the denon-mc7000-autoloop branch from c43ac92 to d7bd32d Compare September 23, 2021 23:26

flosse changed the title ~~[WIP]: Denon MC7000: Handle autoloop button in script~~ Denon MC7000: Handle autoloop button in script Sep 23, 2021

flosse force-pushed the denon-mc7000-autoloop branch from d7bd32d to 990f3f5 Compare September 24, 2021 00:17

toszlanyi reviewed Sep 24, 2021

View reviewed changes

Comment thread res/controllers/Denon-MC7000-scripts.js

uklotzde reviewed Sep 24, 2021

View reviewed changes

Denon MC7000: Handle autoloop button in script

867f9b2

flosse force-pushed the denon-mc7000-autoloop branch from 990f3f5 to 867f9b2 Compare September 27, 2021 14:42

Be-ing mentioned this pull request Sep 27, 2021

Changed beatloop_activate button behaviour to match lp1876003 #4328

Merged

uklotzde suggested changes Sep 28, 2021

View reviewed changes

ronso0 added this to the 2.3.2 milestone Oct 7, 2021

Swiftb0y requested changes Nov 28, 2021

View reviewed changes

daschuer removed this from the 2.3.2 milestone Jan 15, 2022

github-actions Bot added the stale Stale issues that haven't been updated for a long time. label Apr 16, 2022

flosse closed this Apr 20, 2022

Uh oh!

Conversation

flosse commented Sep 19, 2021

Uh oh!

Be-ing commented Sep 19, 2021

Uh oh!

Swiftb0y commented Sep 19, 2021

Uh oh!

flosse commented Sep 19, 2021

Uh oh!

Uh oh!

flosse commented Sep 19, 2021

Uh oh!

Swiftb0y commented Sep 19, 2021

Uh oh!

flosse commented Sep 20, 2021

Uh oh!

Be-ing commented Sep 20, 2021

Uh oh!

flosse commented Sep 20, 2021

Uh oh!

Swiftb0y commented Sep 20, 2021

Uh oh!

flosse commented Sep 20, 2021

Uh oh!

Swiftb0y commented Sep 21, 2021

Uh oh!

flosse commented Sep 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toszlanyi Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uklotzde Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toszlanyi commented Nov 30, 2021

Uh oh!

github-actions Bot commented Apr 16, 2022

Uh oh!

flosse commented Apr 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

toszlanyi Sep 23, 2021 •

edited

Loading

uklotzde Sep 23, 2021 •

edited

Loading