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

Single pan parameter element deprecates pan_L, pan_R #1273

Conversation

oddtime
Copy link
Contributor

@oddtime oddtime commented May 13, 2021

This deprecates the member pan_L and pan_R in Note and Instrument classes, replaces them by a single pan member.
Discussed in #1242.
Old drumkits and songfiles are imported as well. There is a custom setter/getter for the GUI widgets issue: the pan value is there in range [0;1], while in the Note/Instrument class and Sampler pan law functions it is used the range [-1;1].

Fixes a bug described in #1272. Fixes some incoeherent formulas described in #1167.

oddtime added 2 commits May 11, 2021 14:25
… to read float from xml and tell if the node was found, and use this function to get the pan from 'pan' node or ('pan_L','pan_R')
@oddtime oddtime changed the base branch from master to development May 13, 2021 08:56
Copy link
Contributor

@theGreatWhiteShark theGreatWhiteShark left a comment

Choose a reason for hiding this comment

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

Looks good. Could you update the two drumkits and the demo songs in data/ which we ship along with Hydrogen too?

@@ -244,14 +232,13 @@ void CoreActionController::setStripPan( int nStrip, float fPanValue, bool bSelec
InstrumentList *pInstrList = pSong->getInstrumentList();

auto pInstr = pInstrList->get( nStrip );
pInstr->set_pan_l( fPan_L );
pInstr->set_pan_r( fPan_R );
pInstr->setPanWithRangeFrom0To1( fValue );
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a benefit in keeping the range of the pan in the Mixerline from 0 to 1? How about we make it [-1,1] instead and note it done in the CHANGELOG? Since the values in existing songs will be properly mapped to the new values and the rotary is still a centered one, old users won't probably notice the difference while for new one the symmetric behavior might be more intuitive.

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 think it is ok to adopt [-1,1].

}

setStripPan( i, fPanValue, false );
float fValue = pInstr->getPanWithRangeFrom0To1();
Copy link
Contributor

Choose a reason for hiding this comment

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

I see. By changing the range we might break existing scripts relying on MIDI/OSC feedback of the pan in the range of [0,1]. But I think it's not as crucial as a mismatch between the pan value the user sets in the Mixerline and the one that is written in the .h2song file. What's your opinion on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, we need those getters and setters for the midi at least.
I am not a user of osc, but I imagined that I may have broken it. So I trust on your opinion

What about the note pan properties ruler? Maybe the way it is now is ok as it has not an associated widget value?

Copy link
Contributor

Choose a reason for hiding this comment

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

After some thought: how about that: all the pan functions in the MIDI / OSC part continue to use the range [0,1] and we introduce additional ones for the range [-1,1], like PAN_ABSOLUTE_SYM and PAN_RELATIVE_SYM. The asymmetric ones we mark as deprecated in version 1.3 (printing a deprecation message each time they are used in the ERRORLOG + a notice in the CHANGELOG) and remove them in 1.4. This won't break anything at first and gives everyone enough time to adopt their scripts/software.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.
You mean to write a new function setStripPan() for the range [-1;1]?

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean to write a new function setStripPan() for the range [-1;1]?

Yes. Having a dedicated function associated with the new symmetric PAN endpoints will make things more consistently than adding an argument in setStripPan.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Not to demand the job, but some info/example would speed up the process since I don't know how the osc syntax works.
I see that OscServer depends on MidiAction functions.
Is Osc some kind of messages that the user writes on the shell? If so, can values be fractional? I see they are generally assumed as int in [0;127]

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, so first of all in order to easily test OSC commands I do recommend the oscsend shell program. It's bundled in the liblo-tools repo (AFAIR you are using a Debian-based Linux).
The following command will set the pan value of the first mixer strip to 0.9 (note that you have to enable OSC support in the PreferencesDialog (and maybe restart) first).

oscsend localhost 9000 /Hydrogen/PAN_ABSOLUTE/1 f 0.9

I see that OscServer depends on MidiAction functions.
Is Osc some kind of messages that the user writes on the shell? If so, can values be fractional? I see they are generally assumed as int in [0;127]

You can think of OSC as something like an extension of MIDI to allow for various other message types, like floats, strings, and arbitrary blobs of data, to easily achieve control via a network and some other stuff.

Unfortunately, the OSC interface is not as clean as it could be. All numerical input has to be provided as floats, even if Hydrogen just wants integer, and also functions requiring no input value at all, like stopping the playback, are registered as requiring a float input too. This is to mitigate limitations of certain OSC implementations, like TouchOSC.

PAN_ABSOLUTE/<STRIP_NUMBER> requires a value between 0.0 and 1.0. PAN_RELATIVE/<STRIP_NUMBER> either 1 or -1 (or any value other than 1). In the new version of the documentation I noted the input ranges, types, and basic information about all OSC commands. But it's not available online yet but still dormant in the other repo,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I have pushed a commit for this...
What would be the benefit of PAN_RELATIVE_SYM?

Copy link
Contributor

Choose a reason for hiding this comment

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

What would be the benefit of PAN_RELATIVE_SYM?

You're right. I guess none. Sorry for the noise :)

@oddtime
Copy link
Contributor Author

oddtime commented May 31, 2021

Looks good. Could you update the two drumkits and the demo songs in data/ which we ship along with Hydrogen too?

Yes ;)

data/DefaultSong.h2song Outdated Show resolved Hide resolved
@theGreatWhiteShark
Copy link
Contributor

@oddtime One of the test still fails since one of the shipped drumkits is not valid.

@oddtime
Copy link
Contributor Author

oddtime commented Jun 7, 2021

restoring
<?xml version="1.0" encoding="UTF-8"?>
in the drumkit files makes the test passing...

@theGreatWhiteShark
Copy link
Contributor

restoring
<?xml version="1.0" encoding="UTF-8"?>
in the drumkit files makes the test passing...

could you push this fix? We changed the PR policy in the repo a couple of weeks ago and I can only merge if all tests are passing

Copy link
Contributor

@theGreatWhiteShark theGreatWhiteShark left a comment

Choose a reason for hiding this comment

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

Looks like we are done (except for the xml version in the drumkit.xml files). Nice work. Thanks a lot!

@oddtime
Copy link
Contributor Author

oddtime commented Jun 8, 2021

Thanks! Fix pushed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants