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

allow pan element in .h2pattern and drumkit #1249

Conversation

theGreatWhiteShark
Copy link
Contributor

while keeping backward compatibility.

Now, either pan or both pan_L and pan_R must be present.

@oddtime The pan parameters in the patterns are defined within [-0.5,0.5] while the one in the instruments are within [-1.0,1.0]. I guess you already know this and intend to streamline this one too, right?

while keeping backward compatibility.

Now, either `pan` or both `pan_L` and `pan_R` must be present.
@theGreatWhiteShark theGreatWhiteShark linked an issue Apr 24, 2021 that may be closed by this pull request
@oddtime
Copy link
Contributor

oddtime commented Apr 24, 2021

The pan parameters in the patterns are defined within [-0.5,0.5]...

I guess you meant [0;0.5] for note_pan (L/R) and [0;1] for instrument_pan (L/R) (they were gains for the channels originally)

I remember that. If you use the getRatioPan( panL, panR) of Sampler class to convert them to a scalar pan, the range could be arbitrary, as getRatioPan is a function of the ratio panL/panR. That gives the best compatibility

But since there is a division getRatioPan() implies truncations.

If we skip getRatioPan() we can use two dedicated inverse functions for note and instrument pan without divisions, exactly like in v1.0 style and there's no truncation

I don't know...

@oddtime
Copy link
Contributor

oddtime commented Apr 24, 2021

Other reflections:
When I proposed #1242, in order to avoid the truncations at each load and save, i.e. pan degenerating every time you save close and reopen a file (or not since the xml printer approximates the values, but even in this way I wouldn't like the approximation...), I thought to use two dedicated inverse pan laws (instead of getratiopan() ). In fact there was that test that you studied, failing because of truncation.
Since you are proposing the "pan" element in drumkit/pattern/ songfiles I think that we will not save pan_l and pan_r anymore by default and we can always use getratiopan() when importing from old files. But new saved files will be no compatible with old h2 versions...
We could enable a file menu option "save as old version"...?
I trust on you whether to leave Note panL/panR bounded in [0;1] or [0;0.5] or [0, inf).

@theGreatWhiteShark
Copy link
Contributor Author

But since there is a division getRatioPan() implies truncations.

In fact there was that test that you studied, failing because of truncation.

I think the truncation is okay because it just occurs once and the resolution of the pan values set by the GUI is not bigger than O(10^{-2}). For the failing tests I had to increase the resolution of the output to see what was going on. It just affects the last digit of the float.

Since you are proposing the "pan" element in drumkit/pattern/ songfiles I think that we will not save pan_l and pan_r anymore by default and we can always use getratiopan() when importing from old files.

Yes. This PR should be the basis on which we can deprecate the pan_L and pan_R values and use a single pan one instead.

But new saved files will be no compatible with old h2 versions...
We could enable a file menu option "save as old version"...?

Forward compatibility is a promise almost no FLOSS project gives (including ourselves) and I think this is OK since one has free access to the latest version. Also, as soon as a pattern or drumkit file is not validated it will be loaded by the legacy routines instead. So, you can have a basic usage in older versions.

I trust on you whether to leave Note panL/panR bounded in [0;1] or [0;0.5] or [0, inf).

The old ranges won't be touched. But I would vote for handling both the Note and Instrument pan in the same way using the same range. I did assume [-1.0,1.0] would be best but we can also use a different one.

@oddtime
Copy link
Contributor

oddtime commented Apr 24, 2021

The old ranges won't be touched. But I would vote for handling both the Note and Instrument pan in the same way using the same range. I did assume [-1.0,1.0] would be best but we can also use a different one.

I agree. [-1,1] makes an elegant symmetry (and the current formula works for that range).

Ok for all!

<xsd:element name="pan_R" type="h2:psfloat" default="1.0"/>
<xsd:choice minOccurs="0" maxOccurs="1">
<xsd:sequence>
<xsd:element name="pan_L" type="h2:psfloat" default="0.5"/>
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 special reason why here the default value has changed from 1.0 to 0.5?
I know that both the default pairs (0.5, 0.5) and (1,1) are converted into pan = 0, but maybe there's something I misunderstand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, those default values are redundant at best and misleading everywhere else. The XSD files are just used for validation and in this context the default values can be viewed was comments.

It's a good idea to have make them consistent with the default values used in the C++ code but IMHO an even better idea to get rid off them entirely.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out we use 0.5 as default value anyway.

@theGreatWhiteShark theGreatWhiteShark merged commit d175c5d into hydrogen-music:development May 6, 2021
@theGreatWhiteShark
Copy link
Contributor Author

@oddtime would you like to rewrite the pan handling (loading/storing/class members)?

@oddtime
Copy link
Contributor

oddtime commented May 6, 2021

@oddtime would you like to rewrite the pan handling (loading/storing/class members)?

Ok!

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.

More about the the pan_l pan_r members
2 participants