Added support of specifying different crossfade length for additional attack and release files https://github.com/GrandOrgue/grandorgue/issues/1760

[oleg68]

Resolves: #1760

This PR introduces the nes ODF keys

• Pipe999Attack999LoopCrossfadeLength
• Pipe999Attack999ReleaseCrossfadeLength
• Pipe999Attack999LoopCrossfadeLength

The behavior of Pipe999LoopCrossfadeLength for multi-attack pipes is changed: earlier it was applied for all attack sample files, now it is applied only to the attack sample file specified with the Pipe999. All other attack sample files take the value 0 unless Pipe999Attack999LoopCrossfadeLength are specified for them.

The old odf Pipe999ReleaseCrossfadeLength specifies the some value for the release is loaded from the main attack sample file Pipe999. It also used as default value of crossfade for releases are loaded from all other sample files of this pipe and may be overriden with Pipe999Attack999ReleaseCrossfadeLength and Pipe999Attack999LoopCrossfadeLength for switching to the releases have been loaded from certain attack and release sample files.

Only Pipe999ReleaseCrossfadeLength is used for switching between different attack samples (ex. Tremulant ON/OFF).

[larspalo]

@oleg68 The comment description above is a bit messed up and thus difficult to understand. Luckily, the help description in the code for this PR is good and explain it very well instead. Everything seems to be working as advertised in all the tests I've performed.

Are there any specific reason why the inheritance couldn't be the same (none at all) also for the Pipe999ReleaseCrossfadeLength? Also, if no release is loaded from the main pipe sample - then warn about it as a non used parameter if present? As I see it, there wouldn't be any loss of possibilities to configure each level with such an approach. The gain from no inheritance at all would be a similar behavior for both types of values and thus a more logical effect of the values supplied (I think it would be more predictable and their usage would be more easily seen in the odf for the one constructing it).

Eventually, the attack switching crossfade for tremulant/non-tremulant samples should have its own configuration to emulate the startup and slowdown time - and until then no harm is done by not inheriting the value (but respecting it if valid and supplied together with a release).

[oleg68]

Are there any specific reason why the inheritance couldn't be the same (none at all) also for the Pipe999ReleaseCrossfadeLength?

The first reason is that a transition between differerent samples always requires some crossfade that almost independent on the target sample itself, but the transition between an end and a start of the same loop normally does not require any crossfade unless the sample is odd. Even if some attack sample is odd we may not assume that all other attack samples are same odd. That is why release crossfade is inherited and loop crossfade is not.

The second reason is the compatibility with the old behavior. Now we have a lot of multi-release sample sets and very few multi-attack sample sets. If we broke inheritance of release crossfade length it would impact lots of existing sample sets, but changing of inheritance of loop crossfade most probably does not impact any sample set (because 0 is goot for most cases).

Also, if no release is loaded from the main pipe sample - then warn about it as a non used parameter if present?

No, it is not. The parameter is still used for an attack/attack transition and as default for other release samples.

As I see it, there wouldn't be any loss of possibilities to configure each level with such an approach. The gain from no inheritance at all would be a similar behavior for both types of values and thus a more logical effect of the values supplied (I think it would be more predictable and their usage would be more easily seen in the odf for the one constructing it).

Now (before this PR) there is a capability of sppecifying neither LoopCrossfadeLength, ReleaseCrossfadeLength, nor Gain at the sample file level. It exists only at the pipe level and is inherited to all sample files. This PR breaks inheritance only of LoopCrossfadeLength, but leaves them for all other parameters.

(I think it would be more predictable and their usage would be more easily seen in the odf for the one constructing it).

1. It would broke the existing sample sets
2. It would require to specify more keys in ODFs than now.

Eventually, the attack switching crossfade for tremulant/non-tremulant samples should have its own configuration to emulate the startup and slowdown time - and until then no harm is done by not inheriting the value (but respecting it if valid and supplied together with a release).

I agree, but this would be a completely new functionality. It is more delay than crossfade. It is not so simple. So I would defer it.

[larspalo]

If we broke inheritance of release crossfade length it would impact lots of existing sample sets

@oleg68 Right, it's likely only a very small number of original sample sets that would be affected anyway. I suspect that most usage for any of the crossfade values will come from conversions done by OdfEdit...

No, it is not. The parameter is still used for an attack/attack transition and as default for other release samples.

I do understand that it's not warned about and ignored in this PR. What I tried to say was that it perhaps would be better if it shouldn't be used as a default at all, and for symmetry with the additional attacks would only be allowed when a release actually was loaded with it (main pipe sample, that is).

It would broke the existing sample sets

This PR already breaks any existing sample sets that use LoopCrossfadeLength values and have multiple attacks. There are a few of them out there that uses multiple attacks. Yes, but very few (or close to none) original ones that I know of that use the LoopCrossfadeLength though... The risk that any existing (original) sample set would be seriously broken by not anymore inheriting any of the crossfade values is most likely negligible, or equal to none at all! It's just a change of behavior that would be similar for both types of crossfades then.

It would require to specify more keys in ODFs than now.

No, not necessarily in all cases, rather the opposite if I with this PR would like to only specify a release crossfade length for the release loaded with the main pipe sample and not having it applied to any other release(s). In such cases I must specifically override all other releases, which would result in more lines with values needing to be entered. And of course I understand that this wasn't even possible before... And of course I know that there are ways to work around that too by simply never loading the release sample from the main pipe sample. But the bottom line is that it would require a change of strategy anyway for sample set producers! That's the main reason for why I'd opt for a similar behavior for all the crossfades as it would be easier to know exactly when what value was used.

this would be a completely new functionality. It is more delay than crossfade. It is not so simple. So I would defer it.

Sure, I just tested it with this PR and it does indeed work beautifully - but it's not possible to specify perfectly for obvious reasons...

However, for this PR, if you really, really, don't want the crossfade values behave in similar fashion, I won't protest anymore about it even though I still think it's wrong. My reasoning is that there are likely so few original sample set creators that will use any of the odf crossfade values anyway since the benefits of using them are not really tangible. The release alignment and calculated crossfade values are working fine without any manual intervention at all (most of the time).

[oleg68]

I do understand that it's not warned about and ignored in this PR. What I tried to say was that it perhaps would be better if it shouldn't be used as a default at all, and for symmetry with the additional attacks would only be allowed when a release actually was loaded with it (main pipe sample, that is).

I totally support your desire for symmetry. Unfortunally, a rule and an exception are not symmetric. Of cause, we should minimise the quantity of exceptions.

ReleaseCrossfade (specified or calculated) is a rule, as well as Gain and lots of other attributes, but LoopCrossfade is an exception, because it corrrects a Recording/Looping error, otherwise it would not be necessary.

This PR already breaks any existing sample sets that use LoopCrossfadeLength values and have multiple attacks.

Yes, but I consider the probability of it is much less than the probability of impact of ReleaseCrossfadeLength because:

1. The number of Multi-Attack sample sets is quite less than one of Multi-Release sample sets
2. The necessarity of LoopCrossfade is less than the ReleaseCrossfade despite it may be calculated
3. The example of OdfEdit showed necessarity only of changing the LoopCorrection behavior

it would require a change of strategy anyway for sample set producers! That's the main reason for why I'd opt for a similar behavior for all the crossfades as it would be easier to know exactly when what value was used.

You said about the symmetry and mentioned another attribute - Pipe999Gain. I can agree that only few samplesets use crossfade values, but Pipe999Gain is used widely. If we broke inheritance of Pipe999Gain to secondary sample files in the same way, it would affect a lot of existing sample sets. And the number of lines in ODF will increase significantly.

So I consider inheritance is a good thing: it reduce the ODF complexity. So I'd like to leave it in most cases except the error corrections that should be as much as precise so they should not be inherited. And I argue against a strategy of eliminating inheritance at all.

There are some other ODF keys that are valid on a single sample file level, ex. AttackVelocity, CuePoint, AttackStart, ReleaseEnd and others while most of other Pipe999xxx keys are inherited.

Sure, I just tested it with this PR and it does indeed work beautifully - but it's not possible to specify perfectly for obvious reasons...

The only thing I find is not beautiful is that two ODF keys Pipe999LoopCrossfadeLength and Pipe999ReleaseCrossfadeLength with different inheritance rules have very similar names. May be introduce a new name instead of Pipe999LoopCrossfadeLength and Pipe999Attack999LoopCrossfadeLength? Any ideas?

However, for this PR, if you really, really, don't want the crossfade values behave in similar fashion, I won't protest anymore about it even though I still think it's wrong.

So I ask you to approve this PR or to propose another name for the new (not inherited) ODF key.

[larspalo]

ReleaseCrossfade (specified or calculated) is a rule

@oleg68 Well, kind of - as it's always used in one of the ways you mention. But similar to the loop crossfade - it really should not be necessary to manually specify in most cases. The default calculated value should provide the necessary transition well enough, and any specification should really be an exception (in my opinion) for something that, for some un-clear/un-known reason, needs to be done in another way than the default calculated version.

I have a decent amount of original sample sets made for GO, as well as quite a few SP conversions and I just made a grep for all the .organ files that have a ReleaseCrossfadeLength specification in them... Guess what I came up with! Not one (!!!) uses it except for the OdfEdit conversions and the few cases where I've tested the behavior to understand it better! Thus, fearing to break anything by changing/removing the inheritance behavior for any of the odf crossfade values is totally misplaced as that feature basically was never used!

You're right that inheritance when doing voicing adjustments (which the crossfade values are not - but the gain adjustment is) is indeed a good thing as it can simplify the work a lot. Both the crossfade values (and a few of the other attributes you cite) are there for correcting anomalies that the samples might contain and GO can't handle well enough (yet).

[larspalo]

@oleg68 But to be fair towards you, I think that you're work to enable the corrections of sample anomalies to be applied where they really are needed (on the individual sample level) and not just wildly on every attack/release sample for a pipe is brilliant!

[oleg68]

@larspalo You do not want that Pipe999ReleaseCrossfadeLength would be used for Pipe999Release001, don't you? But what value shoud be used for the attack/attack transition?

In my approach it is still Pipe999ReleaseCrossfadeLength. Another possible option is to use a calculated value there. I'm not ready to introduce a new key for this purpose now.

[larspalo]

@oleg68 I just want to remove the inheritance of the release crossfade value to other releases just the same as you've done for the LoopCrossfadeLength for other attacks. For now, I think that if it's present, the Pipe999ReleaseCrossfadeLength can still be used for the attack switching, and if not present - the calculated value. In the future we can change that to fit the tremulant startup and slow down feature.

As I said, I don't think this is a terribly important feature as such, but the possibility to make the adjustments on individual sample level that you've introduced is absolutely the right thing to do!

[oleg68]

@larspalo I removed the inheritance of release crossfades,

[larspalo]

@oleg68 Ok, I'll test the build! Thanks!

[oleg68]

@rousseldenis Could you approve this PR?