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

Improved Waveshaper / Spline-based graph replacement #4367

Open
wants to merge 52 commits into
base: master
Choose a base branch
from

Conversation

SecondFlight
Copy link
Member

@SecondFlight SecondFlight commented May 18, 2018

image

image

image

Fruity Waveshaper is one of the plugins I use the most in FL. It's a fantastic general-purpose digital distortion. The Waveshaper plugin by Diizy is good and definitely has its place, but it is lacking in a few key areas:

  1. It is incredibly difficult to create a smooth curve. If you want a smooth curve and then a harsh edge it becomes even harder.
  2. Diizy's waveshaper uses graph.h, which uses 200 discrete points to store the curve. This means resolution is limited, and this limits the steepest curve possible in the shaper.

The original plugin is very similar to the 8-bit shaper by Xfer. Again, not a bad thing, but I certainly think another option would provide a better workflow and overall general-purpose plugin.

The changes I plan to make are two-fold:

  1. Replace the graph. This will be a fairly large project. In the long run I am interested in improving automation to some extent, and my hope is that this will help me get a feel for how to do that and make things easier for me in the future.
  2. Overhaul the DSP. This is a necessary side effect of the first change. Because the DSP is so simple, switching from a discrete to a vector-based graph will change pretty much everything in the calculations.

Todo:

  • Vector Graph
    • Add bare functioning graph replacement
    • Create data structure for internal data storage
    • Add function to get output float based on input
    • Add initial revision of drawing code based on internal data
    • Connect click/drag events
    • Allow points to be created via right click
    • Make point positions adjustable via click + drag
    • Add context menu on point for deleting point
    • Allow curve type to be changed
    • Make curves adjustable via click + drag
    • Allow curve to be reset with right click
    • Add more curve types
    • Clean up
    • Refactor with usability in mind
    • Allow snap to grid
    • Add comments documentation
    • Remake UI in new style
    • Implement saving and loading
  • DSP
    • Process samples by calling function from vector graph
    • (VectorGraph) Attempt optimizations
  • Waveshaper 2
    • Remove unnecessary code from the original Waveshaper
    • Add asymmetric feature
    • Clean up
    • Add comments
    • Implement saving and loading

@SecondFlight SecondFlight changed the title Improved Waveshaper Improved Waveshaper / Spline-based graph replacement May 28, 2018
m_outputModel( 1.0f, 0.0f, 5.0f, 0.01f, this, tr( "Output gain" ) ),
m_wavegraphModel( 0.0f, 1.0f, 200, this ),
m_clipModel( false, this ),
m_vectorGraphModel( this, true )
Copy link
Member

Choose a reason for hiding this comment

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

You should match the order of initialization and declaration to avoid compiler warning. It means m_vectorGraphModel should be initialized before m_clipModel.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I've been ignoring that but I will get to it eventually :)

@SecondFlight
Copy link
Member Author

SecondFlight commented May 29, 2018

Does anybody know why the circleci build is failing? This error seems cryptic to me.

Makefile:151: recipe for target 'all' failed
make: *** [all] Error 2
Exited with code 2

Edit: It's always after updating Xpressive. I wonder if I accidentally updated the submodules...
Edit 2: Nah, I don't think that's it.

@@ -80,6 +80,7 @@ IF("${PLUGIN_LIST}" STREQUAL "")
VstEffect
watsyn
waveshaper
waveshaper2
Copy link
Member

Choose a reason for hiding this comment

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

Minor, but use tabs instead of spaces.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do, I edited that with a different editor. Thanks for catching this.

@SecondFlight
Copy link
Member Author

Oh that was literally it. Well I can't complain.

@SecondFlight
Copy link
Member Author

Right, that makes sense. I don't think that a decision here is something I should be making myself then, it sounds like it will span across the entire software and affect most future development. Perhaps this is something I should create an issue for to start a discussion.

@SecondFlight
Copy link
Member Author

The question in my mind is, how much do we want to rip up the current system to implement these UI changes? Having something external that says "this knob hooks up here" would be great, but it's a completely different paradigm from what we currently have, and implementing it would involve a lot of well-thought-out core changes in a number of areas.

But in the end I don't know what's best and I'm going off of assumptions. I think I'll make an issue for discussion later today.

@fundamental
Copy link
Contributor

My own opinion as an observer is that with the current LMMS development community and the current internal LMMS architecture an organized large scale UI upgrade isn't feasible at this time. Any work would need to be much smaller scale otherwise the trend of individuals building moderate sized goals, getting frustrated, and subsequently leaving the project, will likely continue. For Zyn the used approach was to upgrade the internal architecture to make large scale GUI changes feasible with restricted developer resources, so that may be an option for LMMS, though keep in mind that's a very large project within itself.

@SecondFlight
Copy link
Member Author

And I really don't feel equipped to do that kind of overhaul, both from lack of general experience as well as a lack of knowledge of the inner workings of the software. My current plan is to slowly start developing widgets that could be useful later so as to save time for whoever puts it all together, be it me or somebody else. Still, I completely agree that implementing the UI for an individual plugin or feature shouldn't take weeks and should be doable by someone fairly new to the codebase.

@budislav
Copy link

budislav commented Jun 3, 2018

@SecondFlight Just to answer on your questions, when I said gui scaling I meant that it should look crisp and pixel perfect on all resolutions even high display resolutions like "retina". FL studio have some nice settings for scaling gui so you if user feel that his gui is too small he can just scale it up. https://www.image-line.com/support/flstudio_online_manual/html/envsettings_general.htm
This is why I think there should not be images in gui at all, I think you understand :)

@SecondFlight
Copy link
Member Author

/root/project/src/gui/widgets/VectorGraph.cpp:632:64: error: 'rightPoint' may be used uninitialized in this function [-Werror=maybe-uninitialized]
    else if (checkRight && potentialSnappedValue > rightPoint->x())

Oh come on, checkRight will only be true if rightPoint is initialized.
sigh
I'll fix this tonight.

src/gui/widgets/VectorGraph.cpp Outdated Show resolved Hide resolved
return false;
}

float VectorGraph::calculateSample(float input)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to have this as a separate function? I think one can use model()->calculateSample directly.

src/gui/widgets/VectorGraph.cpp Outdated Show resolved Hide resolved
include/VectorGraph.h Outdated Show resolved Hide resolved
return castModel<VectorGraphModel>();
}

inline int getWidth()
Copy link
Member

Choose a reason for hiding this comment

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

Please consider not using get prefix unless the meaning is ambiguous without it.

Copy link
Member

Choose a reason for hiding this comment

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

It can be confused with QWidget::width(), so I think we should check if we really need this.

@SecondFlight
Copy link
Member Author

SecondFlight commented Aug 21, 2018

@PhysSong,
Thanks for reviewing this. I'll wrap up this PR and address the comments once 1.3 becomes a development focus.

@PhysSong PhysSong self-assigned this Dec 24, 2022
@PhysSong
Copy link
Member

I've resolved merge conflicts and applied minor fixes. Modernization and other reviews are not applied yet. I'll try to continue working on this.

Comment on lines +235 to +238
static inline bool floatEqual(float a, float b, float epsilon)
{
return qFabs(a - b) < epsilon;
}
Copy link
Member

Choose a reason for hiding this comment

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

No reason to reside in a class.

Comment on lines +441 to +444
VectorGraphPoint * VectorGraphModel::getPoint(int index)
{
return & m_points[index];
}
Copy link
Member

Choose a reason for hiding this comment

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

Returning by address looks weird.

return result;
}

float calculateSingleCurve(float input, VectorGraphPoint * point);
Copy link
Member

Choose a reason for hiding this comment

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

Should be moved to VectorGraphPoint instead.

#include <QWidget>
#include <QPainter>
#include <QVector>
#include <QtMath>
Copy link
Member

Choose a reason for hiding this comment

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

Standard C++ math functions look better than Qt math functions.

{
Q_OBJECT
public:
enum TensionType
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 this should be in the model class.

#include "Song.h"


#define onedB 1.1220184543019633f
Copy link
Member

Choose a reason for hiding this comment

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

constexpr instead

@szeli1 szeli1 mentioned this pull request Mar 23, 2024
32 tasks
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.

4 participants