search

GrandOrgue / grandorgue

GrandOrgue software
Other
149 stars 39 forks source link

Fixed not loading a pipe if some loop was not suitable for crossfade https://github.com/GrandOrgue/grandorgue/issues/1724 #1763

Closed oleg68 closed 6 months ago

oleg68 commented 6 months ago

Resolves: #1724

This PR adds capanility to load a pipe even some (but not all) loops are bad for using with Pipe999LoopCrossfadeLength specified. These loops are just ignored and worning messages appear in the Log window.

larspalo commented 6 months ago

The build for this PR always crash in my tests. Conditions and ways to repeat:

The correct warning is issued when opening the organ, but when I try to play the pipe GO crash. I have not created a debug build to exactly trace it down, but it's fairly easy to guess.

If no loops are valid with current crossfade value then maybe that sample should not be loaded at all, perhaps with an additional note of that in the log warning (or perhaps replaced with a DUMMY). There could of course be other possible scenarios too, like ignoring the crossfade value with a warning and just load the sample without crosssfading of that single loop... Or failing the loading of the organ like it did earlier.

If more than one loop exist in the file and at least one is valid then it works as intended.

oleg68 commented 6 months ago

@larspalo Thank you for your testcase. I fixed the crash.

larspalo commented 6 months ago

@oleg68 Yes, it seems to be fixed now. Do you really need the comments in src/grandorgue/sound/GOSoundAudioSection.cpp line 755 and forward?

Except that they still are prime examples of poor commenting practice, you have the called function just 135 lines above in the same file! So, even if your IDE doesn't show them (like Eclipse does) when you hover over the call, it shouldn't be that hard to search and compare them manually.

It's just sad to have such low quality comments there, especially when one compare to the comments in for instance the header file where the comments actually are beneficial!

larspalo commented 6 months ago

Also, perhaps the copyright year should be updated? Should this be done on a file to file basis when we edit them or should we batch change them all (at the latest when a new release for this year is done)?

oleg68 commented 6 months ago

@larspalo I removed the parameter names and changed the copyright headers in the files are modified.

oleg68 commented 6 months ago

@rousseldenis could you approve this pr?