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

Surge Streaming with non-. locales (was Linux: VST and LV2 Step sequence bars are not restored properly) #1900

Closed
xard-dev opened this issue May 17, 2020 · 42 comments · Fixed by #1903
Labels
Linux Issues which only occur on Linux Modulation Modulation related issues
Milestone

Comments

@xard-dev
Copy link

Describe the bug
Step sequence bars don't get restored properly when loading DAW session: Tested on Ardour pre6 and Qtractor. Also tested on both LV2 and VST versions of the plugin.

Please let us know your surge version

  • Surge Version: 1.7.NIGHTLY-2020-05-15-3e09ffc
  • Plugin type VST2, LV2
  • Bits 64

To Reproduce
Steps to reproduce the behavior:

  1. Create new session in DAW
  2. Create track with Surge VST2 or LV2 plugin version
  3. Select built-in preset which step sequences like Acid Seq 2 or 3
  4. Save session
  5. Exit DAW
  6. Reopen DAW and load session
  7. Open Surge instance

Expected behavior
Step sequence is kept as it was originally set in the preset.

Screenshots
Any bars not being either 100%, 0% or -100% are missing. Screenshot of the issue:
image

Desktop (please complete the following information):

  • OS: Ubuntu 20.04
  • Host Ardour, Qtractor
  • Version Nightly prerelease 6, 0.9.12

Additional context
This might or might not be Linux specific bug; testing needed on other platforms.

@baconpaul
Copy link
Collaborator

This happens with the VST2 and the LV2?
With the LV2 we have all sorts of problems saving values which the linux folks are working on but if you see it with the VST2 also then that is a more fundamental bug. @tank-trax do you see this on linux also?
We’ve been modifying the step sequencer recently in the nightlies so entirely possible we broke it indeed!

@baconpaul baconpaul added this to the 1.7 beta 1 milestone May 17, 2020
@baconpaul baconpaul added Linux Issues which only occur on Linux Modulation Modulation related issues LV2 labels May 17, 2020
@baconpaul
Copy link
Collaborator

As an FYI I do not see this problem in a quick test on any of the following

  • macOS AU HostingAU,
  • mac VST3 reaper
  • Linux VST3 Reaper
  • Linux LV2 Carla

My guess is this is LV2 streaming problems again. The steps in the step sequencer are not part of the parameter set but are part of the streamed state of the session, so I think this is related to #1768 and #1619 and #1773. Let me tag it LV2 and also bring this to @jpcima and @falkTX attention.

@xard-dev
Copy link
Author

Yes, it happens on both VST2 and LV2:
image

@baconpaul
Copy link
Collaborator

OK thank you! I’m unable to support the VST2 so I can’t help you there; and my tests show that it is clearly not in every combo of host and OS (that is: if it were linux specific linux reaper VST3 and Carla LV2 wouldn’t work).

Do any of your hosts support VST3?

@xard-dev
Copy link
Author

Currently my go to options don't but I can see what I can do about carla / reaper or any other vst3 capable host.

@xard-dev
Copy link
Author

xard-dev commented May 17, 2020

Testing with Carla LV2 and it has the same problem:
surge_test
For some reason Carla reports VST3 as an option but does not find surge VST3 even though it has been installed by the package.

@baconpaul
Copy link
Collaborator

Can you zip up and attach that Carla session?
Like I said when I just put a new LV2 into a Carla, modified the step seq, saved, quit, and restarted, the steps came back just fine.
Thanks

@xard-dev
Copy link
Author

xard-dev commented May 17, 2020

I'm not familiar with Carla, but I hope this covers all the needed files:
carla_surge_test.zip

And what I did was exactly as I wrote in the repeated steps with this time adding only the LV2 version of surge.

@baconpaul
Copy link
Collaborator

thanks

It also occurs to me I didn’t do exactly what you said. I started a fresh session and set the values. I wonder if loading a preset on linux goes down a path which results in incorrect persistence.

Lemme look some this afternoon

Thanks for the great report

@xard-dev
Copy link
Author

No problem; I'd gladly go deeper on the state restoring if there are any ways to debug the state data. Printing out all state variables after selecting preset and after reloading host would be a tremendous help.

Thank you for the fast replies!

@baconpaul
Copy link
Collaborator

Yeah so when I do a carla open / load lv2 / load acid seq 3 / save / quit / open / load i get the right values but when I load your .carxp file I get the wrong values.

That file has the internal state as a base64 encoded blob but let me stare at the code also some.

@xard-dev
Copy link
Author

I'd suspect that something goes wrong with the serialization of the base64 data or just before writing it.

@baconpaul
Copy link
Collaborator

yeah but it's the same code on all the flavors which is why I'm confused.

The code in question to deserialize is SurgePatch.cpp line 1500 or so and to serialize around line 1793 and hasn't changed in forever.

Clearly that is loading back incorrect values. I wonder if it is also writing them.

I also wonder if this is somehow related to your mouse issue.. We recently changed the code to have the LFOUI update the values. Look around line 1207 of CLFOGui.cpp. I wonder if we are just setting values to -1,0,1 somehow when we draw in your environment and the problem isn't streaming at all? I would start there - if you can run a daw with out attached put print statements those three spots and see?

I'm headed back to my Sunday afternoon but maybe those hints help?

@xard-dev
Copy link
Author

Okay, I'll try printing what happens around here:

{ 
    f *= 12;
    f = floor(f);
    f *= (1.f / 12.f);
}

I haven't setup my compilation environment fully yet to 20.04 so this might some time while I gather surge dependencies to get the compilation process rolling, but I'll report back when I'll get results.

@baconpaul
Copy link
Collaborator

Yeah that should only happy if you press ctrl while dragging (it is how the ss quaantizes and has been there forever; our Recent change is to add an alternate based on tuning). If you are hitting that in other circumstances that would indeed be the bug!

All the prerequisites can be easily found by installing GCc and cmake then just doing grep apt-get azure-pipelines.yml And running those apt-gets - that’s how I set up my Ubuntu 20.04 vm a couple of weeks ago!

@xard-dev
Copy link
Author

xard-dev commented May 17, 2020

Okay, after having some issues with missing packages, missing submodules and all kinds of funny business I managed to get the LV2 version built and installed on local LV2 user path.

It's very nice that you can have the system LV2 installed and build local testing versions and hosts will override the system one for the local.

Okay; Results.

{
                  std::cout << "F value before:" << f << std::endl;
                  f *= 12;
                  f = floor(f);
                  f *= (1.f / 12.f);
                  std::cout << "F value after:" << f << std::endl;
}

prints... absolutely nothing. Nothing at all. The else statement of 1207 line isn't ever reached.
I tested with debug prints like above in the beginning of CMouseEventResult function and they do show up as expedcted.

But as far as i know repeating the steps with the debug prints above results no output.

@baconpaul
Copy link
Collaborator

That should print if you ctrl-drag. But the steps[i] = f below is the place where you want to print, as well as the two streaming spots.

Basically:

1: Is CLFOGUI changing steps at all (that’s the steps[i] = f there and)
2: Is SurgePatch streaming the value properly (thats the line 17whatzit in SurgePatch to xml)

If the XML has the right thing and CLFOGUI isn’t handling it and you still get blown up then it is a serialization thing in your host. If the XML has the wrong thing and CLFOGuil hasn’t touched it then it is a (scary bug I can’t figure out yet). If the CLFOGUI touches it incorrectly then it is related to the mouse. That’s my theory at least. Like I said I don’t get the same error as you at stream time yet (and am not really coding this afternoon either which is why I’m just tossing hints in here)

@xard-dev
Copy link
Author

xard-dev commented May 17, 2020

It's okay, I'm really like fish out of water when it comes to c++ so bear with me here.

Printing f values above steps[i] results while drawing not-so-linear line from -1 to 1 preserving the final values for each step:

F value in step[0]: -1
F value in step[1]: -0.875
F value in step[1]: -0.8125
F value in step[2]: -0.6875
F value in step[3]: -0.625
F value in step[4]: -0.53125
F value in step[5]: -0.46875
F value in step[6]: -0.40625
F value in step[7]: -0.3125
F value in step[8]: -0.21875
F value in step[9]: 0.03125
F value in step[10]: 0.21875
F value in step[11]: 0.4375
F value in step[12]: 0.65625
F value in step[13]: 0.8125
F value in step[14]: 0.96875
F value in step[15]: 1

and loading that translates to only -1 and 1 values being preserved as expected.

@baconpaul
Copy link
Collaborator

where is that print statement? That's from the GUI?

So then something (something) must quantize them before they stream. if you put a print at 1793 of SurgePatch.cpp before the XML is written do you get those values going out?

You see this in QTractor and ardour5 right? but not Carla?

@xard-dev
Copy link
Author

That was from above the step[i] set.
CLFOGui.cpp 1220~

            std::cout << "F value in step[" << i << "]: " << f << std::endl;
            ss->steps[i] = f;
            invalid();
         }

All my hosts behave the same with surge: Ardour 6 pre, Qtractor and carla.

@xard-dev
Copy link
Author

Also confirming that value stored in ss->steps[i] is valid. I'm not sure if this was needed to be checked or not, but printed the contents anyway.

@baconpaul
Copy link
Collaborator

wow i really wonder what it is

i just opened the acid seq in a fresh ardour5 session, saved the session, restarted ardour, reloaded, and it was all fine.

what on earth could be blowing up your streaming I wonder!

@xard-dev
Copy link
Author

for (int s = 0; s < n_stepseqsteps; s++)
            {
               std::cout << "Step serialize [" << s << "]:" << stepsequences[sc][l].steps[s] << std::endl;
               sprintf(txt, "s%i", s);
               if (stepsequences[sc][l].steps[s] != 0.f)
                  p.SetAttribute(txt, float_to_str(stepsequences[sc][l].steps[s], txt2));
            }

output during save:

Step serialize [0]:-0.96875
Step serialize [1]:-0.78125
Step serialize [2]:-0.625
Step serialize [3]:-0.46875
Step serialize [4]:-0.375
Step serialize [5]:-0.25
Step serialize [6]:-0.125
Step serialize [7]:-0.03125
Step serialize [8]:0.0625
Step serialize [9]:0.125
Step serialize [10]:0.25
Step serialize [11]:0.34375
Step serialize [12]:0.40625
Step serialize [13]:0.5
Step serialize [14]:0.75
Step serialize [15]:1

Now this is getting weird...

@falkTX
Copy link
Contributor

falkTX commented May 17, 2020

Do you have your system in a language other than english?
This could be a string parsing error, where "." and "," are used depending on the system.

@baconpaul
Copy link
Collaborator

Yeah I was thinking the exact same
I bet it is internationalization

@baconpaul
Copy link
Collaborator

Replace your comment with

Printf (“value is %f”, step[i] )

And do you get a comma rather than a .?

@xard-dev
Copy link
Author

Of course I have:

Step serialize [0]:-0.78125
Step float_to_str:-0,781250
Step serialize [1]:-0.75
Step float_to_str:-0,750000
Step serialize [2]:-0.71875
Step float_to_str:-0,718750
Step serialize [3]:-0.6875
Step float_to_str:-0,687500
Step serialize [4]:-0.59375
Step float_to_str:-0,593750
Step serialize [5]:-0.5
Step float_to_str:-0,500000
Step serialize [6]:-0.375
Step float_to_str:-0,375000
Step serialize [7]:-0.21875
Step float_to_str:-0,218750
Step serialize [8]:-0.09375
Step float_to_str:-0,093750
Step serialize [9]:0.03125
Step float_to_str:0,031250
Step serialize [10]:0.125
Step float_to_str:0,125000
Step serialize [11]:0.25
Step float_to_str:0,250000
Step serialize [12]:0.34375
Step float_to_str:0,343750
Step serialize [13]:0.40625
Step float_to_str:0,406250
Step serialize [14]:0.65625
Step float_to_str:0,656250
Step serialize [15]:1
Step float_to_str:1,000000

as you can see above; here comma is official decimal separator.

@falkTX
Copy link
Contributor

falkTX commented May 17, 2020

FYI I use this code in carla when dealing with float save and restore:

class CarlaScopedLocale {
public:
    CarlaScopedLocale() noexcept
        : locale(::strdup(::setlocale(LC_NUMERIC, nullptr)))
    {
        ::setlocale(LC_NUMERIC, "C");
    }

    ~CarlaScopedLocale() noexcept
    {
        if (locale != nullptr)
        {
            ::setlocale(LC_NUMERIC, locale);
            ::free(locale);
        }
    }

private:
    const char* const locale;
};

might be useful.

@baconpaul
Copy link
Collaborator

Yeah right
So we gotta force into the c locale when we stringify
That explains it

@xard-dev
Copy link
Author

I guess that means mystery solved!

@baconpaul
Copy link
Collaborator

Thanks! Mind if I just borrow that guard class?

@falkTX
Copy link
Contributor

falkTX commented May 17, 2020

Sure, do as you wish with it. there are other alternatives too.
Qt uses a library to do the double/float conversions for example, because this shit is nasty and hard to deal with properly.

@xard-dev
Copy link
Author

xard-dev commented May 17, 2020

I actually feel bit stupid not think of this any sooner as I've been encountering so many bash scripting bugs related to fact that developers forget to force locale and then parse command output.

@baconpaul
Copy link
Collaborator

yeah this is ancient ancient code which has been there forever in the depth of surge which isn't used much but is used in this spot. OK will figure out the right fix this week. Thanks!

@baconpaul baconpaul changed the title Linux: VST and LV2 Step sequence bars are not restored properly Surge Streaming with non-. locales (was Linux: VST and LV2 Step sequence bars are not restored properly) May 17, 2020
@baconpaul baconpaul removed the LV2 label May 17, 2020
@xard-dev
Copy link
Author

xard-dev commented May 17, 2020

This might by the way solve some other more subtle restore related problems as well.

@baconpaul
Copy link
Collaborator

Ha yeah!

And if I do a LANG=fr_FR && Carla then I can reproduce it no problem.

@baconpaul
Copy link
Collaborator

Like: I bet you can't save mod routing either. Sigh.

Like I said this is ancient code - from before the synth was open sourced. This bug has been there all along. Amazed you are the first person to find it. Appreciate the clear report.

@xard-dev
Copy link
Author

And vice versa launching qtractor with:
LC_ALL=C qtractor
and doing the whole test step routine works just fine.

@baconpaul
Copy link
Collaborator

OK great
I'll fix it sometime this week and update this issue when I do.

Appreciate the clarity and patience.

@xard-dev
Copy link
Author

No problem; This was actually kind of fun excerise 😃

baconpaul added a commit to baconpaul/surge that referenced this issue May 17, 2020
Forever, it seems, we've had a bug in our XML streaming
if you have a . , internationalization swap. Fix that here
by forcing "C" locale when we stream the XML (even though
this doesn't touch the streaming; it touches float_to-str).

Thanks to @falkTX for help

Closes surge-synthesizer#1900
baconpaul added a commit that referenced this issue May 17, 2020
Forever, it seems, we've had a bug in our XML streaming
if you have a . , internationalization swap. Fix that here
by forcing "C" locale when we stream the XML (even though
this doesn't touch the streaming; it touches float_to-str).

Thanks to @falkTX for help

Closes #1900
@baconpaul
Copy link
Collaborator

OK just pushed 4f954d4 which contains a fix. In that branch I can save a sequence in a french Carla (une Carla francaise?) and it persists properly.

@baconpaul
Copy link
Collaborator

thanks for the help @falkTX and the careful reporting @xard-dev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Linux Issues which only occur on Linux Modulation Modulation related issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants