yoshimi-devel — A list for yoshimi development discussion

[Yoshimi-devel] Cmake warning

[Will G. <wil...@mu...>]

I've noticed the following recently

/home/will/yoshimi-code/src/Misc/SynthEngine.cpp: In constructor
‘SynthEngine::SynthEngine(uint)’:
/home/will/yoshimi-code/src/Misc/SynthEngine.cpp:107:15: warning: member
‘SynthEngine::interchange’ is used uninitialized [-Wuninitialized] 107 |     ,
rootCon{interchange.guiDataExchange.createConnection<InterfaceAnchor>()} | 

I don't understand how this can be the case :(

-- 
Will J Godfrey

Re: [Yoshimi-devel] A bug in the release :(

[ichthyo <pr...@ic...>]

On 23.09.24 16:57, ichthyo wrote:
> Another issue that was pointed out on the Github pull request
> https://github.com/Yoshimi/yoshimi/pull/207#pullrequestreview-2322393639
>
> As part of my rework, I used some generic helpers and created a new header
> Util.hpp. There is a function using std::set in this header, and basically I
> wanted to avoid including <set> directly, since then this include would be
> pulled into a large part of the whole code base.


We know from experience that header includes can have a huge impact on build
times. Just recently Kristian did some re-orderings at the global level, with
the result that Yoshimi now builds at least three times faster, which makes
a huge difference in everyday work.

I had similar experirences on several occasions in the past.

But I must admit, that I acted based on *assumption* in this case,
which is not what an engineer should do.


The reality is: the structure of the includes in yoshimi is far from optimised
in any way. We have some very large code entities (Config, and SynthEngine),
which on the one hand are feature rich, while on the other hand are also
referred to from almost everywhere. So these are kind of "super spreaders",
since they pull in lots of dependencies already, even while the header uses
quite some forward declarations. Add to this that the downstream headers
are also not included with a star-shaped topology, but have quite some
cross usages.


So this gives rise to the suspicion, that we are engaged into a phantom
discussion here and a treatment of imaginary pains.


And a quick empirical test confirms this suspicion.

If we include <set> and <algorithm> right into globals.h,
the effect on build time is *negligible*
Both user time and real end-to-end time show a statistic spread of roughly
0.8sec in my measurements. And within those bounds, there is no difference
between those scenarios.

Observed values on Release build are:
  User time ~ 7min 20.3±0.4 sec and
  Real time ~ 1min  5.3±0.4 sec (with 8 cores and -j16 )



So the bottom line is: we can forget about that *premature optimisation*
regarding compile times and simple replace the forward declaration by an
#include <set>

In the current code base, an #include <algorithm> should not be necessary,
since the ambiguity is related to std::set and the std::find algorithm
is not used on a signature but only in the implementation (which the compiler
does not type check unless it is actually instantiated)

-- Hermann


PS: That being said -- we should still consider / discuss the structure of
the "Util.h". I introduced this driven by immediate convenience, and did
not think too much. I'm under the impression that the utils defined there
could be relocated into existing utility headers.

Re: [Yoshimi-devel] A bug in the release :(

[ichthyo <pr...@ic...>]

On 23.09.24 16:57, ichthyo wrote:
> Basically I see the following avenues to resolve the problem
> (1)....

and (4) yet another solution would be to split up Util.hpp thematically,
since we have already a collection of more focussed utility headers.

However, this would not directly resolve the tension/conflict regarding
this contains() function, since this function deliberately attempts
to abstract away /what/ container or entity cointains the thing we are
looking for.
So this function is written in a way that it can be used on a std::string,
on a std::set, on a generic STL container which then will be searched
linearly with the find-algorithm...

Thus if you use contains(someString, val), you certainly do not want
to include <set> or <algorithm>, which was why I added those forward
declarations.


Thus, if we'd intend to resolve that problem by splitting up Util.h,
then we'd probably have to split up those various overloads of contains()
and maybe invent a new header SearchFuncs.hpp

-- Hermann

Re: [Yoshimi-devel] A bug in the release :(

[ichthyo <pr...@ic...>]

On 23.09.24 16:57, ichthyo wrote:
> #ifdef GUI_FLTK
> ///.................original code here
> #else
>      guiDataExchange{[this](CommandBlock const& block){ /* do nothing */ }},
> #endif

...err, leave the argument name out, otherwise the compiler will print a
warning; we can also leave out the this-closure in that case, so effectively
the optimiser will then be able to remove any related invocation altogether


 >      guiDataExchange{[](CommandBlock const&){ /* do nothing */ }},

Re: [Yoshimi-devel] A bug in the release :(

[ichthyo <pr...@ic...>]

On 23.09.24 10:46, Will Godfrey wrote:
> We forgot to test headless build,

...and what is most annoying, I had that on my list, but somehow
in the fury of the last parts of integration I overlooked that. Most
of the other things on this list were ideas and possible improvements
or concerns that had been sorted out meanwhile.

> and that's pretty important to one of our users who reported this:
>
> yoshimi/src/Interface/InterChange.cpp:107:56: error: ‘toGUI’ was not
> declared in this scope
> 107 |     guiDataExchange{[this](CommandBlock const& block){
> toGUI.write(block.bytes); }},
>

So, drawing from the way matters are assumed to work, then a headless build
should actually never use the guiDataExchange. And while it would probably
by quite difficult to remove (hide by compile exclusion) the guiDataExchange
member, since we would have to hide/exclude then a lot of further code in
the core, what we can do is define another Lamda function in this case,
which just does nothing. I.e.


#ifdef GUI_FLTK
///.................original code here
#else
     guiDataExchange{[this](CommandBlock const& block){ /* do nothing */ }},
#endif



Another issue that was pointed out on the Github pull request
https://github.com/Yoshimi/yoshimi/pull/207#pullrequestreview-2322393639


As part of my rework, I used some generic helpers and created a new header
Util.hpp. There is a function using std::set in this header, and basically
I wanted to avoid including <set> directly, since then this include would
be pulled into a large part of the whole code base.

The problems seem to arise only with some newer versions of the standard
library, where the forward declaration I provided seemingly generates an
ambiguity. Now I am unsure how to proceed best.

The reporter of the problem suggests a cheap solution, just bite the
bullet and include the damn header. They even propose additionally to include
<algorithm>, and I do absolutely not understand why this would be necessary.

Basically I see the following avenues to resolve the problem

(1) the careless solution, just #include <set>, maybe even <algorithm>
     and don't give a shit about compilation times. Personally I would
     totally reject that idea, because for me this is the typical mindset
     of a juvenile programmer, who pulls in half the internet just to
     avoid using one's own brain even for the most simple stuff.

     But it is very much possible that I am overreacting here and acting
     as a typical "old timer".

(2) attempting to fix the ambiguity, possibly by also adding a suitable
     forward declaration for polymorphic_allocator. This would require
     to find out what version of the library actually causes the problem
     and then working from there (using a docker container to test the build)

(3) reconsidering the usage of the "contains()" function. Yes, personally
     I like to use such a shorthand, because IMHO it makes the code clearer.
     But that's me. Right now we have only one single usage, which is in
     InstanceManager

uint InstanceManager::SynthGroom::allocateID(uint desiredID)
{
     if (desiredID >= 32 or (desiredID > 0 and contains(registry, desiredID)))
         desiredID = 0; // use the next free ID instead


     if (not desiredID)
     {
....

It would easy to resolve that by just defining "contains()" in an anonymous
namespace in InstanceManager.cpp. However, that would somehow defeat the purpose
of using such short-hand utilities, because then, over time, several definitions
of such tiny short-hand functions would spread out over the code base.

So this is IMHO to quite some degree a matter of coding style and taste, and we
should discuss our attitude towards using such short-hand functions. As said,
it is always simple to code the functionality up explicitly; the short-hand
function just makes the intention more clear at usage site (it tells the
reader *what* you want, not *how* it's done)

-- Hermann

[Yoshimi-devel] A bug in the release :(

[Will G. <wil...@mu...>]

We forgot to test headless build, and that's pretty important to one of our
users who reported this:

yoshimi/src/Interface/InterChange.cpp:107:56: error: ‘toGUI’ was not declared 
in this scope
107 |     guiDataExchange{[this](CommandBlock const& block){ 
toGUI.write(block.bytes); }},

-- 
Will J Godfrey

Re: [Yoshimi-devel] Note 'on' time reporting.

[ichthyo <pr...@ic...>]

On 19.09.24 19:55, Will Godfrey wrote:
> Thanks for this Hermann. It worked, but the config window and actual panel
> format could get out of sync, so I bit the bullet and did a deep dive.
>
> In the latest build it is now full in sync, and hopefully more correct.

Ah... I see, you made the callback of the button in the pannel invoke
changepanelstyle(panelType), and the latter does a direct redraw when
the settings window is open.

Yes, it seems perfectly in sync now. Even if I close yoshimi, then
move in another window file; then it comes up with the other layout
and the right selection in the settings. :-D

-- Hermann

Re: [Yoshimi-devel] Note 'on' time reporting.

[Will G. <wil...@mu...>]

Thanks for this Hermann.
It worked, but the config window and actual panel format could get out of sync,
so I bit the bullet and did a deep dive.

In the latest build it is now full in sync, and hopefully more correct.

-- 
Will J Godfrey

Re: [Yoshimi-devel] Note 'on' time reporting.

[ichthyo <pr...@ic...>]

On 18.09.24 23:46, Will Godfrey wrote:
> On Wed, 18 Sep 2024 23:27:43 +0200 ichthyo <pr...@ic...> wrote:
>
>> No, I mean the FL_choice (drop-down select box) to control the "mixer panel
>> layout". It is at the bottom of the first tab in the settings dialog. It is
>> populated with the two possible options, but there is no code to preselect
>> the entry that is currently active. Thus, if you open the settings dialog,
>> the drop-down seems to be empty. Only if you expand it, then you see the
>> two options.
>
> Ah! Got it! I'd completely forgotten about that one. I never use it, I always
> use the button in the mixer panel itself. Yes, that should be done,....

The following fix is more a guess, but it seems to work....
However, I do not really understand that code and I have no
idea if adding this here could have adverse effects


diff --git a/src/UI/MasterUI.fl b/src/UI/MasterUI.fl
index 4f6907fb..ceb0f7ce 100644
--- a/src/UI/MasterUI.fl
+++ b/src/UI/MasterUI.fl
@@ -3682,11 +3682,13 @@ Alan Calvert started the project}
      {
          towide->hide();
          todeep->show();
+        configui->PanelLayout->value(1);
      }
      else if (tmp == 2)
      {
          towide->show();
          todeep->hide();
+        configui->PanelLayout->value(0);
      }

      panelwindow->size_range(defW, defH, 0, 0, 0, 0, 1);

Re: [Yoshimi-devel] Note 'on' time reporting.

[Will G. <wil...@mu...>]

On Wed, 18 Sep 2024 23:27:43 +0200
ichthyo <pr...@ic...> wrote:

>> I take it you are referring to the 'Solo' menu.  
>
>No, I mean the FL_choice (drop-down select box) to control the
>"mixer panel layout". It is at the bottom of the first tab in the
>settings dialog. It is populated with the two possible options, but
>there is no code to preselect the entry that is currently active.
>Thus, if you open the settings dialog, the drop-down seems to be empty.
>Only if you expand it, then you see the two options.
>
>As said, I'd propose to add a fix into MasterUI::Showpanel(),
>i.e. at the moment, when the panel layout is set/established, then
>we could also select the proper option in the settings dialog box.

Ah! Got it!
I'd completely forgotten about that one. I never use it, I always use the
button in the mixer panel itself. Yes, that should be done, and I remember now
why it was added. It was to deal with the situation with a low resolution
screen where it was possible for the button, and the close one to be off the
screen

-- 
Will J Godfrey {apparently now an 'elderly'}

Re: [Yoshimi-devel] Note 'on' time reporting.

[ichthyo <pr...@ic...>]

On 18.09.24 19:00, Will Godfrey wrote:
> None of the GUI geometry controls are accessible to the core. They have no
> relation to anything but appearance.

It used to be a setting in the Instance-Config, so in theory it
would have been possible to control it from an settings/state-file.
However, the setting did not work, it was just ignored or overridden
by what is currently in the window-layout files.

So basically you removed a setting that did not work and no one
(including myself) had complained about it not working or investigated
why it did not work.

And on top of that comes your argument, that it is anyway an GUI internal
geometry/layout setting. Thus I'd say, it was reasonable to remove the setting!


>> Moreover, the Select box in the Settings does not select the
>> correct entry. It comes up empty, and only if you click it open,
>> you see the two entries.
>
> I take it you are referring to the 'Solo' menu.

No, I mean the FL_choice (drop-down select box) to control the
"mixer panel layout". It is at the bottom of the first tab in the
settings dialog. It is populated with the two possible options, but
there is no code to preselect the entry that is currently active.
Thus, if you open the settings dialog, the drop-down seems to be empty.
Only if you expand it, then you see the two options.

As said, I'd propose to add a fix into MasterUI::Showpanel(),
i.e. at the moment, when the panel layout is set/established, then
we could also select the proper option in the settings dialog box.

As said, this is just a minor / nitpicking thing.

-- Hermann

Re: [Yoshimi-devel] Note 'on' time reporting.

[Will G. <wil...@mu...>]

On Wed, 18 Sep 2024 14:43:24 +0200
ichthyo <pr...@ic...> wrote:

>On 18.09.24 08:48, Will Godfrey wrote:
>> It never used "single_row_panel", so I removed it.  
>
>I for one use it.
>Most of the time, I use the single row layout.
>For some kind of tasks, I prefer the multi row layout.

Ah. A bit of confusion there. Yes I use the function sometimes. What I meant was
that the function doesn't use the variable called single_row_panel.

None of the GUI geometry controls are accessible to the core. They have no
relation to anything but appearance. At that time I though it best to keep them
contained within the GUI. All of this data is stored in config/windows.
The settings track instance IDs, but don't care where those IDs come from. I
though it very unlikely that anyone would want to run stand-alone and plugin
style at the same time.

>Moreover, the Select box in the Settings does not select the
>correct entry. It comes up empty, and only if you click it open,
>you see the two entries.

I take it you are referring to the 'Solo' menu. I've still not seen the problem
here, but I'll take a look again - and try to remind myself how I put it all
together!

I can't understand why it has stopped working. I don't remember any changes in
that area for years.

-- 
Will J Godfrey

Re: [Yoshimi-devel] Fail with Reaper :(

[Will G. <wil...@mu...>]

On Wed, 18 Sep 2024 16:38:19 +0200
ichthyo <pr...@ic...> wrote:

>On 18.09.24 16:05, ichthyo wrote:
>> The Thread in Yoshimi code is attempting to load the GUI-Plugin, but
>> trapped there in a safety loop I had added to ensure the Core plugin is up
>> and initialised before the GUI plugin can attach.  
>
>I have attached the patch.
>As usual, you can directly apply it as Git-commit (including my explanation)
>by stepping into the Yoshimi dir and then
>
>git am Bugfix-LV2-start-up-synchronisation-Yoshimi-UI.patch
>
>
>Hopefully this has resolved the problem....
>
>-- Hermann
>
That was quick :)

So we're in with 2.3.3 rc1

-- 
Will J Godfrey

Re: [Yoshimi-devel] Fail with Reaper :(

[ichthyo <pr...@ic...>]

On 18.09.24 16:05, ichthyo wrote:
> The Thread in Yoshimi code is attempting to load the GUI-Plugin, but
> trapped there in a safety loop I had added to ensure the Core plugin is up
> and initialised before the GUI plugin can attach.

...and the fix is quite obvious: seemingly I was to defensive in assuming
that the core plugin will be activated before the GUI-Plugin can be loaded.

Yoshimi is using a non-standard extension and a native-gui-scheme, which is
not much documented in LV2 (quite contrary to the core standard and the
official way to build GUIs). Thus I am somewhat poking in the dark what
is the expected procedure for LV2-hosts to bring up a GUI.

Anyway, we can set the isReady flag earlier, at the point where the SynthEngine
has been booted up and we are about to return the LV2_Handle from instantiate()

A quick test shows the Yoshimi GUI comes up now in Reaper, Carla and QTractor.


I have attached the patch.
As usual, you can directly apply it as Git-commit (including my explanation)
by stepping into the Yoshimi dir and then

git am Bugfix-LV2-start-up-synchronisation-Yoshimi-UI.patch


Hopefully this has resolved the problem....

-- Hermann

Re: [Yoshimi-devel] Fail with Reaper :(

[ichthyo <pr...@ic...>]

On 18.09.24 14:28, Will Godfrey wrote:
> Fetch the evaluation version of Reaper, and adding Yoshimi to an empty
> project seems to work correctly. However if you save this, then on
> reloading Reaper freezes (gkrellm reports one core pegged at 100%).

To share the initial observations....

- Problem is reproducible with the most minimal setup.
   No need even to connect MIDI. A single track with a Yoshimi plugin suffices.
- when creating the project, yoshimi GUI can be opened and sound be produced
   with the help of the virtual keyboard. Sound is OK, instruments load.

Attach with debugger after re-opening the project so that Reaper hangs.

- we got 21 Threads
- Thread #1 is in Yoshimi code
- Thread #21 is our Background thread
- one Thread is reading a Socket
- one Thread does syscalls
- the thread "reaper/video" is in a usleep-polling-loop
- all other threads are workers, marked as "reaper/mediafx" or "/livefx"
   these are all blocked waiting on a FUTEX

The Thread in Yoshimi code is attempting to load the GUI-Plugin, but
trapped there in a safety loop I had added to ensure the Core plugin
is up and initialised before the GUI plugin can attach.

The previous version had no such safeguard, and for that reason does not hang.


Now I'll have to investigate what precisely happens when opening those plugins.
My first suspicion is that Reaper attempts to load both the Core plugin and
the GUI plugin concurrently, or maybe even first loads the GUI plugin and
then somehow gets trapped.

-- Hermann

Re: [Yoshimi-devel] Fail with Reaper :(

[ichthyo <pr...@ic...>]

On 18.09.24 14:28, Will Godfrey wrote:
>> Fetch the evaluation version of Reaper, and adding Yoshimi to an empty
>> project seems to work correctly. However if you save this, then on
>> reloading Reaper freezes (gkrellm reports one core pegged at 100%).
>
> Some further thoughts on this.
>
> Starting a new 'master' project works. Saving that project, then reloading it
> fails. Loading it into V2.3.2 works.
>
> Starting a new V2.3.2 project works. Saving that project, then reloading it
> works. Loading it into 'master' fails.
>
> Logically, that suggests the problem is specifically the loading process in
> 'master'.

This might give already some clues where to look....

> We've spent a lot of time on this, so do we carry on, or take a break till
> later in the year?

What I certainly want to do is to try to reproduce the problem and
find out what is going on. We agreed that we wanted to improve the
way Yoshimi connects to the GUI. It was clear from start that this
would not be an easy task. Up to now it let to lots of follow-up
work, but everything we did was also an improvement of the general
shape of the code base.

If the problem in Reaper turns out to be nasty, then backing out
and rather building an interim release with some fixes would be
an option. Or releasing later, of course. But right now we do
not know what is the problem so it's difficult to see what
would be the best way forward.

-- Hermann

Re: [Yoshimi-devel] Note 'on' time reporting.

[ichthyo <pr...@ic...>]

On 18.09.24 08:48, Will Godfrey wrote:
> It never used "single_row_panel", so I removed it.

I for one use it.
Most of the time, I use the single row layout.
For some kind of tasks, I prefer the multi row layout.

And I use state files. Unfortunately, restoring the mixer layout
did not work in the previous releases (which was an annoyance I
never got time to investigate). So, this release basically removes
a config setting that did not work... :-D


Moreover, the Select box in the Settings does not select the
correct entry. It comes up empty, and only if you click it open,
you see the two entries.

In the previous release, it came up with always the first entry
selected, so that one was broken too.


Would it make sense to fix that in MasterUI::Showpanel(),
where also the panel type seems to be set. Since that function
is in MasterUI, it could access the field configUI->PanelLayout

The call path is

Init() -> loadWindowData() -> Showpanel()

so this might work?

-- Hermann

Re: [Yoshimi-devel] Note 'on' time reporting.

[ichthyo <pr...@ic...>]

On 18.09.24 08:48, Will Godfrey wrote:
> It's REPORT_NOTE_ON_TIME in the code. The switch is the one marked 'Log Load
> Times'. It is shared with loading instruments. This might not be ideal, and
> maybe we should have a separate switch, although I found it interesting to
> see the two reported together..

OK, but this switch does not yet properly guard the problematic code
in SynthEngine yet. So we can not just remove the compile time guard
and be happy.

It would especially concern me if Yoshimi is delivered in a state
where IO (like printing to console) happens from the realtime processing
threads enabled by default, and even so frequently as on every note.

We know, one can use IO for diagnostics. We developers do it all the time.
But it interferes with the correct behaviour of realtime processing.
IO blocks, it is *tremendously* expensive (especially number to string
conversions and formatting) and the mere fact of accessing other thread's
memory creates a random slow down.

It just happens randomly, and thus it is not question /if/,
but a question /when/ and /where/ it happens.

So if we give such a feature to users, we should ensure it is switched
off by default, and there should be a warning to the user, at least
in the tooltip of the config setting.

-- Hermann

Re: [Yoshimi-devel] Fail with Reaper :(

[Will G. <wil...@mu...>]

On Wed, 18 Sep 2024 08:42:53 +0100
Will Godfrey <wil...@mu...> wrote:

>Reported by Art and confirmed by me.
>
>Fetch the evaluation version of Reaper, and adding Yoshimi to an empty
>project seems to work correctly. However if you save this, then on reloading
>Reaper freezes (gkrellm reports one core pegged at 100%).
>Switch LV2 version to Yoshimi V2.3.2 and the same project file loads and runs
>correctly. 
>
>Reaper stores the data as an uncompressed flat file. I can't see anything in it
>that looks suspicious
>

Some further thoughts on this.

Starting a new 'master' project works.
Saving that project, then reloading it fails.
Loading it into V2.3.2 works.

Starting a new V2.3.2 project works.
Saving that project, then reloading it works.
Loading it into 'master' fails.

Logically, that suggests the problem is specifically the loading process in
'master'.

It's really strange (and frustrating) that only Reaper seems to have a problem
with it.

We've spent a lot of time on this, so do we carry on, or take a break till
later in the year?

-- 
Will J Godfrey

[Yoshimi-devel] Fail with Reaper :(

[Will G. <wil...@mu...>]

Reported by Art and confirmed by me.

Fetch the evaluation version of Reaper, and adding Yoshimi to an empty
project seems to work correctly. However if you save this, then on reloading
Reaper freezes (gkrellm reports one core pegged at 100%).
Switch LV2 version to Yoshimi V2.3.2 and the same project file loads and runs
correctly. 

Reaper stores the data as an uncompressed flat file. I can't see anything in it
that looks suspicious

-- 
Will J Godfrey

Re: [Yoshimi-devel] Note 'on' time reporting.

[Will G. <wil...@mu...>]

On Wed, 18 Sep 2024 01:39:34 +0200
ichthyo <pr...@ic...> wrote:

>On 18.09.24 00:10, Will Godfrey wrote:
>>>> Another possibility that might be practical is to use the switch setting
>>>> to enable/disable the timing code. That way, the only overhead would be
>>>> a couple of bool tests.  
>>
>> DOH!
>>
>> Just actually looked at the code. The switch is already there! I've just
>> modified it slightly so it encloses both the initial time call, as well as
>> the comparison, so I've made it permanently active and removed the build
>> option.  
>
>Its cool to get rid of a build option!
>
>Where can this feature now be switched off? Probably I am looking
>at the wrong place, but in the "Switches" tab I can find only a checkbox
>for the variable `Config::showTimes`. Moreover, are you referring to
>REPORT_NOTE_ON_TIME or to REPORT_NOTES_ON_OFF ?
>The latter seems to output a report only on the CLI.

It's REPORT_NOTE_ON_TIME in the code.
The switch is the one marked 'Log Load Times'. It is shared with loading
instruments. This might not be ideal, and maybe we should have a separate
switch, although I found it interesting to see the two reported together..

>>> Another tiny observation: in the "Main Settings" tab of the settings
>>> dialog, the drop-down list at bottom "Mixer Pannel Layout" is empty, even
>>> while the actual setting (one row with 16 strips) is retained correctly.  
>>
>> I'm not seeing this here. However, if you just nudge the mixer window size,
>> does it then correctly show?  
>
>Rescaling the settings window does not change anything. The current state is
>just not selected for this FL_choice.
>
>Looking at the one before, which is the instrument format, matters are
>different. There we immediately also select the current setting in make_window
>
>           o->add("Legacy");o->add("Yoshimi");o->add("Both");
>           o->value(synth->getRuntime().instrumentFormat - 1);
>
>The next one, the FL_coice "PanelLayout" does only populate the values,
>but does not select the current one. If I select the other option, the
>Panel layout changes, but the choice is again not selected in the Config-UI.
>
>Looking into the code, then the other one I use for comparison here, the
>"instrument" type, has a handling in returns_update.
>
>How is the panel layout persisted?
>I can see that in my config in Git from the previous version, there
>used to be a setting in the instance Config, called "single_row_panel"
>This setting seems to be gone since last release. The information
>seems only to be somehow retained in the window layout files. There
>seems to be GUI variable "MasterUI::paneltype", which is compared
>to several values (type == 1 and type == 2 but in MasterUI::Init()
>we set type = 5. And I can find no code that accesses this
>panel type to select the appropriate entry in the FL_select.

It never used "single_row_panel", so I removed it. The layout is stored in the
window position/size data (.config/yoshimi/windows/). It is purely a GUI feature
and has no effect on any running matters.

-- 
Will J Godfrey

Re: [Yoshimi-devel] Note 'on' time reporting.

[ichthyo <pr...@ic...>]

On 18.09.24 00:10, Will Godfrey wrote:
>>> Another possibility that might be practical is to use the switch setting
>>> to enable/disable the timing code. That way, the only overhead would be
>>> a couple of bool tests.
>
> DOH!
>
> Just actually looked at the code. The switch is already there! I've just
> modified it slightly so it encloses both the initial time call, as well as
> the comparison, so I've made it permanently active and removed the build
> option.

Its cool to get rid of a build option!

Where can this feature now be switched off? Probably I am looking
at the wrong place, but in the "Switches" tab I can find only a checkbox
for the variable `Config::showTimes`. Moreover, are you referring to
REPORT_NOTE_ON_TIME or to REPORT_NOTES_ON_OFF ?
The latter seems to output a report only on the CLI.


>> Another tiny observation: in the "Main Settings" tab of the settings
>> dialog, the drop-down list at bottom "Mixer Pannel Layout" is empty, even
>> while the actual setting (one row with 16 strips) is retained correctly.
>
> I'm not seeing this here. However, if you just nudge the mixer window size,
> does it then correctly show?

Rescaling the settings window does not change anything. The current state is
just not selected for this FL_choice.

Looking at the one before, which is the instrument format, matters are
different. There we immediately also select the current setting in make_window

           o->add("Legacy");o->add("Yoshimi");o->add("Both");
           o->value(synth->getRuntime().instrumentFormat - 1);

The next one, the FL_coice "PanelLayout" does only populate the values,
but does not select the current one. If I select the other option, the
Panel layout changes, but the choice is again not selected in the Config-UI.

Looking into the code, then the other one I use for comparison here, the
"instrument" type, has a handling in returns_update.

How is the panel layout persisted?
I can see that in my config in Git from the previous version, there
used to be a setting in the instance Config, called "single_row_panel"
This setting seems to be gone since last release. The information
seems only to be somehow retained in the window layout files. There
seems to be GUI variable "MasterUI::paneltype", which is compared
to several values (type == 1 and type == 2 but in MasterUI::Init()
we set type = 5. And I can find no code that accesses this
panel type to select the appropriate entry in the FL_select.

No doubt this is a minor itch, but maybe something got mixed up
in the course of the Config clean up done since last release?

-- Hermann

Re: [Yoshimi-devel] Note 'on' time reporting.

[Will G. <wil...@mu...>]

On Tue, 17 Sep 2024 21:04:38 +0200
ichthyo <pr...@ic...> wrote:

>> Another possibility that might be practical is to use the switch setting to
>> enable/disable the timing code. That way, the only overhead would be a
>> couple of bool tests.

DOH!

Just actually looked at the code. The switch is already there! I've just
modified it slightly so it encloses both the initial time call, as well as the
comparison, so I've made it permanently active and removed the build option.

Running a quite hardworking project that always gives three Xruns when the
buffer size is only 64 frames, still gives the same whether or not note
reporting is included.

>Another tiny observation: in the "Main Settings" tab of the settings dialog,
>the drop-down list at bottom "Mixer Pannel Layout" is empty, even while the
>actual setting (one row with 16 strips) is retained correctly. Is this the
>case also on your system? maybe my default configuration is not up-to-date.
>I could find the callback where changes to this selection are forwarded to
>the core, but I could not find the code which processes and sets the current
>state. Maybe you can recall the place where this happens?

I'm not seeing this here. However, if you just nudge the mixer window size, does
it then correctly show? If so, it's an FLTK redraw timeout. We've had this in
other places, and the solution was to make {window}Rtext() to repeat several
times on 'show()'

-- 
Will J Godfrey

Re: [Yoshimi-devel] Note 'on' time reporting.

[ichthyo <pr...@ic...>]

>> On 17.09.24 16:15, Will Godfrey wrote:
>>> Indeed, do we need to make it optional at all? The reporting is already
>>> user switchable,...
> On Tue, 17 Sep 2024 19:04:39 +0200
> ichthyo <pr...@ic...> wrote:
>> Accessing the system clock always costs some performance; it is an
>> overhead comparable to an ordered read from an atomic variable, so it's
>> not negligible.
>
> On 17.09.24 19:55, Will Godfrey wrote:
> OK, good point.
>
> I think that leaves two practical options.
>
> The obvious one is to hide the 'Log Load times' line in Settings->Switches
> when it hasn't been compiled for - not quite sure how we do that!

On which tab is this setting on? is it one of the toggles?
Anyway, we do disable check boxes for secondary instances, and we do it
right in the code in make_window(). There is an extra field code1{ ... }
which is typically used for that purpose. At that place we could also
hide a widget. Since we can not use conditional compilation directly
in the FLTK generated code, we'd have to place a static flag somewhere,
probably best into the Config object. This flag would then be set by
conditionally compiled code.

> Another possibility that might be practical is to use the switch setting to
> enable/disable the timing code. That way, the only overhead would be a
> couple of bool tests.
And testing a bool costs less than one nanosecond, but only under the assumption
that this piece of data is in the cache and it's hard to guess if this is the
case. But, placing that into perspective, our note-on/off code is anything
from highly optimised -- I'd guess any note-on processing causes some core
memory IO, because, what gets really cache-hot is the audio processing code.

Thus my gut feeling would be that testing yet another bool would not even
matter on this rather critical path. On the bright side, this would mean
we could get rid of some conditional compilation flags, which are a constant
source of hidden bugs, since there is a tendency to forget about them
(at least for me!)


> I actually use this looking for sources of Xruns created by a lot of note-on
>  events at the same time, or an 'expensive' note on at the same time as
> (say) an instrument change on a different part.
Sure, but would a musician, which is not also an engineer approach that topic
the same way? See, I for my part check in my presets into Git and often look
into the XML. But I've noticed time and again that other musicians without
engineering background tend to approach the use of technology in a
much more intuitive way

Sometimes I am thinking, maybe we should move into the direction of a
"developers mode". That would be a compile switch, but at once activate several
diagnostics, like toggle on assertions, produce more diagnostic messages and
then also measure runtimes at some crucial points. Obviously without seriously
degrading performance.



Another tiny observation: in the "Main Settings" tab of the settings dialog,
the drop-down list at bottom "Mixer Pannel Layout" is empty, even while the
actual setting (one row with 16 strips) is retained correctly. Is this the
case also on your system? maybe my default configuration is not up-to-date.
I could find the callback where changes to this selection are forwarded to
the core, but I could not find the code which processes and sets the current
state. Maybe you can recall the place where this happens?

-- Hermann

Re: [Yoshimi-devel] Note 'on' time reporting.

[Will G. <wil...@mu...>]

On Tue, 17 Sep 2024 19:04:39 +0200
ichthyo <pr...@ic...> wrote:

>On 17.09.24 16:15, Will Godfrey wrote:
>> Not a big issue, but I'm wondering if (in CMakeLists.txt) we should make this
>> compiled by default. I always have it enabled here.
>>
>> Indeed, do we need to make it optional at all? The reporting is already user
>> switchable, and no doubt some people have been confused when it seems to do
>> nothing, not having been compiled in.  
>
>Accessing the system clock always costs some performance; it is an overhead
>comparable to an ordered read from an atomic variable, so it's not negligible.
>Thus I would argue that it is not justified to put such a merely diagnostic
>feature on such a critical code path by default.
>
>Maybe it is in fact not relevant or it goes alongside with existing
>synchronisation overhead. But the point I'd make is, it is hard to prove
>such an added feature really does /not/ matter. Think of different audio
>systems, plugin hosts, different platforms. So without an extended
>investigation, we're just guessing and in such a situation I'd err to the
>safe side and not add anything to a hot path which is not strictly required.
>
>-- Hermann

OK, good point.

I think that leaves two practical options.

The obvious one is to hide the 'Log Load times' line in Settings->Switches when
it hasn't been compiled for - not quite sure how we do that!

Another possibility that might be practical is to use the switch setting to
enable/disable the timing code. That way, the only overhead would be a couple
of bool tests. Possibly with a bit of code duplication that could be reduced to
just one bool.

I actually use this looking for sources of Xruns created by a lot of note-on
events at the same time, or an 'expensive' note on at the same time as (say) an
instrument change on a different part.

P.S.
With Kristian's confirmation, and after I've dealt with Art's list of User Guide
typos and errors, I'll set V2.3.3 rc1.
 
-- 
Will J Godfrey