-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Add RemoveDCOffset effect #5917
base: master
Are you sure you want to change the base?
Conversation
This is not a complete code review, but in fact -- the .h file is not needed at all. You can just define the class locally in the .cpp file. More than that -- I would also wrap the whole thing in an anonymous namespace, and save a little bit of space in the symbol tables that the linker has to cope with. Audacity wasn't originally written this way back in the late twentieth century, but nowadays, static registration objects do a lot of magic to push things into registries (of effects, macro commands, whatever) allowing scattered definitions of details, and making it unnecessary for those details to expose a header file. |
That's in resources/EffectsMenuDefaults.xml, probably the best category for this effect would be the "noise removal and repair" category. |
auto pOffset = offsets.begin(); | ||
auto pExtent = extents.begin(); | ||
for (const auto channel : channels) { | ||
const auto extent = oneChannel ? *pExtent++: maxExtent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Patrick, thanks for your contribution!
It looks like you don't use extent
, in the end.
I will have to research anonymous namespaces, and I'll try to set that up. May not be able to make it work because I don't fully understand what goes on behind the scenes.
Okay, added the effect to that category. I'll test it a bit more and see if everything works.
That's just a remnant of the Normalize feature that I didn't understand the use of. I'll scan through the program again to get a better understanding and remove unnecessary code. Edit: For some reason, I cannot get the effect to categorize correctly. Also, once I tried the anonymous namespace tip I kept getting errors, so I feel that would be safer to let someone with more experience handle. |
Anonymous namespace just means this -- add the line |
If/when this PR gets pulled for this new effect, are you planning to remove the DC offset removal from the Normalize effect or just leave it in there? |
hey @PatrickKallenbach, haven't heard from you in a while. Are you still interested in finishing this PR? Where did you get stuck on? |
Resolves: #2408
The original issue was requesting a DC Offset tool similar to what the Normalize effect can do before normalization. In my university class, we were assigned to find and contribute to an open source project, so I thought I should contribute to this project which I have used myself in the past. The new effect is modified from the existing Normalize effect to automatically remove the offset, bypassing the menu similar to the Invert effect. I am certain improvements could be made upon the modified files, perhaps adding more features, or removing steps that were required for the Normalize effect but not for DC offset removal. I also only added the effects to the src/effects folder, but I do not know if there is something else that needs to know of the updated effects, since it does not currently categorize with the other related effects.
Recommended: