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

Hercules DJControl Jogvision: First release (all MIDI mapped) #2370

Merged
merged 35 commits into from
Jun 12, 2020

Conversation

perseo22
Copy link
Contributor

Hercules DJControl Jogvision fully (I hope!) mapped.

https://www.mixxx.org/wiki/doku.php/hercules_dj_control_jogvision

A job mainly from DJ Phatso, with contributions by Kerrick Staley and me, David TV (aka perseo22, aka hasses1.07)

res/controllers/Hercules_DJControl_Jogvision-scripts.js Outdated Show resolved Hide resolved
res/controllers/Hercules_DJControl_Jogvision-scripts.js Outdated Show resolved Hide resolved
res/controllers/Hercules_DJControl_Jogvision-scripts.js Outdated Show resolved Hide resolved
res/controllers/Hercules_DJControl_Jogvision.midi.xml Outdated Show resolved Hide resolved
res/controllers/Hercules_DJControl_Jogvision.midi.xml Outdated Show resolved Hide resolved
res/controllers/Hercules_DJControl_Jogvision.midi.xml Outdated Show resolved Hide resolved
@Holzhaus
Copy link
Member

Thanks for contibuting! Code looks mostly good to me except for a few small issues. I think removing redundancy could make the code drastically shorter.

Pinging @Be-ing, I'm sure he also wants to take a look at this.

@Be-ing
Copy link
Contributor

Be-ing commented Nov 26, 2019

I think removing redundancy could make the code drastically shorter.

Yes, the first thing that stands out about the code is that only half of it is needed. There's no need to have separate functions for identical features on different decks. You can determine the deck from the MIDI channel or from the "group" parameter passed to the JS function which you'd have to specify in the XML.

@perseo22
Copy link
Contributor Author

Hi all!
Working in all your comments!
Will try to upload a new commit very soon!

@perseo22
Copy link
Contributor Author

perseo22 commented Dec 2, 2019

Hi all!

I just pushed a commit with all the suggested changes :)

Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

Thanks! I'm not sure why the diff is so broken, did you change the line terminators (DOS/Unix)?

In any case, I reviewed the code and didn't notice major issues. Most of my suggestions are whitespace fixes or replace == with type-safe ===. I'm not sure why you mass-set values in the init method though.

res/controllers/Hercules_DJControl_Jogvision-scripts.js Outdated Show resolved Hide resolved
res/controllers/Hercules_DJControl_Jogvision-scripts.js Outdated Show resolved Hide resolved
res/controllers/Hercules_DJControl_Jogvision-scripts.js Outdated Show resolved Hide resolved
res/controllers/Hercules_DJControl_Jogvision-scripts.js Outdated Show resolved Hide resolved
res/controllers/Hercules_DJControl_Jogvision-scripts.js Outdated Show resolved Hide resolved
res/controllers/Hercules_DJControl_Jogvision-scripts.js Outdated Show resolved Hide resolved
res/controllers/Hercules_DJControl_Jogvision-scripts.js Outdated Show resolved Hide resolved
res/controllers/Hercules_DJControl_Jogvision-scripts.js Outdated Show resolved Hide resolved
res/controllers/Hercules_DJControl_Jogvision-scripts.js Outdated Show resolved Hide resolved
engine.setParameter("[EffectRack1_EffectUnit1]", "mix", 0.6);
engine.setParameter("[EffectRack1_EffectUnit2]", "mix", 0.6);
engine.setParameter("[QuickEffectRack1_[Channel1]]", "super1", 0.5);
engine.setParameter("[QuickEffectRack1_[Channel2]]", "super1", 0.5);
Copy link
Member

Choose a reason for hiding this comment

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

Why do you mass-set these values? I think that this would cause issues if Mixxx gets MIDI hotplug support in the future. If a user accidently disconnects the controller and then reconnects it, this could cause abrupt changes in the audio output.

Copy link
Contributor

Choose a reason for hiding this comment

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

This was done in the original script to "work around" the limited controls available on the Jogvision (which were built out of the bundle with Serato).

Since there is only one encoder that sends 3 midi signals to control the FX amount (meaning it would affect the level of all 3 effects simultaneously), I figured I could instead configure the fx rack as auto-FX for all 3 buttons and preset the levels to an appropriate amount for each of them.

The same goes for the motion sensor "Air FX", which goes back to 0 by default. In order to use if with the Filter, it needed to be set to the middle position, and it was decided that only the lower range would be used.

There is surely a more practical and elegant ways to do all this for sure, but my coding knowledge are still limited. Any suggestions are much welcomed.

@perseo22
Copy link
Contributor Author

Hi!
A new push has been made taking into account all your suggestions ;)
Thanks!

@perseo22
Copy link
Contributor Author

perseo22 commented Jan 3, 2020

Happy new year to all!!
I just pushed what I think could be the very final commit, with a couple of regression bugs fixed (VuMeter and head CUE/MIX button).
Beyond that, I don't know what else I could/should do.

Thanks a lot!!!!!

@DJPhatso
Copy link
Contributor

DJPhatso commented Jan 3, 2020

Happy New year David!

Just tried the new update, and I have to say it works quite well.

The only two little things I've noticed are related to the Beat LED:

  • the LED will only move forward when using the jogwheel;
  • when the two decks are synced, the two LED are not "linked" to the same step as they are with other DJ software.

Not sure if this is a limitation of the current Mixxx state or something that can be modified in your code.

@perseo22
Copy link
Contributor Author

perseo22 commented Jan 9, 2020 via email

@DJPhatso
Copy link
Contributor

DJPhatso commented Jan 9, 2020

Think you miss read my comment. Forward is fine. Moving backward when using the jogwheel is the problem :-)

As for the linked state, I guess we'll have to see what the Mixxx guys have to say about this. Since this is not a common feature, maybe their code just doesn't take LED feedback into consideration.

@perseo22
Copy link
Contributor Author

perseo22 commented Jan 9, 2020 via email

Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

Apart from a few code-style issues this looks good to me.

res/controllers/Hercules_DJControl_Jogvision-scripts.js Outdated Show resolved Hide resolved
res/controllers/Hercules_DJControl_Jogvision-scripts.js Outdated Show resolved Hide resolved
res/controllers/Hercules_DJControl_Jogvision-scripts.js Outdated Show resolved Hide resolved
res/controllers/Hercules_DJControl_Jogvision-scripts.js Outdated Show resolved Hide resolved
res/controllers/Hercules_DJControl_Jogvision-scripts.js Outdated Show resolved Hide resolved
res/controllers/Hercules_DJControl_Jogvision-scripts.js Outdated Show resolved Hide resolved
res/controllers/Hercules_DJControl_Jogvision-scripts.js Outdated Show resolved Hide resolved
res/controllers/Hercules_DJControl_Jogvision-scripts.js Outdated Show resolved Hide resolved
res/controllers/Hercules_DJControl_Jogvision-scripts.js Outdated Show resolved Hide resolved
res/controllers/Hercules_DJControl_Jogvision-scripts.js Outdated Show resolved Hide resolved
@Holzhaus
Copy link
Member

By the way, does this mapping use any features that aren't in 2.2? I didn't see any, so I'd suggest you rebase this on 2.2 and target that branch instead. That will allow it to go into 2.2.4.

@perseo22
Copy link
Contributor Author

Hi!
I did the suggested changes, commited, pushed, rebased against 2.2 and pushed again.

Hope you find it correct! (my knowledge on git is pretty low... :( )

Thanks!!!!
-David.

@Holzhaus
Copy link
Member

Holzhaus commented Jan 16, 2020

I think your rebased commits were rejected, you need to use the -f flag when pushing.

@Holzhaus
Copy link
Member

I think your rebased commits were rejected, you need to use the -f flag when pushing.

...or you did you forget to remove all lines containing commits that are unrelated to this PR during rebase?

@perseo22
Copy link
Contributor Author

perseo22 commented Jan 16, 2020 via email

@perseo22
Copy link
Contributor Author

perseo22 commented Jan 16, 2020 via email

@Holzhaus
Copy link
Member

Holzhaus commented Jan 16, 2020

# First, create a backup in case something goes wrong:
$ git branch -c my-backup-branch

# Next, start an interactive rebase
$ git rebase -i 2.2

Now, a text editor opens. Delete all lines that do not belong to your PR, so that the file only contains these lines:

pick af95a70e68 Hercules DJControl Jogvision: First release (all MIDI mapped)
pick 9261e962a0 Hercules DJControl Jogvision: reduced and cleaned the size of the Javascript code
pick 940ef3fc09 Hercules DJControl Jogvision: code cleanup and added orange color to track led
pick 15ec1efaeb CONTROLLER Hercules_DJControl_Jovision: - Fixed regression not updating the VuMeters - Fixed regression not updating the CUE/MIX buttons
pick dff8912811 Controller - Hercules DJControl Jogvision: - Minor code-style changes (use caleCase, use "double quotes" and other if/else styles)
pick d9dc0f3d86 Controller: Hercules DJControl Jogvision some more log trace at shutdown

Then save the file and close the editor.

Now, push the changes to the repository:

$ git push -f origin Controller_Hercules_DJControl_Jogvision

@perseo22
Copy link
Contributor Author

perseo22 commented Jan 16, 2020 via email

@Holzhaus
Copy link
Member

@perseo22 Ok, your git version is almost 4 years old, so the first command is slightly different:

# Create backup branch
$ git checkout -b my-backup-branch

# Switch back to original branch
$ git checkout Controller_Hercules_DJControl_Jogvision

Also, it looks like you added multiple remotes to you repository, so that git doesn't know if it should use the git branch 2.2 from the origin remote or another one.
You can check which remotes are configured with:

$ git remote -v

Assuming that you have a remote origin, you can rebase via:

$ git rebase -i origin/2.2

David TV added 6 commits January 16, 2020 13:53
- Fixed regression not updating the VuMeters
- Fixed regression not updating the CUE/MIX buttons
- Minor code-style changes (use caleCase, use "double quotes" and other if/else styles)
some more log trace at shutdown
Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

Ok, this looks mostly good. It's a bit unfortunate that this changes effect settings in init() because you might have multiple controllers attached, but maybe this provides better UX for users that would otherwise wonder why the effects don't work properly. Not sure.

@Be-ing Any comments?

res/controllers/Hercules_DJControl_Jogvision-scripts.js Outdated Show resolved Hide resolved
engine.stopTimer(ledRotationTimer);
ledRotationTimer = 0;
}
//Set all LED states to off
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
//Set all LED states to off
// Set all LED states to off

David TV added 6 commits May 11, 2020 09:10
and added beatNax variable declaration at the beginning
- Added jogwheel outer led movement when scratch and back/forward spin
Fixed minor code standards
Fixed an ancient bug using "beatjumpSize" instead of "beatjump_size"
(shame on me...)
Modified the outer jog led (spin) leds to use a better algorithm (borrowed from the 'Pioneer-DDJ-SX-scripts.js' mapping)
- Changed jogwheel outer led movement based on playposition (much better!)
- Added a user variable to set or not some effects at ini
- Minor code convention corrections
@perseo22
Copy link
Contributor Author

Hi all!
Just pushed a new version, with minor code changes, and a variable to allow the user decide if initialize or not some effects at startup.

…rd) and fix leads positions,

what allows backward led activation!! (didn't have that working previously)
@perseo22
Copy link
Contributor Author

Hi!
Another push!

  • Changed beats led algorithm to match song beats (forward and backward) and fix leads positions, what allows backward led activation!! (didn't have that working previously)

@perseo22
Copy link
Contributor Author

perseo22 commented May 19, 2020

Happy New year David!

Just tried the new update, and I have to say it works quite well.

The only two little things I've noticed are related to the Beat LED:

* the LED will only move forward when using the jogwheel;

* when the two decks are synced, the two LED are not "linked" to the same step as they are with other DJ software.

Not sure if this is a limitation of the current Mixxx state or something that can be modified in your code.

Point number 1 (the LED will only move forward when using the jogwheel) solved in v. 1.11 ;)

David TV added 6 commits May 19, 2020 15:36
v1.12
Added "alternate" beat leds mode
Changed "follow" beats led algorithm with a better/more elegant version
Cleanup (deletion of unused function DJCJV.beatInactive)
Added unused button combo "shift-<MULTI FX>" to do a 'beats_translate_curpos' (align
deck's beat_grid to current playposition)
Added "shift"+AIRFX to do a high pass (apart from the default low pass)
Fixed a variables being declared but never used
added better beat_active matching
@perseo22
Copy link
Contributor Author

perseo22 commented Jun 2, 2020

Hi @Holzhaus , everything ok?
Is there anything else I can do?
Thanks!

Reset shift-"Loop ON", to do what Hercules wanted it to be: select FX2 for given channel. Now,
the loop creation is done with "MODE"+"Loop ON" combo (here, MODE key works like "SHIFT" key ;)
Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

Added a "beatDetection" variable to use old school one (way better beat match, but no song position relative)
@uklotzde uklotzde modified the milestones: 2.2.4, 2.2.5, 2.2.x Jun 12, 2020
@Holzhaus Holzhaus merged commit c768ef3 into mixxxdj:2.2 Jun 12, 2020
@perseo22
Copy link
Contributor Author

Hi @Holzhaus !

Could you please take a look at the latest push I did to this mapping?
I sincerely think it's much better coded (prettier, and simpler), and adds a couple of nice things!

Thanks a lot in advance!!

@Holzhaus
Copy link
Member

Please open a new Pull Request, then we can do a proper review.

@uklotzde uklotzde modified the milestones: 2.2.5, 2.3.0 May 11, 2021
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.

5 participants