yokemura / Magical8bitPlug2

GNU General Public License v3.0
302 stars 19 forks source link

Won't compile with v5.4.7 #1

Closed gazayas closed 4 years ago

gazayas commented 4 years ago

I think it has something to do with this update

I changed the float pointers in Settings.h to std::atomic<float>* like this:

std::atomic<float>* osc = nullptr;
std::atomic<float>* gain = nullptr;
std::atmoic<float>* maxPoly = nullptr;

...

std::atomic<float>* isVolumeSequenceEnabled_raw = nullptr;
std::atomic<float>* isPitchSequenceEnabled_raw = nullptr;
std::atomic<float>* isDutySequenceEnabled_raw = nullptr;
std::atomic<float>* pitchSequenceMode_raw = nullptr;

This threw an error from this line of code (line 150) in PluginProcessor.cpp

int type = roundToInt (*settingRefs.osc);

I removed the pointer, which threw an error concerning roundToInt. It seems like that function had trouble casting the atomic float to a double, which is where it got stuck (specifically line 478 in juce_MathsFunctions.h)

I'm not sure if you're considering updating to v5.4.7 (Maybe I'll have to go to 5.4.6 to edit things), but this is the problem I encountered so I thought I'd make this issue.

yokemura commented 4 years ago

Thanks for your report! Let me take a look to this issue, as I'm willing to keep the code as up-to-date as possible,

atsushieno commented 4 years ago

I encountered the same issue, but I simply add cast to float*.

If those SettingsRef members are accessed by multiple threads, then it makes sense to convert them to std::atomic<float>*, but if not then there is no need to do so. JUCE AudioParameterValueTreeState has to be made atomically thread safe as plugin devs might want to have multiple threads manipulate the parameters and JUCE had to be suitable for such strict uses.

gazayas commented 4 years ago

I see, so there's no need to explicitly make them atomic floats?

Casting the values to float* worked for me. I changed the assignments in the constructor in Settings.h to something like this:

isAdvancedPanelOpen_raw = (float*) parameters->getRawParameterValue ("isAdvancedPanelOpen_raw");

Do you think this would be better?

atsushieno commented 4 years ago

I guess so but it would be @yokemura who can judge whether it should be multi thread ready or not :-)

yokemura commented 4 years ago

Honestly speaking, I don't know which is better. If it doesn't affect the performance I'd take the safer way.

gazayas commented 4 years ago

I honestly feel the same way. If it's okay with you, I think I will take @atsushieno's advice and simply cast the atomic float pointers to regular float pointers.

That way, line 150 in PluginProcessor.cpp doesn't need to be changed.

I'll try to update the pull request soon!

yokemura commented 4 years ago

Sorry but I didn't have a time to check it on my environment but I've just built it and seen it works, so I've merged the pull request to the newly created develop branch.

gazayas commented 4 years ago

Awesome, thanks for the merge!