Warning messages: Unused ReleaseCrossfadeLength

[rousseldenis]

On several existing sample sets, the following warnings are displayed (Example with ChaiseDieu, but with sample sets from Piotr Grabowski too):

GrandOrgue 3.14.0

@oleg68 @larspalo Is this a wanted behavior ?

[larspalo]

@rousseldenis You're a bit late to the discussion about this (#1760 and #1724 among others) since it's implemented months ago. Bottom line to fix the warnings is: just re-create the odf with OdfEdit and the problem goes away. No original GO odf is affected, only those created/converted with an earlier version of OdfEdit.

The main issue is that now the Pipe999ReleaseCrossfadeLength is only valid if a release is actually loaded with/in the Pipe999 sample (it has LoadRelease=Y). For any additional releases, which normally is used with the converted sample sets, you need Pipe999Release999ReleaseCrossfadeLength specified.

[rousseldenis]

@rousseldenis You're a bit late to the discussion about this (#1760 and #1724 among others) since it's implemented months ago. Bottom line to fix the warnings is: just re-create the odf with OdfEdit and the problem goes away. No original GO odf is affected, only those created/converted with an earlier version of OdfEdit.

@larspalo Yes, I know about recreating through OdfEdit. But I wasn't remembering what was the conclusion about old sample sets. I suppose sample set editor that has managed ODF for GrandOrgue had maybe done some changes from HW's one (and so, the ODF is different in HW and GO).

I will re-create the ODF for GrandOrgue.

In addition of this, I'm wondering how to better manage warnings as they are non blocking ones but generating a separate window that is quite annoying.

[eturpault]

Within tomorrow evening I will publish a new version of OdfEdit (v2.11) which uses the attributes Pipe999Attack999LoopCrossfadeLength and Pipe999Release999ReleaseCrossfadeLength introduced in GrandOrgue 3.14.0 (and will use HasIndependentRelease as well).

[larspalo]

In addition of this, I'm wondering how to better manage warnings as they are non blocking ones but generating a separate window that is quite annoying.

Well, the ultimate fix is to have higher quality odfs and samples that won't generate any warnings!

It's also possible to uncheck the "Perform strict ODF" and "Check ODF for HW1 compatibility" checkboxes to silent some of the warnings. But the best "fix" is to correct the odfs/samples that generate the warnings.

Overall in the recent development of GO there has been a move towards allowing more and more "faults" in the odfs/samples and still allow loading/opening/using an organ (with some warnings). This however doesn't mean that the warnings are meaningless! The ones you or any other user really should "bug" about the warnings are the producers of the (low quality) odfs/samples so that the issues are fixed at the root.

[eturpault]

The warning reported in this issue has appeared with GrandOrgue v3.14.0. A same sample set (generated with OdfEdit v2.8, the latest version which was still using the attribute Pipe999ReleaseCrossfadeLength for HW to GO conversion) does not generates this warning with GrandOrgue v3.13.3, whereas it generates this warning with GrandOrgue v3.14.0.

So the root cause here is not a "low quality" ODF, it is that the attribute Pipe999ReleaseCrossfadeLength is no more supported by GO v3.14.0. Probably this issue is a regression caused by the addition of the attribute Pipe999Release999ReleaseCrossfadeLength in GO v3.14.0. There is no problem with Pipe999LoopCrossfadeLength, it is still supported by GO v3.14.0.

You can reproduce this issue for example with the Friesach sample set (original GrandOrgue version recovered from the Piotr Grabowski website). In the Rank001 section add the following line :

Pipe001ReleaseCrossfadeLength=150

Loading the modified ODF, you will get the following warning with GO v3.14.0 and not with GO v3.13.3.

Warning: Unused ODF entry 'Rank001/Pipe001ReleaseCrossfadeLength'

[larspalo]

@eturpault Yes, in that particular case (Pipe999ReleaseCrossfadeLength - which incidentally still can be valid if the Pipe999 also actually loads a release from the attack sample) the warning comes from a change in requirements for the usage of that attribute! But more generally, warnings appear because of mistakes in the odf or faults in samples or image files or any combination thereof.

However my main point and reason of approving such a change, that actually brings finer configuration control, still holds true as far as I know: no original GO sample set that was publicly available depended on that setting (it was never used)! Thus the change of behaviour would only affect sample sets that were converted from HW to GO with OdfEdit - and they could easily be "fixed" by just re-converting them again.

[oleg68]

@eturpault @rousseldenis There were made an incompatible change in 3.14.0: earlier Pipe999ReleaseCrossfadeLength were used for all release samples of the pipe, now it is used only for the release loaded from the Pipe999 sample file. This incompatibility was discussed in #1760 and @larspalo insisted on the new behavior.

[eturpault]

If only ODF generated by OdfEdit were using this attribute Pipe999ReleaseCrossfadeLength, it is not a problem to have made this change, as said by Palo it will be solved by regenerating the ODF with OdfEdit > v2.8 (ideally with v2.11 which is coming soon).

[larspalo]

@oleg68 To be honest, in #1760 I suggested that the Pipe999ReleaseCrossfadeLength should be inherited to any release children, but you insisted that they should not be and that that value only should be valid if a release was loaded from the Pipe999 sample! Anyway, I agreed because of the higher flexibility this change allow and that the incompatibility issue really wasn't an issue at all!

it is that the attribute Pipe999ReleaseCrossfadeLength is no more supported by GO v3.14.0.

@eturpault Just to prove that your statement above is wrong I present a new version of the release x-fade test sample set I made a while ago. Pipe999ReleaseCrossfadeLength is valid if a release is loaded from the Pipe999 sample file.

TestReleaseXfade.zip

[eturpault]

Yes more exactly it is no more supported as it was supported before v3.14.0, the rule has changed.

[oleg68]

I suggested that the Pipe999ReleaseCrossfadeLength should be inherited to any release children, but you insisted that they should not be

I insisted on stopping inheritance only of Pipe999LoopCrossfadeLength, but on keeping inheritance of Pipe999ReleaseCrossfadeLength https://github.com/GrandOrgue/grandorgue/pull/1774#issuecomment-1896493711.

But you did not want different inheritance for these keys. So we dicided to stop the inheritance everywhere.

Our discussion was at #1774 instead of #1760

[oleg68]

@rousseldenis This is an intended (new) behavior and we wont fix it. Should we close the issue or move it to discussion?

[larspalo]

So we dicided to stop the inheritance everywhere.

@oleg68 We might have misunderstood each other in some parts of the details, or just had too different visions about it, but I still do think that the implementation we finally agreed upon is an improvement since with it all the CrossfadeLength attributes now behave in the same way and they allow for individual specifications on all levels.

[rousseldenis]

@rousseldenis This is an intended (new) behavior and we wont fix it. Should we close the issue or move it to discussion?

Yes I know.

I wanted to be sure about the behavior with old sample sets. For instance, ones that were written for previous versions of GrandOrgue and ones that sample sets writers supported (e.g.: piotr's ones before Nancy). Most users does not know about OdfEdit.

[Klassikfreund]

Wie generiere ich mit OdfEdit die ODF neu? Ich habe die ODF (in meinem Beispiel Arnstadt) mit dem OdfEdit 2.11 geladen, wie geht es jetzt weiter?

[eturpault]

To regenerate the ODF, simply open the Hauptwerk ODF with OdfEdit 2.11 as you did, OdfEdit generates a new ODF for GrandOrgue in the OrganDefinitions folder of the Hauptwerk ODF.

With the ODF generated by OdfEdit v2.11 you should not have warnings when loading it in GrandOrgue 3.14.0.

[Klassikfreund]

Vielen Dank für die schnelle Antowrt! Alles O.K.

[ahall41]

I have recently upgraded to a later version of GrandOrgue. I now find that all the ODFs I have created (using my own scripts, not OdfEdit) come up with this warning and no longer sound good. I should not have to amend all my scripts (over 100 - but not all use ReleaseCrossFadeLength) as a result of this change. I would appreciate it if the original behaviour could please be maintained.

"If we broke inheritance of release crossfade length it would impact lots of existing sample sets" (@larspalo) - yes, it has!

[larspalo]

@ahall41 No official sample set on https://github.com/GrandOrgue/grandorgue/wiki/Sample-Sets was affected as far as I know. If anyones privately constructed odfs could have been impacted wasn't possible to know. But now, as a result of the compromise we developers agreed upon, this is the intended behaviour - which actually greatly increases the possibility of fine tuning the usage of the attribute(s).

I'm sorry for any inconvenience this might have caused you or anyone else, but the simplest way to fix it is either to begin using OdfEdit or to create another script that will correct the odfs by removing the wrongly used Pipe999ReleaseCrossfadeLength and transfer that value to all the extra releases as Pipe999Release999ReleaseCrossfadeLength.

As an example of the latter approach I attach below a command line application I've written that does exactly that (if you compile and use it directly, you do so at your own risk). After taking the second route your private odfs will have restored "the old inherited behaviour" from Pipe999ReleaseCrossfadeLength, which no longer exist, by simply copying it to the actually existing releases.

FixRelXfade.zip

[ahall41]

I have about 40 ODFs published (mostly demo but quite a few full) on MPS Orgelseit (listed on the Wiki page) which have ReleaseCrosssfades, and a few others have also published them. Perhaps you could change this entry, as there are now far more (213 right now) than the few listed? I only needed to change a few lines in one of my core modules. It will take some effort to go through all of them and republish on MPS Orgelseite. What is done is done - but - please - any future changes need to be backwards compatable - or at least put a warning and instructions in the release notes!

[ahall41]

@larspalo Missing from your Wiki, under Piotr Grabowski (all "compromised") are also Alessandria, Begard, Chorzow, Obervellach and Oloron ... But I'm a bit confused why Green Positiv doesn't throw a warning?

[larspalo]

Missing from your Wiki

@ahall41 It's the GrandOrgue software Wiki, not my personal.

But I'm a bit confused why Green Positiv doesn't throw a warning?

I think that's actually a bug in the current implementation that's triggered when multiple attacks are present. I'll create a separate issue for that.

[larspalo]

@ahall41 Actually, with the 3.14.0 release not only the behaviour of the Pipe999ReleaseCrossfadeLength changed. Also the Pipe999LoopCrossfadeLength changed so that it would not be inherited by any other attack. Not many sample sets ever used that feature in combination with multiple attacks, but if any would ever have done it, its behaviour would now have been changed since GO 3.14.0.

To be all incusive I've altered the program I uploaded earlier above to include changing any X-fade values on the pipe level to be applied to all the additional attacks and releases like if it was inherited in the linked file below. Any usage of a compiled version of it to change any odf is at ones own risk, of course. The recommended approach is to convert all HW files for usage with GrandOrgue by means of a current version of OdfEdit instead.

FixOldXfades.zip

I unfortunately cannot promise that the future development of GO won't change anything else that potentially can break backwards compatibility, but we really do try to take it into account every time we make changes to the code. You know what they say: you can't make an omelette without breaking a few eggs... I sure hope that everyone using GO will appreciate the effort we as developers put into advancing GO as a software though.

[ahall41]

This change is neither forwards nor backwards compatible - something I was told was a big no no when I started as a programmer way back. And if you have to, then at least provide a means of conversion, as well as documenting it fully in the release notes? Your GoOdf removes the old ReleaseCrosssfades, whereas OdfEdit retains them. It would be useful if both of these could do the conversion (not everyone will have a C compiler). I do appreciate the effort everyone is making. I've forgotten how to code in C (never did get to grips with C++, went down a VB -> Java -> PHP route), so decided the best I could do was to attempt to create some ODFs for some of the HW sample sets out there. I think there are quite a few available for GrandOrgue as a result? Thanks for the reminder about LoopCrossfade. I'll add this to my scripts (I don't think I have many, if any ODFs with multiple attacks, though).

[larspalo]

LoopCrossfade - which (incidentally) does not throw up any warning.

No, it doesn't cause warnings since it will be used for one (main) attack sample (only) but is not inherited by any other attack anymore (which it was earlier) - so the behaviour is actually changed even though it's not immediately visible.

Your GoOdf removes the old ReleaseCrosssfades

Well, that depends... If the release crossfade would be valid for GO then it's kept by GoOdf, but if not then it's removed. The Pipe999ReleaseCrossfadeLength would still be valid if the pipe attack sample also loads the release (Pipe999LoadRelease=Y).

It would be useful if both of these could do the conversion (not everyone will have a C compiler).

What conversion are you thinking about now? Only OdfEdit can convert HW sample sets to GO whereas GoOdf is designed to work with just .organ files for GO.

[ahall41]

LoopCrossfade - which (incidentally) does not throw up any warning.

I realised that (deleted it). I would not have spotted the issue with ReleaseCrossfade had it not thrown up warnings (and my organ sounding not quite right). But I think you could issue a warning if LoopCrossfade is set and Attack999LoopCrossfade isn't?

What conversion are you thinking about now? Only OdfEdit can convert HW sample sets to GO whereas GoOdf is designed to work with just .organ files for GO.

I'm thinking for converting existing ODFs (GoOdf does an excellent job of converting old style panels to new). As mentioned not everyone will have access to a C compiler (especially on Window$).

[larspalo]

I'm thinking for converting existing ODFs

Ok, in order to "migrate" old style X-fade values to the new style individual X-fade values as if they were inherited?

I think I could do that in GoOdf relatively easily. I can see that the (old) values are actually read into the first (main) attack sample but they are only written back if considered valid (if not, they are actually grayed out but still existing for as long as the file is open in the program)...

One possibility is to add a conditional option in the Tools menu. If that option is checked before opening an .organ file the X-fade values will be copied to all other attacks/releases in the same pipe as if inherited at the end of .organ file parsing. On sample sets that already use the new individual X-fade values that copying procedure could potentially create problems as values could be accidentally overwritten. Having to first make a distinct choice in the Tools menu to "Parse legacy x-fades" would allow to "fix" the inheritance at load time easily if deemed necessary and also provide some protection against accidents, especially if that menu check would only be set for the duration of a session and never stored in the config file.

Thoughts about that?

[ahall41]

Yes - best to make it a choice, rather than leave it automatic. But you could also prompt where legacy X fades are detected?

[larspalo]

@ahall41 In GoOdf v0.12.0 I've implemented the option to convert old (legacy) x-fade values to the new individual equivalents like suggested. Also there will be a log window that notify if obvious cases of legacy x-fades are detected in an .organ file. The option to convert the x-fades must be set (checked) before opening the .organ file for them to take effect. I suggest testing the feature on copies at first...

[larspalo]

@ahall41 After testing/adding a few more warnings for errors when parsing an .organ file, and having fixed some more switch logic controls, I've now decided to remove the log warning about possible legacy Pipe999LoopCrossfadeLengths since there's no way to determine if it would be a conscious choice to only add the Loop x-fade to the main attack or if was done in the legacy manner expecting it to be inherited by other attacks. That case simply needs to be known and handled manually in order to avoid excessive log messages.

The Pipe999ReleaseCrossfadeLength in combination with LoadRelease=N is a clear fault though (now) and easy to detect/warn for so that will be kept.

The parse legacy x-fades option will, if enabled/checked, still convert all types of x-fades from old behaviour to new, but the questionable log warning/message for the Pipe999LoopCrossfadeLength won't be issued anymore in future versions (like it is in the 0.12.0 version of GoOdf).

[ahall41]

Yes, you could deliberately want a cross fade of 0 (default). Parse Legacy x-fades may be poor terminology. Import could be a better choice?

[larspalo]

Import could be a better choice?

Yes, perhaps something like that would be more obvious than "Parse Legacy X-fades". Maybe "Import Legacy X-fades", "Load Legacy X-fades" or "Open w. Legacy X-fades".

On the other hand, maybe that whole feature could be reworked... As the values actually are read but grayed out and not written back by default they already exist at the "pipe" level. The "Import Legacy X-fades" menu option (actually a command) could be implemented not as a parsing option for opening an organ file, but instead just as a command that would transfer all the existing X-fade values already read in the main (first) pipe attack to all other pipe attacks/releases without the need for re-opening the .organ file? Would that be a better option? Maybe with a first message dialog explaining what will happen next and a question if that's really what the user want (Yes/No)? The log warning that legacy x-fades are detected could of course be kept anyway.