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

Working on Implementation of Panning #1061

Merged
merged 28 commits into from
Feb 2, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
5703755
Note & Instr. Pan interaction
oddtime Jan 3, 2021
c908c32
pan laws as static member functions of Sampler
oddtime Jan 3, 2021
83db742
add descriptive comments, make getRatioPan() obligatory, avoid the mu…
oddtime Jan 10, 2021
617ffd3
added other pan law functions
oddtime Jan 10, 2021
f847974
select pan Law from preferences
oddtime Jan 10, 2021
60574be
added 3 linear weighted pan laws
oddtime Jan 10, 2021
e977d81
save pan law as integer index in .h2song
oddtime Jan 10, 2021
90bd8a5
added 3 polar pan laws
oddtime Jan 10, 2021
f164f78
add constant k norm pan law
oddtime Jan 10, 2021
6e5cfbc
better manage of GUI pan Law combobox items, add 3 quadratic pan laws
oddtime Jan 11, 2021
cf4f147
add 3 constant Norm pan laws, commented unused method
oddtime Jan 11, 2021
19329e7
translatable, further descriptive comments
oddtime Jan 11, 2021
c7dd952
pan law members moved to Sampler, pan law enum class, save in song as…
oddtime Jan 18, 2021
f1cf556
add PanLawDialog, remove from PreferencesDialog
oddtime Jan 18, 2021
093bb6a
deprecate pointers to pan law static member function
oddtime Jan 18, 2021
7ecf62b
mixerSetting button in mixer master line
oddtime Jan 19, 2021
f606a60
correct and clean
oddtime Jan 19, 2021
2655e93
correct and clean 2
oddtime Jan 19, 2021
dad1cc0
merge master
oddtime Jan 19, 2021
99ae760
sharper icon
oddtime Jan 20, 2021
5089fab
add tooltip in pan law combo box
oddtime Jan 20, 2021
98813cd
headings in pan law combobox
oddtime Jan 23, 2021
9b5a79b
merge master
oddtime Jan 23, 2021
e799a46
typo
oddtime Jan 23, 2021
dc21783
mantainabilty changes
oddtime Jan 28, 2021
07c7afc
a line was missed
oddtime Jan 28, 2021
bface81
changelog, some graphic in doc comments
oddtime Jan 30, 2021
11bb83f
replace test.ref.flac because the new pan law functions introduce neg…
oddtime Feb 1, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 55 additions & 4 deletions src/core/Sampler/Sampler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,29 @@ void Sampler::noteOff(Note* pNote )
}


/// 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!!!!!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@oddtime oddtime Jan 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if ( fPan_L >= fPan_R ) {
return fPan_R / fPan_L - 1.;
} else {
return 1. - fPan_L / fPan_R;
}
}

//pan laws
float Sampler::ratioStraightPolPanLaw( float fPan ) {
// this return pan_L in the straight polygonal pan law, "ratio" pan parameter in [-1;1]
// It doesn't return ERROR if p is out of [-1;1] !!!!!
if ( fPan <= 0 ) {
return 1.;
} else {
return ( 1. - fPan );
}
}
//------------------------------------------------------------------

/// Render a note
/// Return false: the note is not ended
/// Return true: the note is ended
Expand All @@ -257,6 +280,30 @@ bool Sampler::renderNote( Note* pNote, unsigned nBufferSize, Song* pSong )
return 1;
}

// new instrument and note pan interaction--------------------------
// note pan moves the pan in a smaller pan range centered at instrument pan

// get the note and instrument pan parameters in [-1,1]
// TODO a new song member, set in preferences, must point the desired pan parameter Sampler member function
float (*getPan)( float, float );
// use "ratio" pan parameter (due to the current pan law)
getPan = &this->getRatioPan;
oddtime marked this conversation as resolved.
Show resolved Hide resolved
float fNotePan = getPan( pNote->get_pan_l(), pNote->get_pan_r() );
float fInstrPan = getPan( pInstr->get_pan_l(), pInstr->get_pan_r() );

// resultant pan: note pan moves the pan in a smaller pan range centered at instrument pan
float fPan = fInstrPan + fNotePan * ( 1 - fabs( fInstrPan ) );
oddtime marked this conversation as resolved.
Show resolved Hide resolved

// Pass fResPan to the Pan Law
// 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@oddtime oddtime Jan 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 😉 )

Copy link
Contributor Author

@oddtime oddtime Jan 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

float a = 0.5; // max gain (it has been 0.5 until version 1.0)
oddtime marked this conversation as resolved.
Show resolved Hide resolved
float fPan_L = a * panLaw( fPan );
float fPan_R = a * panLaw( -fPan );
//---------------------------------------------------------

bool nReturnValues [pInstr->get_components()->size()];

for(int i = 0; i < pInstr->get_components()->size(); i++){
Expand Down Expand Up @@ -604,9 +651,12 @@ bool Sampler::renderNote( Note* pNote, unsigned nBufferSize, Song* pSong )
cost_L = cost_L * pNote->get_velocity(); // note velocity
cost_R = cost_R * pNote->get_velocity(); // note velocity
}
cost_L = cost_L * pNote->get_pan_l(); // note pan


cost_L *= fPan_L; //pan
//cost_L = cost_L * pNote->get_pan_l(); // note pan
cost_L = cost_L * fLayerGain; // layer gain
cost_L = cost_L * pInstr->get_pan_l(); // instrument pan
//cost_L = cost_L * pInstr->get_pan_l(); // instrument pan
cost_L = cost_L * pInstr->get_gain(); // instrument gain

cost_L = cost_L * pCompo->get_gain(); // Component gain
Expand All @@ -619,9 +669,10 @@ bool Sampler::renderNote( Note* pNote, unsigned nBufferSize, Song* pSong )
cost_L = cost_L * pSong->get_volume(); // song volume
cost_L = cost_L * 2; // max pan is 0.5
oddtime marked this conversation as resolved.
Show resolved Hide resolved

cost_R = cost_R * pNote->get_pan_r(); // note pan
cost_R *= fPan_R; // pan
//cost_R = cost_R * pNote->get_pan_r(); // note pan
cost_R = cost_R * fLayerGain; // layer gain
cost_R = cost_R * pInstr->get_pan_r(); // instrument pan
//cost_R = cost_R * pInstr->get_pan_r(); // instrument pan
cost_R = cost_R * pInstr->get_gain(); // instrument gain

cost_R = cost_R * pCompo->get_gain(); // Component gain
Expand Down
3 changes: 2 additions & 1 deletion src/core/Sampler/Sampler.h
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,8 @@ class Sampler : public H2Core::Object
* layer will be loaded with a nullptr instead.
*/
void reinitializePlaybackTrack();

static float getRatioPan( float fPan_L, float fPan_R );
oddtime marked this conversation as resolved.
Show resolved Hide resolved
static float ratioStraightPolPanLaw( float fPan );

private:
std::vector<Note*> m_playingNotesQueue;
Expand Down