LogWindow sometimes obscured. Some samples added to .organ file with incorrect path. Wave tremulant addition tweak. #109
Conversation
window might be obscured and the messages missed. Define a SHOWLOGWINDOW macro to wrap this. Only call MakeRelativeTo() if we have an absolute path. removeBaseOdfPath() may not need to calculate a relative path if the filename does not start with a path separator. Log any files that cannot be found when reading a .organ file and mention that they will be excluded/deleted. When adding a Wave tremulant and only a single ignore Wave tremulant attack exists, set that attack to play when Wave tremulant is off. Also log that this has been done. This behaviour is enabled when the bool setSingle is true (the default).
Add a workflow_dispatch for testing.
is a later version than the version installed on ubuntu-20.04 so use MakeAbsolute() and GetFullPath() instead.
Workflow file was modified out-of-band.
environment on github.
|
Thanks for your interest in GoOdf and it's great that you think it's useful! Sorry that I've not been able to respond so quickly but we've been without current here (due to a snow storm and a loss of power) for a few days, so no internet connection either until now.
It's done for the Msw port to function as expected. Further testing is needed to verify. But it's unlikely to change.
This is very good! I'll definitely include this in one way or another.
This is something I'll need to investigate to secure the correct behaviour in every case expected. |
|
I did very casual testing on a Win10 VM. The main window still appeared without the Raise() call. I am not a windows user though. My logging of missing "DUMMY" or "REF:*" files is probably not helpful. I've added a check for that in ff81740. I also added support for a command line .organ file argument in 3f1d122. I found myself typing "GoOdf organ.organ" the same as I would "GrandOrgue organ.organ" too many times. It was a "nice to have." |
It appears the first time the program is opened, but if I remember correctly, on subsequent loadings of .organ files the main window "disappeared" and was needed to be called back by clicking on it in the tray. I'm not really a windows user myself either, but others are.
No, likely not. I think that checking for correctly existing REF: already exist - but I'm unsure about if any logging is done. DUMMY is always valid and cannot really be missing.
I don't mind that. A way to register GoOdf as a program to open .organ files on all platforms directly from a file manager would also be nice. |
|
Another small thing is that I'll want to keep the On: [push] criteria in .github/workflows/release.yml so that every push also will result in an intermediate build that can be tested by anyone. On a side note, it would be better if you did any work/changes in a separate branch for each feature and created a separate PRs to GoOdf master from each one. I'm sorry that I have so little time right now to test this because I really thing some of the changes you're proposing would be very valuable indeed. Hopefully, I'll get to it soon! Thanks. |
src/Rank.cpp
Outdated
| @@ -1100,6 +1100,29 @@ void Rank::addTremulantToPipes( | |||
|
|
|||
| // if there are any matching attacks we add them | |||
| if (!pipeAttacksToAdd.IsEmpty()) { | |||
| // warn if all previous attacks ignore wave tremulant | |||
There was a problem hiding this comment.
I'm not certain that this whole section will work as I would expect from it. Let me first explain the recommended workflow for adding separate wave tremulant samples when they are not in a sub directory to where the non-tremmed samples are, which for instance would be common to any sonus paradisi sample set.
- First add the non-tremmed samples with a suitable method with the "Load pipes to play when wave tremulant is off" checkbox checked. Also make sure to empty the Tremulant folder prefix: textbox!
- Then add the tremmed samples separately with "Add tremulant samples from..." button.
With the method you're using/suggesting here things could be a bit confusing if multiple attacks do exist!
I think it would perhaps be better to just add a log warning if no attack is detected as set to play when wave tremulant is off for the Add tremulant samples from... loading method (thus scanning all existing attacks). It's difficult to assert if it's intentional or not though, so I'd just add a warning if the case is detected. As potentially attacks could be added later with the correct option as well, I don't think this method should change already existing attacks at all!
There was a problem hiding this comment.
If multiple attacks exist, then only a warning is shown. If the bool setSingle is false, then no attack will be modified. No method is provided to toggle setSingle, a user accessible check box will need to be added.
Another issue I was trying to solve was when there are less tremulant pipes than non-tremulant pipes. This is the case in the Barton SoloStrings and SoloStringsTrem ranks I had. I was hoping to avoid error prone hand editing to ignores-tremulant all the non-tremulant pipes without matching tremulant pipes.
I think that having at least a warning would be a good idea: "Have you forgotten to select Load pipes to play when wave tremulant is off"
1113 becomes if (!hasNonWave) {
1114 has a more helpful message
remove lines 1106, 1117-1125
There was a problem hiding this comment.
If multiple attacks exist, then only a warning is shown.
But how to know if it's intentional or not.
I think that having at least a warning would be a good idea
Yes, that was what I suggested in my initial comment. One should parse all the attacks, and if not at least one of them has "play when wave tremulant is off" - then issue a warning (when loading the wave tremulant samples). Still this might be perfectly valid since the user can load the non-tremmed versions later. I outlined the recommended procedure above.
There was a problem hiding this comment.
Pushed the wrong button and lost my much more detailed response! Oh well. Older, wiser (jury's out on that!).
Fortunately GoOdf does not need to actually read the samples. I have a script that generates a test sample-set tree (empty *.wav). I'll look at coding a warning popup.
There was a problem hiding this comment.
As I wrote in the README.md file:
" Always work on copies or on files you don't mind having destroyed!"
Anyway, thanks for trying to improve the program!
There was a problem hiding this comment.
I have a mental block. Do you have an example of a real-world ODF that uses all three states of isTremulant (NULL, true, false) on a single pipe in a meaningful way? A rank with a mix of invdividual pipes with isTremulant NULL, or isTremulant defined makes perfect sense. All three states in a single pipe, not so much.
Here's a contrived single pipe with four attacks. Attacks 0 and 1 will always be selected, with attack 2 when Tremulant is on, and attack 3 when Tremulant is off.
Pipe001=hwfake8\036.wav
Pipe001AttackCount=3
Pipe001Attack001=hwfake8\036_bis.wav
Pipe001Attack002=hwfake8_tr\036.wav
Pipe001Attack002IsTremulant=1
Pipe001Attack003=hwfake8_rel\036.wav
Pipe001Attack003IsTremulant=0
| Attack | Switch | IsTremulant | Played |
|---|---|---|---|
| 0 | ON | NULL | Yes |
| 1 | ON | NULL | Yes |
| 2 | ON | 1 | Yes |
| 3 | ON | 0 | No |
| 0 | OFF | NULL | Yes |
| 1 | OFF | NULL | Yes |
| 2 | OFF | 1 | No |
| 3 | OFF | 0 | Yes |
There was a problem hiding this comment.
I've pushed 382de96. No attacks/releases are modified, and only one Dialog is displayed per rank update if there are any detected isTremulant mixes. More places where this could happen were uncovered in testing, so there are more lines changed than expected!
There was a problem hiding this comment.
@larspalo would you be able to run https://gist.github.com/kwhite-uottawa/4c1486e06a873c23d250d28228b0cf52#file-checktrem-sh on your .organ files? It should be silent unless there are pipes with three tremulant states. It's a script that runs awk to convert Pipe* lines to SQL which is then to be piped to sqlite3.
Output over the .organ files I have collected find ../../Organs/ -name '*.organ' | sed -e 's+.*+./checktrem.sh "&" | sqlite3+' | sh -v 2>&1 | tee checktrem.log is in https://gist.github.com/kwhite-uottawa/4c1486e06a873c23d250d28228b0cf52#file-checktrem-log
Output for suspect .organ files looks like this:
./checktrem.sh "../../Organs/Buckerburg/Audio/BuckerburgODF-trem.organ" | sqlite3
rank name pipeno what nsamples nnulls ndefs toterrs
------- ----------------- ------ ------- -------- ------ ----- -------
Stop016 37 Quintadena 16' 001 attack 2 1 1 300
Stop016 37 Quintadena 16' 001 release 5 3 2 300
Stop016 37 Quintadena 16' 002 attack 2 1 1 300
Stop016 37 Quintadena 16' 002 release 5 3 2 300
Stop016 37 Quintadena 16' 003 attack 2 1 1 300
There was a problem hiding this comment.
I've got no problem reported for most sample sets, nor did I expect it. Barton is a special case of course. Personally, I don't see much point in fussing about the tremulant states either. A simple warning, well phrased, if the case is detected is absolutely good enough for me.
There was a problem hiding this comment.
I've got no problem reported for most sample sets, nor did I expect it. Barton is a special case of course. Personally, I don't see much point in fussing about the tremulant states either. A simple warning, well phrased, if the case is detected is absolutely good enough for me.
Barton is indeed a special case! Doing the right thing for a beginning GoOdf user with that sample set can be a challenge.
I had removed updating the tremulant and only display a warning message in 382de96 Better phrasing would be helpful. I just don't have any ideas.
wxMessageDialog msg(::wxGetApp().m_frame, wxT("Warning: A mix of tremulant and non-tremulant pipes in a rank."), wxT("Pipe Tremulants Warning"), wxOK|wxCENTRE|wxICON_EXCLAMATION);
Not disheartened at all, really! Yes, the PR should have been broken down to much more manageable individual PRs. Sorry, about the chaos -- I'd thought that the pull request was static as of the time I made it and didn't change as I made what I thought were only local to me subsequent changes. I'm a github newbie, obviously. |
Co-authored-by: larspalo <larspalo@users.noreply.github.com>
Use a more appropriate name. Co-authored-by: larspalo <larspalo@users.noreply.github.com>
I have been unable to generate a testcase that reliably triggers the issue this was supposed to resolve. the problem that �removeBaseOdfPath stringToReturn.StartsWith(wxFILE_SEP_PATH)
it might be hidden behind the main window when an ODF file with errors is loaded.
a pipe with samples selected with isTremulant is NULL or isTremulant 0/1 A pipe should typically either have no isTremulant samples or only isTremulant samples.
if errors were reported when a .organ file was loaded. Since the log window is now raised in 1f1178d, raising each SHOW_LOG_WINDOW is no longer necessary. Testing has revealed that when running GoOdf remotely a Raise() for each SHOW_LOG_WINDOW is quite time expensive.
| @@ -797,6 +804,17 @@ void Rank::addToPipes( | |||
|
|
|||
| // if there are any matching attacks we add them | |||
| if (!pipeAttacksToAdd.IsEmpty()) { | |||
|
|
|||
There was a problem hiding this comment.
I think that all this (in more or less every place it's done) and the relevant macros perhaps would be better done as a public function of a Pipe. Then one could just ask the Pipe if it has conflicting isTremulant values in the attacks and save a few repeated lines. The function would just return a bool value which in turn would decide whether to trigger the warning message or not.
There was a problem hiding this comment.
And possibly fire that message dialog itself as well...
There was a problem hiding this comment.
And possibly fire that message dialog itself as well...
That had occurred to me too. I'm not a C++ programmer. Even though I certainly understand and agree with what you're saying (public function of Pipe, etc.) implementation might take me some figuring out.
There was a problem hiding this comment.
Actually, as a message dialog would block the execution, it could be better for the flow to just log the warning and proceed.
I've been very happily using GoOdf to quicky create .organ files to test problems with the recent GrandOrgue release. Thanks @larspalo for this great program!
I experienced a few unexpected "gotchas" (for me at least!) while investigating GrandOrgue/grandorgue#2004 (comment)
FWIW: I use a non-tiling window manager (cwm) that does not display window frames, etc.