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

EQ Rack #330

Closed
wants to merge 90 commits into from
Closed

EQ Rack #330

wants to merge 90 commits into from

Conversation

badescunicu
Copy link
Contributor

For more details, check: http://www.mixxx.org/wiki/doku.php/eq_rack

This PR still suffers from https://bugs.launchpad.net/mixxx/+bug/1335823. To reproduce it, check "Show All Effects" checkbox several times.

… are changed, do ramping in the next process() call
@daschuer
Copy link
Member

daschuer commented Sep 7, 2014

Thank you for the pull request!
I cannot reproduce the Bug https://bugs.launchpad.net/mixxx/+bug/1335823 on Ubuntu 13.10 using this branch. Is is till an issue for you with the latest version of this branch and with current master?

m_highEqFreq(0.0),
m_pEffectsManager(pEffectsManager) {

// Initialize the EQ Effect Rack
Copy link
Member

Choose a reason for hiding this comment

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

// Get the EQ Effect Rack
It is already initialized

@daschuer
Copy link
Member

daschuer commented Sep 7, 2014

Thank you Nicu. IMHO this is a solid base for all advanced EQ stuff we may introduce.

I would really like to see this in 1.12. What do others think?
Two important things are missing, to have no regression compared to 1.11:
We need to add the kill buttons to the EQ effects and we need to merge a 4th order EQ.

m_pEffectsManager(pEffectsManager) {

// Get the EQ Effect Rack
m_pEQEffectRack = m_pEffectsManager->getEQEffectRack().data();
Copy link
Member

Choose a reason for hiding this comment

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

I think we should hold a strong reference to the EQEffectRack. This ensures that it is not detested early elsewhere.
You can user .data() inside the connect statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I do that, I get this: Warning [Main]: QObject: shared QObject was deleted directly. The program is malformed and may crash.

I've read about a similar issue here[1] but I'm not sure if our warning is caused by the same concept.

[1] - http://blog.codef00.com/2011/12/15/not-so-much-fun-with-qsharedpointer/

Copy link
Member

Choose a reason for hiding this comment

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

Ok, then it seam there is an underlying issue. Probably something to do with the memory leak you are facing in the open bug. Please leaf a comment here and we can issue that in a separate commit.

@daschuer
Copy link
Member

Thank you Nicu! I think this is now almost ready.

Anyone else likes to review?

Unfortunately we cannot merge it without regression, because it is blocked by the Button Parameter discussion, pending since 15 Jul :-(
@rryan : I really like to have this part of 1.12. but we need to have a conclusion on #281
Please outline a solution for the kill buttons, than we are able to discuss which solution suits best.
Thank you.

@kain88-de
Copy link
Member

still valid after the recent EQ rack PR from @daschuer ?

@daschuer
Copy link
Member

daschuer commented Nov 7, 2014

We can close this as well. My new PR is based on this. Thank you @badescunicu for all your work.

@daschuer daschuer closed this Nov 7, 2014
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