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

Time to update vst3sdk pointer? #373

Closed
baconpaul opened this issue Jan 22, 2019 · 24 comments
Closed

Time to update vst3sdk pointer? #373

baconpaul opened this issue Jan 22, 2019 · 24 comments

Comments

@baconpaul
Copy link
Collaborator

Our vst3sdk pointer is very old. We are using the Jun 11 pointer and there have been 5 updates since, including ones from @whydoubt and others who have worked on surge.

I'm running into a vstgui problem now (when you setScale on a frame, opition menus appear at the wrong spot; they double-scale the mouse coordinate). Would love to know if it is fixed.

So I could, I suppose, pull vst3sdk to a latest (to hash 82380a8faa33824123104e005bcffb0451151d88) and see what happens

But before I do that work: Anyone object?

@baconpaul
Copy link
Collaborator Author

Hmm well I still think it might be time to update our vst3sdk pointer
but I was able to work around the vst2 bug a bit grossly
so I'm not going to do this work anytime soon unless I run into another unsquashable bug

@kurasu
Copy link
Collaborator

kurasu commented Jan 25, 2019

no objections here.. if it breaks everything we can always switch back

@baconpaul
Copy link
Collaborator Author

Just as note to self, since i may not do this today

If you do

git checkout -b vst3sdk-update-373
 git submodule update --remote --merge --recursive

the code no longer builds on mac. Looks like a namespace issue.

So this isn't an instant change.

@baconpaul
Copy link
Collaborator Author

From looking the problem seems to be of two types

  1. Things like CPoint and CColor are now in the VSTGUI namespace so either we need a lot of using namespace VSTGUI or a lot of VSTGUI:
  2. enums are scoped in classes. So kNoDrawStyle -> COptionMenu::kNoDrawStyle

tedious.

@whydoubt
Copy link
Contributor

I have a branch containing the needed changes (namespace and scoping), waiting for when they were useful. I've been busy elsewhere the last couple of weeks though, and hadn't been keeping up with the state of the project.

@baconpaul
Copy link
Collaborator Author

Can you attach a .patch file here at least? Or if you branch is rebaseable, toss in a PR? Would be good to know about how much work it is. Happy to work starting at your branch also if you want. Thank you.

@baconpaul
Copy link
Collaborator Author

OK I have this all building mac now with the updated pointer. Will begin some testing and seeing if it builds elsewhere.

baconpaul/vst3sdk-update-373 is my branch

@esaruoho
Copy link
Collaborator

hmm.. i wonder what kinds of gems we would find in https://github.com/whydoubt/surge/branches/all and how much of it is outdated (already implemented) and how much is yet to be merged in to the main repo.. would be interesting to know.

@baconpaul
Copy link
Collaborator Author

Dunno. I do know my vst3sdk update didn't build on windows when I tried, and I'm not going to fix it tonight. But we can get this pointer moved this week (which will also help @sagantech).

@baconpaul
Copy link
Collaborator Author

Oh and with the pointer moved, everything seems to work fine on mac. On windows, it's puking on a std::numeric_limits<uint32_t>::max()

@sagantech
Copy link
Contributor

sagantech commented Jan 27, 2019

Yes I had that too. Checking on fix...

@baconpaul
Copy link
Collaborator Author

Did you get around it?

@sagantech
Copy link
Contributor

Does your surgesynthesizerIO.cpp start like this:

// Copyright 2005 Claes Johanson & Vember Audio
//-------------------------------------------------------------------------------------------------------
#include "SurgeSynthesizer.h"
#include "DspUtilities.h"
#include <time.h>
#if MAC
#include <pthread.h>
#else
#define NOMINMAX 1

@sagantech
Copy link
Contributor

I think #define NOMINMAX 1 is the key

@baconpaul
Copy link
Collaborator Author

OK let me check

@baconpaul
Copy link
Collaborator Author

Yeah if I define NOMINMAX in premise it goes away. Thank you!

@baconpaul
Copy link
Collaborator Author

premake

@sagantech
Copy link
Contributor

No prob.

@baconpaul
Copy link
Collaborator Author

Don't suppose you figured out the IDropTarget one also did you?
That's the last one on my list.

@baconpaul
Copy link
Collaborator Author

Sigh. I have figured it out
If you do a using namespace VSTGUI too early then when you load windows.h it conflates with vstgui::idroptarget and ::idroptarget.
That means I need to do a lot of surgery to not do the (gross) thing of including using namespace in my .h which I had hoped would save us.
So damn tedious. Giving up for now.

baconpaul added a commit to baconpaul/surge that referenced this issue Jan 28, 2019
This substantial diff undertakes the mechanical changes to update
to the Jan 2019 master version of vst3sdk (hash 82380a8f). This
move consuming nearly 7 months of changes introduced several breaking
changes in our code which required modification, and introduced a couple of
new dependencies and links.

The primary thing breaking the builds was that most classes, including
common ones like CRect and CPoint, are now in the VSTGUI namespace.
This diff follows the convention of never using a namespace in a header
file, and thus has quite a few changes which are namespace additions in
headers and using statements in cpp files. But ironically, since my first
attempt was to just put using in a header and break the rule for once,
if you put using in the header, the code doesn't compile on windows
since VSTGUI::IDropTarget and ::IDropTarget collide. So has to be this way.

On linux several xcb and xkb dependencies emerge with this vst, so they
were added to the azure pipeline. Also the vstgui_linux.cpp is no longer
needed in the way we link, and if linked, doesn't work.

On windows, an ambiguity in std::numeric_limits<T>::max() requires us to
set -DNOMINMAX=1 for, alas.

On mac, we expand the number of errors reported to 'as many as you generate'.

Commands used to update the module were

 git checkout master
 git checkout -b vst3sdk-update-373
 git submodule update --remote --merge --recursive
 git add vst3sdk/
 git commit
 git submodule update --init --recursive

This commit addresses github issue surge-synthesizer#373.
@baconpaul
Copy link
Collaborator Author

OK yeah that was tedious. I got it done though. See the PR for details. Any eyeballs appreciated.

baconpaul added a commit to baconpaul/surge that referenced this issue Jan 28, 2019
This substantial diff undertakes the mechanical changes to update
to the Jan 2019 master version of vst3sdk (hash 82380a8f). This
move consuming nearly 7 months of changes introduced several breaking
changes in our code which required modification, and introduced a couple of
new dependencies and links.

The primary thing breaking the builds was that most classes, including
common ones like CRect and CPoint, are now in the VSTGUI namespace.
This diff follows the convention of never using a namespace in a header
file, and thus has quite a few changes which are namespace additions in
headers and using statements in cpp files. But ironically, since my first
attempt was to just put using in a header and break the rule for once,
if you put using in the header, the code doesn't compile on windows
since VSTGUI::IDropTarget and ::IDropTarget collide. So has to be this way.

On linux several xcb and xkb dependencies emerge with this vst, so they
were added to the azure pipeline. Also the vstgui_linux.cpp is no longer
needed in the way we link, and if linked, doesn't work.

On windows, an ambiguity in std::numeric_limits<T>::max() requires us to
set -DNOMINMAX=1 for the time being, alas.

On mac, we expand the number of errors reported to 'as many as you generate'.

Commands used to update the module were

 git checkout master
 git checkout -b vst3sdk-update-373
 git submodule update --remote --merge --recursive
 git add vst3sdk/
 git commit
 git submodule update --init --recursive

This commit addresses github issue surge-synthesizer#373.
@whydoubt
Copy link
Contributor

Just got mine rebased. Haven't tested it though.
https://github.com/whydoubt/surge/tree/sdk_update

@baconpaul
Copy link
Collaborator Author

@whydoubt cool - i got a pr together last night that builds and works Mac win Lin. Curious how you solved the windows numeric-limits max problem tho? @sagantech found and I confirmed that nominmax in windows dealt with it but wonder if you found another way.

Thanks!

baconpaul added a commit that referenced this issue Jan 28, 2019
This substantial diff undertakes the mechanical changes to update
to the Jan 2019 master version of vst3sdk (hash 82380a8f). This
move consuming nearly 7 months of changes introduced several breaking
changes in our code which required modification, and introduced a couple of
new dependencies and links.

The primary thing breaking the builds was that most classes, including
common ones like CRect and CPoint, are now in the VSTGUI namespace.
This diff follows the convention of never using a namespace in a header
file, and thus has quite a few changes which are namespace additions in
headers and using statements in cpp files. But ironically, since my first
attempt was to just put using in a header and break the rule for once,
if you put using in the header, the code doesn't compile on windows
since VSTGUI::IDropTarget and ::IDropTarget collide. So has to be this way.

On linux several xcb and xkb dependencies emerge with this vst, so they
were added to the azure pipeline. Also the vstgui_linux.cpp is no longer
needed in the way we link, and if linked, doesn't work.

On windows, an ambiguity in std::numeric_limits<T>::max() requires us to
set -DNOMINMAX=1 for the time being, alas.

On mac, we expand the number of errors reported to 'as many as you generate'.

Commands used to update the module were

 git checkout master
 git checkout -b vst3sdk-update-373
 git submodule update --remote --merge --recursive
 git add vst3sdk/
 git commit
 git submodule update --init --recursive

This commit addresses github issue #373.
@baconpaul
Copy link
Collaborator Author

OK merged into master. @whydoubt if you have a better way than the NOMINMAX I'm all ears! Can you just drop it into another issue? Thanks!

baconpaul added a commit to baconpaul/surge that referenced this issue Jul 10, 2019
This substantial diff undertakes the mechanical changes to update
to the Jan 2019 master version of vst3sdk (hash 82380a8f). This
move consuming nearly 7 months of changes introduced several breaking
changes in our code which required modification, and introduced a couple of
new dependencies and links.

The primary thing breaking the builds was that most classes, including
common ones like CRect and CPoint, are now in the VSTGUI namespace.
This diff follows the convention of never using a namespace in a header
file, and thus has quite a few changes which are namespace additions in
headers and using statements in cpp files. But ironically, since my first
attempt was to just put using in a header and break the rule for once,
if you put using in the header, the code doesn't compile on windows
since VSTGUI::IDropTarget and ::IDropTarget collide. So has to be this way.

On linux several xcb and xkb dependencies emerge with this vst, so they
were added to the azure pipeline. Also the vstgui_linux.cpp is no longer
needed in the way we link, and if linked, doesn't work.

On windows, an ambiguity in std::numeric_limits<T>::max() requires us to
set -DNOMINMAX=1 for the time being, alas.

On mac, we expand the number of errors reported to 'as many as you generate'.

Commands used to update the module were

 git checkout master
 git checkout -b vst3sdk-update-373
 git submodule update --remote --merge --recursive
 git add vst3sdk/
 git commit
 git submodule update --init --recursive

This commit addresses github issue surge-synthesizer#373.

Former-commit-id: 3ecc5ad39a32ba04aa1edd7bc1073e8775abe781 [formerly eb31ce4]
Former-commit-id: a501dd286059614c336111cf3cb9fc73a0a76394
Former-commit-id: fd155d3393e0b4b1ad4f7d19c8e69b220e6275c9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants