Skip to content

Conversation

@ferranpujolcamins
Copy link
Contributor

This small PR should be quick to review and improves a little the user experience with AutoPan.
Change list:

  1. Reverse smoothing parameter (https://bugs.launchpad.net/mixxx/+bug/1489907)

  2. Moves width parameter to the first place.
    This makes AutoPan consistent with echo and flanger where the first knob is the principal responsible of the effect's perceived intensity.

  3. Changes the default parameter values to something that sounds cool (IMHO).

Now parameter=0 -> 0-smoothing.
This make AutoPan consistent with echo and flanger where the first knob is the principal responsible of the effect perceived intensity.
@jclaveau
Copy link

jclaveau commented Oct 8, 2015

Hi Ferran,
Thanks for giving a look at this effect. I'm glad if it interests you.

  1. Reverse smoothing parameter (https://bugs.launchpad.net/mixxx/+bug/1489907)

If you think so, I chose the "smoother by default" version first to have an effect that changes the output as few as possible. However, a feeling from someone having mixed with it gives probably a better point of view.

  1. Moves width parameter to the first place.
    This makes AutoPan consistent with echo and flanger where the first knob is the principal responsible of the effect's perceived intensity.

=> 100% agreed, thanks for the notice.

  1. Changes the default parameter values to something that sounds cool (IMHO).

Same as 1) so : follow your heart, my head told me make the more unobtrusive effect when I implemented it :)

There is something else I never did with this effect : enable the sync by default. I think this it is really better with. More, presently the sync toggler is not visible in LateNight so sync can't be enabled on it.

@jclaveau
Copy link

jclaveau commented Oct 8, 2015

Something more : I forgot to delete the buffer and the delay here.
https://github.com/ferranpujolcamins/mixxx/blob/Some-love-to-AutoPan/src/effects/native/autopaneffect.h#L72-L74

Would you please add it to this PR?

@Be-ing
Copy link
Contributor

Be-ing commented Oct 8, 2015

I think this should be targeted at 1.12 considering the small scope of the changes and that it contributes to fixing https://bugs.launchpad.net/mixxx/+bug/1296089

@jclaveau
Copy link

jclaveau commented Oct 8, 2015

@Be-ing AutoPan is not targeted for 1.12 actually (even if I'd love it would). The Phaser too :
http://www.mixxx.org/forums/viewtopic.php?f=1&t=5062&p=24766&hilit=autopan#p24766

@ferranpujolcamins
Copy link
Contributor Author

  1. Reverse smoothing parameter (https://bugs.launchpad.net/mixxx/+bug/1489907)

This is just a semantic issue: when the knob called "smoothing" was at zero, the smoothing was on. It feels more coherent if the smoothing is on when the knob called "smoothing" is at its maximum position.

Another thing is which default value we give to that parameter.

Same as 1) so : follow your heart, my head told me make the more unobtrusive effect when I implemented it :)

I chose the default values so they affect obviously the sound because of this bug: effects need defaults that sound like an effect

There is something else I never did with this effect : enable the sync by default. I think this it is really better with. More, presently the sync toggler is not visible in LateNight so sync can't be enabled on it.

I have no idea how fx can collect tempo info yet, so I can't help right now. Furthermore, this was a quick PR.

Something more : I forgot to delete the buffer and the delay.
Done.

@jclaveau
Copy link

jclaveau commented Oct 8, 2015

I have no idea how fx can collect tempo info yet, so I can't help right now. Furthermore, this was a quick PR.

It's already done, the only thing to do is to change the default value of this parameter to 1 :
https://github.com/ferranpujolcamins/mixxx/blob/Some-love-to-AutoPan/src/effects/native/autopaneffect.cpp#L41-L51

Thanks for the free and for taking care of the compliance of this effect with the others. :)

@ferranpujolcamins
Copy link
Contributor Author

So the only remaining thing for this is to show a toggle paramater that enables it?

@daschuer
Copy link
Member

daschuer commented Oct 8, 2015

We are planning to release fixes in the master branch short after the 1.12 release.
Most important are the fixed mp3 seek and read issues in master.
These effect improvements will be automatically part of it.

@jclaveau
Copy link

jclaveau commented Oct 8, 2015

The parameter already exists but is shown only in Deere and Shade, not in LateNight.

So the only remaining thing is to change this line
https://github.com/ferranpujolcamins/mixxx/blob/Some-love-to-AutoPan/src/effects/native/autopaneffect.cpp#L49
To be like : periodUnit->setDefault(1);

And, as you said, choose a default value for the smoothing. I have one example in head, which could be a good base : I sometimes heard tracks finishing with an autopan having a quite strong smoothing and a period of one beat.

@jclaveau
Copy link

jclaveau commented Oct 8, 2015

@daschuer nice to read it :)

@rryan
Copy link
Member

rryan commented Dec 31, 2015

@ferranpujolcamins when you get a chance could you fix the conflicts? thanks!

Conflicts:
	src/effects/native/autopaneffect.cpp
@ferranpujolcamins
Copy link
Contributor Author

@rryan Done

rryan added a commit that referenced this pull request Jan 1, 2016
@rryan rryan merged commit 5f87b7a into mixxxdj:master Jan 1, 2016
@rryan
Copy link
Member

rryan commented Jan 1, 2016

thanks!

@ferranpujolcamins ferranpujolcamins deleted the Some-love-to-AutoPan branch January 1, 2016 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants