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

[WIP] Effects Refactoring #1705

Closed
wants to merge 47 commits into from

Conversation

kshitij98
Copy link
Contributor

@kshitij98 kshitij98 commented Jun 7, 2018

The pull request is at a very initial stage and a lot of work still needs to be done on it.

The current scope of this PR is:

  • Consolidate EffectChain and EffectChainSlot class
  • Remove EffectChainManager
  • Remove EngineEffectRack
  • Remove EffectRack
  • Consolidate Effect/EffectSlot
  • Remove unnecessary code from EffectParameter
  • Implement parameter hiding and rearrangement (skip implementing GUI for now)
  • Reimplement saving/loading from XML with new EffectChainPreset & EffectPreset classes.

There is some confusion with the current naming of classes. The newly proposed class names are:

  • EffectState -> EffectProcessorState
  • EffectChain/EffectChainSlot -> EffectRack
  • EngineEffectChain -> EngineEffectRack (loaded using EffectRackPreset)
  • Effect/EffectSlot -> EffectSlot
  • EngineEffect -> EngineEffectSlot (loaded using EffectPreset)
  • EffectProcessor -> EffectProcessor

NOTE: XML loading and saving has been commented out and will be re-implemented with the new Preset classes

Copy link
Contributor

@Be-ing Be-ing left a comment

Choose a reason for hiding this comment

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

This looks like a decent start. Untangling the mess from the old architecture won't happen all at once, so I don't expect you to take care of everything I am pointing out now in the next commit.

In general, wherever the old EffectChainSlot class called a function of m_pEffectChain, remove the old function of the EffectChain class and move its contents to the function of the old EffectChainSlot class where it was called.

src/effects/effectchainslot.cpp Outdated Show resolved Hide resolved
src/effects/effectchainslot.cpp Outdated Show resolved Hide resolved
src/effects/effectchainslot.cpp Outdated Show resolved Hide resolved
src/effects/effectchainslot.cpp Outdated Show resolved Hide resolved
src/effects/effectchainslot.cpp Outdated Show resolved Hide resolved
src/effects/effectchainslot.cpp Outdated Show resolved Hide resolved
src/effects/effectchainslot.cpp Outdated Show resolved Hide resolved
src/effects/effectchainslot.cpp Outdated Show resolved Hide resolved
src/effects/effectchainslot.cpp Outdated Show resolved Hide resolved
src/effects/effectchainslot.cpp Outdated Show resolved Hide resolved
src/effects/effectchainslot.cpp Outdated Show resolved Hide resolved
src/effects/defs.h Outdated Show resolved Hide resolved
src/effects/effect.cpp Outdated Show resolved Hide resolved
src/effects/effectchainslot.cpp Outdated Show resolved Hide resolved
src/effects/effectchainslot.cpp Outdated Show resolved Hide resolved
src/engine/effects/engineeffect.cpp Outdated Show resolved Hide resolved
src/effects/effectrack.cpp Outdated Show resolved Hide resolved
src/effects/effectchainslot.cpp Outdated Show resolved Hide resolved
src/effects/effectchainslot.cpp Outdated Show resolved Hide resolved
src/effects/effectchainslot.cpp Outdated Show resolved Hide resolved
src/effects/effectchainslot.h Outdated Show resolved Hide resolved
src/effects/effectchainslot.h Outdated Show resolved Hide resolved
src/effects/effectrack.cpp Outdated Show resolved Hide resolved
src/effects/effectrack.cpp Outdated Show resolved Hide resolved
src/effects/effectrack.cpp Outdated Show resolved Hide resolved
src/effects/effectrack.cpp Outdated Show resolved Hide resolved
@Be-ing
Copy link
Contributor

Be-ing commented Jun 8, 2018

Please make your next commit simply removing commented out and outdated code. Then it will be easier to review the code you actually change in following commits.

src/effects/effectrack.cpp Outdated Show resolved Hide resolved
@Be-ing
Copy link
Contributor

Be-ing commented Jun 9, 2018

I am repeating the plan we came up with in our private Zulip chat:

  1. Finish simplifying EffectChainSlot
  2. Remove EffectChainManager
  3. Remove EffectRack/EngineEffectRack
  4. Consolidate Effect/EffectSlot
  5. Reimplement saving/loading from XML with new EffectChainPreset & EffectPreset classes.

@daschuer
Copy link
Member

I think we will need the EffectRack layer.

@Be-ing
Copy link
Contributor

Be-ing commented Jun 10, 2018

Why? It does nothing but overcomplicate the code. Everything done at the effect rack layer can and should be moved down to the effect chain layer. The superfluous rack layer requires breaking the abstraction of the chain layer.

@kshitij98 kshitij98 force-pushed the effects_refactoring branch from b11322c to ad9c680 Compare June 10, 2018 15:32
@Be-ing
Copy link
Contributor

Be-ing commented Jun 10, 2018

@kshitij98 did you change the email you configured to be associated with your Git commits? The new commits are showing as being from a different GitHub user.

@kshitij98
Copy link
Contributor Author

kshitij98 commented Jun 10, 2018

@kshitij98 did you change the email you configured to be associated with your Git commits? The new commits are showing as being from a different GitHub user.

My laptop was not working fine yesterday so I had to use a friend's pc for the same. I forgot to change the config setting of git. Should I reset the author of the previous commits?

@Be-ing
Copy link
Contributor

Be-ing commented Jun 10, 2018

When you're done consolidating EffectChain/EffectChainSlot, rebase and squash your work down to one commit to remove the temporary intermediate code from the Git history. At that time you can remove the other email associated with the new commits. For now let's just keep going.

src/preferences/dialog/dlgprefeq.cpp Outdated Show resolved Hide resolved
src/engine/effects/engineeffectchain.cpp Show resolved Hide resolved
src/engine/effects/engineeffectchain.cpp Outdated Show resolved Hide resolved
src/engine/effects/engineeffect.cpp Outdated Show resolved Hide resolved
src/effects/effectrack.cpp Outdated Show resolved Hide resolved
src/effects/effectchainslot.cpp Outdated Show resolved Hide resolved
src/effects/effectchainslot.cpp Outdated Show resolved Hide resolved
src/effects/effectchainslot.cpp Outdated Show resolved Hide resolved
src/effects/effectchainslot.cpp Outdated Show resolved Hide resolved
src/effects/effectchainslot.cpp Show resolved Hide resolved
@kshitij98 kshitij98 force-pushed the effects_refactoring branch from 1da9dc2 to 5372b09 Compare June 10, 2018 23:12
src/effects/effectchainslot.cpp Outdated Show resolved Hide resolved
src/effects/effectchainslot.cpp Outdated Show resolved Hide resolved
@kshitij98 kshitij98 force-pushed the effects_refactoring branch from fa08ebc to b6cdbfa Compare June 12, 2018 07:54
src/effects/effectchainslot.cpp Outdated Show resolved Hide resolved
src/effects/effectchainslot.cpp Outdated Show resolved Hide resolved
@Be-ing
Copy link
Contributor

Be-ing commented Jun 12, 2018

The regular effects are not being applied if they are enabled for an input channel on startup. After toggling the routing switch they work. I am trying to debug this...

@Be-ing
Copy link
Contributor

Be-ing commented Jun 12, 2018

I have sent you a pull request to fix the initialization process.

src/effects/effectchainslot.cpp Outdated Show resolved Hide resolved
src/effects/effectchainslot.cpp Outdated Show resolved Hide resolved
src/effects/effectchainslot.cpp Outdated Show resolved Hide resolved
src/effects/effectchainslot.h Outdated Show resolved Hide resolved
src/effects/effectchainslot.h Outdated Show resolved Hide resolved
src/effects/effectchainslot.h Outdated Show resolved Hide resolved
src/effects/effectrack.cpp Outdated Show resolved Hide resolved
src/effects/effectchainslot.h Show resolved Hide resolved
src/effects/effectchainslot.h Outdated Show resolved Hide resolved
src/effects/effectchainslot.h Outdated Show resolved Hide resolved
@Be-ing
Copy link
Contributor

Be-ing commented Jun 12, 2018

EffectChainSlot is down to about 600 lines of code :D

@Be-ing
Copy link
Contributor

Be-ing commented Jun 12, 2018

I am thinking we should rename EffectChainSlot to EffectChain now. What do you think?

@kshitij98
Copy link
Contributor Author

Please write more descriptive commit messages. Even something as simple as "rename some variables in EffectSlot" is better.

I have edited the previous commit messages

@Be-ing
Copy link
Contributor

Be-ing commented Dec 29, 2018

kshitij98#10

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.

Unfortunately this PR is still blocked by the open discussion about the future CO namespace. We are talking here about 8000 alias COs.

I understand the reasons for the renaming, but we should pretty sure that this reason outweighs all the related work and that the new namespace helps us for future changes. Later renaming because of new features will be harder due to the created naming legacy.

I think both is doubtful, and the old name space is well considered and not that bad. I also do not like the idea of cycling the names, this will create even more confusion. Unit and Rack means here totally different things. A perfect recipe for confusion.

If it turns out I am the only one with this opinion we can do the change, otherwise lets postpone the renaming and revert this PR to the existing nomenclature.

for (int deck = 1; deck <= iNumDecks; ++deck) {
QMenu* deckMenu = addSubmenu(QString("Deck %1").arg(deck), eqMenu);
for (int effect = kMaxEqs - 1; effect >= 0; --effect) {
const QString group = EqualizerEffectChainSlot::formatEffectSlotGroup(
Copy link
Member

Choose a reason for hiding this comment

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

What is EqualizerEffectChainSlot? This is not Part of the wiki description.

Copy link
Contributor

Choose a reason for hiding this comment

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

We have not done the renaming yet. EqualizerEffectChainSlot is what the wiki describes as EqualizerEffectRack.

src/effects/effectslot.cpp Outdated Show resolved Hide resolved
src/effects/effectslot.cpp Show resolved Hide resolved
src/effects/effectslot.cpp Show resolved Hide resolved
@daschuer
Copy link
Member

So the current state here is that the Rack classes where removed, but the CO namespace is still unchanged? If we stick with this for a first step, it will work for me.

@Be-ing
Copy link
Contributor

Be-ing commented Dec 31, 2018

Yes, that is the current state of the code.

#include "engine/effects/engineeffectchain.h"
#include "util/assert.h"

namespace {
const QString kStandardEffectRackGroup = "[EffectRack1]";
Copy link
Member

Choose a reason for hiding this comment

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

unused?

#include "engine/effects/engineeffectchain.h"
#include "util/assert.h"

namespace {
const QString kStandardEffectRackGroup = "[EffectRack1]";
const QString kOutputEffectRackGroup = "[OutputEffectRack]";
Copy link
Member

Choose a reason for hiding this comment

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

unused?

#include "engine/effects/engineeffectchain.h"
#include "util/assert.h"

namespace {
const QString kStandardEffectRackGroup = "[EffectRack1]";
const QString kOutputEffectRackGroup = "[OutputEffectRack]";
const QString kQuickEffectRackGroup = "[QuickEffectRack1]";
Copy link
Member

Choose a reason for hiding this comment

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

unused?

const QString kStandardEffectRackGroup = "[EffectRack1]";
const QString kOutputEffectRackGroup = "[OutputEffectRack]";
const QString kQuickEffectRackGroup = "[QuickEffectRack1]";
const QString kEqualizerEffectRackGroup = "[EqualizerRack1]";
Copy link
Member

Choose a reason for hiding this comment

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

unused

const QString kStandardEffectRackGroup = "[EffectRack1]";
const QString kOutputEffectRackGroup = "[OutputEffectRack]";
const QString kQuickEffectRackGroup = "[QuickEffectRack1]";
const QString kEqualizerEffectRackGroup = "[EqualizerRack1]";
const QString kEffectGroupSeparator = "_";
const QString kGroupClose = "]";
Copy link
Member

Choose a reason for hiding this comment

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

kGroupOpen is missing. We use [ and ] everywhere as characters in the code. Imho this one clutters the code more than it helps, can we just remove it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be in favor of removing it.

@Be-ing Be-ing modified the milestones: 2.3.0, 2.4.0 Feb 7, 2019
Copy link
Contributor

@Be-ing Be-ing left a comment

Choose a reason for hiding this comment

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

This is off to a good start!

src/effects/effectpreset.cpp Outdated Show resolved Hide resolved
src/effects/effectchainpreset.h Outdated Show resolved Hide resolved
src/effects/effectchainpreset.h Outdated Show resolved Hide resolved
src/effects/effectxmlelements.h Outdated Show resolved Hide resolved
- Added EffectChainPreset class
- Added EffectPreset class
- Added EffectsManager::loadEffectChainPresets
file.close();

QDomElement root = doc.documentElement();
QDomElement rackElement = XmlParse::selectElement(root, EffectXml::Rack);
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment explaining that this element is superfluous but it is kept for backwards compatibility.

@Be-ing
Copy link
Contributor

Be-ing commented Apr 1, 2020

continued here: #2618

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants