Incorrect writing of ODF file

[davidgritter]

I have found two scenarios when ODF files are written incorrectly. First, when A foreign ODF is read that has an enclosure defined in the old format, there are errors in writing the ODF GUI element from GOODF. At least one parameter is written incorrectly and another that is necessary is not written at all.

Second, when a foreign ODF is read that has stops referencing ranks where the first rank pipe is other than 1, the first rank pipe is written out twice. The attached document provides examples of both. saveerrors.odt

[davidgritter]

This is with fresh GOODF built on March 23

[larspalo]

@davidgritter I'll investigate the first case further.

For the first issue I can just say that the general approach I've taken in GOODF is to try to never write out things in the ODF that's not really needed, the default values can be safely omitted when writing - but they should of course always be read correctly - even when the supplied values from a read odf themselves are wrong. I'll review that part to make sure that the handling of the default values conform to what GO actually wants them to be. If you read some of the OdfEdit issues I think you'll find references to problems with some of the enclosure element values that are wrong when transferred to GO.

The second issue should be fixed with my latest commit. Please test it.

[davidgritter]

Note that the enclosure definition in the foreign ODF as shown in the document I attached is correct. I check this carefully, since I know OdfEdit makes lots of mistakes. I believe issue one in the document involves both reading and writing issues: Note in the document I attached above that the enclosure PositionX in the foreign ODF is 992. However, if you look on the GOODF GUI, you see that it is read as -1. This is clearly a read error. This read error seems to occur in other elements as well, particularly when the value in the parameter is a larger number relative to the dimensions of the GUI panel.

Note in the document I attached above that in the GOODF GUI, that MouseAxisStart is shown as 568. Since this was not in the foreign ODF, GOODF appears to have derived it correctly. However when GOODF writes the parameter, it appears in the written ODF as 571; so this is a write error.

I have tested your latest commit, and the issue #2 in the document attached above is indeed corrected and working properly now.

[larspalo]

@davidgritter Valid values for the PositionX are from 0 to panel width. In GOODF I see that I currently have the allowed max value to be less than panel width. I'll change this, but there are more things to fix to get all things right according to how GO handles things.

[larspalo]

@davidgritter I've tried to improve the reading and writing of Gui enclosures with my latest commit. Please test it and report back.

The main goal I have for GOODF is that is should produce a valid .organ file that GO can use, even when the values from a read ODF is wrong. Thus, I've tried to make reading and writing conform to what GO actually use (and not only what is described in the help), even if it in some cases can produce strange results...

[davidgritter]

The Enclosures read from the foreign ODF now produce ODF entries acceptable to GrandOrgue in the cases I tested. I noticed that the mouse axis values are strange numbers to me, but they produce the results in GrandOrgue that are typical of enclosures from other GrandOrgue ODFs. ( I have yet to be able to produce a slider in GrandOrgue where the slider lever always remains exactly under the mouse, but I think this is a GrandOrgue issue not related to GOODF)

[larspalo]

@davidgritter Great. I've adjusted the basic calculations to be exactly what GO itself uses internally. As I previously told the OdfEdit author, it's generally better (== more exact) to use the mouse wheel (or two finger) scrolling for controlling enclosure levels in GO than to click hold and drag.

You mentioned earlier that other elements suffered from similar issues. If you find any, please report them too. I think it's essential that GOODF should behave the same as GO when it comes to the ODF values, especially so that any future panel rendering exactly will mimic what happens when the organ is opened/loaded in GO.

[davidgritter]

Other elements suffer from the issue of reading -1 for displayX or DisplayY when the number in the ODF being read is close to the panel x or y dimension. I've seen this with manuals, switches, as well as with enclosures. Only Enclosures seem to have had the mouse parameter issues that you fixed.

[larspalo]

@davidgritter I've examined the GUI button default values and compared to the GO source and tried to fix all of them to conform to GO specs. Please test the button derived elements (stops, switches, tremulants, pistons, divisionals, generals etc) for extreme values, just the same as with the enclosures earlier.

There are a few strange things about current GO default layouts for all button derived elements (drawstops and pistons) especially with regard to the basic row numbers when not using PositionX and Y. It seems that it's currently possible to specify row numbers up to 99 for both buttons (from 0) and drawstops (from 1) even if the specified display metrics value (DispDrawstopRows) is less. I contemplated imposing limits for this in GOODF, but since the resulting ODF will be valid in GO anyway as of now, I've refrained from doing so. Valid values in GO for DispButtonRow range from 0 to 99 + m_metrics->NumberOfExtraButtonRows(), and for DispDrawstopRow they range from 1 to 99 + m_metrics->NumberOfExtraDrawstopRowsToDisplay(). Thus the DispDrawstopRows value in display metrics seems to be ignored as a limit, but it might still affect the layout.

I'll go over the manuals later in a similar way.

[larspalo]

@davidgritter I've committed fixes for both manuals and labels as well now.

[davidgritter]

I'm not clear on what changes you made that I can test. As nearly as I can tell, Switches and other button based objects import correctly if their displayx or display y are near the limits of the panel, but manuals and expression elements do not in the latest commit. Am I supposed to be looking for some other change?

[larspalo]

@davidgritter I think I've changed all the elements to allow the full panel width the same way as GO itself does. If you still find any issues still with (valid) PositionX and PositionY values for any of the elements (as well as in the GUI panels within GOODF), please report them.

[davidgritter]

Using GOODF built on April 5, I encounter the following problems:

Panel is 1016 x 680, and includes a background image of the same size.

Enclosure001 loads with PositionX=-1 instead of 992 from the following odf instance:

[Enclosure001] Name=Volume Slider far AmpMinimumLevel=1 Displayed=Y DispLabelText= PositionX=992 PositionY=55 MouseRectLeft=1 MouseRectTop=0 MouseRectWidth=19 MouseRectHeight=568 BitmapCount=50 Bitmap001=..\OrganInstallationPackages\002384\images\Slider\slider00.png Bitmap002=..\OrganInstallationPackages\002384\images\Slider\slider01.png...

bitmaps are 571 units y high, and 21 units wide

Manual001, pedal, loads with PositionY=-1 when it should be 640. (Note the foreign ODF for this is in error, since the bitmaps for the individual pedals are missing. GOODF correctly ignores individual pedal widths and offsets and uses the built in pedal representation.

[Manual000] Name=PEDAL Displayed=Y NumberOfLogicalKeys=32 NumberOfAccessibleKeys=32 FirstAccessibleKeyLogicalKeyNumber=1 FirstAccessibleKeyMIDINoteNumber=36 NumberOfStops=13 NumberOfSwitches=0 NumberOfCouplers=7 NumberOfDivisionals=0 NumberOfTremulants=0 PositionX=347 PositionY=640 Width_A=9 Width_Ais=9 Width_B=18 Width_C=9 Width_Cis=9 Width_D=9 ...

Manual004, choir, loads with PositionY=-1 when it should be 540. (Note the foreign ODF includes the correct bitmaps for all the keys, so GOODF correctly displays the custom keyboard, but in the wrong location.

[Manual004] Name=CHOIR Displayed=Y NumberOfLogicalKeys=61 NumberOfAccessibleKeys=61 FirstAccessibleKeyLogicalKeyNumber=1 FirstAccessibleKeyMIDINoteNumber=36 NumberOfStops=9 NumberOfSwitches=0 NumberOfCouplers=8 NumberOfDivisionals=0 NumberOfTremulants=0 PositionX=347 PositionY=540 Width_A=8 Width_Ais=2 Width_B=9 Width_C=6

The Great and Swell manuals with Position Y of 483 and 430 respectively appear in the correct locations.

two enclosures show a Position Y=-1 with 590 expected:

[Enclosure003] Name=Exp.2 AmpMinimumLevel=1 Displayed=Y DispLabelText= PositionX=647 PositionY=590 MouseRectTop=0 MouseRectWidth=28 MouseRectHeight=35 BitmapCount=16 Bitmap001=..\OrganInstallationPackages\002384\images\expressionPedal1\expres01.bmp Bitmap002=..\OrganInstallationPackages\002384\images\expressionPedal1\expres02.bmp

As I mentioned before, switch elements appear to be read correctly

[larspalo]

@davidgritter Can you compress the .organ file and attach it so that I can examine it more closely? I can see that it's supposed to be an old style main panel type, which might confuse things if also a [Panel000] is present. Does the original odf open in GO without any issues?

[davidgritter]

attached is the .organ file I've been reading from. the related images, pipes, etc can be downloaded and used as is from Augustine Organs. As originally converted using ODFedit, it would load in GrandOrgue, but would not work. I removed a label element for each switch that had been assigned to panel001, as they were not necessary, without changing the number of elements, so now it will complain about missing elements. However, it does create loadable organs when read by GOODF, then written out. Redemeer Aeolian-Skinner_surround orig.tar.gz

[larspalo]

@davidgritter Thanks. It's as I suspected. This odf uses a mixed up way of creating the odf by both specifying a new style [Panel000] and at the same time writing the gui related things in the structural organ elements. Basically, one can draw the conclusion that it's written by someone who didn't fully understand how the new panel format was supposed to work (and why) vs the old style. I don't think it's problematic to fix this issue though, (that is, to read the values correctly in GOODF).

The root of the issue as for now is that the actual display metrics for [Panel000] is not read until after the structural elements - thus only the default display metrics values are "valid" at the time of gui element reading. If the odf was completely done with the old style it would have worked just fine. The simple fix for this is to push the reading of just the [Panel000] display metrics much earlier in the parsing. I'll see if I get the time to do that later this evening.

[davidgritter]

yes, I've used this organ for testing before, because even the Hauptwerk xml file is messed up much as you describe. Later organs from Augustine often translate much more easily. On a good note, I noticed that the foreign ODF has the incorrect number of panels (3), and that GOODF does not get confused by this.

[larspalo]

@davidgritter Please test the latest commit and see if this fixes the issue (as it should).

[davidgritter]

latest commit is working properly