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

Upgrade ZynAddSubFx to version 2.5 #1991

Closed
wants to merge 60 commits into
base: master
from

Conversation

Projects
None yet
@curlymorphic
Contributor

curlymorphic commented Apr 20, 2015

Open Bugs

Bug Fixed Reporter Description Affects Wine 64 32
1 - @curlymorphic crash when clicking apply changes when editing a pad synth - - y
2 - @curlymorphic crash, when opening bank presets from the zyn gui, @tresf's notes here: #1991 (comment) - - y
3 - @tresf File, Open Parameters 3x hangs/crashes software Yes y y
4 - @musikBear zasfx crashes immediately when the zasfx-gui button is clicked - - y

This is the upgrade for the ZynAddSubFx instrument.

We will need the windows dependencies before this can be merged but I believe we may get them today.

Including updates from @fundamental so we can have more than one instance.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Apr 20, 2015

Member

@curlymorphic I believe the Zyn stuff needs to be PR'd against the Zyn project located here:

https://github.com/LMMS/zynaddsubfx

This of course requires the decoupling of Zyn and LMMS changes.

Member

tresf commented Apr 20, 2015

@curlymorphic I believe the Zyn stuff needs to be PR'd against the Zyn project located here:

https://github.com/LMMS/zynaddsubfx

This of course requires the decoupling of Zyn and LMMS changes.

@curlymorphic

This comment has been minimized.

Show comment
Hide comment
@curlymorphic

curlymorphic Apr 21, 2015

Contributor

I believe the Zyn stuff needs to be PR'd against the Zyn project located here:

Sure, will separate that and do a PR in the morning.

Contributor

curlymorphic commented Apr 21, 2015

I believe the Zyn stuff needs to be PR'd against the Zyn project located here:

Sure, will separate that and do a PR in the morning.

ZynAddSubFx stopped the Freeze when opening the ui
I have added 2 new Flags isPlaying and isLoading.
If the plugin is in the load state when  processAudio() is called
 the buffer remains empty, but the priory inversion is evaded.

When loading the plugin, isPlying is checked, and the current audio buffer
is proessed before the loading takes place.
@curlymorphic

This comment has been minimized.

Show comment
Hide comment
@curlymorphic

curlymorphic Apr 21, 2015

Contributor

I have managed to get the opening and closing of the ui to no longer freeze or segfaults I have posted the details in #1860.

Before I split this into 2 patches, would it be possible to have a code review please. I would rather not have to make changes to both branches as I can see errors creeping in.

I assume when I make the second pull request, I leave this pull request as is, so the non zyn changes are merged?

I assume the second pull request is just to contain corrisponding files and folders to whats already in https://github.com/LMMS/zynaddsubfx?

Is there anything else I need to do that I have over looked?

Contributor

curlymorphic commented Apr 21, 2015

I have managed to get the opening and closing of the ui to no longer freeze or segfaults I have posted the details in #1860.

Before I split this into 2 patches, would it be possible to have a code review please. I would rather not have to make changes to both branches as I can see errors creeping in.

I assume when I make the second pull request, I leave this pull request as is, so the non zyn changes are merged?

I assume the second pull request is just to contain corrisponding files and folders to whats already in https://github.com/LMMS/zynaddsubfx?

Is there anything else I need to do that I have over looked?

@curlymorphic

This comment has been minimized.

Show comment
Hide comment
@curlymorphic

curlymorphic Apr 21, 2015

Contributor

Maybe this will help with any code review. It's the diff between what Toby done, and my work.

master-zynaddsubfx-experimental...curlymorphic:zynUpgrade

Contributor

curlymorphic commented Apr 21, 2015

Maybe this will help with any code review. It's the diff between what Toby done, and my work.

master-zynaddsubfx-experimental...curlymorphic:zynUpgrade

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Apr 21, 2015

Member

@curlymorphic naturally reviewing a PR with 208 changed files can be a bit overwhelming, but focusing at the few LMMS-only files, this looks very good.

I also like the use of counters, and the new liblo/MiddleWare usage. :)

@lukas-w, once we merge this in, Apple Travis builds will continuously fail since Homebrew doesn't have recipes for rtosc yet.

-Tres

Member

tresf commented Apr 21, 2015

@curlymorphic naturally reviewing a PR with 208 changed files can be a bit overwhelming, but focusing at the few LMMS-only files, this looks very good.

I also like the use of counters, and the new liblo/MiddleWare usage. :)

@lukas-w, once we merge this in, Apple Travis builds will continuously fail since Homebrew doesn't have recipes for rtosc yet.

-Tres

@curlymorphic

This comment has been minimized.

Show comment
Hide comment
@curlymorphic

curlymorphic Apr 21, 2015

Contributor

I will list all the changes I made to non lmms files and there reasons here.

plugins/zynaddsubfx/zynaddsubfx/src/Effects/Effect.h

Wrapped the Effect Class in the Zyn namespace, to avoid linking errors with the lmms Effect class

plugins/zynaddsubfx/zynaddsubfx/src/Effects/*.h

Added using namespace Zyn; to all subclasses of effect, due to above change.

plugins/zynaddsubfx/zynaddsubfx/src/Misc/MiddleWare.cpp

commented out debuging messages.

plugins/zynaddsubfx/zynaddsubfx/src/Effects/Echo.h l75
commented out declaration of int samplerate. this was overrideing the base samplerate.

The remaining changes are patched from @fundamental addressing the static rtosc buffers, that would not allow Lmms to run multiple instance, and to remove make warnings

Contributor

curlymorphic commented Apr 21, 2015

I will list all the changes I made to non lmms files and there reasons here.

plugins/zynaddsubfx/zynaddsubfx/src/Effects/Effect.h

Wrapped the Effect Class in the Zyn namespace, to avoid linking errors with the lmms Effect class

plugins/zynaddsubfx/zynaddsubfx/src/Effects/*.h

Added using namespace Zyn; to all subclasses of effect, due to above change.

plugins/zynaddsubfx/zynaddsubfx/src/Misc/MiddleWare.cpp

commented out debuging messages.

plugins/zynaddsubfx/zynaddsubfx/src/Effects/Echo.h l75
commented out declaration of int samplerate. this was overrideing the base samplerate.

The remaining changes are patched from @fundamental addressing the static rtosc buffers, that would not allow Lmms to run multiple instance, and to remove make warnings

@fundamental

This comment has been minimized.

Show comment
Hide comment
@fundamental

fundamental Apr 21, 2015

Contributor

It looks pretty reasonable to me, though I haven't really studied the lmms side of things. The only complaint is that there's a bunch of whitespace changes which might make merging in future changes marginally more difficult.

Contributor

fundamental commented Apr 21, 2015

It looks pretty reasonable to me, though I haven't really studied the lmms side of things. The only complaint is that there's a bunch of whitespace changes which might make merging in future changes marginally more difficult.

@curlymorphic

This comment has been minimized.

Show comment
Hide comment
@curlymorphic

curlymorphic Apr 21, 2015

Contributor

Removed changed whitespace.

Contributor

curlymorphic commented Apr 21, 2015

Removed changed whitespace.

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Apr 21, 2015

Member

I think liblo and rtosc should be added to the linux install script for travis, since this PR is failing for every platform atm.

Member

Umcaruje commented Apr 21, 2015

I think liblo and rtosc should be added to the linux install script for travis, since this PR is failing for every platform atm.

@curlymorphic

This comment has been minimized.

Show comment
Hide comment
@curlymorphic

curlymorphic Apr 21, 2015

Contributor

I think liblo and rtosc should be added to the linux install script for travis, since this PR is failing for every platform atm.

They will also need adding to a repository. Until this very moment That thought had not occured to me.
What repository does travis use. I dont think rtosc is in any public repos.

@tresf Any Idea what I have do, to achive this?

Contributor

curlymorphic commented Apr 21, 2015

I think liblo and rtosc should be added to the linux install script for travis, since this PR is failing for every platform atm.

They will also need adding to a repository. Until this very moment That thought had not occured to me.
What repository does travis use. I dont think rtosc is in any public repos.

@tresf Any Idea what I have do, to achive this?

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Apr 21, 2015

Member

What repository does travis use. I dont think rtosc is in any public repos.

Currently, travis uses Ubuntu's PPA as well as Toby's PPA and the Mac stuff uses Homebrew's repos.

@tresf Any Idea what I have do, to achive this?

No I'm not aware of the steps specifically, but Israel gave us his PPA steps about a year ago, so that might be a good building block: https://github.com/LMMS/lmms/wiki/Packaging-Ubuntu-PPA

Mark may have a better idea for permanent packaging. I'm not sure if he's involved in binaries repos or not or if perhaps Filipe from KX would be willing to help out (since his LMMS PPA is by far the best and most popular Debian/*buntu mirror we have).

-Tres

Member

tresf commented Apr 21, 2015

What repository does travis use. I dont think rtosc is in any public repos.

Currently, travis uses Ubuntu's PPA as well as Toby's PPA and the Mac stuff uses Homebrew's repos.

@tresf Any Idea what I have do, to achive this?

No I'm not aware of the steps specifically, but Israel gave us his PPA steps about a year ago, so that might be a good building block: https://github.com/LMMS/lmms/wiki/Packaging-Ubuntu-PPA

Mark may have a better idea for permanent packaging. I'm not sure if he's involved in binaries repos or not or if perhaps Filipe from KX would be willing to help out (since his LMMS PPA is by far the best and most popular Debian/*buntu mirror we have).

-Tres

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Jul 12, 2015

Member

But i have had very strict orders from tresf to NEVER EVER mention this xp only bug EVER again.. EVER.. !

That may be true, but we need more testing on different versions of windows, to Confirm what platfroms are having problems, this new issue may not be an xp only bug, and may be fixable

His comment is in regards to an XP-only issue that's been around for a while. Fixing it is low priority (we've gone as far as to call it a won't-fix) because we can't reproduce it on any other platforms besides XP.

As far as testing on other OSs in general, that's always a good idea, whether it's XP, Vista, 7, 8, 10, ReactOS.... :)

Member

tresf commented Jul 12, 2015

But i have had very strict orders from tresf to NEVER EVER mention this xp only bug EVER again.. EVER.. !

That may be true, but we need more testing on different versions of windows, to Confirm what platfroms are having problems, this new issue may not be an xp only bug, and may be fixable

His comment is in regards to an XP-only issue that's been around for a while. Fixing it is low priority (we've gone as far as to call it a won't-fix) because we can't reproduce it on any other platforms besides XP.

As far as testing on other OSs in general, that's always a good idea, whether it's XP, Vista, 7, 8, 10, ReactOS.... :)

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Jul 13, 2015

Member

It failed in install:
zynfail

@musikBear if you can't do a proper uninstall and reinstall you should not be submitting bug reports.

Member

tresf commented Jul 13, 2015

It failed in install:
zynfail

@musikBear if you can't do a proper uninstall and reinstall you should not be submitting bug reports.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Jul 13, 2015

Member

I've started a bug tracking table in the original post. Please update as necessary. I'm looking into the preset crash now.

Member

tresf commented Jul 13, 2015

I've started a bug tracking table in the original post. Please update as necessary. I'm looking into the preset crash now.

@musikBear

This comment has been minimized.

Show comment
Hide comment
@musikBear

musikBear Jul 13, 2015

@tresf

if you can't do a proper uninstall and reinstall you should not be submitting bug reports.

..sometimes you are
i dunno
I normally do not participate in mudslinging sessions, but -This time i will do exactly as i use to do
So i wont speak a word
🙈 🙊

musikBear commented Jul 13, 2015

@tresf

if you can't do a proper uninstall and reinstall you should not be submitting bug reports.

..sometimes you are
i dunno
I normally do not participate in mudslinging sessions, but -This time i will do exactly as i use to do
So i wont speak a word
🙈 🙊

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Jul 13, 2015

Member

Your screenshot says 0.4.x. I shouldn't need to explain any further.

@musikBear To be a bit more diplomatic, I'll rephrase...

If you cannot completely uninstall LMMS from your system in-between tests, your results are invalid as we do not know the state of your system at the time of testing which means it is not an accurate test.

When the 0.4.5 version appears in your screenshots, that is symptomatic of an improper uninstall. The lingering DLLs and registry keys from an missed or incomplete uninstall could cause issues far beyond the scope of this bug report.

In other words... Please purge LMMS from your system and reinstall to ensure that the bugs you are reporting are attributed to the zynwin branch only.

Member

tresf commented Jul 13, 2015

Your screenshot says 0.4.x. I shouldn't need to explain any further.

@musikBear To be a bit more diplomatic, I'll rephrase...

If you cannot completely uninstall LMMS from your system in-between tests, your results are invalid as we do not know the state of your system at the time of testing which means it is not an accurate test.

When the 0.4.5 version appears in your screenshots, that is symptomatic of an improper uninstall. The lingering DLLs and registry keys from an missed or incomplete uninstall could cause issues far beyond the scope of this bug report.

In other words... Please purge LMMS from your system and reinstall to ensure that the bugs you are reporting are attributed to the zynwin branch only.

@musikBear

This comment has been minimized.

Show comment
Hide comment
@musikBear

musikBear Jul 13, 2015

tresf..........
"LMMS 0.4.5" is a f o l d e r name
A folder
a library
i know you are not all that familiar with windows terminology, but then diplomatic phrasing should be even more important.
In fact -i think that diplomatic phrasing always should be chosen over less diplomatic phrasing. That would in general spare anyone to repost a set of sentences, rewritten from harshness, to a 'more diplomatic phrasing'
That is all for this time.
But man, were you dead wrong here @tresf

musikBear commented Jul 13, 2015

tresf..........
"LMMS 0.4.5" is a f o l d e r name
A folder
a library
i know you are not all that familiar with windows terminology, but then diplomatic phrasing should be even more important.
In fact -i think that diplomatic phrasing always should be chosen over less diplomatic phrasing. That would in general spare anyone to repost a set of sentences, rewritten from harshness, to a 'more diplomatic phrasing'
That is all for this time.
But man, were you dead wrong here @tresf

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Jul 13, 2015

Member

"LMMS 0.4.5" is a f o l d e r name

Semantics aside, this is pre-populated because your registry is dirty. Here are manual purge instructions:

#2077 (comment)

-i think that diplomatic phrasing always should be chosen over less diplomatic phrasing. That would in general spare anyone to repost a set of sentences, rewritten from harshness, to a 'more diplomatic phrasing'

Well, I'm sorry then. Please be advised, when you post screenshots showing QtCore4.dll is in use -- that message is a classic resource-in-use issue -- so I hope you can understand how posting that can slow down the bug hunting.

i know you are not all that familiar with windows terminology

If you believe this to be true, I invite you to elaborate (albeit off-topic, IMO).

-Tres

Member

tresf commented Jul 13, 2015

"LMMS 0.4.5" is a f o l d e r name

Semantics aside, this is pre-populated because your registry is dirty. Here are manual purge instructions:

#2077 (comment)

-i think that diplomatic phrasing always should be chosen over less diplomatic phrasing. That would in general spare anyone to repost a set of sentences, rewritten from harshness, to a 'more diplomatic phrasing'

Well, I'm sorry then. Please be advised, when you post screenshots showing QtCore4.dll is in use -- that message is a classic resource-in-use issue -- so I hope you can understand how posting that can slow down the bug hunting.

i know you are not all that familiar with windows terminology

If you believe this to be true, I invite you to elaborate (albeit off-topic, IMO).

-Tres

@curlymorphic

This comment has been minimized.

Show comment
Hide comment
@curlymorphic

curlymorphic Jul 16, 2015

Contributor

copy and paste from #1911

Yeah, IIRC, the title "feature freeze" was introduced by Vesa in one of the few posts he has made since April. This thread probably isn't the place to discuss this, but since we're on the topic I'll elaborate a bit...

At this point, we could easily freeze and release RC1 without Zyn and @curlymorphic and I are both OK with this.

@Umcaruje, yourself (@Wallacoloo), @curlymorphic have made some progress getting Zyn functional, but we're still stuck on a few hard-crash items. Overall, Zyn 2.5 has postponed the 1.2 release since at least April. That combined with some very busy schedules and we're here in July. :)

As far as the feature freeze, that's normally done on a dedicated branch to allow master to continue getting features, but we've held off on splitting to a stable-1.2 branch intentionally so that the rather complex Zyn 2.5 code can be merged before the branch occurs, but we can certainly hold off on that until 1.3 (or 2.0, depending on who you ask).

Again, this is probably the wrong area to discuss this, but if we can reach a mutual decision on Zyn 2.5, we'll know how to proceed. Thoughts welcome.

-Tres

Contributor

curlymorphic commented Jul 16, 2015

copy and paste from #1911

Yeah, IIRC, the title "feature freeze" was introduced by Vesa in one of the few posts he has made since April. This thread probably isn't the place to discuss this, but since we're on the topic I'll elaborate a bit...

At this point, we could easily freeze and release RC1 without Zyn and @curlymorphic and I are both OK with this.

@Umcaruje, yourself (@Wallacoloo), @curlymorphic have made some progress getting Zyn functional, but we're still stuck on a few hard-crash items. Overall, Zyn 2.5 has postponed the 1.2 release since at least April. That combined with some very busy schedules and we're here in July. :)

As far as the feature freeze, that's normally done on a dedicated branch to allow master to continue getting features, but we've held off on splitting to a stable-1.2 branch intentionally so that the rather complex Zyn 2.5 code can be merged before the branch occurs, but we can certainly hold off on that until 1.3 (or 2.0, depending on who you ask).

Again, this is probably the wrong area to discuss this, but if we can reach a mutual decision on Zyn 2.5, we'll know how to proceed. Thoughts welcome.

-Tres

@curlymorphic

This comment has been minimized.

Show comment
Hide comment
@curlymorphic

curlymorphic Jul 16, 2015

Contributor

I feel we are at a point where we need to make a decision on the timing of this zasf upgrade.

I currently have no idea of the amount of work required. The history of this upgrade, has been just 1 more bug fix away from completion, since 20 april when i opened this pull request. Whist the current bug list is small, they are mainly critical bugs, I am unsure, when debugging on widow what else may arise.

I am very conscious of the amount of time this has been holding up the release of the 1.2 testing phase.

I am proposing 3 options.

  1. Carry on with the ZASF upgrade, then start the 1.2 testing.
  2. Create a 1.2 branch, and start 1.2 rc testing now. With a view to a 1.2.1 release for the ZASF upgrade. This would allow us to proceed with 1.2, and then merge in this branch after release.
  3. Put all our efforts into testing/fixing/releasing 1.2, then resume with this project.

Thoughts appreciated.

Contributor

curlymorphic commented Jul 16, 2015

I feel we are at a point where we need to make a decision on the timing of this zasf upgrade.

I currently have no idea of the amount of work required. The history of this upgrade, has been just 1 more bug fix away from completion, since 20 april when i opened this pull request. Whist the current bug list is small, they are mainly critical bugs, I am unsure, when debugging on widow what else may arise.

I am very conscious of the amount of time this has been holding up the release of the 1.2 testing phase.

I am proposing 3 options.

  1. Carry on with the ZASF upgrade, then start the 1.2 testing.
  2. Create a 1.2 branch, and start 1.2 rc testing now. With a view to a 1.2.1 release for the ZASF upgrade. This would allow us to proceed with 1.2, and then merge in this branch after release.
  3. Put all our efforts into testing/fixing/releasing 1.2, then resume with this project.

Thoughts appreciated.

@musikBear

This comment has been minimized.

Show comment
Hide comment
@musikBear

musikBear Jul 16, 2015

i fear that the zasfx update may act more difficult, than expected.
For that reason i lean towards '3', but i know how absurdly fast in and outs of complex code simply is vaporized, by even a few weeks away from the code. @curlymorphic You are now so emerged in the zasfx code, that leaving it, could be a 'vaporize situation', so i kind of think you should not put that aside now, but stay on that. The schedule for the zyn-update should then be 1.2.1, which then could be a dedicated zasfx-upgrade release.
-But if the choice is between 1,2 & 3 - i would say 3

musikBear commented Jul 16, 2015

i fear that the zasfx update may act more difficult, than expected.
For that reason i lean towards '3', but i know how absurdly fast in and outs of complex code simply is vaporized, by even a few weeks away from the code. @curlymorphic You are now so emerged in the zasfx code, that leaving it, could be a 'vaporize situation', so i kind of think you should not put that aside now, but stay on that. The schedule for the zyn-update should then be 1.2.1, which then could be a dedicated zasfx-upgrade release.
-But if the choice is between 1,2 & 3 - i would say 3

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Jul 16, 2015

Member

My heart says 1, but my obligation to the community says 3. I vote 3.

Member

tresf commented Jul 16, 2015

My heart says 1, but my obligation to the community says 3. I vote 3.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine
Member

zonkmachine commented Jul 16, 2015

3

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Jul 21, 2015

Member

A bit of news from the Windows building attempts, we have a working Windows build of master that was created with msys2 on Windows 64-bit.

Here's the tutorial (it has much room for improvement):
https://github.com/LMMS/lmms/wiki/Compiling-lmms-(On-Windows)

Next steps are creating a debug build and running lmms.exe against the debugger.

image

image

Member

tresf commented Jul 21, 2015

A bit of news from the Windows building attempts, we have a working Windows build of master that was created with msys2 on Windows 64-bit.

Here's the tutorial (it has much room for improvement):
https://github.com/LMMS/lmms/wiki/Compiling-lmms-(On-Windows)

Next steps are creating a debug build and running lmms.exe against the debugger.

image

image

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Jul 24, 2015

Member

I am very conscious of the amount of time this has been holding up the release of the 1.2 testing phase.

But 1.1 was released in December. That was just half a year ago so maybe we don't need to rush 1.2 It would be really cool to see this one in the next release.

Member

zonkmachine commented Jul 24, 2015

I am very conscious of the amount of time this has been holding up the release of the 1.2 testing phase.

But 1.1 was released in December. That was just half a year ago so maybe we don't need to rush 1.2 It would be really cool to see this one in the next release.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Jul 24, 2015

Member

It would be really cool to see this one in the next release.

😎

Member

tresf commented Jul 24, 2015

It would be really cool to see this one in the next release.

😎

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Sep 15, 2015

Member

I'm bumping this to milestone 1.3 unless something drastic happens beforehand. 👍

Member

tresf commented Sep 15, 2015

I'm bumping this to milestone 1.3 unless something drastic happens beforehand. 👍

@tresf tresf added the enhancement label Sep 15, 2015

@tresf tresf added this to the 1.3.0 milestone Sep 15, 2015

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Feb 29, 2016

Member

@LMMS/developers can someone with git super powers do a few things things:

  1. Merge this commit into something usable? Dave and I kept hitting issues when trying to merge d71e78b.
  2. Make this a new upstream branch e.g. zyn-2.5
  3. Then all @LMMS/developers can work from the same branch and submit patches as needed?

Some background, @curlymorphic had this working 95% on Linux, but Windows Show GUI button wasn't working (and Mac's never worked).

This branch (or more appropriately, it's bugs) inspired us to work towards a true Windows build/debug environment (99% done via #2205), #2177 (comment).

Member

tresf commented Feb 29, 2016

@LMMS/developers can someone with git super powers do a few things things:

  1. Merge this commit into something usable? Dave and I kept hitting issues when trying to merge d71e78b.
  2. Make this a new upstream branch e.g. zyn-2.5
  3. Then all @LMMS/developers can work from the same branch and submit patches as needed?

Some background, @curlymorphic had this working 95% on Linux, but Windows Show GUI button wasn't working (and Mac's never worked).

This branch (or more appropriately, it's bugs) inspired us to work towards a true Windows build/debug environment (99% done via #2205), #2177 (comment).

@Fastigium

This comment has been minimized.

Show comment
Hide comment
@Fastigium

Fastigium Feb 29, 2016

Contributor

@tresf I've been reading the history on this a bit, and would like to be sure what needs to be done before I try anything. Therefore, some questions:

  1. With "merge this commit into something usable", do you mean "merge or rebase this pull request onto the current master"?
  2. What about @curlymorphic's zynwin branch? Am I right in assuming that there is windows-specific work already done in that branch that should be included somehow?
  3. With "new upstream branch", do you mean a branch in the LMMS/lmms repo?

Also, it seems to me that the merging/rebasing would become a lot easier if the commits in this PR were squashed first. Does that seems like a good idea? And is it alright if everything is squashed into a single commit or should some kind of logical separation be maintained?

And on a final note: 1.2.0 is not quite there yet. Per the response to #1991 (comment), is this the right moment to resume work on this?

Contributor

Fastigium commented Feb 29, 2016

@tresf I've been reading the history on this a bit, and would like to be sure what needs to be done before I try anything. Therefore, some questions:

  1. With "merge this commit into something usable", do you mean "merge or rebase this pull request onto the current master"?
  2. What about @curlymorphic's zynwin branch? Am I right in assuming that there is windows-specific work already done in that branch that should be included somehow?
  3. With "new upstream branch", do you mean a branch in the LMMS/lmms repo?

Also, it seems to me that the merging/rebasing would become a lot easier if the commits in this PR were squashed first. Does that seems like a good idea? And is it alright if everything is squashed into a single commit or should some kind of logical separation be maintained?

And on a final note: 1.2.0 is not quite there yet. Per the response to #1991 (comment), is this the right moment to resume work on this?

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Feb 29, 2016

Member
  1. With "merge this commit into something usable", do you mean "merge or rebase this pull request onto the current master"?

Merging was the major hurdle originally, but rebasing against current master would eventually have to happen as well.

  1. What about @curlymorphic's zynwin branch? Am I right in assuming that there is windows-specific work already done in that branch that should be included somehow?

Very good question, I had forgotten about that branch. I can email him directly if needed but the comparison isn't helping me much to understand what's different other than what appears to be a bunch of fast-forwarded commits. Let me know if you need me to reach out to him. 👍 curlymorphic/lmms@zynUpgrade...curlymorphic:zynwin

  1. With "new upstream branch", do you mean a branch in the LMMS/lmms repo?

Yes. Two disadvantages to use our own personal forks for collab items is

  1. Out of sight out of mind
  2. Lack of permission from other admins causes use to fork against forks and PR against PRs, etc.

We don't do a lot of collab items currently, but I think we have a stable enough dev team currently to use this process. You'll notice @lukas-w uses this technique for 99% of his commits, allowing stuff like this to happen.

Also, it seems to me that the merging/rebasing would become a lot easier if the commits in this PR were squashed first. Does that seems like a good idea? And is it alright if everything is squashed into a single commit or should some kind of logical separation be maintained?

Another great question. From a high-level, the Zyn source update needs to be in a separate commit so we can at least isolate our changes from upstream Zyn changes... but .... as each one of Dave's commits are reviewed, this may offer time to split things up. @grejppi has stressed that squashing everything isn't always a good thing, some commits are best left separated. 👍

And on a final note: 1.2.0 is not quite there yet. Per the response to #1991 (comment), is this the right moment to resume work on this?

Unless something drastic happens to the state of Zyn 2.5, this certainly won't make 1.2, so yes, this can certainly be shelved for a later date. 👍

Member

tresf commented Feb 29, 2016

  1. With "merge this commit into something usable", do you mean "merge or rebase this pull request onto the current master"?

Merging was the major hurdle originally, but rebasing against current master would eventually have to happen as well.

  1. What about @curlymorphic's zynwin branch? Am I right in assuming that there is windows-specific work already done in that branch that should be included somehow?

Very good question, I had forgotten about that branch. I can email him directly if needed but the comparison isn't helping me much to understand what's different other than what appears to be a bunch of fast-forwarded commits. Let me know if you need me to reach out to him. 👍 curlymorphic/lmms@zynUpgrade...curlymorphic:zynwin

  1. With "new upstream branch", do you mean a branch in the LMMS/lmms repo?

Yes. Two disadvantages to use our own personal forks for collab items is

  1. Out of sight out of mind
  2. Lack of permission from other admins causes use to fork against forks and PR against PRs, etc.

We don't do a lot of collab items currently, but I think we have a stable enough dev team currently to use this process. You'll notice @lukas-w uses this technique for 99% of his commits, allowing stuff like this to happen.

Also, it seems to me that the merging/rebasing would become a lot easier if the commits in this PR were squashed first. Does that seems like a good idea? And is it alright if everything is squashed into a single commit or should some kind of logical separation be maintained?

Another great question. From a high-level, the Zyn source update needs to be in a separate commit so we can at least isolate our changes from upstream Zyn changes... but .... as each one of Dave's commits are reviewed, this may offer time to split things up. @grejppi has stressed that squashing everything isn't always a good thing, some commits are best left separated. 👍

And on a final note: 1.2.0 is not quite there yet. Per the response to #1991 (comment), is this the right moment to resume work on this?

Unless something drastic happens to the state of Zyn 2.5, this certainly won't make 1.2, so yes, this can certainly be shelved for a later date. 👍

@Fastigium

This comment has been minimized.

Show comment
Hide comment
@Fastigium

Fastigium Mar 1, 2016

Contributor

yes, this can certainly be shelved for a later date. 👍

Right, I propose to do that and possibly mail curlymorphic when this is picked up again. As for comparing the zynwin and zynUpgrade branches, just looking at their commit logs may give more insight: https://github.com/curlymorphic/lmms/commits/zynwin vs https://github.com/curlymorphic/lmms/commits/zynUpgrade. It would seem that a lot of more recent commits were added to zynwin.

Contributor

Fastigium commented Mar 1, 2016

yes, this can certainly be shelved for a later date. 👍

Right, I propose to do that and possibly mail curlymorphic when this is picked up again. As for comparing the zynwin and zynUpgrade branches, just looking at their commit logs may give more insight: https://github.com/curlymorphic/lmms/commits/zynwin vs https://github.com/curlymorphic/lmms/commits/zynUpgrade. It would seem that a lot of more recent commits were added to zynwin.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Feb 5, 2017

Member

Closing this for now. We can reopen at a later date, perhaps with 3.0.

Member

tresf commented Feb 5, 2017

Closing this for now. We can reopen at a later date, perhaps with 3.0.

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