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

Add the Microwave synthesizer to LMMS #5000

Closed
wants to merge 14 commits into from

Conversation

LostRobotMusic
Copy link
Contributor

@LostRobotMusic LostRobotMusic commented Jun 2, 2019

(Don't merge yet)

I've been working on this thing full-time for seven months, so hopefully it doesn't suck 😆
Important disclaimer: This is the first C++ program I've ever written in my life. The code will be ugly. Please be gentle (but obviously point out any problems you find).

I had approximately 33 people test this out, so hopefully it's mostly bug-free.

Here's a Pastebin link to its current manual, which is also included within the synthesizer: https://pastebin.com/ADbcj3Uv
It explains most of its features.

Graph.cpp and Graph.h were edited to give Microwave a cool new graph style.
Knob.cpp and Knob.h were edited to add a new colored knob type, which allows the knobs to be colored within the code.
SampleBuffer.h was edited to allow choosing which ear of a waveform to load.
LcdSpinBox.cpp was edited to fix a bug where custom LCD Spinbox images were ignored.
Otherwise, all files included in this PR were not in LMMS previously.

@LostRobotMusic LostRobotMusic added needs code review A functional code review is currently required for this PR needs style review A style review is currently required for this PR labels Jun 2, 2019
@DomClark
Copy link
Member

DomClark commented Jun 3, 2019

Some comments on style:

  • We use the m_ prefix for member variables; you seem to have followed this rule well where you have modified existing files, but it needs to be applied to the new files as well.
  • There are some very long lines in here; whilst you don't need to follow the length limit of 80 characters too strictly, lines that are too long make the code hard to read. Try splitting some of the longer lines up in appropriate places.
  • We don't use _ as a prefix for function arguments any more; it might be present on code you copied from elsewhere in the program, but in the new files it should be removed.
  • We don't use spaces immediately within parentheses any more, and we add a space between keywords and parentheses (write if (foo) rather than if( foo )).
  • We don't use parentheses around return statements. Again, they might be present in code copied from elsewhere, but they should be removed in the new files.

These are the main issues I spotted; see our full coding conventions at https://github.com/LMMS/lmms/wiki/Coding-conventions. These changes are likely to impact a lot of the code, so I'll do a functional review (and maybe a more specific style one) once you've made them.

@LostRobotMusic
Copy link
Contributor Author

LostRobotMusic commented Jun 3, 2019

@DomClark Oh boy, fixing that took a loooong time, heh...
Find + Replace was the driving force behind it, so hopefully I didn't wreck anything (though I did do a ton of manual editing/checking, and I did test it many times and everything seemed to work fine).

@DomClark
Copy link
Member

DomClark commented Jun 4, 2019

That looks much better, thank you! There are a few places where a space has gone missing after a close-parenthesis, but those are few and far between so I'll point them out individually. I'll do a full review now, although it might take a little while since there's a lot of code to look at and I have another few PRs to review.

@DomClark DomClark self-requested a review June 4, 2019 13:23
include/Graph.h Show resolved Hide resolved
plugins/Microwave/Microwave.cpp Outdated Show resolved Hide resolved
include/Graph.h Outdated Show resolved Hide resolved
@JohannesLorenz
Copy link
Contributor

JohannesLorenz commented Jun 28, 2019

Just adding a few thoughts, it's not a review.

  1. In zyn, we translate the wave into spectrum, then filter out the high freqs, and then translate back to signal. That way, we get a perfect lowpass and avoid aliasing. But I guess that's out of scope for this synth?
  2. Use the "sub" tab, draw a wave that's looking mostly like f(x)=c, c!=0, i.e. draw a line above 0. This clicks, because there's currently no fade in algorithm. The same also for fade out
  3. In zyn, we make all oscillators centered, i.e. the sum of all oscillator points is zero. I don't know if that's for aesthetics, but at least, if it's centered, you don't lose space. Imagine you add another centered signal (e.g. a recorded voice) with another synth: If your wave is not centered, it's more likely that the sum clips either at 1.0 or -1.0 (or hits your master compressor, or whatever).
  4. Minor issue: the number left to "open wavetable" (bottom left) could need a tooltip.

Edit: All those issues have been fixed/clarified via PM.

@SecondFlight
Copy link
Member

@JohannesLorenz

This clips

Do you mean 'clicks', or does the signal actually always clip when the sub oscillator has a straight non-zero line?

@JohannesLorenz
Copy link
Contributor

Clicks, of course. Sorry, I'll fix it.

@LostRobotMusic
Copy link
Contributor Author

Alright, I made it so the GUI code chaos is actually readable now, and I also fixed some minor filter bugs (which makes the feedback freak out sometimes) and added tooltips to the remaining widgets that need them. 🎉

Now, as far as I know, the only bug in there is the one where sometimes the graph color is wrong depending on whether the oscillator is enabled. I should get that fixed soon.

Other than that, I think it's pretty much ready (except the code/style reviews of course). Fancy.

@LostRobotMusic
Copy link
Contributor Author

LostRobotMusic commented Jul 19, 2019

Alright, just fixed that bug and made some last checks to make sure the code's okay. It should be 100% ready for code/style review now (but remember that I'm a beginner so a lot of the code is probably still ugly, haha).
Note that a lot of the dropdown box art is temporary, I will add some new dropdown box icons once Roxas is done drawing them (before this PR is merged).

float m_waveforms[8][MAINARRAYLEN] = {{0}};
bool m_mainFilled[8] = {false};
float m_storedsubs[64][STOREDSUBWAVELEN] = {{0}};
float m_subs[64][SUBWAVELEN] = {{0}};
Copy link
Member

@PhysSong PhysSong Jul 19, 2019

Choose a reason for hiding this comment

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

These large arrays make compilers use large memory, especially MSVC. I think we can fix AppVeyor CI by using something like std::unique_ptr or std::vector, but I'm not sure if we really need it.

@LostRobotMusic
Copy link
Contributor Author

Whoops, I accidentally broke everything with that last commit "Fix Matrix display bug" (which ironically introduced many more display bugs than it fixed), I'll get that fixed in a few minutes.

@LostRobotMusic
Copy link
Contributor Author

Alright, it should be fixed now, sorry about that.

@LostRobotMusic
Copy link
Contributor Author

Assuming I got the Github physics right, Microwave should now have its new icons (drawn by H4CKTON3), and I fixed a bug or two I came across as well. Assuming I didn't break anything with the recent changes, that's probably the last change I'll make to Microwave (except for fixing any problems found during code/style reviews, of course).

@Spekular
Copy link
Member

Reminder that the icons' source files should ideally be uploaded to the lmms artwork directory.

@RoxasKH
Copy link
Contributor

RoxasKH commented Jul 29, 2019

Lost Robot made this to show how to make a sound with microwave https://youtu.be/hSkhbDDrPiM.
Anyway then you need to put your touch, i managed to make some great basses with some basics Wavetables downloaded from the internet, mixed to an oscillator and using the modify knob.
There's also this to understand how filter setup works, but you need to download it.
https://cdn.discordapp.com/attachments/570660826932051969/579004551236157440/Microwave_Filter.mp4
P.S: I'm H4CKTON3, i have another name on github

@LostRobotMusic
Copy link
Contributor Author

@BaraMGB Sorry that it's confusing, it's definitely one of the most complex synthesizers out there.
The first three tabs are for generating the sound. For the wavetable tab, which will probably be the most popular part of Microwave, you must enable the oscillator, click on the folder icon, choose a sample, then load it by clicking Confirm. The default wavetable loading algorithm will be the best one in most cases.

Once this happens, Microwave will use the selected formula to try its best to extract up to 256 individual waveforms from the sample, and store that as its wavetable. The Morph knob will select one of those waveforms at a time and play it. However, since it plays the waveforms at the pitch of the note you're playing, many wavetables sound painfully high-pitched and ugly, which may be what you're experiencing. If you instead play notes around the F1 area, and move the Morph knob while you're playing, you should be able to get a really nice sound. I honestly almost instinctively turn down Microwave's pitch by three octaves most times when I load a wavetable. Try loading something like a dubstep growl sample as a wavetable, it'll probably sound very cool.

Note that Microwave also supports importing Xfer Serum wavetables as well, if you use the "Load wavetable file" algorithm.

As soon as you have an awesome wavetable, don't forget about the modify knob and modfiy mode box! There are tons of awesome ways you can warp your sound around realtime. I made all the formulas myself (though some of them obviously show up in other synthesizers, e.g. Atan), and I tweaked them carefully to make sure they sounded good, and completely deleted any I didn't think were awesome. Definitely mess with those to see how they can change your sound.

Now, while Microwave is a wavetable synthesizer, it's also a modular synthesizer. The Matrix tab lets you route any oscillator to (almost) any parameter, even the completely stupid ones! 😆
So, like most synthesizers, you gain access to FM/PM/AM that way, but you also get access to... hundreds more. Hundreds of new modulation types! It's really just meant to have fun and see what crazy sounds you can come up with. :)

For example, you could take your wavetable, set the modify mode to Pulse Width, then take a sine wave Sub Oscillator and use that to modulate the Modify knob. You now have Pulse Width Modulation! And of course, you can do that with any of the other modify modes. Or with Morph, Range, panning, anything you want. You can even modulate oscillators with themselves, or modulate the Matrix box that is controlling a Matrix box that is controlling it... infinite loops are supported, it's cool.

Just remember that all oscillators will be sent directly the audio output unless you have the "Muted" LED enabled. When an oscillator is enabled but muted, it won't make any sound but it can still be used as a Matrix input.

Effects are also routed via Matrix. If you mute an oscillator, send it to "Filter Input", and enable the filter, you should hear the filter work. I added super unique "feedback" and "[feedback] detune" knobs to the filters, which are really awesome. Something I love doing is choosing the Allpass Filter, setting the slope to 8 (96 db), turning the feedback fairly high, then moving the cutoff and resonance knobs, which sounds absolutely amazing.

Notice that you can also modulate filter parameters in the Matrix. To stack onto your AM/PM/FM, give lowpass cutoff frequency modulation a try. Or anything else you think of. There's no wrong move unless it sounds bad. :)

All in all, Microwave requires the largest amount of sound design skill but gives back the largest sound rewards. It's definitely geared toward the more advanced users. However, anybody who learns this will probably have no want for any other wavetable synthesizer (e.g. Xfer Serum) because of its crazy versatility and quality. There are only a few downsides, like the fact that the 250x250 GUI can largely restrict workflow speed, and its lack of a wavetable editor (which I hope to add in the future). It's probably top 5 for most versatile software synthesizers in the world (most versatile doesn't always mean best though, haha). I've already seen people have a ton of fun with it.

@LostRobotMusic
Copy link
Contributor Author

@Spekular Microwave has a crapton of art files though, over 100 I think. Should they still go in there?
...actually, is it possible/allowed to have a subdirectory in the artwork directory? Maybe there could be a Microwave folder in there so it doesn't make a crazy mess of everything.
Oh, and I just thought of something... should I delete any icons that Microwave doesn't use? There's a full alphabet in there, but Microwave only uses a few of those letters.

Copy link
Member

@DomClark DomClark left a comment

Choose a reason for hiding this comment

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

I've done an initial review for you (sorry it took so long). Some comments about the review:

  • Some comments made here may contradict code elsewhere in LMMS. A lot of the code in LMMS is quite old, and the occasional bit might not be quite up to standard.
  • Some comments here may apply in multiple places. Usually I've only made each comment once, but if you know there are bits of code similar to what I've commented on, then check those as well.
  • A few comments are made primarily as examples of general C++ style, and may be overridden when addressing other comments in the review.
  • Hopefully this goes without saying, but I'm not an authority on C++, LMMS's code, or anything like that. If you disagree with any comments I've made, feel free to discuss them. If you want further explanation, just ask.

Some comments that don't really go anywhere else:

General comments:

  • This plugin uses a lot of memory. Just between them, m_storedwaveforms, m_waveforms, m_storedsubs, and m_subs use 280.5MB per Microwave instance. This is more than the rest of the entire program. Is it possible to allocate this when the wavetable/sub is actually used, rather than when the instrument is created?
  • The modulation amount knob (the one that appears when you alt-click/middle-click a matrix-controlled knob) is difficult to interact with. It doesn't appear to be possible to open a context menu for it, open a dialog to enter a value, reset it, automate it, etc.
  • Most of the knobs have such a tiny step that scrolling on them to change the value has hardly any affect at all.
  • Use letters as icons is bad for localisation - the appropriate word in another language may begin with a different letter, possibly even from a different writing system.
  • It looks like a lot of member variables are used as temporary variables. If the value of a variable doesn't need to be preserved from method to method, make it local to the method(s) where you use it.

Comments on artwork: (since GitHub doesn't let you leave comments directly on binary files)

  • LCD graphics: is there a reason to have these in both Microwave's directory and the theme directories?
  • none.png: could you just not use an icon at all instead of using a blank one?
  • Filter/matrix boxes: these are repeated several times in each image.
  • tab1_artwork.png: this would be better without the word "memes" in it.

Edit: GitHub seems to have moved a lot of my comments to the same few lines of Microwave.cpp. I've linked directly to the correct place in the code in the affected comments.

@@ -48,7 +48,8 @@ class LMMS_EXPORT Graph : public QWidget, public ModelView
LinearStyle, //!< connect each 2 samples with a line, with wrapping
LinearNonCyclicStyle, //!< LinearStyle without wrapping
BarStyle, //!< draw thick bars
NumGraphStyles
NumGraphStyles,
BarCenterGradStyle //!< draw color gradient coming from center
Copy link
Member

Choose a reason for hiding this comment

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

BarCenterGradStyle should go before NumGraphStyles.

It's a common idiom in C++ to add an extra enumerator to an enum that isn't used as a normal value of the enum, but instead indicates the number of values the enum can take. Because C++ assigns values sequentially to enumerators (unless you specify them yourself), putting this special enumerator last ensures it automatically has the correct value.

(In this case it doesn't have any effect since we don't seem to use NumGraphStyles anywhere, but it ought to be corrected anyway.)

@@ -189,6 +193,7 @@ private slots:
float m_outerRadius;
float m_lineWidth;
QColor m_outerColor;
QColor m_innerColor;
Copy link
Member

Choose a reason for hiding this comment

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

This could be exposed as a Q_PROPERTY, like the other colours around it. See the top of the definition of this class for how it's done.

@@ -228,7 +228,7 @@ class LMMS_EXPORT SampleBuffer : public QObject, public sharedObject

// protect calls from the GUI to this function with dataReadLock() and
// dataUnlock(), out of loops for efficiency
inline sample_t userWaveSample( const float _sample ) const
inline sample_t userWaveSample( const float _sample, const int channel = 0 ) const
Copy link
Member

Choose a reason for hiding this comment

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

Not necessarily a problem, but this will probably conflict with #4994. Generally, whoever has their code merged last has to deal with the conflict, but @Reflexe can probably let you know whether there's likely to be a problem here.

m_foreground.fill(Qt::transparent);
for( int i=0; i <= length; i++ )
{
QLinearGradient gradient(0, 0, 0, height() / 2.f);
Copy link
Member

Choose a reason for hiding this comment

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

The gradient doesn't need recreating every iteration of the loop, so you can declare it and set it up outside of the loop to avoid this.

for( int i=0; i <= length; i++ )
{
QLinearGradient gradient(0, 0, 0, height() / 2.f);
gradient.setSpread(QGradient::Spread(1));// ReflectSpread, so gradient on bottom half is a reflection of the top half
Copy link
Member

Choose a reason for hiding this comment

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

Use QGradient::ReflectSpread rather than QGradient::Spread(1).

{
if (doInterpolate)
{
srccpy(m_waveforms[which].data(), m_storedwaveforms[which].data(), STOREDMAINARRAYLEN);
Copy link
Member

@DomClark DomClark Jul 29, 2019

Choose a reason for hiding this comment

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

Is this needed here? It seems to recompute the layout of the whole view whenever a knob is released. (Also, again, use emit for signals.)

Edit: GitHub moved this comment to the wrong place. It refers to

updateScroll();

{
if (doInterpolate)
{
srccpy(m_waveforms[which].data(), m_storedwaveforms[which].data(), STOREDMAINARRAYLEN);
Copy link
Member

@DomClark DomClark Jul 30, 2019

Choose a reason for hiding this comment

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

You can use these locations as parameters to the signal where it is emitted. This means that you don't need to remake this connection every time this method is called, and you can hook up the signal directly to Microwave::sendToMatrixAsOutput and get rid of the m_knobView member.
E.g.:

// Where the signal is emitted
emit sendToMatrixAsOutput(m_matrixLocation[0], m_matrixLocation[1], m_matrixLocation[2]);

// Where the knob is created in MicrowaveView
connect(someKnob, &MicrowaveKnob::sendToMatrixAsOutput, this, &MicrowaveView::sendToMatrixAsOutput);

Edit: GitHub moved this comment to the wrong place. It refers to

disconnect(this, &MicrowaveKnob::sendToMatrixAsOutput, 0, 0);
connect(this, &MicrowaveKnob::sendToMatrixAsOutput, this, [this, loc1, loc2, loc3]() { m_knobView->sendToMatrixAsOutput(loc1, loc2, loc3); });

{
if (doInterpolate)
{
srccpy(m_waveforms[which].data(), m_storedwaveforms[which].data(), STOREDMAINARRAYLEN);
Copy link
Member

@DomClark DomClark Jul 30, 2019

Choose a reason for hiding this comment

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

There's quite a bit of code here duplicating that in Knob::mousePressEvent. Perhaps consider factoring it out into a Knob::beginDrag method or something? Then you can also change some of the Knob members back to private.

Edit: GitHub moved this comment to the wrong place. It refers to

AutomatableModel *thisModel = model();
if (thisModel)
{
thisModel->addJournalCheckPoint();
thisModel->saveJournallingState(false);
}
const QPoint & p = me->pos();
m_origMousePos = p;
m_mouseOffset = QPoint(0, 0);
m_leftOver = 0.0f;
emit sliderPressed();
QApplication::setOverrideCursor(Qt::BlankCursor);
s_textFloat->setText(displayValue());
s_textFloat->moveGlobal(this, QPoint(width() + 2, 0));
s_textFloat->show();
m_buttonPressed = true;

{
if (doInterpolate)
{
srccpy(m_waveforms[which].data(), m_storedwaveforms[which].data(), STOREDMAINARRAYLEN);
Copy link
Member

@DomClark DomClark Jul 30, 2019

Choose a reason for hiding this comment

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

"m_filter's" -> "filter's"

Edit: GitHub moved this comment to the wrong place. It refers to

makeknob(m_filtFeedbackKnob[i], knobSmallColored, tr("Feedback"), tr("This knob sends the specified portion of the filter output back into the input after a certain delay. This delay effects the pitch, and can be changed using the m_filter's Detune and Keytracking options."));

/*The following disconnected functions will run if not disconnected upon deletion,
because deleting a ComboBox includes clearing its contents first,
which will fire a dataChanged event. So, we need to disconnect them to
prevent a crash.*/
Copy link
Member

Choose a reason for hiding this comment

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

I don't think a combobox needs to clear its contents upon deletion any more. It looks like that was a fix for a memory leak before a unique_ptr was used for the icons. I think it's now safe to remove clear() from ComboBoxModel::~ComboBoxModel(), then this code can go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like you're right. I think I ran into that problem while using RC6. Completely removing that code on the latest master branch didn't seem to have any negative impact. 👍

@LostRobotMusic
Copy link
Contributor Author

LostRobotMusic commented Jul 30, 2019

@DomClark Thank you for the review!

One thing I should point out... I just noticed that somewhere along the road, I accidentally set WAVERATIO to 16, probably while testing out quality differences. That was unintentional, it's supposed to be 4. So it doesn't actually use that much RAM, sorry about that. Larger values make Microwave take way too long to load, and, as you said, use much more RAM. 4 is probably the best value currently.

LCD graphics: is there a reason to have these in both Microwave's directory and the theme directories?

Nope, no reason at all! I'll fix that.

none.png: could you just not use an icon at all instead of using a blank one?

If no icon is used, then the text (in this case, "None") will replace the empty space. The blank image needs to be there so the dropdown box actually appears blank.

tab1_artwork.png: this would be better without the word "memes" in it.

...I... thought I removed that... 😆

Is it possible to allocate this when the wavetable/sub is actually used, rather than when the instrument is created?

Hm... I think so, using vector magic... I'll see if I can get that working.

The modulation amount knob (the one that appears when you alt-click/middle-click a matrix-controlled knob) is difficult to interact with. It doesn't appear to be possible to open a context menu for it, open a dialog to enter a value, reset it, automate it, etc.

That knob is identical to the corresponding Amount knob in the Matrix box controlling it, they're connected to the same model. The middle/alt click functionality is just there for super high-speed tweaking when you, for example, want to tweak the base value and modulation amount a lot and you don't want to switch tabs fifty times to find a sweet spot. If any of those other knob abilities are needed, you can just visit the Amount knob in the Matrix tab and do it there.

Most of the knobs have such a tiny step that scrolling on them to change the value has hardly any affect at all.

Oh, I never noticed that! I did most of Microwave's development on my own personal copy of LMMS, which I added #4574 to (one of the first PRs I ever made, maybe even the first?), which makes scrolling on knobs significantly more convenient. If you could look at that PR and see if it's good (I've been using it for 11 months and haven't noticed a single problem, I even forgot I was using it), that would be awesome. It just changes two lines and adds one.

It looks like a lot of member variables are used as temporary variables. If the value of a variable doesn't need to be preserved from method to method, make it local to the method(s) where you use it.

I think I did that because back when I started with Microwave, I thought creating and initializing a variable used more CPU than one that was already created, which... I don't think is true. I'll fix that.

Use letters as icons is bad for localisation - the appropriate word in another language may begin with a different letter, possibly even from a different writing system.

Ah, that is true, I didn't even think about that... I'll talk with H4CKTON3 (the artist who made all the icons) to see if we can come up with icons to replace all the letter icons. Then we can probably just delete all of those letter icons entirely.

I'll get to work on all the other stuff. And sorry about the mess in the Graph files, when checking Microwave to make sure it was neat I completely forgot to check the other files I changed.

@JohannesLorenz JohannesLorenz added in progress and removed needs code review A functional code review is currently required for this PR needs style review A style review is currently required for this PR labels Aug 5, 2019
@zonkmachine
Copy link
Member

@douglasdgi As @BaraMGB commented earlier this plugin could use a couple of presets.

@LostRobotMusic
Copy link
Contributor Author

I agree. I'll submit a few once I have all the code finished (just in case it ends up not being compatible after the changes).

@KaiJan57
Copy link

KaiJan57 commented Sep 7, 2019

Small issue here:
https://github.com/DouglasDGI/lmms/blob/6f7190f600d03f707813ce0c074e8fabb1dc8695/plugins/Microwave/Microwave.cpp#L491-L499

The base64 encoder generates huge amounts of data, i.e. depending on configuration, it generates 2826422 characters of base64 per stored waveform. Maybe add a simple compression algorithm?

@enp2s0
Copy link
Contributor

enp2s0 commented Oct 2, 2019

@KaiJan57 I think that's intentional, Microwave stores loaded samples into its configuration instead of loading them at project load like AudioFileProcessor does. This has the advantage of allowing you to send a .xpf file to someone else and have it load as intended without also having to send them your wavetable files.

@PhysSong
Copy link
Member

@douglasdgi Do you have time to address @DomClark's review? Sorry for bumping.

@tresf
Copy link
Member

tresf commented Nov 15, 2019

Small issue here:
https://github.com/DouglasDGI/lmms/blob/6f7190f600d03f707813ce0c074e8fabb1dc8695/plugins/Microwave/Microwave.cpp#L491-L499

The base64 encoder generates huge amounts of data, i.e. depending on configuration, it generates 2826422 characters of base64 per stored waveform. Maybe add a simple compression algorithm?

@KaiJan57 According to Mozilla:

[...] the Base64 version of a string or file will be at most 133% the size of its source (a ~33% increase).

33% is significant, but I wouldn't consider this an issue. Compression may make sense but it's a difficult decision when forced to use base64 to begin with. May I propose that suggestions like this may be worth making a small use-case for compression and sharing sample code to move the PR along faster.

@KaiJan57 I think that's intentional, Microwave stores loaded samples into its configuration instead of loading them at project load like AudioFileProcessor does. This has the advantage of allowing you to send a .xpf file to someone else and have it load as intended without also having to send them your wavetable files.

@noahb01 I don't think you're describing the same things. From what I'm reading @KaiJan57 is only commenting on the file inflation as a result of base64, not the design or purpose of it.

@tresf
Copy link
Member

tresf commented Nov 15, 2019

@douglasdgi testing on macOS 10.15, I get no sound out of this plugin (SDL backend). Shouldn't it make sound by default? I assume the answer is "yes". If that's the case, something is wrong on macOS. If you need a mac to test on, let me know.

Also, you should show off the interface a bit in your PR 😃 . Feel free to steal the following hi-res images...

Screen Shot 2019-11-15 at 12 07 18 AM Screen Shot 2019-11-15 at 12 07 02 AM Screen Shot 2019-11-15 at 12 07 38 AM Screen Shot 2019-11-15 at 12 07 59 AM Screen Shot 2019-11-15 at 12 08 08 AM Screen Shot 2019-11-15 at 12 08 19 AM

@LostRobotMusic
Copy link
Contributor Author

It actually does not play sound by default, for very good reasons, which will not be good reasons in the future for reasons I'll explain right now. I've done fairly well at keeping people updated on Discord about Microwave's status, but I've completely failed to mention anything here.

The idea behind Microwave and the reasons it's a great synthesizer are killed by the fact that it's crammed into a 250x250 GUI. Because of this, the final Microwave design as seen in this PR packed a lot of the versatility that I and everybody else always dreamed of... but it's so painful to use that even I haven't bothered using it in a song yet. The workflow is deplorable at best.

I made it clear that I want this thing to be the best synthesizer in the world, and while that's a stupid and quite possibly unrealistic goal, I know my abilities and I know how those small things can be combined to make what I really want this synthesizer to be. I'm rebuilding it with a much larger GUI now that LMMS is starting to have some support for that, and I'm going to make sure it's amazing both from a versatility standpoint as well as a workflow standpoint.

I'll attempt to have it finished in under a year. I've kept this PR open in case I ran into a situation were I'm unable to access a computer anymore (which actually happened for about a month, but I managed to claw my way back). If you'd like, I could close this PR and then anybody could reopen it and try to finish it up if I disappear again.

Microwave is the first synthesizer I've ever made, so there are a lot of problems with it in many ways. I've had a few dozen people testing it out for the past many months, so now I know where those problems are, and I'm going to make sure I fix all of those (having no default sound included, hehe...). I apologize for the delay, but I guarantee it will be worth it.

@tresf
Copy link
Member

tresf commented Nov 15, 2019

It actually does not play sound by default, for very good reasons

I didn't see any reasons listed, but please change the default behavior to produce sound. There's no good reason to produce zero sound. No other plugins do this (with exception of banks like SF2/GIG or plugins like Carla/VST).

Please provide what's necessary to produce output when the plugin is dragged in. Defending the reason not to is a waste of many people's time. Although some code reviewers are interested in how good of a plugin this is, some reviewers are taking a more objective look at viability (which is why the kind reviews by @PhysSong and @DomClark don't really talk about usability).

If your reason for having no sound can be described in one sentence, please provide it. It's unlikely to change my mind though. This synth is too complex to make no sound on startup. If we need to ship a file with LMMS to support sound, let's do it please.

@tresf
Copy link
Member

tresf commented Nov 15, 2019

If you'd like, I could close this PR and then anybody could reopen it and try to finish it up if I disappear again.

I'm not sure what this is in regards to, but I see no reason to close a perfectly good PR. My comments are about default behavior (sound) and branding (offering screenshots). These are important aspects to users. @Reflexe's build-bot should be making builds soon too, which will yield more bug reports that you can use to improve the plugin. I'm concerned that there's a proposal to close this PR and that there's a proposal to have "anybody [...] finish it up". This is your plugin and you may take your time writing it, but history has proven that developers don't take over other developer's PRs very often, especially ones with 177 changed files. ❤️

# Conflicts:
#	include/Knob.h
#	plugins/CMakeLists.txt
#	src/gui/widgets/Knob.cpp
@PhysSong
Copy link
Member

history has proven that developers don't take over other developer's PRs very often, especially ones with 177 changed files.

There are 167 image files, so there are only a few files to look into. The REAL issue is that Microwave.cpp is very long and pretty unorganized.
I can probably rewrite this plugin from scratch while reusing some of the existing code, in a well-organized way. It may take quite long, though.

@PhysSong PhysSong added rework required Pull requests which must be reworked to make it able to merge or review and removed in progress labels Jan 4, 2021
@PhysSong PhysSong marked this pull request as draft February 5, 2022 04:11
@PhysSong
Copy link
Member

@LostRobotMusic What do you want to do with this PR now?

@LMMS LMMS deleted a comment from rdrpenguin04 Nov 27, 2022
@LostRobotMusic
Copy link
Contributor Author

LostRobotMusic commented Nov 27, 2022

@PhysSong I didn't realize this PR was still open, my bad.
This mess of code, properly implemented, wouldn't be changing parts of LMMS's code aside from adding the plugin itself. The code itself is also messy and horribly written as well. And most importantly, the synthesizer itself is generally a pain to use since it was developed back before LMMS supported instrument GUIs above 250x250. As exciting as many of its features are, this really isn't something that should be put into LMMS.
The entire project is what I would consider to be unsalvageable (it would be easier to write a new plugin from scratch than it would be to revive this one), so I don't really see any reason to keep it open. I'll keep the branch up in case people want to poke through the source code, but I do heavily recommend against that given pretty much every single individual aspect of it really should have been handled differently.

@RoxasKH RoxasKH mentioned this pull request Oct 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rework required Pull requests which must be reworked to make it able to merge or review
Projects
None yet
Development

Successfully merging this pull request may close these issues.