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.

Sign up for GitHub

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

Jump to bottom

vstgui - applying some changes #515

Closed
baconpaul opened this issue Feb 6, 2019 · 9 comments
Closed

vstgui - applying some changes #515

baconpaul opened this issue Feb 6, 2019 · 9 comments
Labels
Design Required We need to design a solution to this issue

Comments

@baconpaul
Copy link
Collaborator

baconpaul commented Feb 6, 2019 •
edited
Loading

I am writing this issue mostly so folks like @jsakkine @falkTX @kzantow @kurasu @esaruoho and others can talk me out of the path I'm thinking of.

We've come across several VSTGUI errors. @sagantech identified quite a few and put them as issues on the VSTGUI repo; I worked around some but just discovered the vst3sdk upgrade broke drag n drop maybe; Linux isn't painting properly. I think it is inevitable we will need to patch vstgui soon.

Now obviously the ideal world would be: we find a patch, we send a PR to steinberg, they integrate it quickly and seamlessly, we do a pointer upgrade on our submodule, and we rebuild.

But it doesn't seem that's necessarily timely. So I had a thought

How about we fork steinberg/vstgui into surge-synthesizer. Once we've made that fork we never touch the master branch. We just constantly pull that from upstream.

But we do three things

  1. We add a branch called 'surge' to our repo in vstgui
  2. We add another submodule "vstgui.surge" which points at our repo and that branch and modify premake to use that software, not the software in vst3sdk/vstgui
  3. We adopt the following workflow
    • Find a vstgui bug we make a new branch in surge (say "surge-menu-check-bug")
    • We send steinberg a PR from that branch
    • But also we merge that branch int our cumulative changes surge branch and move our pointer

That way we are not running any changes which aren't sent as PRs to steinberg, but we can also run ahead of them with fixes.

Every so often, we rebase our surge branch to master and so on.

It's some tricky git machinery but I don't see how we are going to avoid this. Bugs like #394 are real ones.

Your thoughts welcome.
@baconpaul baconpaul added the Design Required We need to design a solution to this issue label Feb 6, 2019
@falkTX
Copy link
Contributor

falkTX commented Feb 6, 2019

I think a vst3sdk fork (for updated vstgui) makes sense, I was trying to push from it from the start.
vstgui development (for linux) can be slow, and since it ends up being part of the binary (it is compiled in), makes sense to have control over it.
waiting for upstream to do new releases is going to take forever...

@sagantech
Copy link
Contributor

I think a vst3sdk fork (for updated vstgui) makes sense, I was trying to push from it from the start.
vstgui development (for linux) can be slow, and since it ends up being part of the binary (it is compiled in), makes sense to have control over it.
waiting for upstream to do new releases is going to take forever...

100% agree.

@baconpaul
Copy link
Collaborator Author

Ok. Does the protocol I suggest seem to be missing anything important?

I think you are correct. We will have to do this. Not happy about it but let's try and do it in a safe way.

Thanks

@baconpaul
Copy link
Collaborator Author

Oh and @sagantech if I get time to do this do you mind if I walk a couple of your changes through the protocol? Presume you are happy not fighting git any more than you have to.

@baconpaul
Copy link
Collaborator Author

Finally I checked and the usage contemplated here is well within the terms of the vstgui license so we are OK on that front. We don't even have to push our PRs back to them if we don't want! (We do need to update our docs to link to their license more cleanly but I put an issue on the website repo for that).

Absent strenuous objections in the next 24 hours or so, I will make this next on my list of "big chunks of dev work" since it will allow us to close quite a few tickets.

@sagantech
Copy link
Contributor

Please implement any changes I have suggested. The whole point of me suggesting them was to improve Surge. I don't care who they get attributed to.

@esaruoho
Copy link
Collaborator

esaruoho commented Feb 6, 2019

@sagantech which changes are those? could you link the issues? you can link an issue by referring to it like #592 (type the # and then the number of the issue).

@sagantech
Copy link
Contributor

@esaruoho I was simply replying to @baconpaul comment:

Oh and @sagantech if I get time to do this do you mind if I walk a couple of your changes through the protocol? Presume you are happy not fighting git any more than you have to.

@baconpaul
Copy link
Collaborator Author

screen shot 2019-02-06 at 4 39 57 pm

Hey here's the right image

So that's building surge using our fork of vstgui with @sagantech's menu change attached

All mergeable and doable

Need to write up some docs and do a couple of other bits of git magic but this will all work fine.

@baconpaul baconpaul mentioned this issue Feb 6, 2019
Merged
baconpaul added a commit to baconpaul/surge that referenced this issue Feb 8, 2019
@baconpaul
Build Surge with a fork of VSTGUI
12aca83 
This diff begins building Surge with a fork of VSTGUI rather than the
VSTGUI which ships with the vst3sdk pointer. It accomplishes it by the following:

1. there is a fork of vstgui called `surge-synthesizer/vstgui` with a branch
called `surge` which is master + diffs
2. There is a submodule in surge called `vstgui.surge` which points at that
branch
3. There are changes to premake to point our vstgui implementation

This version points to a surge which integrates three of @sagantech's
changes

Closes surge-synthesizer#394: Cannot move window after selecting preset
Closes surge-synthesizer#396: Diacritical marks in menus
Changes for submenu checkmarks on windows

Also this diff implements our fork strategy described in surge-synthesizer#515 and
closes surge-synthesizer#515 as a result.
baconpaul added a commit to baconpaul/surge that referenced this issue Jul 10, 2019
@baconpaul
Build Surge with a fork of VSTGUI (surge-synthesizer#527)
50baf3c 
This diff begins building Surge with a fork of VSTGUI rather than the
VSTGUI which ships with the vst3sdk pointer. It accomplishes it by the following:

1. there is a fork of vstgui called `surge-synthesizer/vstgui` with a branch
called `surge` which is master + diffs
2. There is a submodule in surge called `vstgui.surge` which points at that
branch
3. There are changes to premake to point our vstgui implementation

This version points to a vstgui which integrates three of @sagantech's
changes

Closes surge-synthesizer#394: Cannot move window after selecting preset
Closes surge-synthesizer#396: Diacritical marks in menus
Changes for submenu checkmarks on windows

Also this diff implements our fork strategy described in surge-synthesizer#515 and
closes surge-synthesizer#515 as a result.

Former-commit-id: ce8bc4274c88df2915b9b3abac26078f273959a6 [formerly 7314657]
Former-commit-id: 0c72f1c331f2281cbd9de138a495d999c172a39c
Former-commit-id: c97dce7fb3bbc8c8d3a44af05e4c7b8022c6fde1
@bobobo199733 bobobo199733 mentioned this issue Mar 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Design Required We need to design a solution to this issue
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@falkTX @esaruoho @baconpaul @sagantech