Working on Implementation of Panning #1061

oddtime · 2021-01-03T09:29:50Z

See issue #665

I don't want this pull request to be merged now, but just share it.

In Hydrogen the two pan parameters control two separate balances, like a chain, then if they are hard panned to opposite sides, the signal is killed. If there is a reason to mantain this, or somebody likes it, we can add the new feature as option in preferences

What is changed now is, from a user point of view:
note and instrument pan interation: note pan moves the pan in a smaller pan range centered at instrument pan.
In this pull req. a resultant pan is calculated then the balance law is called. The signal is never killed.
Of course if the instrument pan is set to the left, you cannot move the signal to Hard right with the note pan (since note pan range is smaller and centered at instrument pan); to achieve this, other strategies would be required as discussed in #665 (for example note pan bypassing instrument pan, this could be another option in preferences if somebody likes it).

In this pull req, the way hydrogen saves pan is unchanged: pan_L and pan_R for both note and instrument objects, even if it is now strange and h2 needs to convert them to single parameters and then applies the pan law again.

I used some pointers to member functions, in the future hypothesis of different pan laws implementation. Just to test them, but they should become song members.

// Edit: Added many pan law options, set in mixer: new button on top-right

theGreatWhiteShark · 2021-01-05T16:05:00Z

I don't want this pull request to be merged now, but just share it.

Is it still work in progress? If so, you can also convert it into a "draft" (via the right column under the heading "Reviewers")

oddtime · 2021-01-05T18:21:40Z

It is in progress if we decide to add other pan features...
But you can test it and give an opinion on what is already done, described in the first comment

theGreatWhiteShark · 2021-01-09T14:22:07Z

src/core/Sampler/Sampler.cpp

+/// functions for pan parameters and laws-----------------
+float Sampler::getRatioPan( float fPan_L, float fPan_R ) {
+	// returns the single pan parameter in [-1,1] from the L,R gains
+	// It doesn't return ERROR if (L,R) = (0,0) nor if they are negative!!!!!


Regarding the errors in those two functions: If there is no straight forward default value one can return/set, the C++ way of implementing errors would be an exception. But in Hydrogen it's almost never used (and introducing it would make the code most probably harder to maintain).

I would suggest to introduce these limits to Instrument::set_pan and Note::set_pan instead. If > 0, 0 will be assigned and a WARNINGLOG triggered and the same for 1 from above.

I would suggest to introduce these limits to Instrument::set_pan and Note::set_pan instead. If > 0, 0 will be assigned and a WARNINGLOG triggered and the same for 1 from above.

Ok, I keep it for the final version. please don't resolve this conversation

I would suggest to introduce these limits to Instrument::set_pan and Note::set_pan instead.

There is already a check for negative pan:

void Note::set_pan_l( float pan ) { __pan_l = check_boundary( pan, PAN_MIN, PAN_MAX ); }

In this way H2 one can set both pan_L = pan_R = 0. In this case there is a problem to reconvert back to the pan parameter. It could happen importing a bad song.

src/core/Sampler/Sampler.cpp

src/core/Sampler/Sampler.h

src/core/Sampler/Sampler.cpp

theGreatWhiteShark · 2021-01-09T14:33:53Z

src/core/Sampler/Sampler.cpp

+	// use Straight pol pan law.
+	// TODO a new song member, set in preferences, must point the desired pan law Sampler member function		
+	float (*panLaw)( float );
+    panLaw = &this->ratioStraightPolPanLaw;


How about making a single Sampler::panLaw( float, enum) and an enum class for the different pan laws? Inside this function you could use switch-case statement to distinct between the laws.

I preferred to use pointers to bypass the conditions checks and to go straight to the function

As the addresses of pan law methods depend on the Sampler object, maybe this pointer should be saved in some form in the song (for example written as an int or enum, which type is new to me now), but should live in Sampler class.
What do you think?

I preferred to use pointers to bypass the conditions checks and to go straight to the function

I see. But I would like to ask you to use switch-case or if statements nevertheless. It's not because I have a personal reference but the code base of Hydrogen does use the former over function pointers. It's more easy to maintain it if there is a consistent coding style. (Admittedly, the one we deal with is not the best but, well, it works. 😉 )

I understand the reason.
I would just ask you if you would consider consistent this alternative:
float (*m_pPanLaw)( float ) is a member in Sampler;
while import the song, or setting the preferences, the corresponding pan law function address is assigned to m_pPanLaw using if statements instead of that weird array of addresses introduced in commit 60574be

src/core/Sampler/Sampler.cpp

…ltiplication * 0.5 *2

trebmuh

Not a programmer, but still helping where I can 😉

src/gui/src/PreferencesDialog_UI.ui

oddtime · 2021-01-10T19:41:59Z

I am attaching a songfile to test the pan laws!
It is the same mono sound going linearly from left pan to right pan
panning.zip
Note: linearly refers to the pan parameter, not to the perceived angle, which depends on the pan laws.

oddtime · 2021-01-12T13:24:09Z

A summary of what is included until the last commit: different pan laws to set in preferences->audio.
Basically at this point you can hear how different laws sound.
Results sound good in my opinion, there's also a huge variety of options (for the pleasure of research), which is not given in any DAW I know.
With the "linear" pan parameter it seems that you have better control of the stereo position with the pan pot, in my opinion.
Ask if you have any questions. I described something in the comments.

The type of pan law is already saved in the songfile, but I am not convinced about the way it is saved.
I am not totally convinced about the way to store the pan law in the core as well (in which class, using indices, pointers etc).

theGreatWhiteShark · 2021-01-13T14:46:11Z

different pan laws to set in preferences->audio.

The type of pan law is already saved in the songfile, but I am not convinced about the way it is saved.
I am not totally convinced about the way to store the pan law in the core as well (in which class, using indices, pointers etc).

Hydrogen has two main ways for storing information: within each .h2song file and a global hydrogen.conf file. The latter should contain all the customization the user is likely to use for a lot of sessions and the former those stuff that might change with every song. The panning law feels like a global setting (especially since it can be set via the preference dialog). Therefore, I would move it from the Song class into Preferences.

Regarding storing: I think the options the user is able to alter should be in the Preferences and everything else can be a member of Sampler. Or do you think other parts of the program expect the Sampler will use these functions too?

oddtime · 2021-01-13T15:08:57Z

Hydrogen has two main ways for storing information: within each .h2song file and a global hydrogen.conf file. The latter should contain all the customization the user is likely to use for a lot of sessions and the former those stuff that might change with every song. The panning law feels like a global setting (especially since it can be set via the preference dialog). Therefore, I would move it from the Song class into Preferences.

Since the pan law affects how the project sounds and how it will be mixed, it should be stored in the .h2song in my opinion.
We can save it in hydrogen.conf just to keep it saved for the next songs

Or do you think other parts of the program expect the Sampler will use these functions too?

I don't think so

oddtime · 2021-01-13T15:49:28Z

And what about the way of storing in the file? with an integer number? Maybe I would go with a meaningful string

theGreatWhiteShark · 2021-01-16T06:23:17Z

We can save it in hydrogen.conf just to keep it saved for the next songs

I would store it in just one file. Else it will only cause confusion.

Since the pan law affects how the project sounds and how it will be mixed, it should be stored in the .h2song in my opinion.

I do see your point. But if we store it in the Song file the option must not be set via the PreferenceDialog. Do you think user would set different values for each song or just play with it and decide for one that fits them best?

theGreatWhiteShark · 2021-01-16T06:25:42Z

And what about the way of storing in the file? with an integer number? Maybe I would go with a meaningful string

An enum class. This way the variable name is a meaningful name but it's value is an integer which is way more convenient to handle. See https://github.com/hydrogen-music/hydrogen/blob/master/src/core/Preferences.h#L375 for an example.

oddtime · 2021-01-16T10:06:11Z

I do see your point. But if we store it in the Song file the option must not be set via the PreferenceDialog. Do you think user would set different values for each song or just play with it and decide for one that fits them best?

I will try to explain why I would like to save it in the song
Actually my concern is: once you change the type of pan law the song sounds in a different way.
The main reason is not only the change of constraint (constant sum, constant power...) indeed, but the way the pan parameter maps the "virtual stereo positions". I mean that the same pan parameter value may "put" the signal in any point of the half stereo panorama depending on the pan law, except for the "central" and "hard-side" positions. We may ask why do not adopt just one bijection between pan and the "virtual stereo placement"? The reason is, in my thought, that there is no a solution to this problem, because the perceived angle could depend on difficult quantities like frequency (also for psycho-acoustics and non-linearity). Also, even if the perceived angle would depend on gain_L and gain_R only, it is not clear what would be the "linear" dependency between gain_L, gain_R and the perceived angle (I said "linear" but of course we don't know any relation). That is to say, the pan laws that I found on manuals, which are all different and inconsistent in my opinion (i.e. switching between them the angle does change, as you can hear experimentally), are all valid theorically. So I included all that pan laws but introduced the "ratio","linear", "quadratic", "polar" distinction to create classes of pan law that map the sounds always in the same points (at least it seems so by ear, and I have another reasonable argument for that...), with compensated gain depending on which pan law. Plus I added a custom pan law which generalises the constraint L^k+R^k=1, making consistent compromises.
All this is to say that if you share a song or for some reason you have changed the pan law setting preference, when you re-open the song you may listen to a very different mix. Tests by ear confirm that (try to switch between "linear" and "quadratic")
On daws also the pan laws are stored in the project (I searched briefly on the web).

Do you think user would set different values for each song?

If you use pan note property for a surrealistic effect of instruments moving in the space, this could happen. Otherwise I think that every pan law is equivalent, as you will compensate the gain by ear.
Ps: the new pan laws are properly used if samples are mono. If samples are dual channel instead, balance law should be used, in fact the pan section in the mixer strips of stereo tracks in advanced daws gives the alternate option to pan each channel separately or to balance them and I think that the latter doesn't use the center compensation. So this could be a reason to use a different pan law depending on the drumkit since h2 mixer doesn't take care if the instrument samples are mono o stereo.

theGreatWhiteShark · 2021-01-17T07:48:54Z

Yes, you are right. Also when sharing a song the panning law shouldn't be altered by the preferences of the other user.

This still leaves the question of where to put the control/widget to alter this setting. Since it just concerns the internal sound generation it might go to the Mixer. But there isn't a place for it yet.

But wait. Does it only concerns the internal sound generation? Doesn't it affect the MIDI export of the notes too? (disclaimer: I have almost no idea about MIDI)

oddtime · 2021-01-17T09:57:20Z

This still leaves the question of where to put the control/widget to alter this setting.

Ok. I approve putting a new widget in the mixer. I think a button on the right, accordingly with the pull req #525
Another place could be in menu->project

Doesn't it affect the MIDI export of the notes too?

In MIDI, pan has its dedicated message from 0-127 so it seems that midi does not transmit the information of separate gain_L and gain_R.
There is also a MIDI "Balance" message from 0-127 which is used for stereo samples generally.
However these messages act on the midi channel, so we can't control each instrument pan separately unless every instrument is output to a different channel. The note pan property is not available in midi

… string

theGreatWhiteShark · 2021-01-21T15:47:18Z

How about introducing 4 headings/categories in the "Select Pan Law" options: linear, polar, ratio, quadratic. Right now it's a little bit hard to get an overview with so much different options densely packed.

oddtime · 2021-01-21T16:24:07Z

Well I had the same idea but was lazy to set it

oddtime · 2021-01-21T18:46:49Z

Unfortunately I am not an expert of Qt and this seems complex.
Is there a way of putting category headings in a QComboBox, simpler than the following?
https://stackoverflow.com/questions/33012292/grouped-qcombobox

theGreatWhiteShark · 2021-01-22T08:37:38Z

Unfortunately I am not an expert of Qt and this seems complex.
Is there a way of putting category headings in a QComboBox, simpler than the following?
https://stackoverflow.com/questions/33012292/grouped-qcombobox

Well, unfortunately neither am I.

First of all I would recommend using the QtCreator (I was manually editing the UI files before realizing this was not the intended way). Doubling clicking the combobox gives some control over its content. Maybe some lines can be disabled and be display as bold or italic. Also, the resolution combobox in the pattern editor features separators. They might help too

oddtime · 2021-01-22T14:39:06Z

The current pan law combobox already uses separators but there is not so much contrast to see them clearly.

I used Qt creator to create the dialog, but added the combobox items in MixerSettingsDialog.cpp because I can associate the enum class to each (which is in Sampler class). This seemed good to avoid referring to indices.

oddtime · 2021-01-23T10:37:19Z

I found a way

oddtime · 2021-01-24T10:48:41Z

Are you ok with the current namings?
Basically linear and quadratic are abbreviations of "average, linear weights" and "average, quadratic weights", polar means "polar coordinates", ratio well means the ratio :)
I hope this makes sense if you have read the comments in the code, otherwise I can explain it better.

theGreatWhiteShark · 2021-01-24T15:59:11Z

I found a way

Nice!

I'll do my best to review the changes during the next days.

Are you ok with the current namings?
Basically linear and quadratic are abbreviations of "average, linear weights" and "average, quadratic weights", polar means "polar coordinates", ratio well means the ratio :)

Sounds good.

theGreatWhiteShark

Nice. I like it!

Some minor things but almost done.

src/core/Basics/Song.cpp

src/core/LocalFileMgr.cpp

src/core/Sampler/Sampler.cpp

src/gui/src/Mixer/MixerSettingsDialog_UI.ui

oddtime · 2021-01-28T16:05:58Z

Thanks for the check! I did the changes. If you want merge I would add the feature in Changelog

theGreatWhiteShark · 2021-01-29T13:31:50Z

If you want merge I would add the feature in Changelog

Yes, please.

@oddtime one last thing. One of the unit tests does fail. Could you look into it (using ./build.sh t)?

@trebmuh are all of your requested changes covered? I'm fine with everything as soon as the unit test is resolved.

oddtime · 2021-01-29T17:56:30Z

1) test: FunctionalTest::testExportAudio (F) line: 130 /home/scarbo/programming/H2Repo/hydrogen/src/tests/FunctionalTests.cpp
Files differ at sample 89898
- Expected: /home/scarbo/programming/H2Repo/hydrogen/src/tests/data//functional/test.ref.flac
- Actual  : /tmp/hydrogen/test-wiTawq.wav

Any ideas? Files are 2 s long at 44100Hz...
//edit. right it is a stereo track, has 88200 *2 samples

trebmuh · 2021-01-30T10:10:46Z

@trebmuh are all of your requested changes covered?

They were only typo/cosmetic requests. It looks all good from my PoV.

oddtime · 2021-01-30T15:39:49Z

the first tracks are the exported and reference files: test-wiTawq.wav (inverted) test.ref.flac. The third track is the difference

Normalization shows that the difference is not 0 at this sample.

but it something like -50dB...
There is another sample like that, may it be caused by a small CPU approximation error (getting back the pan from panL and panR and recalculating the pan law)?

oddtime · 2021-01-30T15:53:07Z

I modified the c++ file doing the test to print the different samples in the shell:

.........................sample1 = 7484, sample2 = 7485

theGreatWhiteShark · 2021-01-31T15:00:19Z

Yeah, two little differences. One in the middle of the song and one at 3/4. I noticed that when diffing the hexdump of these files too.

I did place some prints to std::cout in the DiskWriterDriver and the Sampler to see what's going on. There were quite a number of (very tiny) differences in the float values written in the former. But in Sampler::renderNote() the cost_* variables have not changed at all. And, AFAIR, this is the very place you did some changes in.

I will keep digging until I find something

oddtime · 2021-01-31T17:03:27Z

Thanks @theGreatWhiteShark !

theGreatWhiteShark · 2021-02-01T20:13:36Z

Okay. Everything's fine.

There is some truncation happening causing very small differences in the resulting output. In principle we could make the pan calculate use double instead of float but IMHO this would be over the top.

Could you take one of these temporary results and replace the test.ref.flac file?

oddtime · 2021-02-01T21:29:53Z

Ok, done.
(but there's still that test.wav which seems the old reference)

…ligible float differences

oddtime added 2 commits January 3, 2021 09:04

Note & Instr. Pan interaction

5703755

pan laws as static member functions of Sampler

c908c32

oddtime mentioned this pull request Jan 3, 2021

Implementation of the panning #665

Closed

oddtime marked this pull request as draft January 5, 2021 16:33

theGreatWhiteShark reviewed Jan 9, 2021

View reviewed changes

oddtime added 3 commits January 10, 2021 16:23

add descriptive comments, make getRatioPan() obligatory, avoid the mu…

83db742

…ltiplication * 0.5 *2

added other pan law functions

617ffd3

select pan Law from preferences

f847974

trebmuh requested changes Jan 10, 2021

View reviewed changes

src/gui/src/PreferencesDialog_UI.ui Outdated Show resolved Hide resolved

src/gui/src/PreferencesDialog_UI.ui Outdated Show resolved Hide resolved

src/gui/src/PreferencesDialog_UI.ui Outdated Show resolved Hide resolved

oddtime added 5 commits January 10, 2021 20:42

added 3 linear weighted pan laws

60574be

save pan law as integer index in .h2song

e977d81

added 3 polar pan laws

90bd8a5

add constant k norm pan law

f164f78

better manage of GUI pan Law combobox items, add 3 quadratic pan laws

6e5cfbc

oddtime force-pushed the panning branch from 90624db to 6e5cfbc Compare January 11, 2021 15:35

oddtime added 2 commits January 11, 2021 17:33

add 3 constant Norm pan laws, commented unused method

cf4f147

translatable, further descriptive comments

19329e7

pan law members moved to Sampler, pan law enum class, save in song as…

c7dd952

… string

add tooltip in pan law combo box

5089fab

headings in pan law combobox

98813cd

oddtime added 2 commits January 23, 2021 17:53

merge master

9b5a79b

typo

e799a46

theGreatWhiteShark requested changes Jan 27, 2021

View reviewed changes

mantainabilty changes

dc21783

a line was missed

07c7afc

theGreatWhiteShark approved these changes Jan 29, 2021

View reviewed changes

changelog, some graphic in doc comments

bface81

oddtime mentioned this pull request Feb 1, 2021

old gmkit test.wav? #1137

Closed

replace test.ref.flac because the new pan law functions introduce neg…

11bb83f

…ligible float differences

theGreatWhiteShark merged commit fdb5bfb into hydrogen-music:master Feb 2, 2021

Working on Implementation of Panning #1061

Working on Implementation of Panning #1061

Conversation

oddtime commented Jan 3, 2021 • edited Loading

theGreatWhiteShark commented Jan 5, 2021

oddtime commented Jan 5, 2021

theGreatWhiteShark Jan 9, 2021

Choose a reason for hiding this comment

oddtime Jan 9, 2021 • edited Loading

Choose a reason for hiding this comment

oddtime Jan 20, 2021

Choose a reason for hiding this comment

theGreatWhiteShark Jan 9, 2021

Choose a reason for hiding this comment

oddtime Jan 9, 2021

Choose a reason for hiding this comment

oddtime Jan 10, 2021 • edited Loading

Choose a reason for hiding this comment

theGreatWhiteShark Jan 13, 2021

Choose a reason for hiding this comment

oddtime Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

trebmuh left a comment

Choose a reason for hiding this comment

oddtime commented Jan 10, 2021

oddtime commented Jan 12, 2021

theGreatWhiteShark commented Jan 13, 2021

oddtime commented Jan 13, 2021

oddtime commented Jan 13, 2021

theGreatWhiteShark commented Jan 16, 2021

theGreatWhiteShark commented Jan 16, 2021

oddtime commented Jan 16, 2021 • edited Loading

theGreatWhiteShark commented Jan 17, 2021

oddtime commented Jan 17, 2021 • edited Loading

theGreatWhiteShark commented Jan 21, 2021

oddtime commented Jan 21, 2021

oddtime commented Jan 21, 2021

theGreatWhiteShark commented Jan 22, 2021

oddtime commented Jan 22, 2021

oddtime commented Jan 23, 2021

oddtime commented Jan 24, 2021

theGreatWhiteShark commented Jan 24, 2021

theGreatWhiteShark left a comment

Choose a reason for hiding this comment

oddtime commented Jan 28, 2021

theGreatWhiteShark commented Jan 29, 2021

oddtime commented Jan 29, 2021 • edited Loading

trebmuh commented Jan 30, 2021

oddtime commented Jan 30, 2021 • edited Loading

oddtime commented Jan 30, 2021

theGreatWhiteShark commented Jan 31, 2021

oddtime commented Jan 31, 2021

theGreatWhiteShark commented Feb 1, 2021

oddtime commented Feb 1, 2021

oddtime commented Jan 3, 2021 •

edited

Loading

oddtime Jan 9, 2021 •

edited

Loading

oddtime Jan 10, 2021 •

edited

Loading

oddtime Jan 13, 2021 •

edited

Loading

oddtime commented Jan 16, 2021 •

edited

Loading

oddtime commented Jan 17, 2021 •

edited

Loading

oddtime commented Jan 29, 2021 •

edited

Loading

oddtime commented Jan 30, 2021 •

edited

Loading