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

Fix C++ standards library portability issue. #4261

Merged
merged 3 commits into from Mar 18, 2018

Conversation

Projects
None yet
3 participants
@tresf
Member

tresf commented Mar 18, 2018

Fixes zyn instrument playback on Mac for certain presets.

This PR is a partial cherry-pick of upstream 2.5 patches: zynaddsubfx/zynaddsubfx@417d49b, zynaddsubfx/zynaddsubfx@edca8ab

Quoting the author, @hselasky from 2014:

The std::polar template has no clear definition for the range of
the input parameters, and some C++ standard library implementations
don't accept negative amplitude among others. Define our own
FFTpolar template, which works like we expect it to.

Closes #4152

@Wallacoloo

This comment has been minimized.

Member

Wallacoloo commented Mar 18, 2018

@tresf windows builds failed:

In file included from /home/travis/build/LMMS/lmms/plugins/zynaddsubfx/zynaddsubfx/src/Params/PADnoteParameters.h:28:0,
                 from /home/travis/build/LMMS/lmms/plugins/zynaddsubfx/zynaddsubfx/src/Params/PADnoteParameters.cpp:24:
/home/travis/build/LMMS/lmms/plugins/zynaddsubfx/zynaddsubfx/src/Params/../DSP/FFTwrapper.h: In instantiation of 'std::complex<_Tp> FFTpolar(const _Tp&, const _Tp&) [with _Tp = float]':
/home/travis/build/LMMS/lmms/plugins/zynaddsubfx/zynaddsubfx/src/Params/PADnoteParameters.cpp:618:94:   required from here
/home/travis/build/LMMS/lmms/plugins/zynaddsubfx/zynaddsubfx/src/Params/../DSP/FFTwrapper.h:64:22: error: 'isnan' was not declared in this scope
         if (isnan(__x))
                      ^
/home/travis/build/LMMS/lmms/plugins/zynaddsubfx/zynaddsubfx/src/Params/../DSP/FFTwrapper.h:64:22: note: suggested alternative:
In file included from /home/travis/build/LMMS/lmms/plugins/zynaddsubfx/zynaddsubfx/src/Params/../DSP/FFTwrapper.h:27:0,
                 from /home/travis/build/LMMS/lmms/plugins/zynaddsubfx/zynaddsubfx/src/Params/PADnoteParameters.h:28,
                 from /home/travis/build/LMMS/lmms/plugins/zynaddsubfx/zynaddsubfx/src/Params/PADnoteParameters.cpp:24:
/opt/mingw32/i686-w64-mingw32/include/c++/4.9.4/cmath:632:5: note:   'std::isnan'
     isnan(_Tp __x)
     ^

I'm not sure why the builds for other platforms are succeeding. Unless fftw3.h is providing its own isnan function at the global scope.

You can see that at some point after this commit, ZASFx changed these from isnan to std::isnan (https://github.com/zynaddsubfx/zynaddsubfx/blob/master/src/DSP/FFTwrapper.h) - may be worth trying to track down which commit did that and cherry-picking it as well.

@Wallacoloo

This comment has been minimized.

Member

Wallacoloo commented Mar 18, 2018

Oh, wow. Github offers git blame functionality right on the website. This is the commit you want: zynaddsubfx/zynaddsubfx@edca8ab

Although, it's still using just sin and cos instead of std::sin and std::cos, but mingw didn't seem to be complaining about those for some reason.

@Wallacoloo Wallacoloo self-requested a review Mar 18, 2018

@PhysSong

This comment has been minimized.

Member

PhysSong commented Mar 18, 2018

For master, it would be enough to cherry-pick two commits into https://github.com/LMMS/zynaddsubfx and bump the submodule.

@tresf

This comment has been minimized.

Member

tresf commented Mar 18, 2018

You can try. (Or I will). One of the files doesn't exist, not sure if that'll cause a conflict.

@tresf

This comment has been minimized.

Member

tresf commented Mar 18, 2018

Yeah, it threw it into conflict... Resolved, pushed to lmms/zynaddsubfx. Documented here: https://github.com/LMMS/zynaddsubfx/wiki.

screen shot 2018-03-18 at 1 22 27 pm

@tresf tresf merged commit 6cd5317 into LMMS:stable-1.2 Mar 18, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tresf tresf deleted the tresf:zyn branch Mar 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment